[Django] #25030: The /admin/login/ should observe external authentication even when it appears in POST
Django
appears in POST
-------------------------------+--------------------
Reporter: adelton | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adelton):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
Proposed patch at https://github.com/django/django/pull/4925.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:1>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* component: contrib.admin => contrib.auth
* needs_better_patch: 0 => 1
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:2>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adelton):
Replying to [comment:2 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:3>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adelton):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:4>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adelton):
Replying to [comment:2 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 when 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:5>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:6>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by carljm):
Sorry, ignore the comment about login via GET and login CSRF; I was
misunderstanding the nature of the GET bs POST issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:7>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:8>
Django
appears in POST
Reporter: adelton | Owner: nobody
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by timgraham):
* status: new => closed
* resolution: => wontfix
Comment:
Feel free to reopen with other ideas.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:9>
Django
appears in POST
Reporter: Jan Pazdziora | Owner: nobody
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25030#comment:10>