fix(auth): Unify ZMI, HTML form, and API login

[rpatterson]

Fix some behavior in the PAS plugin that treated API login requests differently than
classic HTML form login requests, namely that the POST'ed login credentials weren't
extracted and authenticated like they are when the classic HTML login form is POST'ed.
IMO, the root cause here was a minor case of not actually fulfilling the PAS plugin
interface. In the case of the classic HTML login form, it's another PAS plugin that
extracts the POST'ed credentials, but since the API login POST submits JSON, it has to
be the JWT plugin that extracts them.

Also reproduce the same handling that the classic Plone HTML login form does after login
has succeeded. This includes setting the classic HTML login form cookie so that
authenticating to the API also authenticates the user to the classic HTML UI (as well as
the ZMI when the user has the Manager role). But it also includes other core Plone
login behavior.

In the case of Volto, there's one more edge case remaining. Namely, if the browser is
already authenticated such as by having previously submitted the classic HTML login
form, a fresh load of the Volto UI will still show them as logged out even though their
API requests succeed as authenticated. I assume this is a problem in the React
component state that doesn't do an actual API check to see if the user is already
authenticated and if I'm right a fix for that can't be included here.

---

This is my first Volto-adjacent PR. Please LMK what parts of etiquette I may be unaware
of. Also please pick all the nits possible so I can learn how to better form PRs for
the Volto project going forward. IOW, let me have it! ;-)

[mister-roboto]

@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!

[rpatterson]

@jenkins-plone-org please run jobs

[sneridagh]

Overall LGTM, curious about the use case that triggered this.

@alecghica the feature you told me several weeks ago in the Volto Team meeting was different, right?

@buchi @lukasgraf @tisto It would be great if you could also take a look! Thanks!

[mamico]

In the case of Volto, there's one more edge case remaining. Namely, if the browser is
already authenticated such as by having previously submitted the classic HTML login
form, a fresh load of the Volto UI will still show them as logged out even though their
API requests succeed as authenticated. I assume this is a problem in the React
component state that doesn't do an actual API check to see if the user is already
authenticated and if I'm right a fix for that can't be included here.

@rpatterson, time ago I started with https://github.com/plone/plone.restapi/tree/mamico/ext-login that probably could
cover the above use case and different other use cases, like external authentication method (e.g. pas.plugin.authomatic).
Any hints would be appreciated.

[alecghica]

Overall LGTM, curious about the use case that triggered this.

@alecghica the feature you told me several weeks ago in the Volto Team meeting was different, right?

It's related to that.

[rpatterson]

I believe I've addressed all the feedback, @tiberiuichim @tisto. LMK if I missed anything. Otherwise, what are the next steps? I assume a review approval. After that, for this project is the merge button something I push? Any follow up steps expected of me?

[rpatterson]

@jenkins-plone-org please run jobs

[rpatterson]

@rpatterson, time ago I started with https://github.com/plone/plone.restapi/tree/mamico/ext-login that probably could
cover the above use case and different other use cases, like external authentication method (e.g. pas.plugin.authomatic).
Any hints would be appreciated.

Thanks for reaching out, @mamico! First, a minor nit pick, it looks like your 2nd commit is just a line ending change for the 1st commit so I assume they should be squashed together?

Is the auth_token cookie already handled in the Volto auth components? I thought Volto was using the Authorization header? Does it use both? If you happen to know and have the time to educate me, that would be a favor.

Proceeding from there assuming that setting that cookie has the desired effect in the Volto components, then I would say your event subscriber code should be converted to a plone.restapi.pas.plugin:JWTAuthenticationPlugin.updateCredentials(...) method. If done there then portals can use PAS configuration to opt in/out of this behavior like they do for other PAS plugins. See Products.PluggableAuthService.plugins.CookieAuthHelper:CookieAuthHelper.updateCredentialsupdateCredentials(...) for an example. If done that way, I would describe what your code is trying to do would make the JWT PAS plugin a more complete implementation of the PAS plugin interface. IOW, sounds like an improvement to me.

I haven't thought about your code in depth as I would for a PR review so just request a review from me in a PR if you'd like a more detailed evaluation. One potential issue jumps out at me, how would your code set the same JWT token in both the cookie and return it in the JSON response to a login POST?

[tisto]

@rpatterson first of all, thanks for your PR and the time you put into discussing our nitpicking comments. The PR looks good to me. I'd just like to give it a shot locally and maybe @buchi and/or @lukasgraf to have a second look. Then we are good to merge and I can do a release right away. There is no further action from your side required I guess.

@buchi @lukasgraf let us know if you want this fix in plone.restapi 7.x. Since we started the new Plone 5.2/Python3 branch (8.x.x) we have to decide if we want this fix there as well.

[rpatterson]

...There is no further action from your side required I guess.

K, works for me. ;-) Quick caveat, I just tuned into the CI failure from the run I kicked off after my latest push. The output doesn't say much to me so I can't even argue whether or not it's related to my changes. I've never gotten to fluency with Robot tests so it may take me time to understand this failure. OTOH, I've wanted to achieve fluency there so if someone thinks this is related, give me some hints and I'll get to it! Otherwise I'll assume this PR is in others hands until I hear otherwise.

[alecghica]

let us know if you want this fix in plone.restapi 7.x.

@tisto would be great to have it for plone.restapi 7.x.

