Patch to redirect unauthenticated users to login

[Dave Abrahams <dave@…>]

Normally when visiting a Trac page that requires authentication, one just gets a cryptic message about required permissions. It would be better to get redirected to log in, as implemented by the following patch, based on John Hampton's suggestion from ​http://news.gmane.org/find-root.php?message_id=%3c87fya8h07w.fsf%40valverde.peloton%3e

You might also consider the tweaks discussed in this short subthread: ​http://news.gmane.org/find-root.php?message_id=%3c20070128001741.BD0177047C2E%40slytherin.pacopablo.com%3e

[Dave Abrahams <dave@…>]

patch for trac/web/main.py

[osimons]

#6439 closed as a duplicate of this ticket. Please review it - it contains some interesting discussion.

[Noah Kantrowitz]

As mentioned in the other ticket, this is already implmented in the ​PermRedirect plugin. +1 for workforme.

[anonymous]

In order for PermRedirect to work with 0.11dev, this patch also is required:

--- /c/TracDev/trac/trac/web/main.py    2008-03-25 09:43:46.232417400 -0400
+++ web/main.py 2008-04-03 15:57:46.410436200 -0400
@@ -223,14 +223,14 @@

                             req.send(output, content_type or 'text/html')
                 else:
-                    self._post_process_request(req)
+                    self._post_process_request(req, None, None, None)
             except RequestDone:
                 raise
             except:
                 # post-process the request in case of errors
                 err = sys.exc_info()
                 try:
-                    self._post_process_request(req)
+                    self._post_process_request(req, None, None, None)
                 except RequestDone:
                     raise
                 except Exception, e:

[Dave Abrahams <dave@…>]

The PermRedirect plugin isn't working for me, so that's not a workable alternative. I think the problem with PermRedirect may be related to #7451

[Remy Blank <remy.blank@…>]

Patch against 0.11-stable adding redirection of forbidden pages to /login and back

[Remy Blank <remy.blank@…>]

​th:PermRedirectPlugin doesn't work because the /login page redirects to the referrer specified in the HTTP headers, but this header doesn't contain the URL of the original page when /login is reached through a redirect, which is what happens in the plugin.

The patch above is a variation on the attachment:main.py.patch​ that uses the same mechanism to catch a "forbidden" error:

• When a "forbidden" error is caught and the client is unauthenticated, the browser is redirected to /login?referer={url} where {url} is an encoded version of the original URL, including the query. (Yeah, I know, "referer" has a spelling error; but the same error is present in the HTTP header specification, so I thought this would be more consistent)

• The LoginModule redirects to the URL specified in the referer argument if it is present, otherwise it tries to use the Referer HTTP header if present, otherwise it redirects to the entry page.

• The patch also fixes a small typo in the Href class (this is completely unrelated, I just happened to stumble upon it).

This patch also solves #540 at least partially. I have been able to get both Thunderbird and Akregator to fetch and display the timeline correctly when the timeline is restricted to authenticated users.

I'm not sure if this redirection to /login should always be enabled, or if it should be configurable through trac.ini. I understand that there are already (too?) many configuration options, and I'm not sure why I should be able to disable it.

Feedback? Opinions?

[Jonas Borgström]

Good work Remy, I'll try to find some time to test this soon, but by just looking at the changes they look good.

Does this work with the ​th:AccountManagerPlugin as well?

[Remy Blank <remy.blank@…>]

Replying to jonas:

Does this work with the ​th:AccountManagerPlugin as well?

I haven't tested it yet, but I don't think so. It is probably missing the referer argument. I'll have a look at it this evening and patch it if necessary.

[Remy Blank <remy.blank@…>]

It turns out that I was wrong. The patch works well with ​th:AccountManagerPlugin out of the box, because the latter already takes a referer argument, due to the login page coming between the original page and the redirect. Funny.

But looking at the plugin, it seems that its LoginModule duplicates some code from trac, but hasn't received the fix from #3048 which was committed in [6279]. A small modification to the patch allows removing duplicate code from the plugin, and fixing this problem at the same time.

I'll attach updated patches in a few minutes.

[Remy Blank <remy.blank@…>]

Small change allowing to remove duplicate code in ​th:AccountManagerPlugin

[Remy Blank <remy.blank@…>]

Remove duplicate code and fix #3048

[Remy Blank <remy.blank@…>]

I have tested the patches above with both HTTP (digest) authentication (w/o AccountManager) and with AccountManager and form-based login. In both cases, the browser is redirected correctly to the original page after authentication.

[Remy Blank <remy.blank@…>]

The patch could be included in 0.11.1 if it receives some more testing.

Dave, as the ticket reporter, could you please test the patch and give us some feedback about how it works for you? In particular, if you are using AccountManager, are there any interoperability problems?

