feat(auth): Sync classic/API PAS plugin cookies #1303
Conversation
|
@rpatterson thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating: With this simple comment all the jobs will be started automatically. Happy hacking! |
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
from
4bcb11e to
98b3859
Compare
|
Apologies, I went on auto pilot and stopped running the full test suite locally. I can see these errors locally and I'm working on them now. |
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
3 times, most recently
from
ff57cf2 to
84a7c65
Compare
|
@jenkins-plone-org please run jobs |
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
from
84a7c65 to
7fbce08
Compare
|
@jenkins-plone-org please run jobs |
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
from
7fbce08 to
53b936e
Compare
|
@jenkins-plone-org please run jobs |
|
I believe I've addressed all your feedback, @tisto. LMK if I missed anything or if there's anything else I can do to move this forward. Thanks for the review! |
|
I just realized that some of my previous local development build improvements make it pretty easy to create a Zope and Plone instance from before my various auth PRs and then test it under all my PRs. Having done so, it confirms that at least some sort of upgrade step is required. Specifically, because token generation has been moved into |
|
@rpatterson Just out of curiosity, what's your use case? Only authenticated access to Plone Classic? Is there any situation where the anonymous accesses Plone Classic? Does plone api return HTML to Volto? |
I'm on the EEA team, so I'm working under the direction of their leadership. But as I understand it, the use case is to have the experience of using Volto, Plone classic, and the ZMI to be seamless in terms of logging in and out. IOW, it doesn't matter where you log in, you'll be logged in everywhere and ditto for logging out.
I don't really follow these questions. Can you clarify? |
|
What I was wondering, is what is your site's workflow? Do content managers authenticate to Plone Classic to register content (Page, News...)? After this content consumed by Volto to anonymous users? |
|
@wesleybl @rpatterson Although I'm not on the EEA team which is currently handling this feature request, I think I can add some insight. We currently have a Volto website (eea.europa.eu/ims) running as a subfolder of a Plone 4 classic website. The intention is to have editors seamless authentication between the classic Plone and the Volto part. I think the ZMI auth is a nice to have, but I may be wrong. |
Because token generation has been moved into `updateCredentials(...)` [we need an upgrade step](#1303 (comment)) that enables the JWT token plugin for that PAS plugin interface on existing installations in order for authentication to work as before. Also fixes existing plugins outside of a Plone portal that have been configured to use the keyring. I tested this locally by: 1. erasing my local data (ZODB) 2. checking out `master` in the `plone/volto` repo 3. running buildout, including `plonesite` in the API to re-create the portal 4. adding a test user in the Plone portal through the Volto UI 5. add `mr.developer` sources and checkouts in the API buildout 6. disable `plonesite` in the API buildout 7. run buildout to update the code to the PR branches 8. test all the upgrade error conditions around login logout 9. run the `v0006 -> v0007` upgrade step for `plone.restapi:default` 10. confirm all the upgrade error conditions around login logout have been resolved Not that this doesn't address the issue of [existing Zope root `/acl_users/` cookie login set up](#1304 (comment)).
Because token generation has been moved into `updateCredentials(...)` [we need an upgrade step](#1303 (comment)) that enables the JWT token plugin for that PAS plugin interface on existing installations in order for authentication to work as before. Also fixes existing plugins outside of a Plone portal that have been configured to use the keyring. I tested this locally by: 1. erasing my local data (ZODB) 2. checking out `master` in the `plone/volto` repo 3. running buildout, including `plonesite` in the API to re-create the portal 4. adding a test user in the Plone portal through the Volto UI 5. add `mr.developer` sources and checkouts in the API buildout 6. disable `plonesite` in the API buildout 7. run buildout to update the code to the PR branches 8. test all the upgrade error conditions around login logout 9. run the `v0006 -> v0007` upgrade step for `plone.restapi:default` 10. confirm all the upgrade error conditions around login logout have been resolved Not that this doesn't address the issue of [existing Zope root `/acl_users/` cookie login set up](#1304 (comment)).
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
from
53b936e to
ba3561d
Compare
Because token generation has been moved into `updateCredentials(...)` [we need an upgrade step](#1303 (comment)) that enables the JWT token plugin for that PAS plugin interface on existing installations in order for authentication to work as before. Also fixes existing plugins outside of a Plone portal that have been configured to use the keyring. I tested this locally by: 1. erasing my local data (ZODB) 2. checking out `master` in the `plone/volto` repo 3. running buildout, including `plonesite` in the API to re-create the portal 4. adding a test user in the Plone portal through the Volto UI 5. add `mr.developer` sources and checkouts in the API buildout 6. disable `plonesite` in the API buildout 7. run buildout to update the code to the PR branches 8. test all the upgrade error conditions around login logout 9. run the `v0006 -> v0007` upgrade step for `plone.restapi:default` 10. confirm all the upgrade error conditions around login logout have been resolved Not that this doesn't address the issue of [existing Zope root `/acl_users/` cookie login set up](#1304 (comment)).
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
from
ba3561d to
df65b2e
Compare
|
@jenkins-plone-org please run jobs |
I just pushed the upgrade step and related changes. So this should be ready for review and merge. |
The existing Zope root JWT plugin configuration is broken. It is configured to use the `IKeyManager` utility but there is no such utility registered in that context. [An upgrade step](f9097a0) has to be run to fix that. If the browser has previously logged into the Volto UI through the React login component, then the `auth_token` cookie will be set. If that browser is then used to access the ZMI, [the presence of the cookie triggers the token decode code path](9422195), which in turn causes the exception from the broken plugin configuration. Thus any browser logged into the Volto UI may not be able to access the ZMI in order to run the upgrade step that fixes the plugin configuration. Workaround this trap by logging a helpful error instead of causing an exception.
rpatterson
force-pushed
the
fix-unify-auth-logout
branch
from
6bc129b to
85670ea
Compare
|
@jenkins-plone-org please run jobs |
|
I've now run the failing Jenkins job 3 times and it's failing at the same place every time: I've tested that overlay in Plone classic in my local development environment and it seems to be working just fine. I can't imagine how this failure could be related to the changes here and I've been told before that the robot tests are intermittent. So can I either get some help understanding how this is related to the changes here or can we ignore that failure and proceed? CC: @avoinea |
Apparently, the 4th time is the charm. This kind of intermittent CI false-negative is a big problem for an open source community. Personally, I think intermittent false failures/errors/negatives in CI should be immediately "commented out" or otherwise skipped and some part of whatever resources the community or governance has to work with should be dedicated to resolving intermittent CI issues continuously over the longer term. At any rate, AFAIK, this is ready to finish review and merge. LMK if I'm missing anything? CC: @avoinea @rpatterson tiberiuichim @wesleybl @sneridagh |
|
Is anyone able to finish reviewing this or help me draw attention to this to help finish review? CC: @avoinea @tiberiuichim @wesleybl @sneridagh |
|
@rpatterson When I already have the Volto site open in the browser and I authenticate in the Plone Site, when I return to the Volto Site, I do not see the authenticated site. Only after I press F5 on Volto site do I see the authenticated site. Could anything be done? |
|
@wesleybl and vice versa, right? Unless there's no magic notification system like with websockets I fear this is not possible. And I would not expect this. |
|
Sessionstorage (or localstorage) would help here, but not without JS support to poll that storage periodically. |
@jensens vice versa this does not happen. If I have a Volto tab and a Plone tab, without being authenticated to either, after I authenticate to Volto and navigate in Plone, I see Plone authenticated. The Plone site checks the cookie on every request.
I would expect for this. I know several systems that have synchronized authentication, and it is not necessary to press F5 to see the authenticated system. Many users don't know what the F5 does. These users may find that synchronized authentication does not work in some situations. Anyway, I don't think this would be a block. Especially if it harms the Volto's performance. But it would be a plus. |
|
Of course, we still have the cache question, which I don't know how it works in Volto. |
Agreed, @jensens. This might be a nice-to-have feature for some use cases but it's also a significant increase in complexity so I'm not sure how I'd end up voting if reviewing such a PR. Regardless, such behavior should be evaluated in a separate PR and I think this PR is an improvement and results in more correct and less-surprising behavior as is without such additional gloss. At any rate, everything is green in this PR. Should I merge it? Is that something someone else should do and if so, who? Thanks! CC: @avoinea |
|
@jenkins-plone-org please run jobs |
|
Oh, FYI in case it helps anyone save any work with the release process after merge, I'm waiting on this PR to be merged before proceeding with fix(auth): Unify Zope root ZMI w/ API log in/out. CC: @jensens |
|
I would like to have this merged by @tisto or @sneridagh as it is usually done. |
|
@rpatterson if I am not mistaken we agreed, at a @plone/volto-team meeting, that EEA will test this extensively on their live site before we merge this. Did that happen? |
That's not my understanding or recollection, @tisto. How about other EEA folks? I know @alecghica was at that meeting? I think @avoinea was too? I can't recall how well suited the EEA live site deployment story is to using git checkouts with specific branches, nor to run upgrade steps (and install additional profiles for the Zope root work in the other PR). I also know the EEA teams are quite busy, so I would be surprised if this was something that could be done in a timely way. I'm concerned about bit rot. Also, my time dedicated to this unified auth work is running out sometime this month. After that, if I'm able to put any time into this at all, I'll probably have to choose between either shepherding PRs through or making any actual code changes. Given all that, I think there's significant risk that all this work will be wasted if such testing is a requirement for merging. So for my part, I'd rather merge sooner and risk having to get a bug fix out the door than wait for perfect safety to get this merged. What do others think? |
|
We'll come back with a status soon! |
There was a problem hiding this comment.
LGTM.
@rpatterson @mauritsvanrees Just make sure the upgrade step hits plone.app.upgrade / the ADDON_LIST of core profiles with upgrades
Does that mean you've done some sort of additional testing to address @tisto's question, @avoinea? Ditto for #1304? |
|
@rpatterson @alecghica @avoinea IMHO this is something we should rather discuss face-to-face at the next @plone/volto-team meeting. |
Build on #1141 to flesh out the rest of the PAS plugin interfaces that the JWT plugin
should support, namely those methods that handle log in and log out. With these changes
logging in to or out of Plone classic or the Volto UI does the same in the other. This
should result in a much less surprising user experience for those users that will end up
using both interfaces and makes the JWT plugin more correct and complete. See also the
related Volto PR.
Regarding the use case, I understand the intention is to move away from Plone classic as
a supported UI. I think it would be a painful mistake to imagine that's going to be a
reality for all important use cases and user classes anytime soon. We should expect
Volto to be the UI for an increasingly major portion of use cases over time but allow it
time to get to all of them. Personally, I wouldn't plan on classic being something we
could stop supporting for a couple of major versions of Plone.