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

[Jan Pazdziora]

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.

[Jan Pazdziora]

Proposed patch at ​https://github.com/django/django/pull/4925.

[Claude Paroz]

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?

[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.

[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.

[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 could be taken at face value?

[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.

[Carl Meyer]

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

[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.

[Tim Graham]

Feel free to reopen with other ideas.

[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.