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

Support two factor auth (2FA) #3523

Closed
agjohnson opened this issue Jan 16, 2018 · 21 comments
Closed

Support two factor auth (2FA) #3523

agjohnson opened this issue Jan 16, 2018 · 21 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

We should enable 2fa for dashboard users. I keep wanting to add site admin features to the dashboard, but then think about the security aspects of adding these features and find myself also wanting 2fa. There are some libraries that do handle a 2fa workflow for standard django logins, but i don't know if this extends to django + allauth or django + mamacas.

I'm sure we're probably in agreement of this being an important feature, but I'm not sure we can gauge the importance of 2fa for users. I'm sure community users would use a feature like this, and site admins would use this feature -- I doubt this is in high demand for commercial hosting customers though.

The following thoughts come to mind:

  • Is this too hard? Does allauth play well with a 2fa workflow?
  • Is there any reason besides complicating login that we shouldn't?
@agjohnson agjohnson added the Needed: design decision A core team decision is required label Jan 16, 2018
@agjohnson agjohnson added this to the Security milestone Jan 16, 2018
@agjohnson agjohnson added the Improvement Minor improvement to code label Jan 16, 2018
@ericholscher
Copy link
Member

ericholscher commented Mar 27, 2018

I'm +0 on adding it. I don't think RTD is so sensitive that we are a common attack vector. I'm much more worried about building authoring features before building something like this, unless it's simple to do with a pluggable Django app. Unless users are specifically asking for this, I don't see it as a high priority (sadly).

@agjohnson
Copy link
Contributor Author

Yeah, i agree on priority here. This is a feature that i consider more important for commercial hosting, but I also haven't had any requests for this feature though.

@agjohnson
Copy link
Contributor Author

Also, I think a lot of what I want to add would probably be more applicable as a django admin action instead of an on site admin only feature.

@agjohnson agjohnson removed the Needed: design decision A core team decision is required label Mar 29, 2018
@stale
Copy link

stale bot commented Jan 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jan 10, 2019
@ericholscher ericholscher added the Accepted Accepted issue on our roadmap label Jan 10, 2019
@ericholscher
Copy link
Member

Accepted 👍

@stale stale bot removed Status: stale Issue will be considered inactive soon labels Jan 10, 2019
@dojutsu-user
Copy link
Member

@agjohnson

more applicable as a django admin action instead of an on site admin only feature

I am a little confused on this line on what this means?
Also the Needed: design decision tag is removed. Can it be made clear on what needs to be done?

@MatteoGheza
Copy link

Any update on this?

@ericholscher
Copy link
Member

Sadly not -- we'd love to support it, but it isn't on our short term roadmap. If there is a good way to handle this via Django, we'd love to know, but I haven't found one.

@humitos
Copy link
Member

humitos commented Jan 15, 2021

Today I did quick search and I found this one https://django-allauth-2fa.readthedocs.io/en/latest/, which looks like a good candidate since it should integrate directly with our current auth system: django-allauth.

@humitos
Copy link
Member

humitos commented Oct 6, 2022

There is another Python package that could be useful for this https://github.com/justinmayer/kagi

@ericholscher
Copy link
Member

I do wonder about basically just punting on this, and saying to use one of our SSO options if you want 2FA. I think more and more users are going to be defaulting to logging in with those options to enable our VCS SSO anyway.

@humitos humitos added the Priority: low Low priority label Nov 23, 2022
@humitos
Copy link
Member

humitos commented Jan 16, 2024

I think suggesting SSO options is the way to go here. However, is possible to "remove the password" from an existing user after connecting GitHub for example? I did a quick check and I wasn't able to find it.

@stsewd
Copy link
Member

stsewd commented Feb 19, 2024

Django allauth now has built-in support for 2FA https://docs.allauth.org/en/latest/mfa/introduction.html

@humitos humitos removed the Priority: low Low priority label May 21, 2024
@humitos
Copy link
Member

humitos commented May 21, 2024

We talk about supporting this during the offsite if it wasn't super complicated. I'm adding this issue to 2024Q3 to see if we can prioritize it.

@humitos humitos changed the title Support two factor auth Support two factor auth (2FA) May 21, 2024
@ericholscher
Copy link
Member

Yea, now that allauth supports it, would be great to add 👍

@ericholscher
Copy link
Member

Putting this on our next sprint, as this is now supported in AllAuth, hopefully this will be pretty straightforward 🙏

@stsewd
Copy link
Member

stsewd commented Aug 1, 2024

Well, it was easy to integrate. Just install a new dependency django-allauth[mfa], and install the app.

Things to take into consideration:

  • We probably want to override all the templates related to mfa, the default ones don't look great with our theme.
  • I see in the code that the current hostname is used for some things, we should check that the auth works on both domains (rtd.org/com and app.rtd.org/com), or do some overrides. Edit: This is used for webauthn only.
  • The name of the site is taken from the Site object, we should make sure it says Read the Docs or Read the Docs Community or whatever.
  • The recovery codes are not encrypted by default, we need to implement that for ourselves. See encrypt/decrypt in https://docs.allauth.org/en/latest/mfa/adapter.html. I think it makes sense to encrypt the recovery codes. Update: Looks like what they give you to encrypt is a seed, the codes themselves are generated from that seed, so, it could be useful to encrypt that too.

@stsewd stsewd moved this from Planned to In progress in 📍Roadmap Aug 1, 2024
@ericholscher
Copy link
Member

It sounds like overall the main work is templating then and some QA? I assume the templates should be pretty simple, so hopefully not a ton of work to theme.

The name of the site is taken from the Site object, we should make sure it says Read the Docs or Read the Docs Community or whatever.

I confirmed these

@stsewd
Copy link
Member

stsewd commented Aug 1, 2024

Yep, if we are cool with that, I can open a PR with the changes.

stsewd added a commit that referenced this issue Aug 1, 2024
@stsewd stsewd mentioned this issue Aug 7, 2024
@stsewd stsewd closed this as completed in f808dcd Aug 22, 2024
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Aug 22, 2024
@stsewd
Copy link
Member

stsewd commented Aug 22, 2024

Note: support for 2FA has been added, but it won't be exposed to users yet, we will be exposing this feature in the new dashboard (once it's ready).

@agjohnson
Copy link
Contributor Author

@stsewd Could you give the URL for the mfa settings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

No branches or pull requests

6 participants