fix(auth): UI logged-in detection for other auths

[rpatterson]

Defer to the API, specifically the presence of the login action, to determine if a
user is logged in instead of depending on the implementation detail of the UI JWT login
process.

This change results in the Volto UI showing the user as logged in even when they've
previously logged in via the ZMI Basic authentication or via the classic HTML Plone
login form. The userId still can't be retrieved if the user has logged in some other
way than the JWT Volto login component, but this is still a step forward.

Refs #134784

---

Involves some refactoring of actions and user session state handling. I also captured
some development build/setup improvements I either needed for this work or ran across
while working on this. I work hard to keep my commits atomic so it's probably best to
review commit-by-commit rather than review the whole diff. Otherwise, if this community
prefers very atomic PRs (as in approaching 1 PR per-commit), LMK and I'll break this up.

[rpatterson]

See also a related plone.restapi PR

[tiberiuichim]

Awesome to see you moving on to the Volto part, and quite fast! It's like... did you just learn Volto in a week?

Now, for the code part: I think userSession should be moved to the reducers folder, I don't think we need the extra selectors folder. I saw that you're using them in the connect calls, that's a pattern that I didn't see in the existing Volto code. If those functions are reusable but not reducers, then you should move them to the helpers folder.

It's quite a big PR, I'll need several looks probably to fully grasp it. Maybe we can have a call (with @sneridagh) to go through the new introduced API.

[sneridagh]

LGTM, but I have questions :)

Main one is regarding the workflow in mixed environments, having authenticated in Plone Classic, then going to Volto, how does Volto gets the token? It should be related with the PR in p.restapi, but I don't see it now. I will revisit it.

Introducing the selectors (and folder) is fine with me, although it will be needed to add some introductory documentation in the main docs.

Would you say it's breaking? I don't see it now, but due to the nature of the change maybe it would be worth to state it as breaking.

Tomorrow we have the Volto Team Meeting, could you attend and explain the changes? If not, maybe we can find a moment to meet and talk about it.

[sneridagh]

I won't generalise this for training/educational purposes. So the simple approach remains the default, then have the "advanced" deployment testbed.

[rpatterson]

I'm not quite following you here. Can you clarify and LMK if there's I should do here?

[sneridagh]

Fair enough, but yet another file/folder in the root...

[rpatterson]

Are you suggesting a better location?

[rpatterson]

Awesome to see you moving on to the Volto part, and quite fast! It's like... did you just learn Volto in a week?

Well, I have some previous React+Redux experience coming in. Not much, primarily one green-field project, but it was just 2-3 developers, we all did everything (Flask API, UI, CI, CD, orchestration) and I was the one whose hours were fully dedicated, so it kinda ended up being a React+Redux bootcamp. But thanks! :-)

Now, for the code part: I think userSession should be moved to the reducers folder, I don't think we need the extra selectors folder.

I don't think that works here. As I understand it, in Redux selectors are about abstracting what the UI needs to know from state without depending on the technical specifics of how state is interpreted to derive that knowledge. This makes it much easier to change how state is interpreted without having to touch every freaking component in the app to do so as I had to in the commit to move to selectors. IOW, they make Redux code bases more agile, maintainable, and less fragile.

In this case, the ./src/reducers/userSession/userSession.js reducer is (for the purposes of this discussion) about receiving the response to requests to the API endpoint that handles specifically JWT authentication for the UI, doing any transformation on that response data, and then putting it in the global Redux state under the userSession key. That state is specific to a particular implementation of authentication, namely JWT and the Volto UI login component. The issue here is that what the rest of the UI needs to know is just (for the purposes of this discussion) whether a user is authenticated, but how that is known needs to change so that it's not specific to a particular implementation of authentication. I don't even know if it's possible, but I think it would definitely be an anti-pattern to access the actions state from the userSession reducer or vice versa. Selectors OTOH are explicitly about interpreting the entire global Redux state.

I could see an argument that calling this selector userSession could be misleading because it communicates that it is related to that particular implementation of authentication as well. Except that I think the term userSession is nicely abstract and communicates what the UI cares about without describing how it's done. So what I would advocate for is to rename the userSession reducer to jwtAuth or something else that expresses more clearly how implementation specific it is. Either way, that seems like another PR so I stuck with userSession for the selectors because that most clearly expresses to me what it's about.

