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

use only authenticate for create token #819

Merged
merged 15 commits into from
Nov 9, 2024
Merged

use only authenticate for create token #819

merged 15 commits into from
Nov 9, 2024

Conversation

tomwojcik
Copy link
Contributor

Closes #795

rollback 8f65bff

TBD drop Djoser LOGIN_FIELD in favor of Django's User USERNAME_FIELD

@tomwojcik tomwojcik requested a review from haxoza May 3, 2024 20:35
@tomwojcik tomwojcik self-assigned this May 3, 2024
@tomwojcik tomwojcik changed the title rollback 8f65bfff16577c7fb0f52bbabf5fb69f6809ba62, add support for Mo… use only authenticate for create token May 3, 2024
@tomwojcik
Copy link
Contributor Author

@hisie this approach would work for you as well, right?

Copy link
Member

@haxoza haxoza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good and pretty simple.

However, this PR questions the existence of LOGIN_FIELD setting in djoser at all. It only gives consent through silence to define your model auth backend based on LOGIN_FIELD. Yet, this silence maybe misleading with the current documentation:

LOGIN_FIELD

Name of a field in User model to be used as login field. This is useful if you want to change the login field from username to email without providing custom User model.

In fact, to make it really working (your tests are just mocking it), you need to define either custom User model OR custom auth backend.

That is why I like @hisie approach in #810 that provides custom auth backend ready to use. Thanks to that you can easily say in the documentation: "Don't you have custom user model? Then use LOGIN_FIELD setting and this custom auth backend."

If for some reason we do not want to provide custom auth backend then we should do both:

  1. give more info in the docs on how to create custom auth backend with example code
  2. make bold statement in the documentation that custom auth backend is just workaround and the proper way of doing this is to have custom user model with USERNAME_FIELD redefined.

Thoughts?

)
@override_settings(
DJOSER=dict(django_settings.DJOSER, **{"LOGIN_FIELD": "username"})
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge decorators for better readability. It's easy to get confused or just skip one of the override_settings calls. The same applies to tests below this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hisie
Copy link

hisie commented May 9, 2024

Sorry for the delay.
Yes I like this solution @tomwojcik .

As @haxoza says, my proposal of testing with djoser_settings.LOGIN_FIELD was intended to easily keep backwards compatibility with the old behaviour. I don't like the old behaviour at all because it hides custom auth backend authentications, but I can understand that migrating from a running project using djoser LOGIN_FIELD to a custom user with an auth backend should be hard.

Both solutions (@tomwojcik and mine) are good for me since both avoid giving tokens to users who don't match existing authentications.

While not being responsible of the project, I let you decide if you want to keep easily backwards compatibility or not.

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.05%. Comparing base (35c815f) to head (bd23a35).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #819      +/-   ##
==========================================
+ Coverage   99.03%   99.05%   +0.01%     
==========================================
  Files          26       27       +1     
  Lines         833      846      +13     
==========================================
+ Hits          825      838      +13     
  Misses          8        8              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomwojcik tomwojcik requested a review from haxoza June 7, 2024 12:20
@tomwojcik
Copy link
Contributor Author

tomwojcik commented Jun 7, 2024

@hisie @haxoza I think that the previous approach had a vulnerability on Djoser side and the default behavior should change. I added an optional Model backend, (similar to your solution @hisie ), if someone wants backwards compat. Please let me know what you think. I believe it's ready to go as-is.

@tomwojcik tomwojcik modified the milestones: 2.2.3, 2.3.0 Jun 7, 2024
[
("username", "username", "username"),
("email", "email", "email"),
("email", "username", "email"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backwards compat with LoginFieldBackend

Copy link
Member

@haxoza haxoza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for a long delay in reviewing this PR.

@tomwojcik thank you, it looks great! I agree with this approach.

docs/source/settings.rst Show resolved Hide resolved
@hisie
Copy link

hisie commented Jul 19, 2024

Good work. I can't wait to see released. Thank you very much.

Co-authored-by: Przemek Lewandowski <haxoza@users.noreply.github.com>
@tomwojcik tomwojcik merged commit bd23a35 into master Nov 9, 2024
17 checks passed
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.

DJOSER is giving tokens to users that don't pass the AUTHENTICATION_BACKENDS.authenticate
3 participants