ZMI logout handler overrides challenge plugin configuration

[rpatterson]

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

In the Zope root /acl_users, I deactivated the HTTPBasicAuthHelper plugin for the IChallengePlugin interface and activated the CookieAuthHelper plugin for the IChallengePlugin interface. Then, while logged in as a Manager, I clicked the manage_zmi_logout ZMI link.

What I expect to happen:

I expect to see either some sort of "You have been logged out" page or to be redirected to the CookieAuthHelper.login_form.

IMO, ZMI logout should not immediately re-challenge the user to authenticate as this leads to a confusing user experience for most, if not all, authentication types. Specifically, the user never sees a clear confirmation of the effect of their action, some sort of "You have been logged out" message, and they're left to infer that from being challenged for credentials again.

If, despite that poor UX, we decide that ZMI logout should immediately re-challenge, then the decision for how to challenge the user should be delegated to the plugins configuration for the IChallengePlugin interface.

What actually happened:

The browser prompts for HTTP Authorization: Basic ... credentials. This happens because HTTP Authorization: Basic ... assumptions are hard-coded into Products.PluggableAuthService.manage_zmi_logout(...). Namely it sets WWW-Authenticate: basic ... and Status: 401 Unauthorized.

What version of Python and Zope/Addons I am using:

Python 3.9
Plone's buildout.coredev, branch 6.0, Products.PlonePAS added to buildout:auto-checkout

[d-maurer]

Ross Patterson wrote at 2021-12-28 13:32 -0800:

### What I did: In the Zope root `/acl_users`, I deactivated the `HTTPBasicAuthHelper` plugin for the `IChallengePlugin` interface and activated the `CookieAuthHelper` plugin for the `IChallengePlugin` interface. Then, while logged in as a `Manager`, I clicked the `manage_zmi_logout` ZMI link. ### What I expect to happen: I expect to see either some sort of "You have been logged out" page or to be redirected to the `CookieAuthHelper.login_form`.

This is not a `Products.PluggableAuthService` issue: `manage_zmi_logout` is defined by `Zope`'s `App.Management.Navigation`. `Zope` does not know about the optional component `Products.PluggableAuthServie`. If you use `Products.PluggableAuthService` do not use the ZMI logout functionality but call the user folder's `logout` method. The ZMI `logout` targets the case with a trivial user folder and HTTP basic authentication. For HTTP authentication, you cannot log out the user without browser interaction (because the browser has done the login dialog and sends the credentials on all requests). Thus, the only way to logout in this case it to force the browser to rechallenge and let the user provide invalid information for the login dialog. HTTP authentication has not been designed for the server to provide significant information in the challenge (all it can provide is the "realm"). This means, you cannot do much better for HTTP authentication based logout than what the ZMI logout does. If you know that your Zope application consistently use `Products.PluggableAuthService` without HTTP authentication, you can override ?App.Management.Navigation.manage_zmi_logout` on startup to call the `logout` method of the appropriate `acl_users`. If you have done this, the ZMI's logout should work in your special case.

[dataflake]

As Dieter says, that logout link is hardcoded in Zope and only works for basic HTTP authentication as used by the default user folder that ships with Zope itself. Different user folder implementations may define their own logout methods, but stock Zope simply cannot account for all of those.

[rpatterson]

This is not a Products.PluggableAuthService issue: manage_zmi_logout is defined by Zope's App.Management.Navigation.

From Products.PluggableAuthService.manage_zmi_logout:

# monkey patch Zope to cause zmi logout to be PAS-aware

So this is PAS code, hence I reported the issue here.

For HTTP authentication, you cannot log out the user without browser interaction...

Correct, and this is an implementation detail of that particular form of authentication. Supporting different forms of authentication is a core use case of PAS. Hence PAS has a plugin for this specific form of authentication, Products.PluggableAuthService.plugins.HTTPBasicAuthHelper.HTTPBasicAuthHelper. That's where the implementation details of this form of authentication that you describe belong, not in a monkey patch and not in such a way that it can't be controlled by the plugin configurations.

If you have done this, the ZMI's logout should work in your special case.

Configuring different forms of authentication, such as turning them on and off, isn't a special case. It's a core use case of PAS.

...stock Zope simply cannot account for all of those.

Definitely not and that's not what's described as the issue here. This monkey patch makes PAS behave less like PAS, and it need not do so. This is not a report asking for PAS to do everything and cover every case. This is a report about behavior that works against a core use case of PAS.

[dataflake]

I didn't read the original report closely enough, now I get what you're saying. I had totally forgotten that the code contains the override for the ZMI link. I do agree, just setting the WWW-Authenticate header without considering the configuration at all is inappropriate. I bet this hasn't come up before because very few people will replace the root user folder and configure it for an authentication scheme other than basic HTTP auth. I'll take a look when I have time.

[d-maurer]

Ross Patterson wrote at 2021-12-29 23:32 -0800:

> This is not a `Products.PluggableAuthService` issue: `manage_zmi_logout` is defined by `Zope`'s `App.Management.Navigation`. >From `Products.PluggableAuthService.manage_zmi_logout`: # monkey patch Zope to cause zmi logout to be PAS-aware

You are right: someone has put a patch similar to the one I suggested to you into `Products.PluggableAuthService` itself. Unfortunately, he was not completely successful -- as you have observed. Check if the following replacement satisfies your expectations. ```python # monkey patch Zope to cause zmi logout to be PAS-aware zope_manage_zmi_logout = Navigation.manage_zmi_logout def manage_zmi_logout(self, REQUEST, RESPONSE): """Logout current user""" acl_users = self.acl_users return acl_users.logout(REQUEST) \ if IPluggableAuthService.providedBy(acl_users) \ else zope_manage_zmi_logout(self, REQUEST, RESPONSE) Navigation.manage_zmi_logout = manage_zmi_logout ``` Be sure to test it also when a Zope (in contrast to a PAS) user folder is targeted.

