Introduce ExternalAuthenticator and isExternal flag on PolarisCredential
#3250
Conversation
| // if the principal was not found, we can end right there | ||
| if (this.resolvedCallerPrincipal == null | ||
| || this.resolvedCallerPrincipal.getEntity().isDropped()) { | ||
| if (isExternalPrincipal()) { |
There was a problem hiding this comment.
can this be true without returning on line 759?
There was a problem hiding this comment.
It cannot - this was before I moved the condition upto line 748 to avoid principal resolution altogether if it is external. I'll remove this check here to remove redundancy
| List<ResolvedPolarisEntity> toValidate) { | ||
|
|
||
| if (isExternalPrincipal()) { | ||
| PrincipalEntity externalPrincipal = createExternalPrincipalEntity(); |
There was a problem hiding this comment.
This is an entity without a (real) ID... Some it's a kind of ephemeral entity... Would it be possible to avoid creating it at all?
There was a problem hiding this comment.
That's a good question - I'm a bit confused about the role of PrincipalEntity. We do have a public method for getResolvedCallerPrincipal() that returns resolvedCallerPrincipal. It's not being used anywhere in the codebase today, but I thought it'd be safe to populate it as it requires it to be @Nonnull: https://github.com/apache/polaris/blob/23ba2a05adc9c75f3e72aaf2ca370b4886964328/polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java#L272C19-L280
There was a problem hiding this comment.
Conceptually, IMHO, the principal entity should only be required for AuthZ checks (which do not actually require it if it's external). So, any intermediate code that requires it may need to be refactored... but TBH, I do not remember that code very well 😅
| } | ||
|
|
||
| private boolean isExternalPrincipal() { | ||
| return Boolean.parseBoolean(polarisPrincipal.getProperties().getOrDefault("external", "false")); |
There was a problem hiding this comment.
WDYT about making isExternal() a method of PolarisPrincipal?
There was a problem hiding this comment.
I think that's worth considering. I put it into PolarisPrincipal as a part of the property for now because it was already available, but I think adding it as an attribute or adding a class method that infers the property value is up for discussion
| String grantType, | ||
| String scope, | ||
| TokenType requestedTokenType) { | ||
| return TokenResponse.of(OAuthError.invalid_request); |
There was a problem hiding this comment.
I'm not sure about invalid_request... it may actually be well-formed... How about unsupported_grant_type?
There was a problem hiding this comment.
That's a great suggestion - thank you!
| private ResolverStatus resolveCallerPrincipalAndPrincipalRoles( | ||
| List<ResolvedPolarisEntity> toValidate) { | ||
|
|
||
| if (isExternalPrincipal()) { |
|
|
||
| @Override | ||
| public PolarisPrincipal authenticate(PolarisCredential credentials) { | ||
| PolarisCredential externalCredentials = ensureExternal(credentials); |
There was a problem hiding this comment.
instead of overriding the Principal to be external here, maybe we could validate that it is external and raise an exception if it isn't instead. Then we have a clearer separation of responsibility between:
- detecting whether a
PolarisCredential's origin was internal or external - and determining if a
PolarisPrincipalshould be internal, federated or external.
2 participants
Draft PR to introduce an
Externalmode in the PolarisPrincipal. The goal of this PR is to introduce an authentication flow that skips principal resolution in the metastore for enhanced compatibility with authentication -> authorization flows that rely on external IDPs and PDPs.This PR is motivated by @adutra 's mailing list proposal
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)