Opened 17 years ago

Closed 13 years ago

#3583 closed (wontfix)

cookie-based modpython / apache authentication

Reported by: midfield <midfield@…> Owned by:
Component: Contrib apps Version: dev
Severity: Keywords: authentication modpython apache
Cc: faheem@…, andy@…, jzylks@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The modpython authentication handler described in

http://www.djangoproject.com/documentation/apache_auth/

is great but it uses HTTP authentication. This is not exactly transparent: to see protected static content from within a protected django site the user will have to log in TWICE, once for the django bits, and once for the static bits. In general there are problems with mixing authentication types: security, logging out, etc.

I'm pretty sure it should not be too hard to write an authentication handler (hooking into PythonAuthenHandler) which looks for the session / authentication cookie instead of using HTTP authentication. I'd do it myself but it has been a long time since I have written apache / modpython modules, so if anyone out there is an expert some help would be much appreciated.

Attachments (10)

modpython.py (1.5 KB ) - added by mcallister.sean@… 17 years ago.
apache authentication via session and authentication framework (using cookies)
apache_auth.patch (7.2 KB ) - added by Chris Beaven 17 years ago.
apache_auth.2.patch (7.5 KB ) - added by Chris Beaven 17 years ago.
HTTP_FORBIDDEN if user authenticates but does not have correct permissions
modpython.2.py (4.4 KB ) - added by TomFreudenberg 17 years ago.
This is an update to apache_auth.2.patch. First: error of wrong "self" is fixed, second: The check was splitted to check if user is authenticated (if not raise UNAUTHORIZED) and the validate user for permissions (if not raise FORBIDDEN)
apache_auth.3.patch (10.6 KB ) - added by Chris Beaven 17 years ago.
apache_auth.4.patch (10.6 KB ) - added by Chris Beaven 17 years ago.
apache_auth.5.patch (10.7 KB ) - added by Rick van Hattem <Rick.van.Hattem@…> 16 years ago.
Patch for django/contrib/auth/handlers/modpython.py@7403
modpython.3.py (5.5 KB ) - added by Rick van Hattem <Rick.van.Hattem@…> 16 years ago.
django/contrib/auth/handlers/modpython.py with patches to enable both PythonAuthenHandler and PythonAccessHandler
apache_auth.6.patch (10.6 KB ) - added by peterd12 16 years ago.
Modified to apply cleanly against rev. 8584
apach_auth.7.patch (10.5 KB ) - added by peterd12 16 years ago.
Updated to work with refactored django.dispatch. Applies cleanly against rev 8630.

Download all attachments as: .zip

Change History (41)

comment:1 by Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Simon G. <dev@…>, 17 years ago

Summary: REQ: cookie-based modpython / apache authenticationcookie-based modpython / apache authentication

in reply to:  description comment:3 by anonymous, 17 years ago

I wrote a little PythonAccessHandler tightly based on the authhandler mentioned. I don't think you can use the PythonAuthHandler, because that needs all the authentication stuff from apache and I haven't found a way to surpress the prompt.
There is a lot of overhead in the current implementation, as all request middleware components are applied, but only session and authentication middleware would be needed.
I use this apache configuration for the access control:

<Location "/images/full">
            SetHandler python-program
	    PythonPath "['/path/to/proj/'] + sys.path"	
            PythonOption DJANGO_SETTINGS_MODULE myproj.settings
	    PythonDebug On 
		
	    PythonOption DjangoPermissionName '<permission.codename>'
	    PythonAccessHandler my_proj.modpython
</Location>

by mcallister.sean@…, 17 years ago

Attachment: modpython.py added

apache authentication via session and authentication framework (using cookies)

comment:4 by midfield@…, 17 years ago

Hey thanks, that works great. I had to fiddle with the SESSION_COOKIE_DOMAIN but otherwise it seems fantastic. +1 for putting this into the main source, or at least making this more public -- HTTP basic authentication sucks!

in reply to:  4 comment:5 by mcallister.sean@…, 17 years ago

Replying to midfield@gmail.com:

Hey thanks, that works great. I had to fiddle with the SESSION_COOKIE_DOMAIN but otherwise it seems fantastic. +1 for putting this into the main source, or at least making this more public -- HTTP basic authentication sucks!

Could you elaborate on the SESSION_COOKIE_DOMAIN things you had to do?
I could write a patch for django.contrib.auth.handlers.modpython, but i want to get any issues sorted out before I do so.

comment:6 by midfield@…, 17 years ago

