Skip to content

Conversation

@XN137
Copy link
Contributor

@XN137 XN137 commented Oct 29, 2025

Follow-up to #2925

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.

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Oct 29, 2025
@XN137 XN137 force-pushed the prefer-PolarisPrincipal-over-SecurityContext branch 2 times, most recently from f887829 to 6af7b28 Compare October 31, 2025 10:38
@XN137 XN137 marked this pull request as ready for review October 31, 2025 10:56
dimas-b
dimas-b previously approved these changes Oct 31, 2025
Copy link
Contributor

@dimas-b dimas-b left a 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 👍

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Oct 31, 2025
@XN137 XN137 force-pushed the prefer-PolarisPrincipal-over-SecurityContext branch from 6af7b28 to c50f042 Compare November 3, 2025 16:47
@XN137
Copy link
Contributor Author

XN137 commented Nov 3, 2025

rebased after trivial merge conflict in:

runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogViewTest.java

dimas-b
dimas-b previously approved these changes Nov 3, 2025
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.
@XN137 XN137 force-pushed the prefer-PolarisPrincipal-over-SecurityContext branch from c50f042 to b162544 Compare November 3, 2025 16:56
Copy link
Member

@snazy snazy left a 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 :)

Copy link
Contributor

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@adutra adutra merged commit f934443 into apache:main Nov 4, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Nov 4, 2025
@XN137 XN137 deleted the prefer-PolarisPrincipal-over-SecurityContext branch November 4, 2025 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants