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

Verify user_can_authenticate when authenticating user #379

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cfra
Copy link
Contributor

@cfra cfra commented Oct 12, 2020

The standard Django ModelBackend only returns a user from authenticate
if user_can_authenticate is True.

The OIDCAuthenticationBackend doesn't check user_can_authenticate,
in its authenticate method, allowing a user to log-in that has is_active
set to False.

This behavior is a surprising deviation from the way ModelBackend
works.

It can lead to security problems because other important
authentication checks may inadvertently be bypassed, if the user
implements them in user_can_authenticate, expecting
OIDCAuthenticationBackend to call it the same as any other
ModelBackend.

It seems proper for the OIDCAuthenticationBackend to verify
user_can_authenticate and it seems to me like there are no problems
associated with doing so, so let's just call user_can_authenticate
to resolve the problems described above.

The standard Django `ModelBackend` only returns a user from  `authenticate`
if `user_can_authenticate` is `True`.

The `OIDCAuthenticationBackend` doesn't check `user_can_authenticate`,
in its `authenticate` method, allowing a user to log-in that has `is_active`
set to `False`.

This behavior is a surprising deviation from the way `ModelBackend`
works.

It can lead to security problems because other important
authentication checks may inadvertently be bypassed, if the user
implements them in `user_can_authenticate`, expecting
`OIDCAuthenticationBackend` to call it the same as any other
`ModelBackend`.

It seems proper for the `OIDCAuthenticationBackend` to verify
`user_can_authenticate` and it seems to me like there are no problems
associated with doing so, so let's just call `user_can_authenticate`
to resolve the problems described above.
@codecov-io
Copy link

Codecov Report

Merging #379 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   89.60%   89.62%   +0.02%     
==========================================
  Files           7        7              
  Lines         481      482       +1     
==========================================
+ Hits          431      432       +1     
  Misses         50       50              
Impacted Files Coverage Δ
mozilla_django_oidc/auth.py 95.13% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bd65e2...99b45bd. Read the comment docs.

@cfra
Copy link
Contributor Author

cfra commented Feb 2, 2021

Is there any update on this, by any chance? 🤔

@cfra
Copy link
Contributor Author

cfra commented Jul 12, 2021

Any chance this can be reviewed and/or discussed? It is sitting without review for over half a year now. 😥

@indigane
Copy link

Noticed this as well.

This could be a result of old Django conventions, but it feels dangerous to leave it up to the view since nowadays Django handles it in the authbackend, and like noted above developers may expect that of other authbackends that inherit from ModelBackend.

As for the PR, is there a specific reason why the check is made in get_or_create_user and not in authenticate before calling get_or_create_user?

Is the is_active check in the view necessary after this change? If, as noted above, someone has a custom user_can_authenticate then they might not want is_active checked on top of calling user_can_authenticate.

Would creating an issue help get this PR forward?

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.

3 participants