-
Notifications
You must be signed in to change notification settings - Fork 333
Prefer PolarisPrincipal over SecurityContext #2932
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
Prefer PolarisPrincipal over SecurityContext #2932
Conversation
f887829 to
6af7b28
Compare
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.
SecurityContext is an rs-api (Web / REST framework) class, indeed. Consolidating polaris-core dependencies to just PolarisPrincipal looks very reasonable to me 👍
6af7b28 to
c50f042
Compare
|
rebased after trivial merge conflict in: |
The general idea is that `SecurityContext` comes from `jakarta.ws.rs` and there is no reason for non-REST related classes to rely on those details. Instead, once preprocessing of a REST-request has inferred the `PolarisPrincipal` all inner/core code should rely on only that. Note that this simplifies a bunch of tests that had to create their own `SecurityContext` around the principal that they wanted to use, thus having to decide how to implement `isUserInRole` and the other methods.
c50f042 to
b162544
Compare
snazy
left a comment
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.
Boring - I like it :)
adnanhemani
left a comment
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!
Follow-up to #2925
The general idea is that
SecurityContextcomes fromjakarta.ws.rsandthere is no reason for non-REST related classes to rely on those details.
Instead, once preprocessing of a REST-request has inferred the
PolarisPrincipalall inner/core code should rely on only that.Note that this simplifies a bunch of tests that had to create their own
SecurityContextaround the principal that they wanted to use, thushaving to decide how to implement
isUserInRoleand the other methods.