InsaLan / backend-insalan.fr

Backend of the insalan website
MIT License
2 stars 0 forks source link

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

Closed margauxmillour closed 1 month ago

KwikKill commented 8 months ago

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.

KwikKill commented 8 months ago

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 :

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.