plone
fix(auth): Unify Zope root ZMI w/ API log in/out
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 to
successfully 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 and
plone.restapi that result in different, IOW fixed, PAS plugin configurations. As
such, 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.
@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:
@jenkins-plone-org please run jobs
With this simple comment all the jobs will be started automatically.
Happy hacking!
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 Products.PlonePAS PR. How can I see how the Jenkins jobs are defined? What's the right way to tell Plone Jenkins to also use a checkout of another package?
What's the right way to tell Plone Jenkins to also use a checkout of another package?
I found the following:
- clicked around the Jenkins "UI" for the "Pull Request 6.0 on py3.9" job
- found the "Build with Parameters" form
- that form has some small text on it including a link to some "Run pull request jobs" docs
- those docs mention putting multiple PR URLs in the build parameters
How can I see how the Jenkins jobs are defined?
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 buildout:auto-checkout line for a PR URL and that is invoked for each PR URL in the build parameters.
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).
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 /acl_users in a new instance into a separate profile.
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.
From Volto Framework meeting, move changes to the Zope root
/acl_usersin a new instance into a separate profile.
Since these changes aren't specific to the JWT token plugin, I moved these changes to Products.PlonePAS, which seems like the most appropriate place. Unfortunately, CI seems broken ATM, so I'm a bit at a loss of how to proceed.
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:
diff --git a/src/plone/restapi/tests/http-examples/workingcopy_baseline_get.resp b/src/plone/restapi/tests/http-examples/workingcopy_baseline_get.resp
index 70ab31e..683549c 100644
--- a/src/plone/restapi/tests/http-examples/workingcopy_baseline_get.resp
+++ b/src/plone/restapi/tests/http-examples/workingcopy_baseline_get.resp
@@ -47,7 +47,7 @@ Content-Type: application/json
"locked": true,
"name": "iterate.lock",
"stealable": false,
- "time": 807237000.0,
+ "time": 807211800.0,
"timeout": 4294967280,
"token": "0.12345678901234567-0.98765432109876543-00105A989226:1630609830.249"
},
So someone may want to look into that. In the meantime, I'm re-running the tests.
Another of the test failures I see doesn't give output that indicates what might be wrong:
...
Failure in test test_endpoint_shows_field_config (plone.restapi.tests.test_services_querystring.TestQuerystringEndpoint)
Traceback (most recent call last):
File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
yield
File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/unittest/case.py", line 676, in run
self._callTestMethod(testMethod)
File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
method()
File "/home/runner/work/plone.restapi/plone.restapi/src/plone/restapi/tests/test_services_querystring.py", line 67, in test_endpoint_shows_field_config
self.assertEqual(expected_field_config, idx)
File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/unittest/case.py", line 912, in assertEqual
assertion_func(first, second, msg=msg)
File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/unittest/case.py", line 1211, in assertDictEqual
self.fail(self._formatMessage(msg, standardMsg))
File "/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/unittest/case.py", line 753, in fail
raise self.failureException(msg)
AssertionError: {'des[58 chars]ue, 'group': 'Text', 'operations': ['plone.app[303 chars]None} != {'des[58 chars]ue, 'fetch_vocabulary': True, 'group': 'Text',[329 chars]None}
Diff is 680 characters long. Set self.maxDiff to None to see it.
...
Neither can I reproduce the failure locally:
$ ./bin/test -m plone.restapi.tests.test_services_querystring
...
Ran 12 tests with 0 failures, 0 errors and 0 skipped in 1.521 seconds.
...
So I'm assuming this is also unrelated to my changes. Please LMK if there's more I should do about this.
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:
See: https://github.com/plone/plone.restapi/pull/1215#pullrequestreview-746442902
Maybe it's interesting to create an issue about it. Until then, you cannot commit your local changes.
See: #1215 (review)
Thanks for the context!
Until then, you cannot commit your local changes.
I'm sorry, can you clarify what you mean here? The PR you linked to looks like it was merged some time ago. So until what?
I'm sorry, can you clarify what you mean here? The PR you linked to looks like it was merged some time ago. So until what?
We need to find out if the timezone really shouldn't interfere with these date or if this is an error. If it's a error, it needs to be fixed. If it is not an error, the test needs to be fixed by normalizing the value so that they are always the same, as was done in #1215.
We need to find out if the timezone really shouldn't interfere with these date or if this is an error. If it's a error, it needs to be fixed. If it is not an error, the test needs to be fixed by normalizing the value so that they are always the same, as was done in #1215.
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 workingcopy_baseline_get.resp.
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
workingcopy_baseline_get.resp.
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.