Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#25030 closed New feature (wontfix)

The /admin/login/ should observe external authentication even when it appears in POST

Reported by: Jan Pazdziora Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Apache module mod_intercept_form_submit (http://www.adelton.com/apache/mod_intercept_form_submit/) allows PAM authentication to be run by Apache when application's native logon form is submitted. The module attempts the PAM authentication and sets r->user / REMOTE_USER accordingly. The use case is described in more detail at http://www.freeipa.org/page/Web_App_Authentication#Login_form_using_FreeIPA and http://www.freeipa.org/page/Web_App_Authentication/Example_setup#External_identities_for_login_form.

However, Django's /admin/login/ implementation has check

if request.method == 'GET' and self.has_permission(request):

in its login method. So even if I have Apache configured with

LoadModule authnz_pam_module modules/mod_authnz_pam.so
LoadModule intercept_form_submit_module modules/mod_intercept_form_submit.so
<Location /admin/login/>
InterceptFormPAMService django-admin
InterceptFormLogin username
InterceptFormPassword password
</Location>

and in access_log I see the admin user authenticated by the module, since it happened during POST request, /admin/login/ ignores the fact that self.has_permission(request) returns true and prints error message "Please enter the correct username and password for a staff account. Note that both fields may be case-sensitive." However, the session based on REMOTE_USER has actually been created so if you just repeat the same request (http://www.example.com/admin/login/?next=/admin/) with GET by hitting Ctrl+L and Enter, you will get to /admin/ without issues.

Change History (10)

comment:1 Changed 5 years ago by Jan Pazdziora

Has patch: set

comment:2 Changed 5 years ago by Claude Paroz

Component: contrib.admincontrib.auth
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

Regarding the current test failure for the patch, you are right about ticket #19327 not being an issue any more with the new design.

However, your patch is still slightly changing the "overriden-login" behavior. Currently if I have two windows/tabs with a login form and I try to login with two different users, the second POST will log the second user which becomes then the current session user. With your patch, the second POST will redirect to admin index while the first user is still the current session user (already logged-in behavior). I understand that this is an edge use case, as normally users logout before logging in as a different user.

Your current proposal is only addressing the admin login use case, isn't it? I'd be in favor of solving it directly in contrib.auth so every login would benefit from it. We might find a way to short-circuit the contrib.auth login view when request.user is already authenticated and user.backend point to RemoteUserBackend or a subclass. Thoughts?

comment:3 in reply to:  2 Changed 5 years ago by Jan Pazdziora

Replying to claudep:

Regarding the current test failure for the patch, you are right about ticket #19327 not being an issue any more with the new design.

Thanks for the confirmation.

However, your patch is still slightly changing the "overriden-login" behavior. Currently if I have two windows/tabs with a login form and I try to login with two different users, the second POST will log the second user which becomes then the current session user. With your patch, the second POST will redirect to admin index while the first user is still the current session user (already logged-in behavior). I understand that this is an edge use case, as normally users logout before logging in as a different user.

I understand the problem. However, checking the situation you describe (open two tabs with unauthenticated /admin/login/, get two forms, submit the first, submit the second with different username), I get

Forbidden (403)
CSRF verification failed. Request aborted.

So it looks like lately this scenario is not really possible, at least with the django.middleware.csrf.CsrfViewMiddleware which seems enabled by default.

I wonder if we have any way to figure out that the user was just authenticated (their session marked as authenticated) in the current request, presumably by some middlerware, to distinguish from the situation when we piggybacked on some other long-running session.

Your current proposal is only addressing the admin login use case, isn't it? I'd be in favor of solving it directly in contrib.auth so every login would benefit from it.

Yes, I only looked at admin. I will need to dig deeper into contrib.auth to understand the interactions there.

We might find a way to short-circuit the contrib.auth login view when request.user is already authenticated and user.backend point to RemoteUserBackend or a subclass. Thoughts?

That would certainly allow us to target the use case that I have in mind most precisely. But I'm afraid that it might break the logic separation, if we started to poke into users' backends directly -- it feels like something that the application should not really care about.

comment:4 Changed 5 years ago by Jan Pazdziora

I'd appreciate any guidance / preference for further approach. One possibility is to be very explicit and turn POST to GET when the user is already authenticated and thus do it outside of the admin application:

class InterceptionRemoteUserMiddleware(object):
    def process_request(self, request):
        if request.META["PATH_INFO"] == u'/admin/login/' and hasattr(request, 'user') and request.user.is_authenticated():
            request.method = "GET"

However, hardcoding the /admin/login certainly is not nice.

comment:5 in reply to:  2 Changed 5 years ago by Jan Pazdziora

Replying to claudep:

We might find a way to short-circuit the contrib.auth login view when request.user is already authenticated and user.backend point to RemoteUserBackend or a subclass. Thoughts?

I've now updated https://github.com/django/django/pull/4925 with a497f111cf959b23163c030745e4cb18be7eb0a9 where I shortcut when the user is authenticated.

I'm not sure about the RemoteUserBackend check though -- it seems to be a little bit out of place in django/contrib/auth/views.py, singling out that particular backend. Is there any reason why any authenticated request.user couldn't be taken at face value?

Last edited 5 years ago by Jan Pazdziora (previous) (diff)

comment:6 Changed 5 years ago by Carl Meyer

This use case seems highly specialized to me; we should make it possible, but it doesn't need to work out of the box and certainly doesn't provide any justification for wide-ranging changes in the design of contrib.auth.

Allowing login via GET request seems like a recipe for login CSRF attacks, so I don't think the admin should generally be modified to allow that.

Can you just configure Apache to redirect after it intercepts a login? That seems like the most straightforward approach here.

comment:7 Changed 5 years ago by Carl Meyer

Sorry, ignore the comment about login via GET and login CSRF; I was misunderstanding the nature of the GET vs POST issue.

Last edited 5 years ago by Carl Meyer (previous) (diff)

comment:8 Changed 5 years ago by Carl Meyer

It looks to me like this could also be addressed by using a custom AdminSite, overriding the login method, and doing a non-method-specific check for if the user is authenticated in the wrapper.

I don't think the method check should be removed from the default implementation. An incoming POST should be treated as a login attempt, not redirected if the user is already authenticated.

The current implementation handles the by far most common case correctly out of the box, and makes it possible to meet unusual requirements with a bit of custom code. That's the most that can be asked of a framework.

I think this ticket should be closed with no changes to Django.

comment:9 Changed 5 years ago by Tim Graham

Resolution: wontfix
Status: newclosed

Feel free to reopen with other ideas.

comment:10 Changed 2 years ago by Jan Pazdziora

For the record, mod_intercept_form_submit in version 1.0.1 added configuration directive InterceptGETOnSuccess to force the method in the request to be shown (and reported to Django) as GET. So this change is no longer needed.

Note: See TracTickets for help on using tickets.
Back to Top