[wesleybl]

I can't even argue whether or not it's related to my changes.

@rpatterson It's common for some robot tests to fail on Jenkins unrelated to your PR. When this happens, you have to restart the job that failed. To do this, access the job that failed, click on "Login" to authenticate, click on the menu "job number" -> Retry

[mamico]

@rpatterson thanks,

Is the auth_token cookie already handled in the Volto auth components? I thought Volto was using the Authorization header? Does it use both? If you happen to know and have the time to educate me, that would be a favor.

AFAIK, Volto uses the token in the Authorization header when calling the restapi backend, but the token is actually stored in a browser session cookie.
I'm using this approach in a working in progress pas plugins and ... it seems to works fine. With this implementation in p.restapi, some code there could be removed.

Proceeding from there assuming that setting that cookie has the desired effect in the Volto components, then I would say your event subscriber code should be converted to a plone.restapi.pas.plugin:JWTAuthenticationPlugin.updateCredentials(...)..

Good point, I'll try to think about it...

One potential issue jumps out at me, how would your code set the same JWT token in both the cookie and return it in the JSON response to a login POST?

Sorry, I don't understand this last point ... the main picture is: whichever way you authenticate with a legacy plone pas plugin, if correctly the IUserLoggedInEvent event will be called, this code will return to the user a valid auth_token cookie, this is all Volto needs for subsequent calls to the backend.

[rpatterson]

I can't even argue whether or not it's related to my changes.

@rpatterson It's common for some robot tests to fail on Jenkins unrelated to your PR. When this happens, you have to restart the job that failed. To do this, access the job that failed, click on "Login" to authenticate, click on the menu "job number" -> Retry

Thanks! I didn't know I could log into Plone Jenkins. I've restarted it.

To put in my $0.02 that no one asked for, I think an intermittent test is often worse than no test. It frustrates the community of developers and trains them to ignore tests over time. We want our tests to help developers fall in love with testing. This goes double for CI. Does Robot Framework have an equivalent of Python's unittest.TestCase.skipTest(...)?

[rpatterson]

Sorry, I don't understand this last point ...

To clarify, @mamico. Doesn't the login API endpoint view from plone.restapi call plugin.create_token(...) as well and retrun that one in the JSON response? Woudn't that be a different token from the one you set in the cookie?

[mamico]

Sorry, I don't understand this last point ...

To clarify, @mamico. Doesn't the login API endpoint view from plone.restapi call plugin.create_token(...) as well and retrun that one in the JSON response? Woudn't that be a different token from the one you set in the cookie?

@rpatterson. It's the same token, in fact I use the same plugin.create_token(...) as the login API. The only difference is that, using the Volto form login, Volto (js on the browser) call the login API and set the token (in the JSON response) in a cookie for persistence. Using the "legacy" Plone login the code that I propose set, in an HTTP response, the same type of token in the same cookie, and Volto next use it.

[rpatterson]

Apologies for the back and forth, @mamico. Maybe you could create a PR, label it WIP and we can continue discussion there?

@rpatterson. It's the same token, in fact I use the same plugin.create_token(...) as the login API.

Yeah, I saw that, but I thought when I read the def create_token(...): method code that it created a new, different token for each call. Did I read that code wrong? Very possible. ;-)

[rpatterson]

Just a quick reminder, is anyone able to review? (CC: @tisto, @buchi, @sneridagh, @lukasgraf)

[rpatterson]

@jenkins-plone-org please run jobs

[tisto]

@rpatterson @avoinea @tiberiuichim @alecghica we run into some issues with this fix in one of our main Volto sites. The problem can be reproduced as follows:

1. Login as user "myuser" in Volto
2. The user is automatically logged in in "/api" (Plone classic)
3. Logout in Volto
4. The user "myuser" is still logged in in "/api" (Plone classic)

-> when you login again (e.g. with another user) you end up with mixed auth that does all kinds of strange things.

@sneridagh tried to fix this issue recently but it seems this did not do the trick.

I am considering reverting this PR and do another plone.restapi release to fix this (IMHO serious problem), to give us some time to investigate the issue. Opinions?

[tiberiuichim]

I think the problem is that the Volto part is missing, which should have logged you out of Plone as well. I think a simple Volto fix could be implemented, to delete the __ac cookie. Hardcoded for one auth method, but it would work.

[tisto]

@tiberiuichim this is exactly what @sneridagh did if I am not mistaken. This did not solve the issue though. :/

[avoinea]

@tisto It didn't solve all the issues within Volto, but I think it will solve the issue you described above.

[tisto]

@avoinea @tiberiuichim this still happens in latest Volto. Do the following:

1. Open https://volto.kitconcept.com/ and https://volto.kitconcept.com/api (make sure you are logged out)
2. Login in https://volto.kitconcept.com/ (as user: timo:password)
3. Go to https://volto.kitconcept.com/api (you are logged in)
4. Logout in https://volto.kitconcept.com/
5. Go to https://volto.kitconcept.com/api (user timo is still logged in)

The ac_ cookie is still present (on both):

[avoinea]

@tisto I know, the Remove __ac cookie fix is not yet in Volto. @sneridagh tried something, but there where some other issues and reverted.

[rpatterson]

I think I've resolved all the outstanding issues brought up here in #1303, @avoinea @tisto @tiberiuichim. LMK if I'm missing something!