Conversation
Replace the three-augmentor chain (`OidcTenantResolvingAugmentor` -> `OidcPolarisCredentialAugmentor`-> `AuthenticatingAugmentor`) with a single `PolarisSecurityIdentityAugmentor` that delegates OIDC preparation to a new `OidcIdentityPreparer` CDI bean, eliminating the implicit priority-based ordering.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Update developer security documentation to remove obsolete links and describe the consolidated authentication workflow.
Review effort: Lite
Findings: None
What changed in this PR
Consolidates the OIDC authentication augmentor chain into PolarisSecurityIdentityAugmentor, delegating preparation to OidcIdentityPreparer.
Changes:
- Removes priority-based augmentor ordering.
- Moves tenant resolution and credential mapping into a CDI preparer.
- Updates related mappings and tests; removes obsolete augmentor code.
| File | Description |
|---|---|
runtime/service/src/test/java/org/apache/polaris/service/auth/PolarisSecurityIdentityAugmentorTest.java |
Tests consolidated authentication. |
runtime/service/src/test/java/org/apache/polaris/service/auth/external/OidcTenantResolvingAugmentorTest.java |
Removes obsolete augmentor tests. |
runtime/service/src/test/java/org/apache/polaris/service/auth/external/OidcIdentityPreparerTest.java |
Tests OIDC preparation. |
runtime/service/src/test/java/org/apache/polaris/service/auth/external/mapping/DefaultPrincipalRolesMapperTest.java |
Updates mapper configuration tests. |
runtime/service/src/test/java/org/apache/polaris/service/auth/external/mapping/DefaultPrincipalMapperTest.java |
Updates mapper configuration tests. |
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisSecurityIdentityAugmentor.java |
Coordinates preparation and authentication. |
runtime/service/src/main/java/org/apache/polaris/service/auth/external/tenant/OidcTenantResolvingAugmentor.java |
Removes obsolete augmentor. |
runtime/service/src/main/java/org/apache/polaris/service/auth/external/OidcIdentityPreparer.java |
Resolves tenants and creates credentials. |
runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/DefaultPrincipalRolesMapper.java |
Updates tenant configuration lookup. |
runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/DefaultPrincipalMapper.java |
Updates tenant configuration lookup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return QuarkusSecurityIdentity.builder(preparedIdentity) | ||
| .setAnonymous(false) | ||
| .setPrincipal(polarisPrincipal) | ||
| .addRoles(polarisPrincipal.getRoles()) |
There was a problem hiding this comment.
minor: Do we still need Polaris roles in QuarkusSecurityIdentity? It does not look like they are referenced after augmentation (OidcIdentityPreparer.prepare()) 🤔 Is any case they can be accessed via the polarisPrincipal 🤔
There was a problem hiding this comment.
In theory, the roles exposed in QuarkusSecurityIdentity are the only ones that Quarkus recognizes. If we were using Quarkus RBAC, these roles would be essential for things like @RolesAllowed to function properly.
Granted, we are not using Quarkus RBAC – or rather, we are, but just a tiny bit of it. We only use @RolesAllowed("**") so far; these appear in our generated REST interfaces. These annotations essentially are saying "any authenticated principal is allowed".
Se yes, we could remove those roles given our current usage of Quarkus RBAC – but it just feels wrong to me, and could bite us back in the future. WDYT?
| */ | ||
| @ApplicationScoped | ||
| public class OidcPolarisCredentialAugmentor implements SecurityIdentityAugmentor { | ||
| public class OidcIdentityPreparer { |
|
|
||
| private SecurityIdentity authenticatePolarisPrincipal(SecurityIdentity identity) { | ||
| PolarisPrincipal polarisPrincipal = authenticator.authenticate(identity); | ||
| SecurityIdentity preparedIdentity = oidcIdentityPreparer.prepare(identity); |
There was a problem hiding this comment.
Not a blocker: Does it make sense to move the internal authentication logic out of class OidcIdentityPreparer? like this
SecurityIdentity preparedIdentity =
identity.getPrincipal() instanceof JsonWebToken
? oidcIdentityPreparer.prepare(identity)
: identity;
| // by the AuthenticatingAugmentor, which will also validate them. | ||
| return QuarkusSecurityIdentity.builder(identity).addCredential(credential).build(); | ||
| // Note: we don't change the identity roles here; this is done later by | ||
| // AuthenticatingAugmentor, which also validates them. |
There was a problem hiding this comment.
I think we need to update this AuthenticatingAugmentor to PolarisSecurityIdentityAugmentor . The class got renamed in this PR
Replace the three-augmentor chain (
OidcTenantResolvingAugmentor->OidcPolarisCredentialAugmentor->AuthenticatingAugmentor) with a singlePolarisSecurityIdentityAugmentorthat delegates OIDC preparation to a newOidcIdentityPreparerCDI bean, eliminating the implicit priority-based ordering.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)