adutra commented on code in PR #1397:
URL: https://github.com/apache/polaris/pull/1397#discussion_r2060412035
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -56,9 +84,34 @@ public Uni<SecurityIdentity> authenticate(
}
String credential = authHeader.substring(spaceIdx + 1);
+
+ DecodedToken token;
+ try {
+ token = decodeToken(credential);
+ } catch (Exception e) {
+ return configuration.type() == AuthenticationType.MIXED
Review Comment:
> However, InternalIdentityProvider looks like a no-op in the current state
of the code, which looks a bit odd to me
That is very true, but is intentional: we _could_ short-circuit in
`InternalAuthenticationMechanism` and return a `SecurityIdentity` straight
away, rather than invoking ` identityProviderManager.authenticate()`. In that
case, an `IdentityProvider` is not necessary.
BUT: when the identity provider manager is not invoked, no augmentors are
invoked either. I discovered this while testing.
So the TLDR is: we need `InternalIdentityProvider`, even if it's a
passthrough impl for now, because there are augmentors that must be invoked.
> Regarding parsing penalty, if the token is external, the first attempt
will parse and cause an exception on all call, which would be bigger overhead
for the lower priority authenticators, I guess.
True. Let me try to introduce the new `TokenBroker` method and see if that
looks better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
