Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User id as a pk for email confirmation & password reset #129

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Conversation

margauxmillour
Copy link
Contributor

No description provided.

Copy link
Member

@KwikKill KwikKill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot one part, the front url (/verification/<id>/<token>/) is calling our backend on /v1/user/confirm/<id>/<token> with the same parameters. The problem is that /v1/user/confirm/... still expect username.
You also need to change the view and the url in order to use user id instead

Copy link
Member

@KwikKill KwikKill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KwikKill KwikKill requested review from Lugrim and Lymkwi January 30, 2024 22:24
Copy link
Contributor

@Lugrim Lugrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but may break some frontend links, frontend will need to be updated shortly after this is merged

@KwikKill
Copy link
Member

KwikKill commented Feb 9, 2024

LGTM, but may break some frontend links, frontend will need to be updated shortly after this is merged

It should not as the id is only deferred from the confirm view to a backend API call. We may need to update the front props type but that's all.

Copy link
Collaborator

@Lymkwi Lymkwi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR currently does not work for me, i imagine because the ResetPassword view (user/views.py#233) expects a user field in its request, and uses it to try and retrieve a user from its name. This makes the frontend fail, and i think the workflow would be the same without it, and it would still fail.

Moreover, i don't really get the appeal of using the PK instead of the username. Sure, you can see people's username on the website or guess it from somewhere else, but having PKs in the URLs means you have a parameter you can enumerate (even if you need to know the validation UUID; having the UUID means you can just enumerate until you find whoever reset, whereas if a username wasn't known prior, having the UUID does not necessarily mean you can find the user and reset it for them).

insalan/user/tests.py Outdated Show resolved Hide resolved
insalan/user/tests.py Outdated Show resolved Hide resolved
@KwikKill
Copy link
Member

KwikKill commented Feb 15, 2024

This PR currently does not work for me, i imagine because the ResetPassword view (user/views.py#233) expects a user field in its request, and uses it to try and retrieve a user from its name. This makes the frontend fail, and i think the workflow would be the same without it, and it would still fail.

You're right on some points. ResetPasswordview should be edited too.

Moreover, i don't really get the appeal of using the PK instead of the username. Sure, you can see people's username on the website or guess it from somewhere else, but having PKs in the URLs means you have a parameter you can enumerate (even if you need to know the validation UUID; having the UUID means you can just enumerate until you find whoever reset, whereas if a username wasn't known prior, having the UUID does not necessarily mean you can find the user and reset it for them).

The goal of this PR is to remove use of username as user pk in user views so that we can allow users to change it. This should not bring any security problems as the endpoints currently using username are :

  • EmailConfirmView : take a token in parameters so can't be brute force.
  • ResetPassword : take a token in parameters
  • ResendEmailConfirmView : This is not used at the moment and should maybe use email instead.

There still is a UUID that can't be guessed in theses views and if someone find a valid UUID we have other things to be scared of because it's only sent in the confirm mail with the complete URL.
The same thing was used for the ticket with the user id and a UUID.

@KwikKill KwikKill linked an issue Aug 9, 2024 that may be closed by this pull request
@KwikKill KwikKill merged commit 2d84dd6 into dev Oct 4, 2024
4 checks passed
@KwikKill KwikKill deleted the Put_id_as_pk branch October 4, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use user id as a pk for email confirmation & password reset
4 participants