Support non-numeric OIDC principal ID claims in the default principal mapper - #5575
zhang-arvin wants to merge 4 commits into
Conversation
…lt mapper The default OIDC PrincipalMapper converted the configured ID claim with Long.parseLong, so identity providers issuing non-numeric IDs, such as the UUID-based sub claim of Keycloak, failed the authentication with a NumberFormatException. Numeric claim values keep their exact previous behavior (numbers are read as longs, numeric strings are parsed). Non-numeric values, including those that do not fit into a long, are no longer an error: the ID mapping yields nothing and the principal is resolved through the configured name claim instead. The ID claim path documentation and application.properties comments now state that the claim must be numeric and that a name claim is required for identity providers issuing non-numeric IDs.
|
@zhang-arvin As I explained in #5569, I don't think this goes in the right direction. |
adutra
left a comment
There was a problem hiding this comment.
The ID claim path is optional. If no numerical ID claim exists, simply doesn't set the ID claim path at all.
Document that the id claim must carry a numeric value and that non-numeric claims are skipped in favor of the name claim path.
…principal-id # Conflicts: # runtime/service/src/main/java/org/apache/polaris/service/auth/external/mapping/PrincipalMapper.java # site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_oidc.md
|
You're right, and thanks for pointing back to #5569 — I had read the report but not your comment on it, which is the part that settles this. I've now read it: the ID claim path is optional and using Keycloak means leaving Closing this as not a fix for #5569. My apologies for the review noise. One thing worth keeping from the attempt, only if you think it is worth its own change: the failure mode was a thrown |
Fixes #5569
The default OIDC principal mapper assumed the configured ID claim is convertible to a Java
long:An identity provider that issues a non-numeric subject — Keycloak's
subis a UUID — therefore fails theconversion, and the Keycloak integration example can only work by hard-coding
principal_id = 0, whichcollapses every user onto one principal in a real multi-user deployment.
Why this does not need an interface change
PrincipalMapper.mapPrincipalIdalready returnsOptionalLong, andDefaultAuthenticatoralready fallsback to
findPrincipalByNamewhen the mapped ID is empty. The mapper was simply throwing instead ofyielding empty. Non-numeric and non-
long-representable claims now yield empty (with a debug log) so theprincipal resolves by name, while numeric claims — both
Numberand numericString— keep byte-identicalbehavior, including the existing rejection of values outside the
longrange.What changed
DefaultPrincipalMapper— tolerate non-numeric ID claims instead of throwing; numeric behavior unchanged.PrincipalMapper.mapPrincipalIdandOidcTenantConfiguration#idClaimPathdocumenting theoptional/non-numeric contract.
DefaultPrincipalMapperTest— UUID-style claim, plus empty, non-numeric, short and overflow cases, and aUUID-to-name fallback test.
Verification
./gradlew format compileAll— clean (Java 21).:polaris-runtime-service:test --tests DefaultPrincipalMapperTest— 12 tests, 0 failures.Notes
The issue asks for UUID/String principal IDs to be usable. This change makes such an IdP configuration work
by resolving the principal through its name claim rather than by extending the principal ID type; if
maintainers would rather thread a string principal identifier through the model, that is a larger change to
PrincipalEntityand the persistence layer and I am happy to follow a different direction.