jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.
MIT License
1.67k stars 445 forks source link

Explain 2FA requirement if not configured in admin site #219

Open nicomak opened 7 years ago

nicomak commented 7 years ago

Hi, I followed the example site and managed to get the "secret" page working with 2FA setup on my website.

However, to protect the admin site, I found this ReadTheDocs article, but it doesn't seem to work. Is this procedure still the current way to make 2FA work on the admin site ? Using this, when I go to the admin site there is directly a username/password form (instead of asking to activate 2FA like in the "secret" page). And the login form doesn't work, it keeps re-showing the same form even if the credentials are correct.

This is my url config:

from two_factor.gateways.twilio.urls import urlpatterns as tf_twilio_urls
from two_factor.urls import urlpatterns as tf_urls

admin.site.__class__ = AdminSiteOTPRequired
admin.autodiscover()

urlpatterns = [    
    url(r'^secret/$', ExampleSecretView.as_view(), name="secret"),
    url(r"^admin/", include(admin.site.urls)),
    url(r'', include(tf_urls + tf_twilio_urls, "two_factor")),
]

Did I do something wrong ? Any help would be greatly appreciated, thanks !

Bouke commented 7 years ago

The admin configuration is two-fold;

  1. the login view is automatically monkey patched to use this package's login view
  2. AdminSiteOTPRequired requires that all users accessing the page have been authenticated using two factors

And the login form doesn't work, it keeps re-showing the same form even if the credentials are correct.

I'm assuming your user doesn't have a second factor enabled. At the moment I don't think an error is shown. It would be nice to inform you that although your credentials are correct, you cannot proceed as you don't have a second factor. Instead it is redirecting you to the login page, making you think that the login page is broken.

skyl commented 6 years ago

Even better, if you're trying to go to a otp-requiring page, wouldn't it make sense to redirect to the wizard to setup 2fa?

JVanloofsvelt commented 6 years ago

Agreed @Bouke , this confused me a lot too! I feel like it should allow the login to succeed, and then allow 2FA to be set up once you're logged in.

rjskene commented 6 years ago

i must admit to being stumped by this. How do you set 2FA on the admin users if you can't login to them?

GaramNick commented 5 years ago

If you update the login in the AdminSiteOTPRequiredMixin to this, it redirects to the setup:

def login(self, request, extra_context=None):
    redirect_to = request.POST.get(REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME))
    if request.method == "GET" and super(AdminSiteOTPRequiredMixinRedirSetup, self).has_permission(request):
            # Already logged-in
            if request.user.is_verified():
                # User has permission
                index_path = reverse("admin:index", current_app=self.name)
            else:
                # User has permission but no OTP set:
                index_path = reverse("two_factor:setup", current_app=self.name)
            return HttpResponseRedirect(index_path)

        if not redirect_to or not is_safe_url(url=redirect_to, allowed_hosts=[request.get_host()]):
            redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)

        return redirect_to_login(redirect_to)
saschwarz commented 5 years ago

Thanks @GaramNick for your snippet! I ended up putting your idea into a subclass and had to adjust the super() call to has_permission(request) so the default admin version would be called:

class AdminSiteOTPRequiredMixinRedirSetup(AdminSiteOTPRequired):
    def login(self, request, extra_context=None):
        redirect_to = request.POST.get(
            REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME)
        )
        # For users not yet verified the AdminSiteOTPRequired.has_permission
        # will fail. So use the standard admin has_permission check:
        # (is_active and is_staff) and then check for verification.
        # Go to index if they pass, otherwise make them setup OTP device.
        if request.method == "GET" and super(
            AdminSiteOTPRequiredMixin, self
        ).has_permission(request):
            # Already logged-in and verified by OTP
            if request.user.is_verified():
                # User has permission
                index_path = reverse("admin:index", current_app=self.name)
            else:
                # User has permission but no OTP set:
                index_path = reverse("two_factor:setup", current_app=self.name)
            return HttpResponseRedirect(index_path)

        if not redirect_to or not is_safe_url(
            url=redirect_to, allowed_hosts=[request.get_host()]
        ):
            redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)

        return redirect_to_login(redirect_to)

Then:

admin.site.__class__ = AdminSiteOTPRequiredMixinRedirSetup
aseem-hegshetye commented 4 years ago

Any plans on including this in django-two-factor-auth package so we dont have manually do this in each project ?

moggers87 commented 4 years ago

Pull requests welcome :smiley_cat:

aseem-hegshetye commented 4 years ago

Pull request

limolitz commented 2 years ago

Any chance to get this pull request merged?

RyanHope commented 1 year ago

Yes this seems like the way things should be by default.

rob32 commented 9 months ago

thanks @saschwarz and @GaramNick! That helped enormously.

Here is my snippet with imports etc. (is_safe_url is now called url_has_allowed_host_and_scheme)

from django.contrib import admin
from django.contrib.auth import REDIRECT_FIELD_NAME
from django.contrib.auth.views import redirect_to_login
from django.http import HttpResponseRedirect
from django.shortcuts import resolve_url
from django.urls import include, path, reverse
from django.utils.http import url_has_allowed_host_and_scheme

from two_factor.admin import AdminSiteOTPRequired, AdminSiteOTPRequiredMixin
from two_factor.urls import urlpatterns as tf_urls

class CustomAdminSiteOTPRequired(AdminSiteOTPRequired):
    def login(self, request, extra_context=None):
        redirect_to = request.POST.get(
            REDIRECT_FIELD_NAME, request.GET.get(REDIRECT_FIELD_NAME)
        )
        if request.method == "GET" and super(
            AdminSiteOTPRequiredMixin, self
        ).has_permission(request):
            if request.user.is_verified():
                index_path = reverse("admin:index", current_app=self.name)
            else:
                index_path = reverse("two_factor:setup", current_app=self.name)
            return HttpResponseRedirect(index_path)

        if not redirect_to or not url_has_allowed_host_and_scheme(
            url=redirect_to, allowed_hosts=[request.get_host()]
        ):
            redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)

        return redirect_to_login(redirect_to)

admin.site.__class__ = CustomAdminSiteOTPRequired

urlpatterns = [
    # ...
    path("", include(tf_urls)),
    path("admin/", admin.site.urls),
]
andrelccorrea-blinctek commented 6 months ago

The previous comment has the solution. The way the lib currently works is extremely confusing. What is missing for this solution to be implemented as standard?