[rpatterson]

I bet this hasn't come up before because very few people will replace the root user folder and configure it for an authentication scheme other than basic HTTP auth

Good point! I hadn't thought about that. I'm running into it because I'm working on the UX of sharing authentication between Volto which is using a JWT token plugin, Plone classic which is using the cookie/session plugins, and the ZMI. In that case we need a login "event" so that the ICredentialsUpdatePlugin interface is "triggered" so that all plugins can do what they need to do to initialize authentication no matter which method/UI the user uses. Hence having to switch Zope root /acl_users/ to cookie auth because Basic auth has no login "event". So if my work on that is merged, then at some point in the future we can expect many more users to have replaced Zope root /acl_users/ and enabled cookie auth.

I'll take a look when I have time.

Oh, I don't expect that just because I report an issue that someone else will do it for me. ;-) I was mainly reporting it so that at the very least it's a known issue and searchable. I'm a fan of #wontfix with a "Feel free to re-open if you'll do it" comment.

I have actually already partially implemented this in a overriding monkey patch in another package. I did it there because I didn't have much faith that a PR submitted to this repo would ever get to merging. Also, I haven't tested my overridden monkey patch with basic auth to see if that still works. From my reading of HTTPBasicAuthHelper, I don't think it's resetCredentials() actually does the full Basic auth logout "dance", so there's probably a non-trivial amount of futzing to get it working right in the plugin instead of the monkey patch. That said, I'd love to work on it if I got some reassurance that a PR on this matter would be welcome.

[rpatterson]

Check if the following replacement satisfies your expectations.

Yeah, that's pretty much what I was thinking with the caveat from my previous comment that I don't think the HTTPBasicAuthHelper.resetCredentials() is sufficiently complete as is.

[dataflake]

I'll take a look when I have time.

Oh, I don't expect that just because I report an issue that someone else will do it for me. ;-) I was mainly reporting it so that at the very least it's a known issue and searchable. I'm a fan of #wontfix with a "Feel free to re-open if you'll do it" comment.

So am I in many cases.

I didn't have much faith that a PR submitted to this repo would ever get to merging.

Why do you think that? A "good" PR (meaning one that is documented if needed and that contains unit tests) is the best way to get anything in here.

[d-maurer]

Dieter Maurer wrote at 2021-12-30 09:30 +0100:

