Opened 10 years ago

Closed 6 years ago

Last modified 4 years ago

#689 closed enhancement (fixed)

Honor Web server provided authentication

Reported by: Ian@… Owned by: gwilson
Component: Core (Other) Version: master
Severity: normal Keywords:
Cc: telenieko@…, hozer@…, koen.biermans@…, hugh@…, ericvw@…, grf@…, hinnerk@…, me@…, zilingzhao@…, dvd@…, erik.engbrecht@…, david Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by gwilson)

I would like to see a 2nd mod_python handler. one which takes the REMOTE_USER parameter passed to it and for it to use that as the user-id so that I can hook my app into the default intranet security system we use over here.

here is the patch to the original to make that the case.

NOTE: it creates a user-record if none exist for the user.

Helios:/src/django_newadmin/django/core/handlers ianh$ diff 
modpython_apacheauth.py modpython.py
102,103c102
<                 #user_id = self.session[users.SESSION_KEY]
<                 user_id = self._req.user
---
>                 user_id = self.session[users.SESSION_KEY]
106,117c105,106
<                 try:
<                     self._user = users.get_object(username__exact=user_id)
<                 except (users.UserDoesNotExist):
<                     from django.models.auth import User
<                     import md5
<                     import datetime
<                     password_md5 = md5.new('fake').hexdigest()
<                     now = datetime.datetime.now()
<                     self._user = User(None, user_id,'','', user_id+'@cnet.com',password_md5,False,True,False,now,now)
<                     self._user.save()
<             #except (AttributeError, KeyError, ValueError, users.UserDoesNotExist):
<             except (AttributeError, KeyError, ValueError):
---
>                 self._user = users.get_object(pk=user_id)
>             except (AttributeError, KeyError, ValueError, users.UserDoesNotExist):
120d108
< 

Attachments (19)

modpython.patch (1.0 KB) - added by ian@… 10 years ago.
Patch to existing mod_python handler
httpauth.py (1.2 KB) - added by ian@… 10 years ago.
middleware version of same idea
httpdauth.py (1.5 KB) - added by garthk 10 years ago.
Slight revision of Ian's code
httpdauth.2.py (2.8 KB) - added by garthk 10 years ago.
Revision of Ian's module; new version moves settings to the settings module
remote_user.diff (6.2 KB) - added by Koen Biermans <koen.biermans@…> 8 years ago.
patch using both middleware and authentication backend (doc included)
remote_user_2.diff (6.7 KB) - added by Koen Biermans <koen.biermans@…> 8 years ago.
Modified version without settings
689_full.diff (9.9 KB) - added by telenieko 8 years ago.
the code, the docs, the tests; And it works!
689_full-2.diff (9.9 KB) - added by ekarulf 8 years ago.
Changed 689_full to use unusable password
689_full-3.diff (13.7 KB) - added by ramiro 8 years ago.
New version of the patch, incoporates ideas from several people for RemoteUserAuthBackend implementation
689_nojavaversion.diff (12.7 KB) - added by telenieko 8 years ago.
New version (aka. The Non Java One)
t689-r7350.diff (9.9 KB) - added by ramiro 7 years ago.
689.diff (8.0 KB) - added by guettli 7 years ago.
t689-r7609.diff (8.4 KB) - added by ramiro 7 years ago.
Patch updated to apply cleanly to r7609 incorporating fixes and suggestions by Thomas Guettler plus additional fixes and docs content
t689-r7823.diff (8.1 KB) - added by ramiro 7 years ago.
t689-r9099.diff (9.3 KB) - added by ramiro 7 years ago.
Patch updated to apply to trunk as of r9099: Edited and expanded documentation, fixed tests
689-r9733.diff (10.1 KB) - added by telenieko 7 years ago.
Updated patch
689.2.diff (10.0 KB) - added by telenieko 7 years ago.
New patch with Gary's comments
689.3.diff (20.3 KB) - added by gwilson 6 years ago.
689.4.diff (20.6 KB) - added by gwilson 6 years ago.
add to docs paragraph about custom header value

Download all attachments as: .zip

Change History (73)

Changed 10 years ago by ian@…

