#5340: Redirect unauthenticated users to login

54 views
Skip to first unread message

Remy Blank

unread,
Aug 21, 2008, 4:05:10 PM8/21/08
to trac...@googlegroups.com
I would like to move forward with this ticket. I have already applied
the non-controversial part of the patch that adds an optional referer=
argument to /login to pass the URL to redirect to after authentication.
This code was already present in AccountManagerPlugin.

What remains is the following:

--- a/trac/web/main.py
+++ b/trac/web/main.py
@@ -437,6 +437,11 @@
data = {'title': title, 'type': 'TracError', 'message': e.detail,
'frames': [], 'traceback': None}
try:
+ if e.code == 403 and req.authname == 'anonymous':
+ referer = req.abs_href(req.path_info)
+ if req.query_string:
+ referer += '?' + req.query_string
+ req.redirect(req.href('login', {'referer': referer}))
req.send_error(sys.exc_info(), status=e.code, env=env,
data=data)
except RequestDone:
pass

This redirects unauthenticated users trying to access a page generating
a "403 Forbidden" to /login and back after authentication.

It would also solve #540 (Permissions and RSS feeds) in most cases, if
the RSS feed reader supports authentication and redirects, which most
seem to do.

OK to apply?

-- Remy

signature.asc

Noah Kantrowitz

unread,
Aug 21, 2008, 4:11:10 PM8/21/08
to trac...@googlegroups.com
I am still highly opposed to this as a core behavior. I think this is entirely a question of policy, and the base semantics of requesting something you are not authorized for should be to return a 403. If people want to force authentication before you can do anything, they should enforce authentication on / instead of /login.

--Noah

Chris

unread,
Aug 21, 2008, 4:45:24 PM8/21/08
to trac...@googlegroups.com
I'm not a dev and I respect your opinion Noah, but I don't see how beneficial it is to return a 403. It would make sense to inform the end user there is a problem and give them a potential solution. IE: Tell them they could not authenticate and give them the ability to.
C

Noah Kantrowitz

unread,
Aug 21, 2008, 4:50:18 PM8/21/08
to trac...@googlegroups.com

Because this breaks the HTTP model. When you request a resource you are not authorized for, you should get back a 403. You cannot do an HTTP redirect while returning a 403 code. This means the feature cannot be done properly. If you would like to propose an addendum to the HTTP spec,  that’s fine with me, but here and now this is Wrong (tm).

 

--Noah

Noah Kantrowitz

unread,
Aug 21, 2008, 5:00:03 PM8/21/08
to trac...@googlegroups.com

A better solution IMO is to just put a message on the 403/PermissionError page if authname==’anonymous’ like “Maybe you would like to <a href=”/login”>login</a>”.

 

--Noah

Jeffrey Hulten

unread,
Aug 21, 2008, 5:29:01 PM8/21/08
to trac...@googlegroups.com
Is there a was to return a 403 at the protocol level and still display the login page if authname is anonymous?  The HTTP status code and the HTML displayed are not linked anywhere in the specification.

Remy Blank

unread,
Aug 21, 2008, 5:31:40 PM8/21/08
to trac...@googlegroups.com
Noah,

Thanks for your reply.

Noah Kantrowitz wrote:
> I am still highly opposed to this as a core behavior. I think this
> is entirely a question of policy, and the base semantics of
> requesting something you are not authorized for should be to return
> a 403. If people want to force authentication before you can do
> anything, they should enforce authentication on / instead of /login.

(snip)

> Because this breaks the HTTP model. When you request a resource
> you are not authorized for, you should get back a 403.

I think I understand your point of view. From a developer perspective,
and for machine-to-machine communication, it does make sense to be
precise about the reply and to inform the other party that it has done
something that was forbidden, so that it can take appropriate measures.

