-
Notifications
You must be signed in to change notification settings - Fork 365
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!
* Pass AccessConfig into FileIOFactory (apache#2937) it should not be the responsibility of the `FileIOFactory` to know how to infer the `AccessConfig` * Remove EclipseLink Persistence Backend (apache#2963) as per ML decision the deprecated eclipselink backend is being removed in the next version: https://lists.apache.org/thread/16bj5kngf2kfhqv3noxwfm7h9wlzvhyv * feat: Improve PolarisAdminTool default output (apache#2961) * feat: Improve PolarisAdminTool default output * feat: Improve PolarisAdminTool default output * Add Polaris Community Meeting 2025-10-30 (apache#2973) * Remove legacy management endpoints (apache#2276) * Build/nit: cache output of `generatedMarkdownDocs` (apache#2967) `(Java)Exec` tasks are not cacheable by default, as annotated with `@DisableCachingByDefault`. Adding an `outputs.cacheIf { true }` enables caching on those tasks. * Build: Helper to get effective ASF project metadata (apache#2969) This change "bundles" the information of `AsfProject` and the `PublishingHelperExtensions`, which is what the code in `configurePom.kt` did. Bundling these objects allows other consumers, like CycloneDX SBOM generation, to access that same information without having to query remote systems (whimsey.apache.org) again. * Alternative, concise PR template (apache#2945) This PR proposes an alternative PR template that is much shorter, and removes all the redundant claims. It also links to the contribution guidelines for further guidance. * Add docs how to add `Server` header to HTTP responses (apache#2941) * Prefer PolarisPrincipal over SecurityContext (apache#2932) 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. * Last merged commit f934443 --------- Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: fivetran-arunsuri <[email protected]>
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.