Patch to existing mod_python handler

comment:1 Changed 10 years ago by sune.kirkeby@…

Why don't you just write a middleware? That is a lot more appropriate than another handler.

(Oh, and in the future, please create unified diffs (the -u flag), they are much more readable.)

comment:2 Changed 10 years ago by hugo

Yep, a middleware that just searches for the user object based on the REMOTE_USER and stores that in request.user should be simpler, I think.

Changed 10 years ago by ian@…

middleware version of same idea

comment:3 Changed 10 years ago by ian@…

I've attached a middleware version of the same process.
I'd be happy if this went into the 'contrib' (or general middleware) section of django

comment:4 Changed 10 years ago by anonymous

  • Summary changed from allow apache to provide authentication instead of django to [patch] middleware to allow apache to provide authentication instead of django

comment:5 Changed 10 years ago by garthk

If you're using mod_auth_kerb to authenticate your users against Active Directory, REMOTE_USER will be in the format userid@REALM. My attached variant on Ian's middleware code strips out the @, copes with REMOTE_USER being missing, and only adjusts request.user if necessary.

Changed 10 years ago by garthk

Slight revision of Ian's code

Changed 10 years ago by garthk

Revision of Ian's module; new version moves settings to the settings module

comment:6 Changed 10 years ago by adrian

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

Superceded by MultipleAuthBackends page.

comment:7 Changed 9 years ago by Pawel J. Sawicki (http://pjs.name)

If I may add - IMHO this particular enhacement cannot be obtained using the new authentication model. The middleware approach is IMO the only way to get this right.

The basic assumption of the http auth is that you log in into the system only once - using the web browser capabilities. Afterwards you should be automaticaly logged in into the system (or at least verified if you can log in).

The new model (http://www.djangoproject.com/documentation/authentication/) is (as far as I know) only able to verify the credentials after they are provided to a login method - at least I couldn't obtain nothing more. So, in my situation, even though I've been logged in "through http" I also had to provide my username and password in the login form.

Moreover - even if I always returned User.objects.get(id=1) inside authenticate() I had to press submit button or the framework left me unauthorized.

As for the "lack of logout" - I think I've managed to deal with that by

class HttpAuthMiddleware(object):
    def process_request(self, request):
        if request.path == '/accounts/logout/':
            httpResponse = HttpResponse()
            httpResponse.status_code = 401
            httpResponse.headers['WWW-Authenticate'] = 'Basic realm="<same realm as in the http server>"'
            return httpResponse
        else:
            # check for http headers - maybe the user is already logged in using http auth
            ...

Yes, I know - there's a lot of hardcoding, but it works. So, in a sense, the arguments presented in http://www.djangoproject.com/weblog/2005/oct/24/http/ are no longer valid - you *can* logout a user, even if she or he is logged using http auth (inspired by comment made by "Aaron").

...or maybe the docs need some more examples :)

comment:8 Changed 9 years ago by Marc Fargas <telenieko@…>

  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Summary changed from [patch] middleware to allow apache to provide authentication instead of django to Honour web-server provided authentication

Would be nice if Django honoured REMOTE_USER, specially for the kerberos case so one can have multiple django sites with single sign-on (that is, when you signed in your workstation).

Could that be possible that Django honoured REMOTE_USER leaving authentication to the web server? (that is, apache sends the 401, apache required a user name, apache checks it, and passed it to django). That would be very very nice :)

I saw the ticket is closed as duplicate, but the patch is about to honour apache's authentication efforts (so, respecting REMOTE_USER), the reason for the duplicate is that MultipleAuthBackends superceeds that. I don't think it's true, Multiple Auth Backends allow you to provide other authentication mechanisms to Django, but do not allow you to make django honour REMOTE_USER (as the backend is called from a login page).

So... maybe I get it wrong, but the ticket should be open or wontfix.

Cheers,
Marc.

comment:9 Changed 9 years ago by Marc Fargas <telenieko@…>

  • Cc telenieko@… added

comment:10 Changed 9 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:11 Changed 8 years ago by anonymous

  • Cc hozer@… added

