Call for review: #689 - REMOTE_USER authentication
Gary Wilson Jr.
http://code.djangoproject.com/attachment/ticket/689/689.4.diff
Gary
Malcolm Tredinnick
On Thu, 2009-03-12 at 01:40 -0500, Gary Wilson Jr. wrote:
> Just posted an updated patch:
>
> http://code.djangoproject.com/attachment/ticket/689/689.4.diff
If nobody gets to it beforehand, I'll give this a serious look tomorrow.
Cheers,
Malcolm
Jacob Kaplan-Moss
Looks good to me. My only comment would be that I'd move the docs into
howto/ instead of topics/. Other than that, I'm happy with this code
as is. Malcolm's pickier than I am, though, so you might want to see
what he's got to say :P
Jacob
Malcolm Tredinnick
Sorry, I was sick yesterday. But I read this through just now. Looks
just about good to go. Naturally, I have some points, though. :-)
1. A couple of the docstrings take a long time to say what they do. My
feeling is that a docstring like this obscures the real details in a lot
of words:
def clean_username(self, username):
"""
Cleans the passed username to remove any extraneous text, and
returns the cleaned username.
By default, this method returns the passed username
unmodified.
Override this method if you need to do special things with
the
username, like stripping @realm or cleaning something like
cn=user,dc=domain.
"""
I'd go for something shorter:
Performs any cleaning on the "username" prior to using it to
create the user object. Returns the cleaned username.
By default, returns the username unchanged.
I've also added when the method is called, since that adds some context
to the purpose of the method (it's post-input, pre-ORM handling). I'm
ambivalent about whether the LDAP example goes there or in the docs, but
I'd favour something shorter. I don't particularly like the "by default"
phrasing, but nothing better springs to my tired mind at the moment. (I
really want to say it performs *normalisation* on the username, but if
the method's clean_username, I'd stick to *cleaning*.)
2. Regarding the middleware: How does this interact with WSGI-based
systems? Can remote user data be passed in the environment via some
standard wsgi.* variables, rather than a particular HTTP header there?
It would be good to be able to reuse the same middleware (or make it
minimally subclassable) for that case, rather than unnecessarily burning
bridges.
Not suggesting you necessarily implement WSGI authentication support,
but it would be good to know if there's anything relevant we need to be
aware of in the design. Mostly I was looking at the reliance on a
particular HTTP header and wondering if that was the only way we might
enter that path.
I don't know if there's anything to this point. More wondering out loud.
3. Not sure about howto/ vs. topics/ for the new document, in reference
to Jacob's comment, since it's mixing both instructional / tutorial form
with reference manual. Your call. Not worth sweating too much, since we
need to spend some time editing in some separation there in general and
the current stuff you've got looks very clear (I understood it).
Side note: thanks for using "authentication" throughout and not falling
back to "auth". Keeps it clear which part of the process is being
handled. :-)
Good stuff.
Cheers,
Malcolm
Gary Wilson Jr.
<mal...@pointy-stick.com> wrote:
> I'd go for something shorter:
>
> Performs any cleaning on the "username" prior to using it to
> create the user object. Returns the cleaned username.
>
> By default, returns the username unchanged.
>
> I've also added when the method is called, since that adds some context
> to the purpose of the method (it's post-input, pre-ORM handling). I'm
Changed docstring, using your example.
> ambivalent about whether the LDAP example goes there or in the docs, but
> I'd favour something shorter.
I've moved these mentions to the docs.
> 2. Regarding the middleware: How does this interact with WSGI-based
> systems? Can remote user data be passed in the environment via some
> standard wsgi.* variables, rather than a particular HTTP header there?
> It would be good to be able to reuse the same middleware (or make it
> minimally subclassable) for that case, rather than unnecessarily burning
> bridges.
>
> Not suggesting you necessarily implement WSGI authentication support,
> but it would be good to know if there's anything relevant we need to be
> aware of in the design. Mostly I was looking at the reliance on a
> particular HTTP header and wondering if that was the only way we might
> enter that path.
I'm pretty sure the middleware as it stands now would easily be able
to support WSGI authentication. The mod_python handler is actually
populating META['REMOTE_USER'] from the mod_python request object's
user attribute. The wsgi handler sets META to the the environ dict it
is passed:
class WSGIRequest(http.HttpRequest):
def __init__(self, environ):
...
self.META = environ
So if REMOTE_USER is in the environ dict then the RemoteUserMiddleware
will work unchanged. If the authenticated user is passed as some
other environment variable name, then one would just need to subclass
RemoteUserMiddleware and set the header attribute to whatever key to
lookup in META.
> 3. Not sure about howto/ vs. topics/ for the new document, in reference
> to Jacob's comment, since it's mixing both instructional / tutorial form
> with reference manual. Your call. Not worth sweating too much, since we
> need to spend some time editing in some separation there in general and
> the current stuff you've got looks very clear (I understood it).
Looking, at what's currently in each I can see how this might be a
better fit for howto. I'll move it, Jacob.
> Good stuff.
Many thanks are in order to those who worked on the patch before I touched it.
Gary