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

Fix the reset_password_token_created signal to be fired even when no token have been created. #188

Conversation

emickiewicz
Copy link

Bug description

The regression is introduced in V1.4.0 by #181, the the reset_password_token_created.send(...) signal was fired every time when the DJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGE setting was set to True, even when no user was found and then no reset token created.

When no user was found, the signal was fired with parameter reset_password_token at None instead of a Token object.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (improvements in base code)
  • Add test (adds test coverage to functionality)

Current MR

This MR proposes a fix of the bug and adds more test coverage.
The signal is not fired anymore when no token have been created.

Feel free to let me know if you think more tests should be added, or if if you have any other idea on how to improve this fix.

No other existing behavior should be impacted by this fix, so it might be shipped as a minor version as no breaking change is introduced.

Ideas of improvement (with breaking changes) are described bellow.

Note

I think that in order to offer a clear interface for programmatically creating tokens (i.e. without using the DRF API), as the initial MR #181 implemented, we could sightly move the code behavior and approach (not implemented in this MR):

  • Setting DJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGE should indeed control is an error is raised or not, but this should be checked at the view level django_rest_passwordreset.views.ResetPasswordRequestToken.post
  • Move django_rest_passwordreset.views.generate_token_for_email to a new utils module that would also hold the other methods to interact with tokens programmatically.
  • Make django_rest_passwordreset.views.generate_token_for_email always raise a custom Exception class when no user is found. This method should have a consistent result when no user is found, that way developers can have the DJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGE setting to True to protect their API, and also know what is wrong when an error occurred while creating programmatically a token. Returning None is not giving enough information on what went wrong, and how to fix it.

Checklist

  • Automated tests
  • Extends CHANGELOG.md
  • Requires migrations? -> NO
  • Requires dependency update? -> NO

@nezhar
Copy link
Member

nezhar commented Apr 15, 2024

Thanks 💯

@nezhar nezhar merged commit 6815a81 into anexia-it:master Apr 15, 2024
22 checks passed
@emickiewicz emickiewicz deleted the bugfix/reset_password_token_created_signal_fired_without_token branch April 16, 2024 08:29
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