... Check if the following replacement satisfies your expectations. ```python # monkey patch Zope to cause zmi logout to be PAS-aware zope_manage_zmi_logout = Navigation.manage_zmi_logout def manage_zmi_logout(self, REQUEST, RESPONSE): """Logout current user""" acl_users = self.acl_users return acl_users.logout(REQUEST) \ if IPluggableAuthService.providedBy(acl_users) \ else zope_manage_zmi_logout(self, REQUEST, RESPONSE) Navigation.manage_zmi_logout = manage_zmi_logout

This, too, will not be perfect: the problem is a `self` covered by an `acl_users` which is not the one which authenticated the current user. The situation is quite frequent: a global Zope manager, authenticated by the `acl_users` in the Zope root, enters a portal with its own local `acl_users` and activates there `manage_zmi_logout`. The code above will call the local `acl_users`'s `logout` which will (likely) not log out the Zope manager. I assume that this situation has motivated the current patch: it likely has tried to perform a logout both locally as well as globally. One could try to determine the `acl_users` from the current user rather than the location `self`. Still not perfect (it fails e.g. if the local `acl_users` knows a user with identical account information) but maybe good enough for most situations.

[dataflake]

For correct user folder implementations the user object should be acquisition-wrapped in the originating user folder, so that should be a safe way to find out where the login came from.

[rpatterson]

I didn't have much faith that a PR submitted to this repo would ever get to merging.

Why do you think that? A "good" PR (meaning one that is documented if needed and that contains unit tests) is the best way to get anything in here.

If you don't see why from my other recent interactions in this repo, then I would guess we just have different sensibilities and won't see eye to eye on this. I can tell you that I'm definitely not the only senior, gray beard, old-school Zope developer who practices TDD and understands that a development task isn't complete until the documentation is complete who learned long ago not to try with Zope'ish contributions. Recent interactions here haven't given me the sense that's changed.

At any rate, when I can carve out some time, I'll take a stab at a PR for this.

[d-maurer]

Jens Vagelpohl wrote at 2021-12-30 01:08 -0800:

For correct user folder implementations the user object should be acquisition-wrapped in the originating user folder, so that should be a safe way to find out where the login came from.

It does not cover the situation where a global ZMI manager and a local portal user have the same account info (in my development environments, I typically set up this situation to avoid remembering different sets of account information). Because `manage_zmi_logout` is declared public, the local `acl_users` will authenticate in this case (while the top `acl_users` wins for actions requiring real ZMI permissions).

[d-maurer]

Ross Patterson wrote at 2021-12-30 01:15 -0800:

> > I didn't have much faith that a PR submitted to this repo would ever get to merging. > Why do you think that? A "good" PR (meaning one that is documented if needed and that contains unit tests) is the best way to get anything in here. If you don't see why from my other recent [interactions](#105 (comment)) in this repo, then I would guess we just have different sensibilities and won't see eye to eye on this.

You opened the issue 4 days ago and 2 days ago a fixing PR has been merged. Not that bad, isn't it?

[dataflake]

If you don't see why from my other recent interactions in this repo, then I would guess we just have different sensibilities and won't see eye to eye on this. I can tell you that I'd definitely not the only senior, gray beard, old-school Zope developer who practices TDD and understands that a development task isn't complete until the documentation is complete who learned long ago not to try with Zope'ish contributions. Recent interactions here haven't given me the sense that's changed

After reading through that issue I cannot find fault in how Dieter tried to help you. On the other hand some of your comments struck me as quite snide and condescending. Please stick to problem descriptions and, if needed for clarifications, a PR or code example.

[d-maurer]

Dieter Maurer wrote at 2021-12-30 10:23 +0100:

Jens Vagelpohl wrote at 2021-12-30 01:08 -0800: >For correct user folder implementations the user object should be acquisition-wrapped in the originating user folder, so that should be a safe way to find out where the login came from. It does not cover the situation where a global ZMI manager and a local portal user have the same account info (in my development environments, I typically set up this situation to avoid remembering different sets of account information). Because `manage_zmi_logout` is declared public, the local `acl_users` will authenticate in this case (while the top `acl_users` wins for actions requiring real ZMI permissions).

It is worse: when a user would be authenticated (by the current set of credentials) by both a local and a global user folder (whether or not with identical or different login information), the local authentication will win (because `manage_zmi_logout` is public) and the logout will be local, not global. Due to the flexibility of the ZMI (there is no dedicated permission associated with its use), the flexibility of the Zope authentication (search along the publication parents to locate a user folder able to authenticate a user with the (apparently) required roles) and the limitation of some authentication mechanisms (specifically HTTP authentication), it is very hard (maybe impossible) to get a complete solution. One approach could be: * Associate a permission (maybe `View management screens`) with ZMI use (named "ZMI permission") * Let `manage_zmi_logout` use authentication logic from `ZPublisher.BaseRequest.traverse` to locate the user folder able to authenticate with the available credentials a user with the ZMI permission. If no such user folder can be located, the user is not/no longer ZMI logged in; otherwise, a logout is performed for the located user folder * In the case of HTTP authentication, the browser will show its login dialog and resent a request with the new login information to `manage_zmi_logout`. This could potentially cause the logout for a higher up user folder. The PAS `logout` will instead redirect to a different place, it may be necessary for the user to call `manage_zmi_logout` again manually to logout from even higher user folders. Associating a permission with ZMI use would be backward incompatible. The association could be "virtual", i.e. not enforced. Only `manage_zmi_logout` would assume the existence of a ZMI permission (and base its logic thereon). It would not work correctly in cases when the ZMI is used by users not possessing this permission.