But take this from a usability point of view. The average Trac user is
not a machine, and we want to help her as much as possible ("staying out
of the way"). She often has no idea of HTTP error codes, and the
usability of the 403 reply is just bad. The most frequent reason why a
user gets this reply is because she has forgotten to log in. Then we
might as well help her by taking her directly to the authentication
form, and redirect her back to where she wanted to be in the first
place. Yes, we could just add a link to the error page, but is this
additional click really necessary? Not for the user, at least.

Enforcing authentication on / is not a solution, as it prevents leaving
some resources open for anonymous access.

So the only way we can please both sides is by making this behavior
configurable. I guess this could be done in a request filter, but does
this really make sense for 5 lines of code? My preference would be to
add an option in trac.ini. I know there are already too many, but
judging from the CC list of #540, which this also mostly solves, this
functionality is desired.

-- Remy

signature.asc

Remy Blank

unread,
Aug 21, 2008, 5:38:13 PM8/21/08
to trac...@googlegroups.com
Jeffrey Hulten wrote:
> Is there a was to return a 403 at the protocol level and still display
> the login page if authname is anonymous?

Not if you are using HTTP authentication. In that case, you only request
authentication for /login, and the error has to redirect you there. And
a redirect is a 301, 302 or 303, so it can't be at the same time a 403.

If you use form-based authentication (AccountManagerPlugin), then yes,
you could probably do that. It might be a worthwhile addition to the
plugin, if this patch is rejected.

-- Remy

signature.asc

Noah Kantrowitz

unread,
Aug 21, 2008, 5:49:29 PM8/21/08
to trac...@googlegroups.com
Trac has always built on principles of respecting established standards. This is one of the reasons it is so easy to hook in to so many places. As I said, this is a bug in the HTTP spec, and we shouldn't try to work around at it at the cost of changing expected behavior from long-standing standards. I can list plenty of examples as to why this is a bad idea. One, Apache understands error codes coming back from handlers and lets you setup special error pages. Many large sites use this behavior (ditto on the 404 code). I have no problem with the feature being available in a plugin (which is why I wrote the plugin already), but we should not introduce this non-compliant behavior into core.

--Noah

> -----Original Message-----
> From: trac...@googlegroups.com [mailto:trac...@googlegroups.com] On
> Behalf Of Remy Blank
> Sent: Thursday, August 21, 2008 2:32 PM
> To: trac...@googlegroups.com

Remy Blank

unread,
Aug 21, 2008, 6:15:46 PM8/21/08
to trac...@googlegroups.com
Noah Kantrowitz wrote:
> I can list plenty of examples as to why this is a bad idea. One,
> Apache understands error codes coming back from handlers and lets
> you setup special error pages. Many large sites use this behavior
> (ditto on the 404 code).

That's one good reason indeed. I would still be interested in a few
others, as I'm much less knowledgeable in this field than you are. It's
always good to learn something.

> I have no problem with the feature being
> available in a plugin (which is why I wrote the plugin already)

I assume you refer to HttpAuthPlugin. Personally, I'm not comfortable
moving the responsibility for authentication from the web server to Trac
(in the HTTP auth scenario). I am sure Apache (or any other web server)
authentication is much more widely tested than HttpAuthPlugin.

BTW, does HttpAuthPlugin support digest authentication?

> but we should not introduce this non-compliant behavior into core.

Ok, your final word is "use HttpAuthPlugin", or "apply the patch to your
Trac instance", or "put those five lines into some other plugin". Fair
enough.

Other opinions?

-- Remy

signature.asc

Emmanuel Blot

unread,
Aug 21, 2008, 6:21:50 PM8/21/08
to trac...@googlegroups.com
On Thu, Aug 21, 2008 at 11:49 PM, Noah Kantrowitz <no...@coderanger.net> wrote:
>
> Trac has always built on principles of respecting established standards. This is one of the reasons it is so easy to hook in to so many places. As I said, this is a bug in the HTTP spec, and we shouldn't try to work around at it at the cost of changing expected behavior from long-standing standards. I can list plenty of examples as to why this is a bad idea. One, Apache understands error codes coming back from handlers and lets you setup special error pages. Many large sites use this behavior (ditto on the 404 code). I have no problem with the feature being available in a plugin (which is why I wrote the plugin already), but we should not introduce this non-compliant behavior into core.


I'm +1 w/ Noah - for the same reasons.

I think the proposal to add a link on the 403 page to the /login page
is fair enough.

Cheers,
Manu

Noah Kantrowitz

unread,
Aug 21, 2008, 6:29:16 PM8/21/08
to trac...@googlegroups.com

> -----Original Message-----
> From: trac...@googlegroups.com [mailto:trac...@googlegroups.com] On
> Behalf Of Remy Blank
> Sent: Thursday, August 21, 2008 3:16 PM
> To: trac...@googlegroups.com
> Subject: [Trac-dev] Re: #5340: Redirect unauthenticated users to login
>

HttpAuthPlugin addresses a different issue, allowing certain paths to require HTTP authentication when using AccountManager (needed for authenticated RSS feeds and such).

The one that fixes this is PermRedirect. It does exactly what you are suggesting, hooks on PermissionErrors and redirects to /login.

--Noah

Remy Blank

unread,
Aug 21, 2008, 6:48:58 PM8/21/08
to trac...@googlegroups.com
> HttpAuthPlugin addresses a different issue, allowing certain
> paths to require HTTP authentication when using AccountManager
> (needed for authenticated RSS feeds and such).
>
> The one that fixes this is PermRedirect. It does exactly what
> you are suggesting, hooks on PermissionErrors and redirects to
> /login.

Right, I got confused. PermRedirect it is. One thing that would be nice
to add to PermRedirect is to add the "referer=" argument to the
redirect() call, as the "Referer:" HTTP header will not be the initially
requested page.

I'd still be interested in a few other reasons why this whole
"redirecting on permission denied" is a bad idea, if you have a few
minutes to spare.

-- Remy

signature.asc

Noah Kantrowitz

unread,
Aug 21, 2008, 6:54:38 PM8/21/08
to trac...@googlegroups.com

> -----Original Message-----
> From: trac...@googlegroups.com [mailto:trac...@googlegroups.com] On
> Behalf Of Remy Blank
> Sent: Thursday, August 21, 2008 3:49 PM
> To: trac...@googlegroups.com
> Subject: [Trac-dev] Re: #5340: Redirect unauthenticated users to login
>

Yep, that should indeed be added (though I think I already did to support some weird thing with TracForge, or maybe I added it in TF, too many plugins to keep straight). Other reasons it is bad to return a 3xx/200 (depending on the redirect is followed automatically) is that a number of tools (desktop clients, RSS readers, etc) will display a more useful error message is the code is what it should be (rather than getting a different page that is valid but it just can't parse). Really anything that acts like a scraper and isn't smart enough to understand that a redirect to /login means 403. It may break plugins that depend on catching 403s (like HttpAuth) though I think the suggested patch was low enough in the chain to only confuse stream filters.

--Noah

Remy Blank

unread,
Aug 21, 2008, 7:19:39 PM8/21/08
to trac...@googlegroups.com
Noah Kantrowitz wrote:
> ... is that a number of tools (desktop clients, RSS readers, etc)

> will display a more useful error message is the code is what it
> should be (rather than getting a different page that is valid but
> it just can't parse). Really anything that acts like a scraper and
> isn't smart enough to understand that a redirect to /login means 403.

That makes sense, and it only applies to form-based login. With HTTP
auth, if the "scraper" understands redirects and 401, it should do fine.

Anyway, unless some silent majority suddenly speaks up, I'll drop the
rest of the patch and redirect (no pun intended) the ticket to
PermRedirect. BTW, how do you close tickets that are solved by a plugin?
"wontfix"? "worksforme"?

-- Remy

signature.asc

Emmanuel Blot

unread,
Aug 21, 2008, 8:02:43 PM8/21/08
to trac...@googlegroups.com
> Anyway, unless some silent majority suddenly speaks up, I'll drop the rest
> of the patch and redirect (no pun intended) the ticket to PermRedirect. BTW,
> how do you close tickets that are solved by a plugin? "wontfix"?
> "worksforme"?

Most common way is "wontfix" + "plugin" in the keyword list.

Cheers,
Manu

Evan

unread,
Aug 21, 2008, 9:09:48 PM8/21/08
to trac...@googlegroups.com, Evan
It sounds like we can still increase usability somewhat, while
sticking to standards, by adding the link. I suppose meta refresh
tags are pretty well out of favor these days, but it seems like it
would accomplish both goals: return a 403, and redirect the user to
the login url without requiring any action on their part.

--
Evan

Noah Kantrowitz

unread,
Aug 22, 2008, 12:51:10 AM8/22/08
to trac...@googlegroups.com
Evan wrote:
> It sounds like we can still increase usability somewhat, while
> sticking to standards, by adding the link. I suppose meta refresh
> tags are pretty well out of favor these days, but it seems like it
> would accomplish both goals: return a 403, and redirect the user to
> the login url without requiring any action on their part.

meta-refresh for redirects is considered very bad for accessibility and
has been officially deprecated by w3c. Check the wikipedia page
(http://en.wikipedia.org/wiki/Meta_refresh) for a summary of the issues.

Similarly a JavaScript-based solution would be problematic.

--Noah

signature.asc

Michael Renzmann

unread,
Aug 22, 2008, 12:58:23 AM8/22/08
to trac...@googlegroups.com
Hi.

I'm no dev, but still:

> I am still highly opposed to this as a core behavior. I think this is
> entirely a question of policy,

I agree. If people really want to change the default behaviour, they could
enable the PermRedirectPlugin [1]. I use this plugin for one of the Trac
sites I maintain, and it works fine.

Bye, Mike

[1] http://trac-hacks.org/wiki/PermRedirectPlugin

Noah Kantrowitz

unread,
Aug 22, 2008, 1:09:12 AM8/22/08
to trac...@googlegroups.com
Michael Renzmann wrote:
> Hi.
>
> I'm no dev, but still:

Let me nip this one in the bud. Not having commit rights to
trac.edgewall.org doesn't make people's opinions less valid. If you have
something to say, just say it. I, for one, judge input based on the
quality of the actual idea. Come one, come all.

--Noah

signature.asc

David Abrahams

unread,
Aug 22, 2008, 10:05:31 PM8/22/08
to Trac Development


On Aug 21, 1:00 pm, "Noah Kantrowitz" <n...@coderanger.net> wrote:
> Because this breaks the HTTP model. When you request a resource you are not
> authorized for, you should get back a 403. You cannot do an HTTP redirect
> while returning a 403 code. This means the feature cannot be done properly.

Does doing it improperly (as many, many websites now do) have any
technical
implications? For example, does it prevent useful automation?

> If people
> want to force authentication before you can do anything, they should enforce
> authentication on / instead of /login.

The typical scenario that I've seen is one where authentication is
only needed
before you can do *certain* things. For example, one might want to
allow anyone
to view the wiki, but require login to edit it.

Noah Kantrowitz

unread,
Aug 22, 2008, 10:37:11 PM8/22/08
to trac...@googlegroups.com
David Abrahams wrote:
>
>
> On Aug 21, 1:00 pm, "Noah Kantrowitz" <n...@coderanger.net> wrote:
>> Because this breaks the HTTP model. When you request a resource you are not
>> authorized for, you should get back a 403. You cannot do an HTTP redirect
>> while returning a 403 code. This means the feature cannot be done properly.
>
> Does doing it improperly (as many, many websites now do) have any
> technical
> implications? For example, does it prevent useful automation?

Yes, I already cited a number of problems it causes.

>
>> If people
>> want to force authentication before you can do anything, they should enforce
>> authentication on / instead of /login.
>
> The typical scenario that I've seen is one where authentication is
> only needed
> before you can do *certain* things. For example, one might want to
> allow anyone
> to view the wiki, but require login to edit it.

Which Trac doesn't handle well anyway, since the edit buttons would be
hidden to begin with. Short of adding umpteen million options to control
what is shown, I think our current solution for that is about the only
workable one.

--Noah

signature.asc

Jeffrey Hulten

unread,
Aug 22, 2008, 10:54:59 PM8/22/08
to trac...@googlegroups.com
Based on the meaning of 403 vs 401, why are we EVER giving a 403 if someone is anonymous.

From the spec (emphasis mine):

401 Unauthorized

The request requires user authentication. The response MUST include a WWW-Authenticate header field (section 14.47) containing a challenge applicable to the requested resource. The client MAY repeat the request with a suitable Authorization header field (section 14.8). If the request already included Authorization credentials, then the 401 response indicates that authorization has been refused for those credentials. If the 401 response contains the same challenge as the prior response, and the user agent has already attempted authentication at least once, then the user SHOULD be presented the entity that was given in the response, since that entity might include relevant diagnostic information. HTTP access authentication is explained in "HTTP Authentication: Basic and Digest Access Authentication" [43].

403 Forbidden

The server understood the request, but is refusing to fulfill it. Authorization will not help and the request SHOULD NOT be repeated. If the request method was not HEAD and the server wishes to make public why the request has not been fulfilled, it SHOULD describe the reason for the refusal in the entity. If the server does not wish to make this information available to the client, the status code 404 (Not Found) can be used instead.


I have always found the base model for authenticating using basic auth to be clumsy user interaction.  The proper method if a user is not authenticated is to return a 401.  If a user is authenticated and not authorized then a 403 should be provided.  That is the HTTP model.

Noah Kantrowitz

unread,
Aug 23, 2008, 12:59:02 AM8/23/08
to trac...@googlegroups.com
Jeffrey Hulten wrote:
> Based on the meaning of 403 vs 401, why are we EVER giving a 403 if someone
> is anonymous.
>
> From the spec (emphasis mine):
>
> 401 Unauthorized
>
> The request requires user authentication. The response MUST include a
> WWW-Authenticate header field (section 14.47) containing a challenge
> applicable to the requested resource. The client MAY repeat the request with
> a suitable Authorization header field (section
> 14.8<http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.8>).

> If the request already included Authorization credentials, then the 401
> response indicates that authorization has been refused for those
> credentials. If the 401 response contains the same challenge as the prior
> response, and the user agent has already attempted authentication at least
> once, then the user SHOULD be presented the entity that was given in the
> response, since that entity might include relevant diagnostic information.
> HTTP access authentication is explained in "HTTP Authentication: Basic and
> Digest Access Authentication"
> [43]<http://www.w3.org/Protocols/rfc2616/rfc2616-sec17.html#bib43>.

>
> 403 Forbidden
>
> The server understood the request, but is refusing to fulfill it.
> *Authorization
> will not help* and the request SHOULD NOT be repeated. If the request method

> was not HEAD and the server wishes to make public why the request has not
> been fulfilled, it SHOULD describe the reason for the refusal in the entity.
> If the server does not wish to make this information available to the
> client, the status code 404 (Not Found) can be used instead.
>
>
> I have always found the base model for authenticating using basic auth to be
> clumsy user interaction. The proper method if a user is not authenticated
> is to return a 401. If a user is authenticated and not authorized then a
> 403 should be provided. That is the HTTP model.

If we knew HTTP auth would be used, this would be more appropriate. In
most cases just returning a 401 wouldn't help, because the server is
only configured to accept credentials on the /login path. If you setup
authentication on the entire base, and just set it to require on the
/login subpath then this would work better. Unfortunately this is very
clunky to configure on many servers and would probably confused people.
Also if anything other than HTTP auth is being used (which isn't really
possible to detect), sending back a 401 is always wrong.

Another issue that sprung to mind as I was about to hit send, we cannot
initiate a Digest auth transaction and have Apache verify it (nonce
wouldn't match).

--Noah

signature.asc

David Abrahams

unread,
Aug 23, 2008, 1:11:11 AM8/23/08
to trac...@googlegroups.com

on Fri Aug 22 2008, Noah Kantrowitz <noah-AT-coderanger.net> wrote:

> David Abrahams wrote:
>>
>>
>> On Aug 21, 1:00 pm, "Noah Kantrowitz" <n...@coderanger.net> wrote:
>>> Because this breaks the HTTP model. When you request a resource you are not
>>> authorized for, you should get back a 403. You cannot do an HTTP redirect
>>> while returning a 403 code. This means the feature cannot be done properly.
>>
>> Does doing it improperly (as many, many websites now do) have any
>> technical implications? For example, does it prevent useful
>> automation?
>
> Yes, I already cited a number of problems it causes.

Sorry about that; I hadn't read the whole thread when I posted.

>>> If people want to force authentication before you can do anything,
>>> they should enforce authentication on / instead of /login.
>>
>> The typical scenario that I've seen is one where authentication is
>> only needed
>> before you can do *certain* things. For example, one might want to
>> allow anyone
>> to view the wiki, but require login to edit it.
>
> Which Trac doesn't handle well anyway, since the edit buttons would be
> hidden to begin with.

Interesting. It's obvious why that's a problem but somehow I never ran
into it.

> Short of adding umpteen million options to control what is shown, I
> think our current solution for that is about the only workable one.

I don't see why you'd need any options, at least, not to do a reasonably
good job. If the user is unauthenticated and the "authenticated" user
has the permission, you show the button. On the other hand, I can
definitely see the argument for keeping that functionality in a plugin.

--
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

Noah Kantrowitz

unread,
Aug 23, 2008, 1:17:40 AM8/23/08
to trac...@googlegroups.com

That makes the assumption that the user is able to login. If they aren't
the buttons are at best confusing. I'm not saying it wouldn't be
possible to do, but it would be non-trivial complexity and probably
non-trivial runtime overhead (the extra permission checks would probably
add up quickly).

--Noah

signature.asc

David Abrahams

unread,
Aug 23, 2008, 1:18:26 AM8/23/08
to trac...@googlegroups.com

on Thu Aug 21 2008, "Michael Renzmann" <mrenzmann-AT-otaku42.de> wrote:

> Hi.
>
> I'm no dev, but still:
>
>> I am still highly opposed to this as a core behavior. I think this is
>> entirely a question of policy,
>
> I agree. If people really want to change the default behaviour, they could
> enable the PermRedirectPlugin [1]. I use this plugin for one of the Trac
> sites I maintain, and it works fine.

My experience of that plugin over the course of my use of Trac has been
that, more often than not, it is broken. The difficulty of keeping the
details of its implementation consistent with Trac's dispatching code
has been my main argument for bringing the functionality into the core
And yes, I understand and agree with the arguments for not doing so; I
just don't think this case is as cut-and-dried as some others might.

Noah Kantrowitz

unread,
Aug 23, 2008, 3:46:00 AM8/23/08
to trac...@googlegroups.com

Funny, the 0.11 version of the plugin has never had a bug filed against
it other than the known issue where referer headers got messed up when
dealing with redirects (which can now be fixed for the 0.12 version of
the plugin thanks to the referer argument to /login).

--Noah

signature.asc

David Abrahams

unread,
Aug 23, 2008, 2:55:58 PM8/23/08
to trac...@googlegroups.com

on Fri Aug 22 2008, Noah Kantrowitz <noah-AT-coderanger.net> wrote:

> David Abrahams wrote:
>>
>>> Short of adding umpteen million options to control what is shown, I
>>> think our current solution for that is about the only workable one.
>>
>> I don't see why you'd need any options, at least, not to do a reasonably
>> good job. If the user is unauthenticated and the "authenticated" user
>> has the permission, you show the button. On the other hand, I can
>> definitely see the argument for keeping that functionality in a plugin.
>>
>
> That makes the assumption that the user is able to login. If they aren't
> the buttons are at best confusing.

Yes, I was thinking of changing the button label to "Login to Edit"

> I'm not saying it wouldn't be possible to do, but it would be
> non-trivial complexity and probably non-trivial runtime overhead (the
> extra permission checks would probably add up quickly).

One check per button? Is the permission system *that* inefficient?

Noah Kantrowitz

unread,
Aug 23, 2008, 3:22:54 PM8/23/08
to trac...@googlegroups.com
David Abrahams wrote:
>
> on Fri Aug 22 2008, Noah Kantrowitz <noah-AT-coderanger.net> wrote:
>
>> David Abrahams wrote:
>>>> Short of adding umpteen million options to control what is shown, I
>>>> think our current solution for that is about the only workable one.
>>> I don't see why you'd need any options, at least, not to do a reasonably
>>> good job. If the user is unauthenticated and the "authenticated" user
>>> has the permission, you show the button. On the other hand, I can
>>> definitely see the argument for keeping that functionality in a plugin.
>>>
>> That makes the assumption that the user is able to login. If they aren't
>> the buttons are at best confusing.
>
> Yes, I was thinking of changing the button label to "Login to Edit"

My point was this makes no sense on a public-access site where not
everyone has a login.

>
>> I'm not saying it wouldn't be possible to do, but it would be
>> non-trivial complexity and probably non-trivial runtime overhead (the
>> extra permission checks would probably add up quickly).
>
> One check per button? Is the permission system *that* inefficient?
>

Given that each button is its own permission/context, yes. Each check
has to be run through the permissions API. Eventually we might add an
API for mass checks, but I doubt it, seems overcomplicated.

--Noah

signature.asc
Reply all
Reply to author
Forward
0 new messages