I saw that you're using them in the connect calls, that's a pattern that I didn't see in the existing Volto code.

As I understand Redux, this is the pattern for using selectors with class components. If these were function components, then I'd use the useSelector hook instead. The reason you don't see this pattern in the existing Volto code, AFAICT, is because the Volto code hasn't been using selectors before.

Components shouldn't care about how state is interpreted, they should only care about what their inputs are as the component itself would interpret them. So in this case, the components should only take an input of the abstracted fact of whether a user is authenticated. The right place to interpret the Redux state to provide inputs to the React component is when the component is connected to the state. LMK if I've got any of that wrong or if there's a better way to achieve what I'm arguing for here.

If those functions are reusable but not reducers, then you should move them to the helpers folder.

In my experience selectors are a core Redux concept, are specifically about interpreting state, and the convention is to keep them in a selectors folder. OTOH, my understanding is that the helpers folder is for general utilities that aren't specific to either the React library or the Redux framework, akin to what we might use a utils package or module for in a Python project.

It's quite a big PR, I'll need several looks probably to fully grasp it. Maybe we can have a call (with @sneridagh) to go through the new introduced API.

Sorry, I wasn't sure how to break this apart (aside from the minor build, local development stuff). For example, the commits that change how user session state is interpreted demonstrate the value of introducing selectors. But LMK if you have a suggestion for how to break this up more.

Thanks for the feedback, @tiberiuichim!

[rpatterson]

Main one is regarding the workflow in mixed environments, having authenticated in Plone Classic, then going to Volto, how does Volto gets the token? It should be related with the PR in p.restapi, but I don't see it now. I will revisit it.

Volto doesn't necessarily get a token in all cases. Since a token is only necessary to authenticate to the API, it's not needed if the user's browser is already authenticated to the API buy some other means such as ZMI Basic auth or the classic HTML Plone login form. OTOH, I think also setting a token would be a good idea but should also be a separate PR from this and my plone.restapi PR. See the comments in that PR from the other contributor about doing that. Here's hoping they submit a PR for that, but it's not necessary AFAIK.

Introducing the selectors (and folder) is fine with me, although it will be needed to add some introductory documentation in the main docs.

Got a pointer for me as to where to add that? I'm happy to put something brief in there as a part of this PR.

Would you say it's breaking? I don't see it now, but due to the nature of the change maybe it would be worth to state it as breaking.

It shouldn't be, but I wouldn't argue against calling it a breaking change. Probably the most "breaking" thing about it is that is't my first significant contribution and I've touched so many parts of the code base. ;-)

Tomorrow we have the Volto Team Meeting, could you attend and explain the changes? If not, maybe we can find a moment to meet and talk about it.

Alright, I'll try to stay up till 3 AM my time tonight to attend. But please forgive me if I don't make it, I've actually been less of a night owl recently. If I don't make it, I'll happily schedule something with you.

Thanks, @sneridagh!

[sneridagh]

@rpatterson oh please, you don't have to stay awake until that late! We can schedule a meeting when all can have a more sane time frame. Maybe not with all the team but with who is interested. In which TZ are you?

[rpatterson]

@rpatterson oh please, you don't have to stay awake until that late! We can schedule a meeting when all can have a more sane time frame. Maybe not with all the team but with is interested. In which TZ are you?

Sorry, I didn't mean to "martyr" myself there. ;-) I'd actually like to try to make it tonight/tomorrow-morning. Lets see how it goes and schedule something else if I don't.

FYI, I'm in San Francisco, so my time zone is PST8PDT, America/Los_Angeles, currently UTC/GMT-7.

[rpatterson]

Got a pointer for me as to where to add that? I'm happy to put something brief in there as a part of this PR.

https://docs.voltocms.com/recipes/folder-structure/

[rpatterson]

From framework meeting, move to feature.

[rpatterson]

I believe I've previously addressed all feedback from the comments here and I just pushed some further refinement that should address all remaining feedback from the framework meeting. I have an approval and a green merge button, so I'll wait a day for anyone to tell me if I have anything more to do, otherwise I'll go ahead and merge.