[Noah Kantrowitz]

+1 on addition of referer argument for /login. -1 on adding the actual redirect, I would still rather see policy elements like this in plugins to keep from overcomplicating core.

[Remy Blank <remy.blank@…>]

Replying to nkantrowitz:

+1 on addition of referer argument for /login. -1 on adding the actual redirect, I would still rather see policy elements like this in plugins to keep from overcomplicating core.

You mean close this as 'wontfix' and implement a plugin doing the redirect?

I'm not sure this would be a good idea. It would basically mean copying _dispatch_request() from trac/web/main.py (80 lines), adding the functionality that is in the patch above (5 lines), and monkey-patching the function in trac.web.main. Not so nice from a maintenance perspective.

You could also add an extension point for this purpose, or extract the exception handling into a separate function.

But maybe I didn't understand what you mean. Did you mean overcomplicating the code, or overcomplicating the configuration, in case this behavior should be configurable?

[Remy Blank <remy.blank@…>]

Fixed patch that also works with TracForge

[Remy Blank <remy.blank@…>]

The previous patch has been tested by Daniel Wallin (same site as the ticket reporter):

I applied your patch, but it didn't work. I needed to change it like this to make it work. The important change being env.(abs_)?href -> req.(abs_)?href. I have no idea why this was needed, or what the difference between env.href and req.href is. I'm guessing it's related to us using TracForge.

The patch above fixes the two occurrences of env.abs_href and env.href, and works fine even without TracForge.

[Remy Blank]

The part of the patch providing the referer= argument to /login was applied in [7493].

[Remy Blank]

The redirect functionality has been rejected (see ​this thread). The suggested fix was to add a notice to the "403 Forbidden" error page for unauthenticated users, suggesting that they log in. This has been implemented in [7494].

For people who still want the redirect functionality, the ​th:PermRedirectPlugin provides that, and it should be updated shortly to also correctly redirect requests containing a query string.

[anonymous]

The PermRedirectPlugin does not solve the problem if you want to give customers access to e.g. the ticket-area but not the wiki.

[dave@…]

Replying to anonymous:

The PermRedirectPlugin does not solve the problem if you want to give customers access to e.g. the ticket-area but not the wiki.

Would the anonymous poster care to explain why it doesn't solve that problem? I don't understand yet.

[Remy Blank]

Without further explanations, I'll close this again as wontfix, as ​th:PermRedirectPlugin does solve the initial issue.

And for the record, ​#TH2210 tracks the issue where the plugin always redirects to the main page.

[anonymous]

Hi,

After implement the 5340-accountmanagerplugin-loginmodule-r3857.patch , I got this error as below. We used customized auth (PAM). Do you think it will cause the problme like this?

Oops… Trac detected an internal error:

AttributeError: 'LoginModule' object has no attribute '_referer'

There was an internal error in Trac. It is recommended that you inform your local Trac administrator and give him all the information he needs to reproduce the issue.

To that end, you could ==== How to Reproduce ==== While doing a GET operation on /logout, Trac issued an internal error. (please provide additional details here) User Agent was: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9 ==== System Information ====

Trac

0.11

Python

2.4.3 (#1, Jan 14 2008, 18:31:21) [GCC 4.1.2 20070626 (Red Hat 4.1.2-14)]

setuptools

0.6c7

psycopg2

2.0.4 (dec dt ext pq3)

Genshi

0.5

Subversion

1.4.2 (r22196)

jQuery:

1.2.3

==== Python Traceback ==== Traceback (most recent call last): File "/usr/lib/python2.4/site-packages/trac/web/main.py", line 423, in _dispatch_request dispatcher.dispatch(req) File "/usr/lib/python2.4/site-packages/trac/web/main.py", line 197, in dispatch resp = chosen_handler.process_request(req) File "/usr/lib/python2.4/site-packages/acct_mgr/web_ui.py", line 433, in process_request return auth.LoginModule.process_request(self, req) File "/usr/lib/python2.4/site-packages/trac/web/auth.py", line 105, in process_request self._redirect_back(req) File "/usr/lib/python2.4/site-packages/trac/web/auth.py", line 201, in _redirect_back referer = self._referer(req) AttributeError: 'LoginModule' object has no attribute '_referer' a ticket at this site.

The action that triggered the error was:

GET: /logout

TracGuide — The Trac User and Administration Guide

[Remy Blank]

Replying to anonymous:

After implement the 5340-accountmanagerplugin-loginmodule-r3857.patch , I got this error as below.

As 0.11.2 has not been released yet, you need to get Trac from the 0.11-stable branch in SVN. You seem to be using 0.11, which doesn't include the fix mentioned in comment:15.