Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(auth): Sync classic/API PAS plugin cookies #1303
base: main
Are you sure you want to change the base?
feat(auth): Sync classic/API PAS plugin cookies #1303
Changes from 14 commits
d55c7ae
6f4b1b5
2668766
9422195
d9ee713
c941bf2
1228b58
68e9cdb
75aa812
895eeed
03dd65a
c043475
85670ea
f0dba92
e5f5ca1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should these plugins really be enabled by default? Isn't this a specific use case? For those who don't care about integrated authentication, wouldn't that be an unnecessary burden (less performance)?
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.
Yes. The purpose of these changes and this PR is to keep login/logout state in sync between plugins. Those interfaces are the ones that are responsible for that.
This is the common use case. A user who is accessing Plone from multiple interfaces, such as Plone Classic and Volto, and those interfaces use different PAS plugins, such as a cookie plugin and a JWT token plugin, that user has every reason to expect that logging out of one also logs them out of the other. This is the least surprising behavior and as such should be the default.
We can be grateful to PAS for making that a simple matter of deactivating the plugins for those interfaces in that specific use case. ;-)
Login/logout are rare events and as such not a significant impact on the user experience of performance. Both of these operations for both of these plugins are also very cheap and as such wouldn't impact performance anyways. Nor do I know of any plugin for which these operations are other than very cheap.
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.
Feel free to unresolve with details if this still is blocking merge!
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.
Well I'll mark it as unresolved so other people can see your points. But I don't think this is a blocker. I just wish other people would give their opinions too.
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.
Optimization for speed without any measurements does not make sense. I personally doubt this will effect performance on a significant level. Proof me wrong.