I'm quite interesting in Django understanding REMOTE_USER provided by apache, as this seems to be my best chance for having a reasonable openid server implementation (using djangoid) that can backend to a kerberos-single sign-on system.

comment:12 Changed 8 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

This middleware would be appropriate as an optional part of django.contrib.auth, but has some problems (as listed in the patch).

Changed 8 years ago by Koen Biermans <koen.biermans@…>

patch using both middleware and authentication backend (doc included)

Changed 8 years ago by Koen Biermans <koen.biermans@…>

Modified version without settings

comment:13 Changed 8 years ago by Koen Biermans <koen.biermans@…>

  • Cc koen.biermans@… added
  • Has patch set

The patch uses both middleware and auth backend. This solves the session problems I think. By default it auto-creates new users without any extra info, but you can easily override this by subclassing the backend (see the doc for examples).
An example configuration for apache using mod_auth_sspi (preferred module on Windows) is in the doc.
I have only tested this like this, I have no info about mod_ntlm (best used on Linux).
Don't know how to write tests for it though.

comment:14 Changed 8 years ago by telenieko

  • Needs tests set

Changed 8 years ago by telenieko

the code, the docs, the tests; And it works!

comment:15 Changed 8 years ago by telenieko

  • Needs tests unset
  • Patch needs improvement unset

New patch attached:

  • Removed Apache specific documentation from the previous patch (maybe people want to use this with IIS!) and left only Django specific parts of it.
  • The provied backend cannot be used "as-is" to make sure people know what they're doing (or atleast that they read the documentation).
  • Added tests to make sure this works and does not break anything else.

For some reason TRAC won't show my nice "git made" diffs (you'll have to download it to see it!) I think TRAC is too clever and does not want SmileyChris to view my patches without testing them!! see that :P

comment:16 follow-up: Changed 8 years ago by ramiro

I had some unsubmitted work on this ticket with some similar documentation generalization/fixes and with fixes to some Python errors, but your English is far better than mine and you've added tests :-). Also, some time ago I asked for some refactoring guidance on -dev in lieu of [6375]: http://groups.google.com/group/django-developers/browse_thread/thread/44104954ebaa219a/4fd066030cf23bfe?hl=en&lnk=gst#4fd066030cf23bfe
because I intended to attach an enhanced patch to this ticket.

I asked again today on #django-sprint and Florian Apolloner ([6357] initial author) suggested the following directions:

<apollo13> cramm: Final statement, all together :) I would change the patch to a) not inherit from ModelBackend b) don't
implement the auth methods c) implement authenticate and get_user d) return False if password is not None

He explained d) is security-related. It makes sure no user can login through the usual Django way as admin without entering a password en the event the web server configuration is (accidentally) changed to not send the REMOTE_USER envvar. I've tried to implemented what I've interpreted from his ideas:

http://ikki.seeonee.rmorales.net/cgi-bin/hgwebdir.cgi/t689/rev/3bf31ada8526

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by telenieko

  • Patch needs improvement set

Replying to ramiro:

<apollo13> cramm: Final statement, all together :) I would change the patch to a) not inherit from ModelBackend b) don't
implement the auth methods c) implement authenticate and get_user d) return False if password is not None

Ok, I didn't see that it was inheritting ModelBackend! ;)

He explained d) is security-related. It makes sure no user can login through the usual Django way as admin without entering a password en the event the web server configuration is (accidentally) changed to not send the REMOTE_USER envvar.

I feel it fine.

The only think I do not get clearly is what does "b" mean. I'll take a round on #django-sprint to ask apollo13 myself.

I visited you hgweb, will you post the updated patch youself? If not I'll do it later, I use git anyway :=)

comment:18 in reply to: ↑ 17 Changed 8 years ago by apollo13

Replying to telenieko:

The only think I do not get clearly is what does "b" mean. I'll take a round on #django-sprint to ask apollo13 myself.

B should say: don't implement the get_perm methods (as this backend doesn't deal with them). Don't ask me why I wrote auth...

Changed 8 years ago by ekarulf

Changed 689_full to use unusable password

Changed 8 years ago by ramiro

New version of the patch, incoporates ideas from several people for RemoteUserAuthBackend implementation

