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

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

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

[rpatterson]

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

I'm unclear if you two are requesting any changes or not? Can you clarify, @tiberiuichim @sneridagh?

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

How will I know when that happens? That PR was merged a while ago and I don't see any further news there.

[sneridagh]

@rpatterson plone.restapi PR is already released (sorry, busy weeks), but I would like to address this three things:

Changing the props name

I took a closer look, as @tiberiuichim sugggested and indeed we should consider it a breaking change. See: https://github.com/plone/volto/pull/2535/files?file-filters%5B%5D=.js&file-filters%5B%5D=.jsx&file-filters%5B%5D=.yml#diff-fa3c05381d8aba3b4829b3b93ab5d8c2731050768542d342d3e5e8fa8ee8e6f2R234

In the RenderedView, the token is removed and used userLoggedIn instead. This will potentially break all customizations using component shadowing for all customized Views outthere, so we need to mark it as breaking.

That's no issue, we can release Volto 14 whenever we want.

My comment about the Makefile

I would like to maintain the purity of what means to start the backend, what means to start the frontend without abstractions (docker in the middle using traeffic). It's fine to maintain it as a separate command, but not as "canonical". The reasons are mainly for training and newbies to really know what's going on under the hood.

Single sign out - Improved Logout action

Did you look into it? It's ok if not, I can look into that at some point before releasing V14.

[rpatterson]

I took a closer look, as @tiberiuichim sugggested and indeed we should consider it a breaking change.

K, I've made my best guess at correcting that in ./CHANGELOG.md

My comment about the Makefile

I would like to maintain the purity of what means to start the backend, what means to start the frontend without abstractions (docker in the middle using traeffic). It's fine to maintain it as a separate command,

K, moved to a separate target in ./Makefile.

Single sign out - Improved Logout action

Did you look into it? It's ok if not, I can look into that at some point before releasing V14.

No I didn't. My understanding from the discussion is that would be a separate issue/PR and that the next bit I'd work on is the new API endpoint for authenticated user data. Side note, it's been a bit so I don't recall what the logout issue is. Where are the details that issue captured?

LMK if I'm good to merge, @sneridagh?

[sneridagh]

@rpatterson Ok to all. Regarding merging, since we are all in the verge of going on summer holidays, I think that it's unwise to release a major just before we are all gone. I would like to release V14 when we are all back and available, if possible.

/cc @plone/volto-team

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