Modify

Opened 5 years ago

Closed 19 months ago

#5964 closed enhancement (fixed)

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

Reported by: ebray Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: ldap authentication
Cc: rjollos 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 4 years ago by hasienda

  • Keywords needinfo ldap authentication added
  • Owner changed from pacopablo to hasienda
  • Summary changed from Prevent multiple calls to LoginModule._remote_user() to [patch] Prevent multiple calls to LoginModule._remote_user()

Well, I'd really appreciate some more details.

comment:2 in reply to: ↑ description Changed 4 years ago by hasienda

  • Keywords needinfo removed
  • Status changed from new to assigned
  • Type changed from defect to 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 4 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to 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: Changed 3 years ago by techtonik

  • Resolution fixed deleted
  • Status changed from closed to 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 in reply to: ↑ 4 Changed 22 months ago by hasienda

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 20 months ago by hasienda

(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: Changed 20 months ago by 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 ; follow-up: Changed 20 months ago by hasienda

  • Cc rjollos added

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 20 months ago by hasienda

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 20 months ago by hasienda

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 20 months ago by hasienda

(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 19 months ago by hasienda

  • Resolution set to fixed
  • Status changed from reopened to 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.

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from hasienda. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.