Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#17869 closed Bug (fixed)

With RemoteUserMiddleware, users keep being logged in after web server stops sending REMOTE_USER headers

Reported by: Chris Lamb Owned by: Preston Holmes
Component: contrib.auth Version: dev
Severity: Release blocker Keywords:
Cc: enrico@…, krzysiumed@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

(Forwarded from http://bugs.debian.org/663230)

This was reproduced on 1.2.3-3+squeeze2 but the RemoteUserMiddleware code seems to be the same as the 1.3.1-4 in my development machine.

RemoteUserMiddleware relies on a REMOTE_USER variable to be set by the web server with the current user name, so far so good. However it does not log a person out if the variable disappears during the same browser session.

That may never happen with the usual browsers and auth, but it does happen for other setups like DACS that have a logout feature button.

The error is in this bit of django.contrib.auth.middleware.RemoteUserMiddleware:

        try:
            username = request.META[self.header]
        except KeyError:
            # If specified header doesn't exist then return (leaving
            # request.user set to AnonymousUser by the
            # AuthenticationMiddleware).
            return

The except side assumes that if there is no request.META[self.header],
then the user is the anonymous one.

Since I found that it is not always the case, I fixed it adding a simple
"auth.logout(request)" before returning:

        try:
            username = request.META[self.header]
        except KeyError:
            # If specified header doesn't exist then return (leaving
            # request.user set to AnonymousUser by the
            # AuthenticationMiddleware).

	    # Make sure that if the server did not send any headers,
	    # then we are actually logged out
            auth.logout(request)
            return

Attachments (1)

17869.diff (678 bytes ) - added by Christopher Medrela 12 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Chris Lamb, 12 years ago

The fix which worked for Enrico is not perfect as calling auth.logout will flush the session, this would break sessions for logged-out users as they are cleared every request.

What we need to do instead is to check that we think the user "should" be logged in (SESSION_KEY in request.session?) and then make a call to logout.

comment:2 by Enrico Zini, 12 years ago

Cc: enrico@… added

comment:3 by Enrico Zini, 12 years ago

@lamby: good point. I've now changed the site code to only log out if the user is authenticated:

              if request.user.is_authenticated():
                  auth.logout(request)

comment:4 by Christopher Medrela, 12 years ago

Component: Uncategorizedcontrib.auth
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:5 by Christopher Medrela, 12 years ago

I attached the file including enrico's solution, but I can't say if the solution works properly.

by Christopher Medrela, 12 years ago

Attachment: 17869.diff added

comment:6 by Christopher Medrela, 12 years ago

Cc: krzysiumed@… added
Easy pickings: set
Has patch: set

comment:7 by Claude Paroz, 12 years ago

Needs documentation: set
Needs tests: set

comment:8 by bouchardsyl, 12 years ago

Owner: changed from nobody to bouchardsyl
Status: newassigned

comment:9 by bouchardsyl, 12 years ago

Needs documentation: unset
Needs tests: unset

Added a test and documentation.
Please review, see pull request https://github.com/django/django/pull/134

comment:10 by bouchardsyl, 12 years ago

Resolution: fixed
Status: assignedclosed
Triage Stage: AcceptedUnreviewed

comment:11 by bouchardsyl, 12 years ago

PS : please ignore patch in Trac. Everything is in the pull request.

comment:12 by Jannis Leidel, 12 years ago

Resolution: fixed
Severity: NormalRelease blocker
Status: closedreopened
Triage Stage: UnreviewedAccepted

Please don't close tickets if there hasn't been a commit yet. Accepting based on a discussion I had with the OP during DJangoCon and talk with Paul.

comment:13 by bouchardsyl, 12 years ago

After 3 months, I just stumbled upon my un-pulled commits regarding this issue.
Time and commits have passed since, so I made a brand new pull request that works with today's fork of Django (Sept 9, 2012)
Please see https://github.com/django/django/pull/365

comment:14 by Preston Holmes, 11 years ago

Owner: changed from bouchardsyl to Preston Holmes
Status: reopenednew
Version: 1.3master

OK - I'm going to get this ready to commit - I'm adding a check to verify the backend used to authenticate the user.

The docs are vague about it - but I don't see any reason why ModelBackend and RemoteUserBackend can't be able to co-exist.

So I may add one more test that verifies that only users with the matching backend are logged out.

https://github.com/ptone/django/compare/ticket/17869-remoteuser-auth

comment:15 by Aymeric Augustin, 11 years ago

It would be better to use auth.BACKEND_SESSION_KEY, as in clean_username, than to redefine BACKEND_SESSION_KEY in this module.

Otherwise, this looks good to me.

comment:16 by Preston Holmes <preston@…>, 11 years ago

Resolution: fixed
Status: newclosed

In e8269a6729150d26a5497d5eb51226ca19fa21b9:

[1.5.x] Fixed #17869 - force logout when REMOTE_USER header disappears

If the current sessions user was logged in via a remote user backend log out
the user if REMOTE_USER header not available - otherwise leave it to other auth
middleware to install the AnonymousUser.

Thanks to Sylvain Bouchard for the initial patch and ticket maintenance.

comment:17 by Preston Holmes <preston@…>, 11 years ago

In 9741912a9aaad083eaa8b9780cde37e1843cc4ae:

Fixed #17869 - force logout when REMOTE_USER header disappears

If the current sessions user was logged in via a remote user backend log out
the user if REMOTE_USER header not available - otherwise leave it to other auth
middleware to install the AnonymousUser.

Thanks to Sylvain Bouchard for the initial patch and ticket maintenance.

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