Modify

Opened 14 years ago

Closed 11 years ago

Last modified 13 months ago

#5964 closed enhancement (fixed)

[patch] Prevent multiple calls to LoginModule._remote_user()

Reported by: Erik M. Bray Owned by: Steffen Hoffmann
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: ldap authentication
Cc: Ryan J Ollos Trac Release: 0.11

Description

I was having a problem where if a user logs in with invalid credentials using the form-based login, authentication would be performed twice. Why this happens is complicated, and has to do with additional plugins that implement IRequestFilter, so I won't go into the details right now, but am willing to if necessary.

Regardless, I don't think LoginModule._remote_user() (and by extension AccountManager.check_password()) should be called more than once in the request. This is especially a problem in my case, where users who're authenticating against an LDAP server are being locked out of their accounts due to invalid logins much faster than they should.

This was my solution--in my setup the only thing that should be setting the 'REMOTE_USER' environment variable is the account manager plugin. Though maybe a more flexible approach would be desired:

  • web_ui.py

     
    437437
    438438   def authenticate(self, req):
    439439       if req.method == 'POST' and req.path_info.startswith('/login'):
    440             req.environ['REMOTE_USER'] = self._remote_user(req)
     440            if 'REMOTE_USER' not in req.environ:
     441                req.environ['REMOTE_USER'] = self._remote_user(req)
    441442       return auth.LoginModule.authenticate(self, req)
    442443   authenticate = if_enabled(authenticate)

Attachments (0)

Change History (12)

comment:1 Changed 13 years ago by Steffen Hoffmann

Keywords: needinfo ldap authentication added
Owner: changed from John Hampton to Steffen Hoffmann
Summary: Prevent multiple calls to LoginModule._remote_user()[patch] Prevent multiple calls to LoginModule._remote_user()

Well, I'd really appreciate some more details.

comment:2 in reply to:  description Changed 13 years ago by Steffen Hoffmann

Keywords: needinfo removed
Status: newassigned
Type: defectenhancement

Replying to ebray:

I was having a problem where if a user logs in with invalid credentials using the form-based login, authentication would be performed twice. Why this happens is complicated, and has to do with additional plugins that implement IRequestFilter, ![...]

So, if we are talking about interfering plugins, I wouldn't consider this a bug but merely an enhancement. Anyway I'm just trying to integrate the proposed resolution as it seems to still fit into current trunk code with the potential to improve it.

comment:3 Changed 13 years ago by Steffen Hoffmann

Resolution: fixed
Status: assignedclosed

(In [9556]) AccountManagerPlugin: Prevent multiple calls to LoginModule._remote_user(), closes #5964.

Thanks to ebray for pointing this out and providing a solution.

comment:4 Changed 12 years ago by anatoly techtonik

Resolution: fixed
Status: closedreopened

This breaks authentication for FastCGI installation in 0.12.2, where ISTM that REMOTE_USER is set as a result of the following script http://trac.edgewall.org/wiki/TracFastCgi

comment:5 in reply to:  4 Changed 11 years ago by Steffen Hoffmann

Replying to techtonik:

This breaks authentication for FastCGI installation in 0.12.2, where ISTM that REMOTE_USER is set as a result of the following script http://trac.edgewall.org/wiki/TracFastCgi

Sorry for being rather late on this issue, but I'd like to have a bit more discussion on it.

I've been researching the underlying fundamentals, an there seem no contradiction to the behavior of an authenticator to set REMOTE_USER on auth success. And I think we're speaking about acct_mgr.web_ui.LoginModule here, so AcctMgr should be allowed to set this variable on it's own. A later comment to #8545 seems to agree with this conclusion too.

What behavior/solution did you have in mind when re-opening? I would really want to get this sorted for the upcoming release, so I'm listening closely to any suggestion.

comment:6 Changed 11 years ago by Steffen Hoffmann

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.

comment:7 Changed 11 years ago by anatoly techtonik

I remember there was a problem with Nginx configuration that users could not be authenticated. Something that REMOTE_USER should not be present, but still was set in Trac from magically nowhere. I don't remember details. Probably I wanted to open static resources for not authenticated users in NoAnonymous. Maybe somethings else. REMOTE_USER is an environment variable and no one expected Trac plugins to monkey-patch global state to pass information about authenticated user. This was very hard to debug, but again I don't remember the details.

There should be a different way to communicate auth information (or at lest documented one). Without a clear picture of the process I can not refresh my memory and point my finger at the place where the problem is.

comment:8 in reply to:  7 ; Changed 11 years ago by Steffen Hoffmann

Cc: Ryan J Ollos added; anonymous removed

Replying to techtonik:

There should be a different way to communicate auth information (or at lest documented one). Without a clear picture of the process I can not refresh my memory and point my finger at the place where the problem is.

Great, that you seem to still care. As mentioned in the commit message I'm well aware of the issue, and I'll try to resolve it to my best ability. For me it boiled down to "Don't set the variable, if you're not the authenticator.", but in general it was more about documenting actions according to the expressed demand for additional debug output. Meanwhile I've done some research to get a bigger picture, and I've found it perfectly valid to set REMOTE_USER from within the plugin, if indeed an AcctMgr AuthStore was the reason for the successful authentication.

Hopefully we'll finally work it out together, and I might even do a (maintenance) point-release after resolving it and a few more bugs.

comment:9 in reply to:  7 Changed 11 years ago by Steffen Hoffmann

Replying to techtonik:

There should be a different way to communicate auth information (or at lest documented one). Without a clear picture of the process I can not refresh my memory and point my finger at the place where the problem is.

Most likely you refer to #8545 here - closely related, but both have been kept for improved view on the historic implications.

comment:10 in reply to:  8 Changed 11 years ago by Steffen Hoffmann

Replying to hasienda:

Replying to techtonik:

There should be a different way to communicate auth information (or at lest documented one).

Yes it is. Right now I'm testing a promising patch here.

Hopefully we'll finally work it out together, and I might even do a (maintenance) point-release after resolving it and a few more bugs.

Indeed. Confirm, that this is still the plan. Due to other, unrelated reasons I hope to get there even before years end.

comment:11 Changed 11 years ago by Steffen Hoffmann

(In [12444]) AccountManagerPlugin: Revert [9556] and implement a better solution, refs #5964 and #8545.

Undesired recursion on LoginModule.authenticate is prevented by always setting 'user_locked' on the request now. New option 'environ_auth_overwrite' allows to control previously hard-coded (re-)set of environment variable REMOTE_USER.

Finally some log output should ease future authentication issue debugging.

comment:12 Changed 11 years ago by Steffen Hoffmann

Resolution: fixed
Status: reopenedclosed

(In [12482]) AccountManagerPlugin: Publish maintenance release 0.4.1, closes #5964, #8545, #10134, #10625, #10700 and #10701.

This is an update for current stable acct_mgr-0.4 with a number of fixes for issues resolved within the last weeks, i.e.:

  • a final fix for Single-Sign-On functionality (refs #9676),
  • a long-standing HttpAuth login issue and
  • one for acct_mgr.LoginModule, that is relevant if used with web-servers, that evaluate the REMOTE_USER environment variable.

Changeset [12468] is included, that may require a Trac db fix-up. Run python ./contrib/fix-session_attribute-failed_logins.py <env> once on any Trac environment, that had account locking enabled with time constraints before.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.