The SESSION_COOKIE_DOMAIN thing was my fault, not yours. (I was protecting http://static.foo.com, whereas my main site is http://foo.com.) Maybe just something to put in the docs. The only other thing I'd say is that you might consider changing the logic so that if there is no specific permission set, and there's no "staff only / super only", then any authenticated user is allowed. That's a judgment call, but seems like the most obvious thing to me.

In whatever docs you put up, an example using nested <Location> tags and permissions might be nice -- that's the usual way people are going to use this, I imagine.

A question: right now I'm using VirtualHosts, but in the interests of saving memory, I am forcing my main site and the static protected site to both use the "main_interpreter." Is this the right idea?

Thanks a million.

comment:7 by Chris Beaven, 17 years ago

Has patch: set
Needs documentation: set
Patch needs improvement: set

The patch should really just import and use the session and auth middleware the comment above says.

by Chris Beaven, 17 years ago

Attachment: apache_auth.patch added

comment:8 by Chris Beaven, 17 years ago

Component: UncategorizedContrib apps
Needs documentation: unset
Owner: changed from Jacob to Adrian Holovaty
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Right, I actually reneged on my earlier comment that it should 'Just use the session and auth middleware', since we should be covering other back ends, not just contrib.auth.models.User.

What the new patch does:

  • Refactored the contrib.auth.handlers.modpython to cut out the common elements so they can be used by both the existing authenhandler and the new accesshandler functions.
  • Changed the existing authenhandler function to make it work with any back end which takes a username and password as the arguments.
  • Added the accesshandler function which sets up a Django request, applies all active middleware to it, then looks for request.user (which is done by django.contrib.auth.middleware.AuthenticationMiddleware) and validates against it.
  • Documented the new method.

comment:9 by Chris Beaven, 17 years ago

Triage Stage: Ready for checkinDesign decision needed

From discussion about #4354 on IRC, the point was raised that the mod_python docs suggest using HTTP_FORBIDDEN instead of HTTP_UNAUTHORIZED if the user authenticates but does not validate.

Patch 2 contains this further change to the existing authenhandler function...

And now I guess this should go back to a design decision... :(

by Chris Beaven, 17 years ago

Attachment: apache_auth.2.patch added

HTTP_FORBIDDEN if user authenticates but does not have correct permissions

comment:10 by TomFreudenberg, 17 years ago

In Line 42 of apache_auth.2.patch "self" does not exist. It has to be "user.has_perm(options.permission_name)".

    if options.permission_name and (not hasattr(user, 'has_perm') or not user.has_perm(options.permission_name)):

by TomFreudenberg, 17 years ago

Attachment: modpython.2.py added

This is an update to apache_auth.2.patch. First: error of wrong "self" is fixed, second: The check was splitted to check if user is authenticated (if not raise UNAUTHORIZED) and the validate user for permissions (if not raise FORBIDDEN)

comment:11 by Chris Beaven, 17 years ago

Patch needs improvement: set

Good catch of the self bug. I don't like how your authenticate_user method requires an is_authenticated property - you're not taking other backends into account.

Your changes need to be merged into the patch and the patch's docs updated.

comment:12 by TomFreudenberg, 17 years ago

I am not sure if I am familiar enough with this authentication stuff. If you add this handler to check access for "authenticated users" on an apache location, what case is it, that the "is_authenticated" property is missing. Shouldn't you always be able to check "user.is_authenticated" and if not, give no access to your location ?

comment:13 by Chris Beaven, 17 years ago

While the usual case is that authentication returns a contrib.auth User model, it could be from any custom created user model (plugged in as an authentication backend) which may not have an is_authenticated method.

comment:14 by James Bennett, 17 years ago

From looking at the patch, it's actually doing the right thing with respect to is_authenticated; the user argument being passed to authenticate_user must, at that point, be either None or an instance of django.contrib.auth.models.User, because it was obtained by calling django.contrib.auth.authenticate, which can't return anything else (the requirement of returning a User or None is -- per the documentation -- passed on to the authenticate methods of all auth backends).

I do have a hard time following what's going on here, though, because that check seems to be pointless: if user is None, the if not user check will catch it (and should actually be if user is None), and if not it must be an instance of User, in which case is_authenticated is guaranteed to return True. Given that, the authenticate_user function is somewhat redundant, since the if user is not None check can move into validate_user, and that can be called directly with the result of django.contrib.auth.authenticate.

comment:15 by TomFreudenberg, 17 years ago

Just to get it rigth what you have written above:

28 	def authenticate_user(user, options):
29 	    if not user:
30 	        return False
31 	    # Require an is_authenticated property and check it
32 	    if not hasattr(user, 'is_authenticated') or not user.is_authenticated():
33 	        return False
34 	    return True

...

109 	        # Check authentification of the user
110 	        if not authenticate_user(user, options):
111 	            return apache.HTTP_UNAUTHORIZED

is same as only

109 	        # Check authentification of the user by existing of user object
110 	        if not user:
111 	            return apache.HTTP_UNAUTHORIZED

is this sure, then this will be the best I guess.

by Chris Beaven, 17 years ago

Attachment: apache_auth.3.patch added

by Chris Beaven, 17 years ago

Attachment: apache_auth.4.patch added

comment:16 by Chris Beaven, 17 years ago

Patch needs improvement: unset

New patch up:

  • provides a DjangoRaiseForbidden option (off by default to keep the current behaviour) - see docs
  • cookie authentication works very well! It redirects to settings.LOGIN_URL if authentication is required
  • added a section to the docs about preventing caching in Apache

comment:17 by Chris Beaven, 17 years ago

Triage Stage: Design decision neededAccepted

I'm promoting back to Accepted, as we don't have the issue of changing existing behaviour now that I added the DjangoRaiseForbidden option. Tempted to just promote to checkin, but I'll hold off that for now and bring this up in the dev group

comment:18 by alessandro.ronchi@…, 17 years ago

I cannot find to which file applies the apache_auth.4.patch (i've tried the django contrib trunk, modpython.py and modpython2.py here). Can you upload a modpython.py file updated? thanks in advance.

comment:19 by Chris Beaven, 16 years ago

Owner: changed from nobody to Chris Beaven

comment:20 by Rick van Hattem <Rick.van.Hattem@…>, 16 years ago

Replying to alessandro.ronchi@gmail.com:

I cannot find to which file applies the apache_auth.4.patch (i've tried the django contrib trunk, modpython.py and modpython2.py here). Can you upload a modpython.py file updated? thanks in advance.

It's not that hard actually, you have to patch against revision 6000 :)

  1. svn co -r 6000 http://code.djangoproject.com/svn/django/trunk/django/
  2. patch -p0 < apache_auth.4.patch

comment:21 by Chris Beaven, 16 years ago

Please feel free to upload a newer patch if it doesn't patch cleanly to the current trunk. Contact me directly if you need a hand.

comment:22 by Rick van Hattem <Rick@…>, 16 years ago

Here's a new patch for revision 7403

And I've included a complete modpython.py for people that have trouble with merging files.

by Rick van Hattem <Rick.van.Hattem@…>, 16 years ago

Attachment: apache_auth.5.patch added

Patch for django/contrib/auth/handlers/modpython.py@7403

by Rick van Hattem <Rick.van.Hattem@…>, 16 years ago

Attachment: modpython.3.py added

django/contrib/auth/handlers/modpython.py with patches to enable both PythonAuthenHandler and PythonAccessHandler

by peterd12, 16 years ago

Attachment: apache_auth.6.patch added

Modified to apply cleanly against rev. 8584

comment:23 by Faheem Mitha, 16 years ago

apache_auth.6.patch does not apply cleanly for me against rev 8584. Can someone help?
I earlier used apache_auth.4.patch. Have there been non-trivial changes to the patch
since then, and if not, would it be reasonable to try to rebase that patch?

Thanks, Faheem.

comment:24 by peterd12, 16 years ago

I just co'd a fresh copy of rev 8584 and applied this latest patch with no issue. What's the problem you're encountering? You are downloading the raw version of the patch, aren't you? http://code.djangoproject.com/attachment/ticket/3583/apache_auth.6.patch?format=raw

by peterd12, 16 years ago

Attachment: apach_auth.7.patch added

Updated to work with refactored django.dispatch. Applies cleanly against rev 8630.

comment:25 by Faheem Mitha, 16 years ago

Cc: faheem@… added

comment:26 by Faheem Mitha, 16 years ago

apach_auth.7.patch applies cleanly for me against current trunk.
i still need to test it.

Thanks, Faheem.

comment:27 by Andy Baker, 15 years ago

Cc: andy@… added

comment:28 by Jason Zylks, 15 years ago

Cc: jzylks@… added

comment:29 by Chris Beaven, 15 years ago

Owner: Chris Beaven removed

unassigning myself from the ticket since I don't have any personal interest in continuing with it for now

comment:30 by Łukasz Rekucki, 14 years ago

mod_python will most likely be deprecated in Django 1.3 and removed in future versions, so this ticket collides with #13820

comment:31 by Adam Nelson, 13 years ago

Resolution: wontfix
Status: newclosed

No activity on this, no champion, and mod_python is deprecated. Seems like there's no point in keeping this ticket open.

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