-
-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(auth): Unify Zope root ZMI w/ API log in/out #1304
base: main
Are you sure you want to change the base?
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! |
45b7cc6
to
85261dd
Compare
@jenkins-plone-org please run jobs |
The CI failures from the Plone Jenkins jobs are the same ones you get if you run the tests without the corresponding changes from the corresponding |
I found the following:
|
While digging around the above, I also found the Plone Jenkins repo including a file that seems to define the Jenkins jobs which seems to call a script that writes a |
Ready for review. |
I just confirmed that an upgrade step is required at least to update existing JWT token PAS plugins. That still leaves the question of whether we want or need an upgrade step to existing Zope root cookie plugins. On that matter, it still stands that if we don't need one (existing installations will be no worse than they were before without an Zope root cookie upgrade step) but we do want one then I still advocate for merging these PRs (once the required upgrade step is in place) and tackling such an optional upgrade step in separate PR(s). |
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)).
2208db5
to
7c4f359
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)).
7c4f359
to
120738d
Compare
@jenkins-plone-org please run jobs |
I've worked out all the remaining issues I'm aware of, so I think this is ready for review and merge. |
From Volto Framework meeting, move changes to the Zope root |
As with this we're moving away from Basic-Auth at the Zope root level, @plone/framework-team should also have a look into it. |
Move the change of the default Zope root configuration from HTTP Basic auth to the cookie login form [into a separate GenericSetup upgrade step to make the change optional](plone/plone.restapi#1304 (comment)). This reverts commit 132c2c390801ff16393f214c1501252b240cb62a.
Move the change of the default Zope root configuration from HTTP Basic auth to the cookie login form [into a separate GenericSetup upgrade step to make the change optional](plone/plone.restapi#1304 (comment)). This reverts commit 132c2c390801ff16393f214c1501252b240cb62a.
fa95a84
to
ef3f9ce
Compare
Since these changes aren't specific to the JWT token plugin, I moved these changes to |
ef3f9ce
to
9e23e7e
Compare
One of the test failures I saw is something I've seen locally intermittently. Something in the tests makes changes to files in VCS, which seems like a big no-no to me:
So someone may want to look into that. In the meantime, I'm re-running the tests. |
When logging out of Plone via the API endpoint or through the classic logout link, all PAS plugins should reset all credentials.
The Plone login handling code depends on the user's password being at the same place in the request as the classic Plone login form puts it in order to set the correct authentication cookie. Without it, when logging in via the Volto UI component as a user from the Zope root `acl_users` (e.g. `admin` or `SITE_OWNER_NAME`), they aren't logged into Plone classic. The other direction is fine, logging in as `admin` to Plone classic results in a new request to the Volto UI being logged in. Fix that edge case by mimicking the request keys of the login form after parsing the login POST JSON body.
An error that indicates incorrect configuration, set up, or otherwise something an administrator would probably want to know, should also be logged to the server logs. Those log messages should including enough information for an administrator to find the specific instance of the issue (e.g. which Plone portal in a multi-portal ZODB).
Don't assume that all Plone portals will be installed directly into the Zope root or that all ancestors above the Plone portal will have `./acl_users`. I don't have a case for this, I just noticed it when I was reading the code while working on Zope root auth issues. Also clarify the PAS install plugin process with comments.
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)).
Who is "we"? Does this mean someone else is working on that issue right now and that this PR is just on hold until that's done? Or are you saying that you consider determining and fixing that to be a part of this PR? |
Fixing this issue doesn't have to be part of this PR. The only thing you need to do is not commit the date change in |
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)).
Oh, I see what you're saying now! Committing that change was unintentional and I wasn't aware I'd done so, which explains the confusion. Backing it out now. |
9e23e7e
to
971b3b1
Compare
@jenkins-plone-org please run jobs |
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.
971b3b1
to
8d7cfdc
Compare
@jenkins-plone-org please run jobs |
Move the change of the default Zope root configuration from HTTP Basic auth to the cookie login form [into a separate GenericSetup upgrade step to make the change optional](plone/plone.restapi#1304 (comment)). This reverts commit 132c2c390801ff16393f214c1501252b240cb62a.
Branch: refs/heads/master Date: 2022-02-13T16:01:07-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@7e6d9d5 feat(setup): Zope root Basic -> cookie login form Improve the Zope root ZMI login UX, avoid all the HTTP `Authorization: Basic ...` edge cases and hassles. Switch the default authentication challenge for the Zope root `/acl_users` from HTTP `Authorization: Basic ...` to the cookie auth plugins basic login form. This should be a much better UX overall and shouldn't cause any fundamental issues. One can still use HTTP `Authorization: Basic ...` manually by adding credentials to the URL: http://admin:secret@localhost:8080/manage_main But may cause issues where tests expect the HTTP `Authorization: Basic ...` challenge response or existing uses where new Zope instances are created as a part of normal use (SAAS?). We could also consider adding an upgrade step to make this change to existing installations but that would be disruptive to any existing installations that require HTTP `Authorization: Basic ...`. I can't imagine why that would be, but we should probably expect those use cases to come out of the woodwork once an upgrade step is released. Files changed: M src/Products/PlonePAS/setuphandlers.py M src/Products/PlonePAS/tests/test_setup.py Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-02-13T16:01:07-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@660343d style(lint): Cleanup/ignore common linter errors These are linters that my editor uses by default because they are common linters, so might as well fix what we can and ignore the rest. As much as possible, I placed ignores that should apply across the code base in the appropriate configuration file rather than inline comments. Files changed: A mypy.ini M pyproject.toml M setup.cfg M src/Products/PlonePAS/interfaces/memberdata.py M src/Products/PlonePAS/interfaces/membership.py M src/Products/PlonePAS/patch.py M src/Products/PlonePAS/plugins/ufactory.py M src/Products/PlonePAS/sheet.py M src/Products/PlonePAS/tests/test_setup.py M src/Products/PlonePAS/tools/membership.py Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-02-13T16:01:07-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@955a276 refactor(setup): More sensible import step define Files changed: M src/Products/PlonePAS/configure.zcml M src/Products/PlonePAS/profiles.zcml Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-02-25T01:24:34-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@13c1766 feat(setup): Zope root cookie login form profile Move the change of the default Zope root configuration from HTTP Basic auth to the cookie login form [into a separate GenericSetup upgrade step to make the change optional](plone/plone.restapi#1304 (comment)). This reverts commit 132c2c390801ff16393f214c1501252b240cb62a. Files changed: A news/zope-root-cookie.feature A src/Products/PlonePAS/profiles/root-cookie/metadata.xml A src/Products/PlonePAS/profiles/root-cookie/plone-pas-zope-root-cookie.txt M src/Products/PlonePAS/profiles.zcml M src/Products/PlonePAS/setuphandlers.py M src/Products/PlonePAS/tests/test_setup.py Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-02-25T01:24:34-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@1e9cd05 fix(setup): Existing broken Zope root cookie [A previous PR fixed the broken Zope root cookie plugin for new installs](plone/Products.PlonePAS@17deb97) but didn't include an upgrade step for existing Zope instances/ZODBs. The issue is only revealed when `IChallengePlugin` is activated for the broken plugins, such as when the `Products.PlonePAS:root-cookie` profile is installed, and [a `Manager` tries to login to](plone/Products.PlonePAS#66 (comment)) the [Zope root ZMI](http://localhost:8080/manage_main). Add an upgrade step that fixes the issue for existing instances/ZODBs. Files changed: A src/Products/PlonePAS/upgrades.py M src/Products/PlonePAS/profiles.zcml M src/Products/PlonePAS/profiles/default/metadata.xml Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-02-25T01:57:21-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@22ba99f fix(setup): Use new pre/post profile handlers [Per feedback](plone/Products.PlonePAS#66 (comment)), use the new (to me) `post_handler` feature provided by `GenericSetup` as it is much better than littering the import step registry with one-off, profile-specific import steps. Files changed: M src/Products/PlonePAS/profiles.zcml M src/Products/PlonePAS/setuphandlers.py Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-03-08T12:53:01-08:00 Author: Ross Patterson (rpatterson) <[email protected]> Commit: plone/Products.PlonePAS@9810cde style(lint): Backout Python 2 compat lint cleanup [Per feedback](plone/Products.PlonePAS#66 (comment)), back out the changes that are related to Python 2 compatibility since we no longer support versions before Python 3.6. I briefly evaluated actually removing the Python 2 compatibility for these lines, but I note that `six` is still in `install_requires` for the package/dist dependency metadata so I think there's significant work to be done to cleanup no longer needed compatibility code and I don't want to hold up this PR. Files changed: M src/Products/PlonePAS/plugins/ufactory.py M src/Products/PlonePAS/sheet.py Repository: Products.PlonePAS Branch: refs/heads/master Date: 2022-03-09T00:03:21+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/Products.PlonePAS@bd1bcf0 Merge pull request #66 from plone/feat-zope-root-cookie-challenge feat(setup): Zope root cookie login form profile Files changed: A mypy.ini A news/zope-root-cookie.feature A src/Products/PlonePAS/profiles/root-cookie/metadata.xml A src/Products/PlonePAS/profiles/root-cookie/plone-pas-zope-root-cookie.txt A src/Products/PlonePAS/upgrades.py M pyproject.toml M setup.cfg M src/Products/PlonePAS/configure.zcml M src/Products/PlonePAS/interfaces/memberdata.py M src/Products/PlonePAS/interfaces/membership.py M src/Products/PlonePAS/patch.py M src/Products/PlonePAS/profiles.zcml M src/Products/PlonePAS/profiles/default/metadata.xml M src/Products/PlonePAS/setuphandlers.py M src/Products/PlonePAS/sheet.py M src/Products/PlonePAS/tests/test_setup.py M src/Products/PlonePAS/tools/membership.py
8d7cfdc
to
fa939b6
Compare
@jenkins-plone-org please run jobs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@rpatterson @mauritsvanrees Just make sure the upgrade step hits plone.app.upgrade / the ADDON_LIST of core profiles with upgrades
Chanced across this while updating the `Products.PluggableAuthService` version. The `./versions.cfg` file isn't referenced anywhere that I can see. A `./versions.cfg` file is a convention in the Plone world so it's misleading to have an unused one lying around. A developer might reasonable assume that a change in that file should take effect.
Outside of the context of a Plone site, there usually isn't a `plone.keyring.interfaces.IKeyManager` but the GenericSetup "various" import step that adds the JWT token plugin to the Zope root `/acl_users` leaves the default keyring plugin setting which results in the following when authenticating to the Zope root: 2021-12-27 11:25:39,451 ERROR [Zope.SiteErrorLog:22][waitress-3] 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>, '') Fix this by doing an interface for the Plone portal and changing that configuration setting if not being installed into a Plone portal.
Integrate upstream `Products.PluggableAuthService` and `Products.PlonePAS` changes to the Zope root `/acl_users`: - fix some broken plugin configuration on install and upgrade - add a `GenericSetup` profile to convert to a simple cookie login form which fixes logout issues when used with other plugins such as the JWT token plugin Note that the only changes here are adding test coverage confirming the upstream changes have the intended effect when combined with the JWT token plugin. Upstream links: zopefoundation/Products.PluggableAuthService#107 (comment) zopefoundation/Products.PluggableAuthService@44ac67f
fa939b6
to
054fe37
Compare
These changes resolve all the remaining authentication, login, logout edge cases I'm
aware of, in this case those cases involving authentication at the Zope root
/acl_users
level, typically for ZMI access. With these changes I'm able tosuccessfully perform all 4 actions of the matrix of login, logout, Zope root ZMI cookie
login form, and Volto login component trough API login endpoint. The UX of each action
in that matrix feels good and clear, or at least no worse than the surrounding
experience (coughZMIcough).
IOW, I'm doing my happy dance right now. ;-) I'm sure there are remaining edge cases
to be found but having my head around all the involved bits now, I'm pretty confident
they'll be pretty incremental to fix from here.
These changes involve changes to the GenericSetup install steps in both
Products.PlonePAS
andplone.restapi
that result in different, IOW fixed, PAS plugin configurations. Assuch, all of the fixes should probably be tested on a fresh Plone+Volto install.
LMK if it's important to automatically make the changes to existing installations and
I'll start work on an upgrade step. Such migrations/upgrades can be very tricky and can
take a long time to shepherd through to release. As such, I strongly prefer and hope we
don't require an upgrade step. To that end, please test for regressions in existing
installs as much as possible. If there aren't important regressions in existing
installs and we decide and upgrade step would be a good thing to offer to existing
installs, then I strongly suggest we consider that to be a separate task, a separate PR,
IOW that we don't hinge merging this PR on a finished upgrade step.
Note that this PR is now stacked on top of 2 other PRs in this repo awaiting merge.