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)
[x] 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):
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.
nezhar
commented
5 months ago
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 theDJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGEsetting 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_tokenatNoneinstead of aTokenobject.Types of changes
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):
DJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGEshould indeed control is an error is raised or not, but this should be checked at the view leveldjango_rest_passwordreset.views.ResetPasswordRequestToken.postdjango_rest_passwordreset.views.generate_token_for_email to a newutilsmodule that would also hold the other methods to interact with tokens programmatically.django_rest_passwordreset.views.generate_token_for_emailalways 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 theDJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGEsetting toTrueto 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