comment:19 Changed 8 years ago by ramiro

689_full-3.diff contains:

  • Documentation and tests implemented by telenieko
  • Modifications to RemoteUserAuthBackend class to
    • Not inherit from ModelBackend but expose a minimal, similar enough interface
    • Security: Don't allow login as any user w/o password through normal channel when webserver/middleware aren't configured to process auth and to set REMOTE_USER/request.user

Implemented after dicussion with apollo13 and ekarulf on #django-sprint. History available at http://ikki.seeonee.rmorales.net/cgi-bin/hgwebdir.cgi/t689-final/

comment:20 Changed 8 years ago by ramiro

  • Owner changed from nobody to ramiro
  • Status changed from reopened to new

comment:21 Changed 8 years ago by telenieko

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

I think it's ready for checkin now. It passes all the tests and works :)))

comment:22 follow-up: Changed 8 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

teleniko: please don't mark tickets ready for checkin without proper review by a ticket triager or a committer. I'll try to provide a complete review, but for now I've got one question: why the forced subclassing of RemoteUserAuthBackend? That strikes me as pointless busywork; this isn't Java!

comment:23 in reply to: ↑ 22 Changed 8 years ago by telenieko

Replying to jacob:

teleniko: please don't mark tickets ready for checkin without proper review by a ticket triager or a committer.

Sorry

I'll try to provide a complete review, but for now I've got one question: why the forced subclassing of RemoteUserAuthBackend? That strikes me as pointless busywork; this isn't Java!

I was afraid about security, the class given assumes that any user given in REMOTE_USER is valid, so making it "not just works" forcing people to take a look at the documentation would make them be aware that this class doesn't do any kind of validation on REMOTE_USER.

Anyway, the functionality of the patch is for some specific needs so maybe the people that are likely to use this are already aware of this problem so removing the RemoteUserAuthBackend.init() method would be fine (and updating the docs accordingly!), I'll post a patch later with this change so you can decide wich one you prefer! ;)

Changed 8 years ago by telenieko

New version (aka. The Non Java One)

comment:24 Changed 8 years ago by telenieko

  • Owner changed from ramiro to telenieko
  • Patch needs improvement unset
  • Status changed from new to assigned

Here's the new patch as promised! RemoteUserAuthBackend can be used "as-is" now, tests and docs updated accordingly.
Looking at it now maybe it makes more sense this way :)

Hope it's ok!

comment:25 Changed 7 years ago by ramiro

  • Version set to SVN

I'm attaching a new revision of the patch that applies cleanly to r7350 and fixing the django.contrib.auth tests so both the new unittest-based tests and the previously existing doctests are run (in previous patch revision only the first ones were getting exercised).

Changed 7 years ago by ramiro

comment:26 Changed 7 years ago by guettli

  • Cc hv@… added

Changed 7 years ago by guettli

comment:27 follow-up: Changed 7 years ago by guettli

I uploaded a new patch. Changes:

  • Removed old documentation. Subclassing is not necessary any more.
  • Typo: fur*th*er
  • removed documentation in request_response. REMOTE_USER is not available, except you added this middleware.

Somehow Trac does not display this patch, although it was created with 'svn diff'.

comment:28 in reply to: ↑ 27 Changed 7 years ago by ramiro

guettli,

Replying to guettli:

I uploaded a new patch. Changes:

from a quick reviewof your patch it seems the only change you implemented
in relation to t689-r7350.diff is:

  • removed documentation in request_response. REMOTE_USER is not available, except you added this middleware.

