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

Implement Variant of ResetPasswordConfirm that validates a token #59

Closed
wants to merge 1 commit into from
Closed

Implement Variant of ResetPasswordConfirm that validates a token #59

wants to merge 1 commit into from

Conversation

Hall-Erik
Copy link
Contributor

This implements #45

If the user sets DJANGO_REST_PASSWORDRESET_ALLOW_TOKEN_VALIDATION = True in settings,
the password field becomes optional in ResetPasswordConfirm. It will return a 200 if a valid token is provided without a password.

If DJANGO_REST_PASSWORDRESET_ALLOW_TOKEN_VALIDATION is not set, or is set to False, ResetPasswordConfirm behaves as it did before - returning a 400 if a password isn't given.

I also added a couple of tests for this and updated the README.

@anx-ckreuzberger
Copy link
Contributor

This looks good.

  • Not API breaking 👍
  • Minimum code changes 👍
  • Documentation/Readme Updated 👍
  • Tests 👍

That's what a good PR looks like :)

Anyway, personally I would have gone for a separate endpoint, e.g., /validate_token/, but sending the token without a password at the same existing endpoint is fine for me too.

I'm going to leave this open for a week or so, as I would like to see some opinions on whether the implemented endpoint logic within this PR is considered as okay or not.

@Hall-Erik
Copy link
Contributor Author

I agree with you about using separate endpoints. That way when the user gets a 200 from /confirm/, they know it means the password was updated and not just that the token is valid.

@anx-ckreuzberger
Copy link
Contributor

closed in favour of #60

@Hall-Erik Hall-Erik deleted the validateToken branch August 8, 2019 18:18
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.

2 participants