Edgewall Software
Modify

Opened 12 years ago

Closed 11 years ago

Last modified 6 years ago

#5340 closed enhancement (wontfix)

Patch to redirect unauthenticated users to login

Reported by: Dave Abrahams <dave@…> Owned by: Jonas Borgström
Priority: normal Milestone:
Component: general Version: devel
Severity: normal Keywords: patch plugin
Cc: wangchun@…, remy.blank@… Branch:
Release Notes:
API Changes:

Description

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

Attachments (5)

main.py.patch (595 bytes ) - added by Dave Abrahams <dave@…> 12 years ago.
patch for trac/web/main.py
5340-redirect-forbidden-login-r7431.patch (2.3 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Patch against 0.11-stable adding redirection of forbidden pages to /login and back
5340-redirect-forbidden-login-r7435.patch (2.5 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Small change allowing to remove duplicate code in th:AccountManagerPlugin
5340-accountmanagerplugin-loginmodule-r3857.patch (1.0 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Remove duplicate code and fix #3048
5340-redirect-forbidden-login-r7455.patch (2.5 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Fixed patch that also works with TracForge

Download all attachments as: .zip

Change History (27)

by Dave Abrahams <dave@…>, 12 years ago

Attachment: main.py.patch added

patch for trac/web/main.py

comment:1 by osimons, 12 years ago

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

comment:2 by Noah Kantrowitz, 12 years ago

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

comment:3 by Wang Chun <wangchun@…>, 12 years ago

Cc: wangchun@… added

comment:4 by anonymous, 12 years ago

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:

comment:5 by Dave Abrahams <dave@…>, 11 years ago

Type: defectenhancement

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

by Remy Blank <remy.blank@…>, 11 years ago

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

comment:6 by Remy Blank <remy.blank@…>, 11 years ago

Cc: remy.blank@… added
Keywords: patch added

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?

comment:7 by Jonas Borgström, 11 years ago

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?

in reply to:  7 comment:8 by Remy Blank <remy.blank@…>, 11 years ago

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.

comment:9 by Remy Blank <remy.blank@…>, 11 years ago

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.

by Remy Blank <remy.blank@…>, 11 years ago

Small change allowing to remove duplicate code in th:AccountManagerPlugin

by Remy Blank <remy.blank@…>, 11 years ago

Remove duplicate code and fix #3048

comment:10 by Remy Blank <remy.blank@…>, 11 years ago

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.

comment:11 by Remy Blank <remy.blank@…>, 11 years ago

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?

comment:12 by Noah Kantrowitz, 11 years ago

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

in reply to:  12 comment:13 by Remy Blank <remy.blank@…>, 11 years ago

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?

by Remy Blank <remy.blank@…>, 11 years ago

Fixed patch that also works with TracForge

comment:14 by Remy Blank <remy.blank@…>, 11 years ago

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.

comment:15 by Remy Blank, 11 years ago

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

comment:16 by Remy Blank, 11 years ago

Keywords: plugin added
Milestone: 0.11.30.11.2
Resolution: wontfix
Status: newclosed

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.

comment:17 by anonymous, 11 years ago

Resolution: wontfix
Status: closedreopened

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

in reply to:  17 comment:18 by dave@…, 11 years ago

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.

comment:19 by Remy Blank, 11 years ago

Resolution: wontfix
Status: reopenedclosed

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.

comment:20 by anonymous, 11 years ago

Resolution: wontfix
Status: closedreopened

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

in reply to:  20 comment:21 by Remy Blank, 11 years ago

Resolution: wontfix
Status: reopenedclosed

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.

comment:22 by Christian Boos, 11 years ago

Milestone: 0.11.2

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jonas Borgström.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Jonas Borgström to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.