Could you confirm this? (interdiff isn't being of great help)

comment:29 Changed 7 years ago by guettli

I compared old diff and mine (689.diff) with ediff-buffers in emacs.

Changes to t689-r7350.diff (old)

django/contrib/auth/backends.py: Old patch contained not needed whitespace change.

docs/auth_remote_user.txt: furder --> further

docs/auth_remote_user.txt: removed

After this, you'll have to create you authentication backend that will take
care of checking that ``REMOTE_USER`` is valid. But don't be scared,...

docs/request_response.txt: removed REMOTE_USER since it is only available if
you install this middleware. I guess 99% won't do this. And if someone does it,
he knows that already.

comment:30 Changed 7 years ago by guettli

Hi ramiro,

Please add a sentence to configure_user (in auth_remote_user.txt and in the docstring),
that this method will only be called if the user does not already exist in the
django database.

Thank you

Changed 7 years ago by ramiro

Patch updated to apply cleanly to r7609 incorporating fixes and suggestions by Thomas Guettler plus additional fixes and docs content

comment:31 Changed 7 years ago by telenieko

  • Triage Stage changed from Accepted to Ready for checkin

Everything seems fine now without new concerns. Hope it's ready... (if any triager disagrees, please comment #6320)

comment:32 Changed 7 years ago by ramiro

  • Triage Stage changed from Ready for checkin to Accepted

t689-r7823.diff contains:

  • Rebased to apply cleanly to trunk as of r7823
  • Fixes a typo and enhacements to some paragraphs in the docs
  • test added to django/contrib/auth/tests.py:
    • Convert it to use Django TestCase to be in line with recent mods to this file, previously we used unittest's one
    • Drop a bunch of unneeded imports and an unneeded attribute

Rolling the ticket back to Accepted until it gets a review from a triager.

Changed 7 years ago by ramiro

comment:33 Changed 7 years ago by telenieko

  • Triage Stage changed from Accepted to Ready for checkin

As per IRC:

<jacobkm> Nah, go ahead and mark it ready - it is.

comment:34 Changed 7 years ago by adrian

  • Summary changed from Honour web-server provided authentication to Honor Web server provided authentication

comment:35 Changed 7 years ago by Hugh Saunders <hugh@…>

  • Cc hugh@… added

comment:36 Changed 7 years ago by yoz

Is this going to make it into 1.0? It would be so useful.

comment:37 Changed 7 years ago by telenieko

The docs-refactor broke the patch, always up-to-date patch here in colours and raw

comment:38 Changed 7 years ago by ericvw

  • Cc ericvw@… added

comment:39 Changed 7 years ago by anonymous

  • Cc grf@… added

comment:40 Changed 7 years ago by hinnerk

  • Cc hinnerk@… added

comment:41 Changed 7 years ago by guettli

This ticket is on the Version1.1Features list.

Changed 7 years ago by ramiro

Patch updated to apply to trunk as of r9099: Edited and expanded documentation, fixed tests

comment:42 Changed 7 years ago by gwilson

  • Description modified (diff)

comment:43 Changed 7 years ago by anonymous

  • Cc me@… added

comment:44 Changed 7 years ago by zhaoz

  • Cc zilingzhao@… added

Changed 7 years ago by telenieko

Updated patch

comment:45 Changed 7 years ago by telenieko

Just attached an updated patch, it should be RFCheckin.
I took the changes from Ramiro's latest patch except calling REMOTE_USER a variable, I'd prefer to
call it a Header, at least on the web server part. And used the shiny "versionadded::" directive.

comment:46 Changed 7 years ago by dvd

  • Cc dvd@… added

Changed 7 years ago by telenieko

New patch with Gary's comments

comment:47 Changed 7 years ago by gwilson

  • Owner changed from telenieko to gwilson
  • Status changed from assigned to new

comment:48 Changed 7 years ago by gwilson

  • Status changed from new to assigned

comment:49 Changed 7 years ago by eengbrec

  • Cc erik.engbrecht@… added

comment:50 Changed 7 years ago by jacob

  • milestone set to 1.1 beta
  • Triage Stage changed from Ready for checkin to Accepted

Changed 6 years ago by gwilson

Changed 6 years ago by gwilson

add to docs paragraph about custom header value

comment:51 Changed 6 years ago by david

  • Cc david added

comment:52 Changed 6 years ago by gwilson

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

(In [10063]) Fixed #689 -- Added a middleware and authentication backend to contrib.auth for supporting external authentication solutions. Thanks to all who contributed to this patch, including Ian Holsman, garthk, Koen Biermans, Marc Fargas, ekarulf, and Ramiro Morales.

comment:54 Changed 4 years ago by jacob

  • milestone 1.1 beta deleted

Milestone 1.1 beta deleted

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