adutra commented on code in PR #3250:
URL: https://github.com/apache/polaris/pull/3250#discussion_r2705682865
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -49,4 +59,10 @@ static PolarisCredential of(
/** The principal roles, or empty if the principal has no roles. */
Set<String> getPrincipalRoles();
+
+ /** True if the credential represents an external principal. */
+ @Value.Default
+ default boolean isExternal() {
Review Comment:
Should this property have a default value at all? I think it's safer if the
code building the credential explicitly decides if it's external or not.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/internal/broker/NoneTokenBrokerFactory.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth.internal.broker;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.ApplicationScoped;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
+import org.apache.polaris.service.auth.PolarisCredential;
+import org.apache.polaris.service.auth.internal.service.OAuthError;
+import org.apache.polaris.service.types.TokenType;
+
+/** A no-op token broker factory used when authentication is delegated to an
external IdP. */
+@ApplicationScoped
+@Identifier("none")
+public class NoneTokenBrokerFactory implements TokenBrokerFactory {
Review Comment:
Hmm this token broker existed and was deleted a while ago by #2634. Why are
we adding it back?
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/ExternalAuthenticator.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.PolarisPrincipal;
+
+/**
+ * Authenticator for principals supplied by an external identity provider.
+ *
+ * <p>Builds an in-memory {@link PolarisPrincipal} directly from credential
claims without any
+ * persistence lookups.
+ */
+@RequestScoped
+@Identifier("external")
+public class ExternalAuthenticator implements Authenticator {
+
+ @Override
+ public PolarisPrincipal authenticate(PolarisCredential credentials) {
+ PolarisCredential externalCredentials = ensureExternal(credentials);
Review Comment:
I think indeed it would be better if this authenticator rejected any
non-external `PolarisCredential`.
However, the `DefaultAuthenticator` should not do the opposite, as it is
valid to have persisted principals with an external IDP so it should accept any
credential, internal or external.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -764,6 +794,17 @@ private ResolverStatus
resolveCallerPrincipalAndPrincipalRoles(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ private boolean isExternalPrincipal() {
+ return
Boolean.parseBoolean(polarisPrincipal.getProperties().getOrDefault("external",
"false"));
Review Comment:
Same remark: maybe we should expose a `getType` method here that returns
`PolarisPrincipalType`?
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/internal/InternalAuthenticationMechanism.java:
##########
@@ -118,7 +117,7 @@ public Uni<ChallengeData> getChallenge(RoutingContext
context) {
@Override
public Set<Class<? extends AuthenticationRequest>> getCredentialTypes() {
- return Collections.singleton(TokenAuthenticationRequest.class);
+ return Collections.singleton(InternalAuthenticationRequest.class);
Review Comment:
Whoops this is an interesting bug that you fixed 😅 But I wonder how it
worked before 🤔
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -744,13 +745,42 @@ private ResolverStatus resolvePaths(
private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
List<ResolvedPolarisEntity> toValidate) {
+ if (isExternalPrincipal()) {
+ PrincipalEntity externalPrincipal = createExternalPrincipalEntity();
Review Comment:
I think the ideal solution would be to not materialize this entity _at all_
– and very likely, pass an empty set of `activatedEntities` to the
`PolarisAuthorizer`.
But I also understand that this may increase the blast radius of this PR,
and also interfere with #3228.
So, I think having a temporary entity being created here can be a good
compromise, as long as we don't forget to revisit this later.
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/ExternalAuthenticator.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.polaris.core.auth.PolarisPrincipal;
+
+/**
+ * Authenticator for principals supplied by an external identity provider.
+ *
+ * <p>Builds an in-memory {@link PolarisPrincipal} directly from credential
claims without any
+ * persistence lookups.
+ */
+@RequestScoped
+@Identifier("external")
+public class ExternalAuthenticator implements Authenticator {
+
+ @Override
+ public PolarisPrincipal authenticate(PolarisCredential credentials) {
+ PolarisCredential externalCredentials = ensureExternal(credentials);
Review Comment:
In fact I'm wondering whether we need this second `Authenticator`.
We could instead introduce a `polaris.authentication.principal.type` setting
(overridable per realm). Valid values would be the constants of the
`PolarisPrincipalType` enum (e.g. `persisted`, `federated`, `external`).
Then the `DefaultAuthenticator` should be able to handle all kinds of
principals. It would simply read this configuration setting to determine
whether it should fetch the `PolarisEntity `or not. (It would also, of course,
validate that if the principal type is `external` then the `PolarisCredential`
must be external too.)
WDYT?
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java:
##########
@@ -49,4 +59,10 @@ static PolarisCredential of(
/** The principal roles, or empty if the principal has no roles. */
Set<String> getPrincipalRoles();
+
+ /** True if the credential represents an external principal. */
+ @Value.Default
+ default boolean isExternal() {
Review Comment:
Also thinking forward, maybe we should have an enum `PolarisPrincipalType` +
method `getPrincipalType()` ?
If federated principals are implemented eventually, it would be cumbersome
to have two methods `isExternal` and `isFederated` as they are mutually
exclusive.
--
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]
