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 of e.g. ServiceWorker makes login with 2FA impossible #113

Open
simonkern opened this issue Sep 4, 2021 · 4 comments
Open

Use of e.g. ServiceWorker makes login with 2FA impossible #113

simonkern opened this issue Sep 4, 2021 · 4 comments
Labels
Milestone

Comments

@simonkern
Copy link

When using a ServiceWorker, authentication with 2FA enabled is broken. Due to some limitations it is necessary to serve the workerscript from the root of the site. In case you run something like gunicorn behind a load balancer, you will most likely just save the worker script as a template file and serve it using a template view. However, combined with allauth_2fa this causes a problem that ultimately breaks authentication for all users that have 2FA enabled.

In the OTPAdapter (adapter.py line 22) user.id gets stored within the session:
request.session['allauth_2fa_user_id'] = str(user.id)

The presence of allauth_2fa_user_id in the session is then checked in the TwoFactorAuthenticate view, (views.py line 34) . In case it's not present, the user will be redirected to the login view. Otherwise the user with allauth_2fa_user_id will be put into kwargs and authentication will proceed.

allauth_2fa_user_id gets cleared from the session by means of AllauthTwoFactorMiddleware (middleware.py line 26).

When you happen to use a service worker, what will very frequently happen is:

  1. User with 2FA enabled wants to log in
  2. ServiceWorker checks if there is a new version available (requests e.g. /sw.js)
  3. User with 2FA enabled logs in via username and password
  4. allauth_2fa_user_id gets stored in the user's session
  5. User gets redirected to 2FA form
  6. ServiceWorker checks if there is a new version available (requests e.g. /sw.js)
  7. Because ServiceWorker has just requested "/sw.js", middleware removes allauth_2fa_user_id from the user's session, because the condition in middleware.py evaluates to true
  8. User enters his 2FA Token, but allauth_2fa_user_id is already gone and user is redirected to the login page

This szenario is not only limited to the usage ServiceWorkers, but is triggered everytime user makes (for whatever reason) a request between logging in and entering the 2FA token that causes the condition in the middleware to evaluate to true.

I am not sure what the best alternative solution is, or if there is any evil side effect, if we do not delete allauth_2fa_user_id at all. What do you guys suggest?

@akx
Copy link
Member

akx commented Sep 5, 2021

Without digging in deeper, one simple solution/workaround that comes to mind would be to serve /sw.js from a middleware before the 2FA middleware.

simonkern added a commit to simonkern/django-allauth-2fa that referenced this issue Sep 5, 2021
@simonkern
Copy link
Author

simonkern commented Sep 5, 2021

Login also breaks if you happen to do any sort of request in between login and two-factor-authentication, e.g., pulling latest news for a sidebar widget via ajax etc. Therefore, I don't think the above mentioned approach solves this issue for good. I propose the solution in the pull request, which tries to remove allauth_2fa_user_idin two places:

  1. After a successful login and two-factor-authentication of a user that has 2FA enabled. After the initial login (with username and passwort only), an already presentallauth_2fa_user_id will be overwritten anyway. So we only need to take care of it after authentication.
  2. After a successful login of a user that does not have 2FA enabled. We could also clear it before the login - however, this is out of this app's scope.

The only time an artifact of allauth_2fa_user_id can remain in the session is between an abortion of the 2fa-flow and the user's next successful login (which will clear or overwrite it).

Please note: I just commented out the old tests. Before merging, the code should be cleaned up properly. Just let me know, if this approach is fine for you and I'll clean it up properly and update the docs.

@sabipu
Copy link

sabipu commented Oct 13, 2021

@simonkern I don't think the codebase maintainers are all that active about this project.

I did ran into the same problem a while ago and this is what I did to work around it.
#93 (comment)

@simonkern
Copy link
Author

Any chance of getting this or what @sabipu mentioned merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants