Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#17869 closed Bug (fixed)

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

Reported by: lamby Owned by: ptone
Component: contrib.auth Version: master
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 krzysiumed 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by lamby

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 3 years ago by enrico

  • Cc enrico@… added

comment:3 Changed 3 years ago by enrico

@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 Changed 3 years ago by krzysiumed

  • Component changed from Uncategorized to contrib.auth
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:5 Changed 3 years ago by krzysiumed

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

Changed 3 years ago by krzysiumed

comment:6 Changed 3 years ago by krzysiumed

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

comment:7 Changed 3 years ago by claudep

  • Needs documentation set
  • Needs tests set

comment:8 Changed 3 years ago by bouchardsyl

  • Owner changed from nobody to bouchardsyl
  • Status changed from new to assigned

comment:9 Changed 3 years ago by bouchardsyl

  • 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 Changed 3 years ago by bouchardsyl

  • Resolution set to fixed
  • Status changed from assigned to closed
  • Triage Stage changed from Accepted to Unreviewed

comment:11 Changed 3 years ago by bouchardsyl

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

comment:12 Changed 3 years ago by jezdez

  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by bouchardsyl

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 Changed 2 years ago by ptone

  • Owner changed from bouchardsyl to ptone
  • Status changed from reopened to new
  • Version changed from 1.3 to master

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 Changed 2 years ago by aaugustin

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 Changed 2 years ago by Preston Holmes <preston@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 2 years ago by Preston Holmes <preston@…>

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