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

Why is there a check for a specific backend? #196

Closed
saevarom opened this issue Sep 2, 2022 · 6 comments
Closed

Why is there a check for a specific backend? #196

saevarom opened this issue Sep 2, 2022 · 6 comments
Labels
state:needs-answer Needs additional information from the issue creator type:bug

Comments

@saevarom
Copy link

saevarom commented Sep 2, 2022

At some point a check for a specific authentication backend was introduced.
I get this error message:

django.core.management.base.SystemCheckError: SystemCheckError: System check identified some issues:

ERRORS:
?: (rest_registration.E014) invalid authentication backends configuration: LOGIN_DEFAULT_SESSION_AUTHENTICATION_BACKEND is not in AUTHENTICATION_BACKENDS

I implemented my own authentication backend but use rest-registration for the signup process. This error prevents me from upgrading django to a more recent version and is also very restraining. People should be able to use the rest-registration package standalone without it being opinionated about which authentication backend to use.

This could be changed to a warning to indicate that if you have a problem, you should look into the authentication backends setting.

@apragacz
Copy link
Owner

Hi @saevarom,

thanks for reporting the issue.
Also, sorry for the delayed response, I had to dig up when the change was actually introduced.

I'm afraid I can't help your specific case because I don't have all relevant data. If you want me to help you I'd advise to report a bug via templates listed here:
https://github.com/apragacz/django-rest-registration/issues/new/choose . Direct link to the template is here.

But I think I can answer the question in the issue title:

Why is there a check for a specific backend?

For the sake of brevity, I will refer to the django.contrib.auth module as auth later on.

I found out why that change was introduced: #168

In short, if you have multiple backends (used by the auth.authenticate function), and you want to use session login (auth.login) you need to provide the backend explicitly. You can read about this more here:

https://docs.djangoproject.com/en/4.1/topics/auth/default/#django.contrib.auth.login
https://docs.djangoproject.com/en/4.1/topics/auth/default/#selecting-the-authentication-backend

So in other words, LOGIN_DEFAULT_SESSION_AUTHENTICATION_BACKEND DRR setting was introduced to provide a default when the backend for auth.login cannot be derived automatically.

By default, the value of it is set to 'django.contrib.auth.backends.ModelBackend', which matches the Django AUTHENTICATION_BACKENDS set to ['django.contrib.auth.backends.ModelBackend'] by default.

I added the check to avoid ankward situation when someone changes AUTHENTICATION_BACKENDS to something not containing django.contrib.auth.backends.ModelBackend, which could lead to weird situations (potential security risk) that completely different backend will be used in auth.login than the one which was used for authentication.

But then I realized, this is still not perfect. If there are multiple backends, we still may end up using the "wrong" backend in auth.login (not the one which was used for authentication). The only "advantage" is that we will be limited to the one from the AUTHENTICATION_BACKENDS list.

So I started to dig into internals of the Django auth.authenticate function, and I found out that the user model is being annotated with the backend field, which could be re-used later on when calling auth.login.

Unfortunately, that behavior is not dockumented (or to be precise I didn't find anything in the docs), So there is small possibility it could be changed.

My proposal is the following:

  • Change the code to use user.backend field; ensure a test relying on that behavior is added (especially, with multiple AUTHENTICATION_BACKENDS set)
  • update docs for LOGIN_AUTHENTICATOR (include info that in case of override user.backend should be set accordingly)
  • remove LOGIN_DEFAULT_SESSION_AUTHENTICATION_BACKEND and associated checks (and related tests)

That would probably mean that we would need to release a minor/major version (0.8.0), not a patch version as the change would not be backwards-compatible. However, in most cases, that would be the "expected" behavior.

Let me know what you think about it.

@apragacz
Copy link
Owner

Updated proposal:
let's keep LOGIN_DEFAULT_SESSION_AUTHENTICATION_BACKEND, but change it to None by default, and change the check so it will be used only when it's set (not None). In addition, use the user.backend field, if it is provided before falling back to LOGIN_DEFAULT_SESSION_AUTHENTICATION_BACKEND.

@saevarom
Copy link
Author

Hi @apragacz

Thanks for you response. I think your updated proposal would work fine. At least I think it would not prevent me from keeping my current structure or upgrading to newer versions.

apragacz added a commit that referenced this issue Sep 20, 2022
Changes:

*   Use user backend field when available (#196)
@apragacz
Copy link
Owner

@saevarom I released 0.7.3 version which should fix your issue. Let me know if it helped.

@apragacz apragacz added the state:needs-answer Needs additional information from the issue creator label Sep 20, 2022
@saevarom
Copy link
Author

Thank you @apragacz - I will check when I have the next chance

@saevarom
Copy link
Author

Thank you for your help, I believe this has fixed my issue and I can now upgrade Django and django-rest-registration :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:needs-answer Needs additional information from the issue creator type:bug
Projects
None yet
Development

No branches or pull requests

2 participants