-
Notifications
You must be signed in to change notification settings - Fork 670
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
Add option to allow inactive user authentication and token generation #834
base: master
Are you sure you want to change the base?
Add option to allow inactive user authentication and token generation #834
Conversation
…active user authentication
for more information, see https://pre-commit.ci
Thanks for reaching out! So the offending line of code in the original authentication backend is: if not user.is_active:
raise AuthenticationFailed(_("User is inactive"), code="user_inactive") I would much prefer if we made a setting called "skip check is_active" where the setting defaults to False (as in by default check is_active) because I can also see how some users may not create an is_active flag. This would knock out two use caes. Ensure the default authentication rule function also checks for this setting. |
Update default_user_authentication_rule to use the setting, Update JWTAuthentication.get_user() to test user against authenrication rule instead of is_active flag
for more information, see https://pre-commit.ci
@Andrew-Chen-Wang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost ready, just a few changes
@@ -164,6 +165,37 @@ def get_user(self, validated_token: Token) -> AuthUser: | |||
return api_settings.TOKEN_USER_CLASS(validated_token) | |||
|
|||
|
|||
class JWTInactiveUserAuthentication(JWTAuthentication): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class shouldn't be needed given the new settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, i missed it. should be good now
@@ -131,7 +131,8 @@ def get_user(self, validated_token: Token) -> AuthUser: | |||
except self.user_model.DoesNotExist: | |||
raise AuthenticationFailed(_("User not found"), code="user_not_found") | |||
|
|||
if not user.is_active: | |||
# Ensure authentication rule passes | |||
if not api_settings.USER_AUTHENTICATION_RULE(user): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to only validate the user is active, not run the authentication rule, since this function is just looking to fetch a given user. Good forward thinking, but yes this would break backwards compatibility
We can revisit this if you open a GitHub issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it harder i see how it could cause a problem if a user made their own rule, addressed in latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! this looks great
The problem:
Some specific use cases require inactive users to be able to authenticate, but that is prevented by the code even though django allows it with 'AllowAllUsersModelBackend'.
The changes:
Added new authentication class and authentication rule that allow for inactive users
Added tests for them as well
So instead of having to rewrite the code allowing inactive users can be done by adding these settings:
closes #607