[tiberiuichim]

I've just noticed that it changes a few prop names, so this will be a breaking change. @sneridagh what's the procedure for this? Wait for the merge until an appropriate window is found?

[rpatterson]

I've just noticed that it changes a few prop names, so this will be a breaking change. @sneridagh what's the procedure for this? Wait for the merge until an appropriate window is found?

Can you clarify the sense of breaking change that is meant here and how changing prop names is a breaking change, @tiberiuichim?

[tiberiuichim]

I may be wrong, so take this with a grain of salt, but:

One of the extension methods that Volto provides is customization. I think of it as a "transition API" while Volto develops a real extensibility story and I've advocated avoiding this practice. But the gist of it is that when components change their props it could potentially break things for somebody who customized either that component file or their parent that passes down props.

Though that may not be the case for the files touched in this PR, you're mostly passing down props from the connect, making those modules black boxes.

[sneridagh]

@tiberiuichim yeah the props changes, but since they are "connected" (you don't really pass them down) do they matter?

@rpatterson I would wait for the corresponding release in p.restapi before merging.

[tisto]

I fully agree. The holiday season is on. Both @sneridagh and I will be on holiday for three weeks soon and it is too risky to push a major release right before that.

[rpatterson]

I think that it's unwise to release a major just before we are all gone

Ok, I'll assume someone else will merge this when it's time. I'll also do my best to pay attention to this and merge when someone tells me to if that's preferred.

[sneridagh]

@rpatterson I've just merged this and tested it in:

https://volto.kitconcept.com/

There is a moment when the toolbar appears, then disappears. Could you take a look into it asap?

The logout from the frontend is going to be necessary too, eg. if you are logged in in Zope, there is no way to remove the credentials, then the logout (from Volto) does not take place.

[sneridagh]

Let's revert then, so we can look at it more closely.

@avoinea also reported strange behaviours:

• Toolbar flashes when not logged in (strange, because the code in the Toolbar component seems ok)
• No way to logout effectively from the UI (already known, but turned out essential for this PR)
• It seems that when logged in with Zope admin it also behaves strange

[avoinea]

We should add a call to the @logout endpoint on logout action.

[sneridagh]

Given Ross changes in p.restapi I think it should do more than invalidate the token but do the normal Zope and Plone logout.

[avoinea]

@sneridagh True. We need to also amend the @logout endpoint to also call the Zope @@logout.

[rpatterson]

@sneridagh True. We need to also amend the @logout endpoint to also call the Zope @@logout.

Ooh, I like this idea, @avoinea! I've been contemplating the best way to handle Zope
root Authorization: Basic .... What you propose here would put it at the view layer,
which may be more appropriate than the direction I was heading in trying to put it at
the PAS plugin layer. The root here is, of course, the age-old issue of how terrible a
UX browser Authorization: Basic ... handling is and IIRC the issue is not only how to
take actions, like logging in or out, but also some edge cases that are confusing for
users. I know in Plone classic we have some special handling to recognize at least
some/one of these edge cases and we display a message about it to the user, though I
don't remember where that is. Maybe that's another reason to keep any handling about
this at the view level and out of the PAS plugins.

I'll take advantage of this comment to capture some of my thinking to date. My API JWT
PAS plugin PR implements login/logout synchronization between all PAS plugins that
support those PAS "events". If I were to handle this through the PAS plugins, then when
it comes to Zope root /acl_users authentication we need to handle it through the PAS
plugins in that /acl_users, not in the Plone portal's /plone/acl_users.
Synchronizing authentication state between plugins (e.g. API JWT tokens and Zope root
ZMI) requires identifying some sort of event corresponding to a user logging in:

1. A user takes an action in the browser requiring authentication (e.g. typing in the
   ZMI url, clicking a link to something that isn't available to Anonymous, etc.).
2. The server responds with 401 Unauthorized.
3. The browser prompts the user for credentials.
4. The browser re-sends the request with the credentials in the Authorization: Basic ...
   header.
5. The browser sends all future requests with the credentials in the Authorization: Basic ... header as well.

The problem with this is its statelessness and thus there is no login event. From the
server's perspective, there's no difference between the first authenticated request and
the rest, no login event. There are potential workarounds but they all involve
establishing some sort of HTTP session statefulness, which is non-trivial and
well-trodden territory. Of course, any such session statefulness requires establishing
a new session at some point, and at that point we have a login event from the server's
perspective. Of course at that point, why not use this statefulness for authentication
and do away with HTTP Authorization: Basic ... altogether?

For plain Zope, such as at the Zope root, one way to do this is with the cookie
authentication plugin provided by vanilla PAS. That plugin also provides login
challenge handling with a basic HTML login form whose handler correctly calls the PAS
updateCredentials() plugin methods, which is the PAS equivalent of triggering the
login event. PAS already patches the ZMI logout handler to trigger the PAS equivalent
of the logout event. The Zope instance created and set up by the plone/volto buildout
already adds the API JWT plugin to the Zope root /acl_users. So in theory we could
add/enable the cookie auth plugin in the Zope root /acl_users and make it the
first/preferred login challenge plugin and everything should work. I need to determine
if the API @logout endpoint triggers the PAS logout event at the right level to be
handled by the Zope root PAS plugins, but should be something we can handle with
minimal, incremental changes to that view.

I've done some testing in the Zope instance created by the plone/volto repo buildout.
Oddly, there's already a cookie auth plugin in the Zope root /acl_users but it's the
PlonePAS version and it's lacking the basic HTML login form. Even when I add that
form back, it still doesn't work outside the context of a Plone portal:

2021-12-27 11:12:02,243 ERROR   [Zope.SiteErrorLog:22][waitress-0] ComponentLookupError: http://localhost:49080/api/acl_users/credentials_cookie_auth/login
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 162, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 372, in publish_module
  Module ZPublisher.WSGIPublisher, line 266, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module Products.PlonePAS.plugins.cookie_handler, line 106, in login
  Module Products.PluggableAuthService.PluggableAuthService, line 1153, in updateCredentials
  Module Products.PlonePAS.plugins.cookie_handler, line 74, in updateCredentials
  Module zope.component._api, line 165, in getUtility
zope.interface.interfaces.ComponentLookupError: (<InterfaceClass plone.registry.interfaces.IRegistry>, '')

So that seems like a bug somewhere. At any rate, if I move aside and disable the
PlonePAS cookie auth plugin and add/enable the vanilla PAS cookie plugin, I get
prompted with the basic HTML login form when trying to access the ZMI. When I submit
that form I get another exception related to being outside the context of a Plone
portal:

2021-12-27 11:16:36,454 ERROR   [Zope.SiteErrorLog:22][waitress-2] ComponentLookupError: http://localhost:49080/api/acl_users/credentials_cookie_auth/login
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 162, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 372, in publish_module
  Module ZPublisher.WSGIPublisher, line 266, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 63, in call_object
  Module Products.PluggableAuthService.plugins.CookieAuthHelper, line 279, in login
  Module Products.PluggableAuthService.PluggableAuthService, line 1153, in updateCredentials
  Module plone.restapi.pas.plugin, line 165, in updateCredentials
  Module plone.restapi.pas.plugin, line 260, in create_payload_token
  Module plone.restapi.pas.plugin, line 230, in _signing_secret
  Module zope.component._api, line 165, in getUtility
zope.interface.interfaces.ComponentLookupError: (<InterfaceClass plone.keyring.interfaces.IKeyManager>, '')

When I disable Use Keyring in the API JWT token plugin in the Zope root /acl_users,
submitting the login form works to access the ZMI. So that seems like another bug in
how the Zope root /acl_users is set up. With the workarounds to those bugs. I'm able
to get some degree of login/logout synchronization between the Zope root ZMI and the
Volto UI. There's still some weird edge cases going on but I don't have any reason to
think that finding and addressing their root causes will be too difficult.

So having walked through all that, and given that the set up bugs in the Zope root
/acl_users plugins should be fixed anyways, I'm still inclined to resolve the Zope
root auth story through the PAS plugins. It feels more "correct" and I suspect it'll be
more robust and maintainable than a series of conditions and special case handling at
the view level.

I'm mostly just babbling here and capturing my findings and thoughts, but if anyone has
any thoughts please do respond.

[rpatterson]

See my plone.restapi PR for fixes to the above.