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 thelogoutmethod of the appropriateacl_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.

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

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