Separate LOGIN_FIELD logic into a auth backend to avoid giving tokens… #810
Conversation
… to non authorized users
There was a problem hiding this comment.
I like overall approach and idea how to fix that. Thank you for implementing that!
I would only slightly improve the way how is_active flag is checked and use ModelBackend.user_can_authenticate method for that.
@tomwojcik thoughts?
Also, this change is not fully backend compatible so it would be good to bump at least minor version but may be we should also consider adding that to next major version.
| user = User.objects.filter(**kwargs).first() | ||
| if user and not user.check_password(password): | ||
| return | ||
| if user and user.is_active: |
There was a problem hiding this comment.
This is good to have so djoser is compatible with Django's default auth model backend. However to be fully covered in the future I would either try to call super().authenticate() or at least self.user_can_authenticate().
There was a problem hiding this comment.
Thank you for your returns. This approach is a simple copy-paste of the current behaviour of djoser, but exported to a custom Auth backend.
The origin of this piece of code is the availability of using a LOGIN_FIELD without a CustomUserModel( #389 and 8f65bff).
My idea is to keep current djoser behavior for people using this way of declaring user model in djoser config and expecting working like today by using a custom auth backend.
All other people expecting standard django behaviour will have nothing to do. Remember that this custom backend will be in a list with other auth backends, so, if the dev includes the django default backend, both login behaviours (default django one, and custom djoser) will be taken in account.
There was a problem hiding this comment.
Thanks for explaining your point of view! From that perspective it makes some sense it keep it more compatible with current behaviour.
However with any next major release it would be good to use the default Django's auth backend behaviour and implementation.
@tomwojcik would you merge this PR to the upcoming 2.x version or would you wait until version 3? I think I would go for upcoming 2.x with proper documentation in the changelog.
There was a problem hiding this comment.
I'd definitely release the fix in 2.3 as it's a minor security issue, but at the same time I'm not sure if these particular changes are actually needed.
IMO the changes from 8f65bff are incorrect. If the user didn't pass authenticate, then they shouldn't be able to login.
If you need a custom behavior, it's up to the ModelBackend and user manager implementation (like @hisie did). Handling it by Djoser is a shortcut.
There was a problem hiding this comment.
Thank you @haxoza. I have leave a comment of why I have chose to not use "standard django methods".
I know this change is incompatible with current djoser, but i have not found a way of recovering standard django auth backend chain check without adding a new django backend.
| user = User.objects.filter(**kwargs).first() | ||
| if user and not user.check_password(password): | ||
| return | ||
| if user and user.is_active: |
There was a problem hiding this comment.
Thank you for your returns. This approach is a simple copy-paste of the current behaviour of djoser, but exported to a custom Auth backend.
The origin of this piece of code is the availability of using a LOGIN_FIELD without a CustomUserModel( #389 and 8f65bff).
My idea is to keep current djoser behavior for people using this way of declaring user model in djoser config and expecting working like today by using a custom auth backend.
All other people expecting standard django behaviour will have nothing to do. Remember that this custom backend will be in a list with other auth backends, so, if the dev includes the django default backend, both login behaviours (default django one, and custom djoser) will be taken in account.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #810 +/- ##
==========================================
- Coverage 99.03% 98.81% -0.23%
==========================================
Files 26 27 +1
Lines 833 846 +13
==========================================
+ Hits 825 836 +11
- Misses 8 10 +2 ☔ View full report in Codecov by Sentry. |
… to non authorized users.
Corrects #795