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

Skip old_password validation when user has no usable password set #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anuj9196
Copy link
Contributor

In case of social authentication, an unusable password is set.

If settings have OLD_PASSWORD_FIELD_ENABLED then old_password is validated when the user changes the password.
Since the user has not set any password in the past, the check for old_password should be skipped to change the password.

The validation will work when the user has a password set.

In case of social authentication, an unusable password is set.

If settings have `OLD_PASSWORD_FIELD_ENABLED` then `old_password` is validated when the user changes the password.
Since the user has not set any password in the past, the check for `old_password` should be skipped to change the password.

The validation will work when the user has a password set.
@anuj9196 anuj9196 mentioned this pull request Apr 1, 2020
@iMerica
Copy link
Owner

iMerica commented Apr 2, 2020

I think allowing a social-auth user to change their password on an existing session makes the app less secure. For example, if a user creates their account with FB and 9 months later they want to set a new password for local login, they should be required to authenticate with FB again to set the password.

@anuj9196
Copy link
Contributor Author

anuj9196 commented Apr 2, 2020

There are two ways of changing the password.

  1. User is authenticated (change password)
  2. User is not authenticated (reset password)

In case 1: If the user is authenticated, they will use the change password endpoint, which is restricted to login_required. The user can be authenticated using FB. But when OLD_PASSWORD_FIELD_ENABLED is set to True, the request will be validated against the old password, which is still not set for the user.

Since the user is authenticated, I believe it is safe to skip the old password check in this case. Because even user can skip old password validation by setting OLD_PASSWORD_FIELD_ENABLED to False (Default set to False. See: Configuration)

In case 2: Reset password will be done by email validation, where the old password is not required.

This PR resolves the issue of case 1 where if the old password field is enabled and the user has signup using a social account, and the old password field is enabled in change password, the validation will be skipped first time when the user has an unusable password set. But will be validated next time.

I hope, I'm able to describe the use case.

@anuj9196
Copy link
Contributor Author

anuj9196 commented Apr 2, 2020

I think allowing a social-auth user to change their password on an existing session makes the app less secure. For example, if a user creates their account with FB and 9 months later they want to set a new password for local login, they should be required to authenticate with FB again to set the password.

The change password endpoint is login_required hence the user will have to be authenticated in order to change the password.

The old password validation skip will only work if OLD_PASSWORD_FIELD_ENABLED is set toTrue which is by default set to False which still skips the old password check by default.

@iMerica
Copy link
Owner

iMerica commented Apr 2, 2020

That’s not good enough. Let’s say a token was compromised. The malicious user could access protected resources, but only up to a time limit.

If you allow them to set the password without any re-auth you’re helping the malicious user.

@anuj9196
Copy link
Contributor Author

anuj9196 commented Apr 2, 2020

If that's the case then OLD_PASSWORD_FIELD_ENABLED should be True by default as if it is not set to True explicitly then by default the change password is compromised.

Also, If user has signup using FB, what are the other ways if they want to enable normal login by setting a password?

Because, if OLD_PASSWORD_FIELD_ENABLED is not set to True explicitly, it's always open to the malicious users because even if user has a password, old_password is poped from the serializer validator method and anyone with the token can change the password without re-auth.

This PR is just to let the user set password first time without needing an old password after they are authenticated and OLD_PASSWORD_FIELD_ENABLED is set to True

@iMerica
Copy link
Owner

iMerica commented Apr 3, 2020

Thank you for creating this pull request. I think we can close it and make OLD_PASSWORD_FIELD_ENABLED True by default in different pull request. Thoughts?

@anuj9196
Copy link
Contributor Author

anuj9196 commented Apr 3, 2020

But what about If user has signup using FB. what are the other ways if they want to enable normal login by setting a password?

User still won't be able to enable normal login.

Also how the change password is secured if user set this config to False?

@iMerica
Copy link
Owner

iMerica commented Apr 3, 2020

The only secure way of doing this (that I can think of) is to re-auth in the set password flow. Facebook has some notes about this here.

From the end user perspective it looks like this:

  • User created an account on FB.
  • User goes to account settings page.
  • They see a set password option.
  • The click "Begin set password", which redirects them to FB to re-authenticate.
  • Upon successful re-authentication, they are redirected to a page with a form setting a password.
  • They submit the payload, which includes a nonce, password and password_confirm .
  • Validate this nonce server-side.
  • If everything is fine, save the new password.

@anuj9196
Copy link
Contributor Author

anuj9196 commented Apr 3, 2020

@iMerica This is cool too. But this has to be implemented in the change password flow in the library before setting OLD_PASSWORD_FIELD_ENABLED default to True. This flow is missing.

Also this should be implemented in such a way that nonce will be used in old_password in case of social signup. In case of signup using email + password, current password will be used instead of nonce.

I mean there should be only one endpoint with fixed payload attributes and logic should be handled in the server.

@jerinpetergeorge
Copy link
Contributor

IMHO, Using the forgot password option is the best failproof way to set a password on an account which is created by using social authentication.

@KingOfPeru
Copy link

1 question. changing password doesn't return a new token. Is this ok or should I refresh the token by calling this endpoint?
/dj-rest-auth/token/refresh/

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.

None yet

4 participants