Opened 8 years ago

Closed 5 years ago

#3583 closed (wontfix)

cookie-based modpython / apache authentication

Reported by: midfield <midfield@…> Owned by:
Component: Contrib apps Version: master
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: UI/UX:

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@… 8 years ago.
apache authentication via session and authentication framework (using cookies)
apache_auth.patch (7.2 KB) - added by SmileyChris 8 years ago.
apache_auth.2.patch (7.5 KB) - added by SmileyChris 8 years ago.
HTTP_FORBIDDEN if user authenticates but does not have correct permissions
modpython.2.py (4.4 KB) - added by TomFreudenberg 8 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 SmileyChris 8 years ago.
apache_auth.4.patch (10.6 KB) - added by SmileyChris 8 years ago.
apache_auth.5.patch (10.7 KB) - added by Rick van Hattem <Rick.van.Hattem@…> 7 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@…> 7 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 7 years ago.
Modified to apply cleanly against rev. 8584
apach_auth.7.patch (10.5 KB) - added by peterd12 7 years ago.
Updated to work with refactored django.dispatch. Applies cleanly against rev 8630.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 8 years ago by Simon G. <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 8 years ago by Simon G. <dev@…>

  • Summary changed from REQ: cookie-based modpython / apache authentication to cookie-based modpython / apache authentication

comment:3 in reply to: ↑ description Changed 8 years ago by anonymous

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>

Changed 8 years ago by mcallister.sean@…

apache authentication via session and authentication framework (using cookies)

comment:4 follow-up: Changed 8 years ago by midfield@…

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!

comment:5 in reply to: ↑ 4 Changed 8 years ago by mcallister.sean@…

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 Changed 8 years ago by midfield@…

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 Changed 8 years ago by SmileyChris

  • 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.

Changed 8 years ago by SmileyChris

comment:8 Changed 8 years ago by SmileyChris

  • Component changed from Uncategorized to Contrib apps
  • Needs documentation unset
  • Owner changed from jacob to adrian
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Ready for checkin to Design 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... :(

Changed 8 years ago by SmileyChris

HTTP_FORBIDDEN if user authenticates but does not have correct permissions

comment:10 Changed 8 years ago by TomFreudenberg

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)):

Changed 8 years ago by TomFreudenberg

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 Changed 8 years ago by SmileyChris

  • 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 Changed 8 years ago by TomFreudenberg

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 Changed 8 years ago by SmileyChris

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 Changed 8 years ago by ubernostrum

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 Changed 8 years ago by TomFreudenberg

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.

Changed 8 years ago by SmileyChris

Changed 8 years ago by SmileyChris

comment:16 Changed 8 years ago by SmileyChris

  • 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 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 8 years ago by alessandro.ronchi@…

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

  • Owner changed from nobody to SmileyChris

comment:20 Changed 7 years ago by Rick van Hattem <Rick.van.Hattem@…>

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

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 Changed 7 years ago by Rick van Hattem <Rick@…>

Here's a new patch for revision 7403

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

Changed 7 years ago by Rick van Hattem <Rick.van.Hattem@…>

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

Changed 7 years ago by Rick van Hattem <Rick.van.Hattem@…>

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

Changed 7 years ago by peterd12

Modified to apply cleanly against rev. 8584

comment:23 Changed 7 years ago by faheem

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

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

Changed 7 years ago by peterd12

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

comment:25 Changed 7 years ago by faheem

  • Cc faheem@… added

comment:26 Changed 7 years ago by faheem

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

Thanks, Faheem.

comment:27 Changed 7 years ago by andybak

  • Cc andy@… added

comment:28 Changed 6 years ago by jzylks

  • Cc jzylks@… added

comment:29 Changed 6 years ago by SmileyChris

  • Owner SmileyChris deleted

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

comment:30 Changed 5 years ago by lrekucki

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

comment:31 Changed 5 years ago by adamnelson

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

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