#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
437 437 438 438 def authenticate(self, req): 439 439 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) 441 442 return auth.LoginModule.authenticate(self, req) 442 443 authenticate = if_enabled(authenticate)
Attachments (0)
Change History (12)
comment:1 Changed 14 years ago by
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() |
comment:2 Changed 14 years ago by
Keywords: | needinfo removed |
---|---|
Status: | new → assigned |
Type: | defect → enhancement |
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 14 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(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 follow-up: 5 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 Changed 12 years ago by
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 12 years ago by
(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 follow-ups: 8 9 Changed 12 years ago by
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 follow-up: 10 Changed 12 years ago by
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 Changed 12 years ago by
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 Changed 12 years ago by
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 12 years ago by
(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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
(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.
Well, I'd really appreciate some more details.