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

[Erik M. Bray]

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 @@
 
    def authenticate(self, req):
        if req.method == 'POST' and req.path_info.startswith('/login'):
-            req.environ['REMOTE_USER'] = self._remote_user(req)
+            if 'REMOTE_USER' not in req.environ:
+                req.environ['REMOTE_USER'] = self._remote_user(req)
        return auth.LoginModule.authenticate(self, req)
    authenticate = if_enabled(authenticate)

[Steffen Hoffmann]

Well, I'd really appreciate some more details.

[Steffen Hoffmann]

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.

[Steffen Hoffmann]

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

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

[anatoly 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

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

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

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

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

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.

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

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

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

[Steffen Hoffmann]

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