adutra commented on code in PR #499:
URL: https://github.com/apache/polaris/pull/499#discussion_r1863138002
##########
polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java:
##########
@@ -100,6 +103,22 @@ public void setPolarisAuthenticator(
return polarisAuthenticator;
}
+ @JsonProperty("authorizer")
+ public void setAuthorizer(Class<? extends PolarisAuthorizer> clazz) {
+ this.authorizerClass = clazz;
+ }
+
+ public PolarisAuthorizer createAuthorizer() {
+ PolarisConfigurationStore featureConfig = getConfigurationStore();
Review Comment:
Nit: this doesn't look very Dropwizardy. I think you need to inject the
`PolarisAuthorizer` instance, not its class. If there are dependencies to be
injected, setters should be used.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -21,24 +21,68 @@
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import java.util.List;
-import java.util.Set;
-import org.apache.polaris.core.entity.PolarisBaseEntity;
-import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.PolarisConfigurationStore;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
-/** Interface for invoking authorization checks. */
+/**
+ * Interface for invoking authorization checks.
+ *
+ * <p>An implementations of this interface is selected at startup time.
Implementations should
+ * declare a constrictor with a single argument of type {@link
PolarisConfigurationStore}.
Review Comment:
```suggestion
* <p>An implementation of this interface is selected at startup time.
Implementations should
* declare a constructor with a single argument of type {@link
PolarisConfigurationStore}.
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java:
##########
@@ -18,55 +18,38 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
-import java.util.List;
import java.util.Set;
-import org.apache.polaris.core.entity.PolarisEntity;
-import org.apache.polaris.core.entity.PrincipalRoleEntity;
+import org.apache.polaris.core.entity.PrincipalEntity;
/** Holds the results of request authentication. */
-public class AuthenticatedPolarisPrincipal implements java.security.Principal {
- private final PolarisEntity principalEntity;
- private final Set<String> activatedPrincipalRoleNames;
- // only known and set after the above set of principal role names have been
resolved. Before
- // this, this list is null
- private List<PrincipalRoleEntity> activatedPrincipalRoles;
+public interface AuthenticatedPolarisPrincipal extends java.security.Principal
{
- public AuthenticatedPolarisPrincipal(
- @Nonnull PolarisEntity principalEntity, @Nonnull Set<String>
activatedPrincipalRoles) {
- this.principalEntity = principalEntity;
- this.activatedPrincipalRoleNames = activatedPrincipalRoles;
- this.activatedPrincipalRoles = null;
- }
+ AuthenticatedPolarisPrincipal ANONYMOUS =
+ new AuthenticatedPolarisPrincipalImpl(-1, "anonymous", Set.of());
- @Override
- public String getName() {
- return principalEntity.getName();
- }
+ /**
+ * Principal entity ID obtained during request authentication (e.g. from the
authorization token).
+ *
+ * <p>Negative values indicate that a principal ID was not provided in
authenticated data,
+ * however, other authentic information about the principal (e.g. name,
roles) may still be
+ * available.
+ */
+ long getPrincipalEntityId();
- public PolarisEntity getPrincipalEntity() {
- return principalEntity;
- }
-
- public Set<String> getActivatedPrincipalRoleNames() {
- return activatedPrincipalRoleNames;
- }
+ /** A sub-set of the assigned roles that are deemed effective in requests
using this principal. */
+ Set<String> getActivatedPrincipalRoleNames();
- public List<PrincipalRoleEntity> getActivatedPrincipalRoles() {
- return activatedPrincipalRoles;
+ static AuthenticatedPolarisPrincipal create(long entityId, String name,
Set<String> roles) {
+ return new AuthenticatedPolarisPrincipalImpl(entityId, name, roles);
}
- public void setActivatedPrincipalRoles(List<PrincipalRoleEntity>
activatedPrincipalRoles) {
- this.activatedPrincipalRoles = activatedPrincipalRoles;
+ static AuthenticatedPolarisPrincipal fromEntity(PrincipalEntity entity) {
Review Comment:
Both `fromEntity` methods are only used for tests. I wonder if we need them?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/ResolvedPolarisPrincipal.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PrincipalRoleEntity;
+
+/** Holds the results of resolving the authenticated principal's roles and
entity ID. */
+public class ResolvedPolarisPrincipal {
+ private final PolarisEntity principalEntity;
+ // only known and set after the above set of principal role names have been
resolved. Before
Review Comment:
This comment doesn't look accurate.
##########
polaris-service/src/test/resources/polaris-server-anonymous.yml:
##########
@@ -0,0 +1,158 @@
+#
+# 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.
+#
+
+server:
Review Comment:
The only difference between this config file and the "default" one for
integration tests is the authenticator class. But I think we need to keep it
because of the `tokenBroker` field... Dropwizard is really crappy :-(
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/ResolvedPolarisPrincipal.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PrincipalRoleEntity;
+
+/** Holds the results of resolving the authenticated principal's roles and
entity ID. */
+public class ResolvedPolarisPrincipal {
+ private final PolarisEntity principalEntity;
+ // only known and set after the above set of principal role names have been
resolved. Before
+ // this, this list is null
+ private List<PrincipalRoleEntity> activatedPrincipalRoles;
+
+ public ResolvedPolarisPrincipal(
+ @Nonnull PolarisEntity principalEntity,
+ @Nonnull List<PrincipalRoleEntity> activatedPrincipalRoles) {
+ this.principalEntity = principalEntity;
+ this.activatedPrincipalRoles = activatedPrincipalRoles;
+ }
+
+ public ResolvedPolarisPrincipal(@Nonnull PolarisEntity principalEntity) {
Review Comment:
Unused.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/ResolvedPolarisPrincipal.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PrincipalRoleEntity;
+
+/** Holds the results of resolving the authenticated principal's roles and
entity ID. */
+public class ResolvedPolarisPrincipal {
+ private final PolarisEntity principalEntity;
+ // only known and set after the above set of principal role names have been
resolved. Before
+ // this, this list is null
+ private List<PrincipalRoleEntity> activatedPrincipalRoles;
Review Comment:
Can be final.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipal.java:
##########
@@ -18,55 +18,38 @@
*/
package org.apache.polaris.core.auth;
-import jakarta.annotation.Nonnull;
-import java.util.List;
import java.util.Set;
-import org.apache.polaris.core.entity.PolarisEntity;
-import org.apache.polaris.core.entity.PrincipalRoleEntity;
+import org.apache.polaris.core.entity.PrincipalEntity;
/** Holds the results of request authentication. */
-public class AuthenticatedPolarisPrincipal implements java.security.Principal {
- private final PolarisEntity principalEntity;
- private final Set<String> activatedPrincipalRoleNames;
- // only known and set after the above set of principal role names have been
resolved. Before
- // this, this list is null
- private List<PrincipalRoleEntity> activatedPrincipalRoles;
+public interface AuthenticatedPolarisPrincipal extends java.security.Principal
{
- public AuthenticatedPolarisPrincipal(
- @Nonnull PolarisEntity principalEntity, @Nonnull Set<String>
activatedPrincipalRoles) {
- this.principalEntity = principalEntity;
- this.activatedPrincipalRoleNames = activatedPrincipalRoles;
- this.activatedPrincipalRoles = null;
- }
+ AuthenticatedPolarisPrincipal ANONYMOUS =
+ new AuthenticatedPolarisPrincipalImpl(-1, "anonymous", Set.of());
- @Override
- public String getName() {
- return principalEntity.getName();
- }
+ /**
+ * Principal entity ID obtained during request authentication (e.g. from the
authorization token).
+ *
+ * <p>Negative values indicate that a principal ID was not provided in
authenticated data,
+ * however, other authentic information about the principal (e.g. name,
roles) may still be
+ * available.
+ */
+ long getPrincipalEntityId();
- public PolarisEntity getPrincipalEntity() {
- return principalEntity;
- }
-
- public Set<String> getActivatedPrincipalRoleNames() {
- return activatedPrincipalRoleNames;
- }
+ /** A sub-set of the assigned roles that are deemed effective in requests
using this principal. */
+ Set<String> getActivatedPrincipalRoleNames();
- public List<PrincipalRoleEntity> getActivatedPrincipalRoles() {
- return activatedPrincipalRoles;
+ static AuthenticatedPolarisPrincipal create(long entityId, String name,
Set<String> roles) {
Review Comment:
This method is tying the interface to one specific implementation, thus it
cannot be used if the `Authenticator` being used is a custom one. Maybe remove?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AllowAllAuthorizer.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import org.apache.polaris.core.PolarisConfigurationStore;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+public class AllowAllAuthorizer implements PolarisAuthorizer {
Review Comment:
Since we have 2 authz implementations now, shouldn't we annotate
`PolarisAuthorizer` with `@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property =
"class")`?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/CoreAuthorizer.java:
##########
@@ -119,8 +122,8 @@
* the expanded roles of the calling Principal hold SERVICE_MANAGE_ACCESS on
the "root" catalog,
* which translates into a cross-catalog permission.
*/
-public class PolarisAuthorizerImpl implements PolarisAuthorizer {
- private static final Logger LOGGER =
LoggerFactory.getLogger(PolarisAuthorizerImpl.class);
+public class CoreAuthorizer implements PolarisAuthorizer {
Review Comment:
Maybe `DefaultPolarisAuthorizer`?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/ResolvedPolarisPrincipal.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.core.auth;
+
+import jakarta.annotation.Nonnull;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.polaris.core.entity.PolarisEntity;
+import org.apache.polaris.core.entity.PolarisEntityCore;
+import org.apache.polaris.core.entity.PrincipalRoleEntity;
+
+/** Holds the results of resolving the authenticated principal's roles and
entity ID. */
+public class ResolvedPolarisPrincipal {
+ private final PolarisEntity principalEntity;
+ // only known and set after the above set of principal role names have been
resolved. Before
+ // this, this list is null
+ private List<PrincipalRoleEntity> activatedPrincipalRoles;
+
+ public ResolvedPolarisPrincipal(
+ @Nonnull PolarisEntity principalEntity,
+ @Nonnull List<PrincipalRoleEntity> activatedPrincipalRoles) {
+ this.principalEntity = principalEntity;
+ this.activatedPrincipalRoles = activatedPrincipalRoles;
+ }
+
+ public ResolvedPolarisPrincipal(@Nonnull PolarisEntity principalEntity) {
+ this(principalEntity, List.of());
+ }
+
+ public String getName() {
+ return principalEntity.getName();
+ }
+
+ public PolarisEntity getPrincipalEntity() {
+ return principalEntity;
+ }
+
+ public Set<String> getActivatedPrincipalRoleNames() {
Review Comment:
Maybe compute eagerly?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthenticatedPolarisPrincipalImpl.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.core.auth;
+
+import java.util.Set;
+
+public final class AuthenticatedPolarisPrincipalImpl implements
AuthenticatedPolarisPrincipal {
Review Comment:
I think this should be moved to `polaris-service` close to
`BaseAuthenticator` since that's where it's being instantiated. `polaris-core`
doesn't need to know the actual implementation of `AuthenticatedPrincipal`.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthEntitySelector.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.core.auth;
+
+import org.apache.polaris.core.entity.PolarisEntitySubType;
+import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Defines which sub-set of a {@link PolarisResolutionManifest} should be used
for various
+ * authorization check parameters.
+ */
+public interface AuthEntitySelector {
Review Comment:
```suggestion
@FunctionalInterface
public interface AuthEntitySelector {
```
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/CoreAuthorizer.java:
##########
@@ -529,27 +542,26 @@ public void authorizeOrThrow(
}
}
- /**
- * Based on the required target/targetParent/secondary/secondaryParent
privileges mapped from
- * {@code authzOp}, determines whether the caller's set of
activatedGranteeIds is authorized for
- * the operation.
- */
- public boolean isAuthorized(
- @Nonnull AuthenticatedPolarisPrincipal authenticatedPolarisPrincipal,
- @Nonnull Set<PolarisBaseEntity> activatedEntities,
- @Nonnull PolarisAuthorizableOperation authzOp,
- @Nullable PolarisResolvedPathWrapper target,
- @Nullable PolarisResolvedPathWrapper secondary) {
- return isAuthorized(
- authenticatedPolarisPrincipal,
- activatedEntities,
- authzOp,
- target == null ? null : List.of(target),
- secondary == null ? null : List.of(secondary));
+ private boolean isSelfReset(
Review Comment:
Okay, it took me a while to realize that this comes from
`PolarisAdminService.authorizeBasicTopLevelEntityOperationOrThrow()`. I agree
with the change.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/ActivatedEntitySelector.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.core.auth;
+
+import java.util.Set;
+import java.util.function.Function;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Defines which roles should be considered by authorization checks.
+ *
+ * @see PolarisAuthorizer
+ */
+public enum ActivatedEntitySelector {
Review Comment:
De we need this? It's just a wrapper around a function that takes a
`PolarisResolutionManifest` as input. Since we only have 2 enum constants,
couldn't you use a boolean instead?
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/CoreAuthorizer.java:
##########
@@ -119,8 +122,8 @@
* the expanded roles of the calling Principal hold SERVICE_MANAGE_ACCESS on
the "root" catalog,
* which translates into a cross-catalog permission.
*/
-public class PolarisAuthorizerImpl implements PolarisAuthorizer {
- private static final Logger LOGGER =
LoggerFactory.getLogger(PolarisAuthorizerImpl.class);
+public class CoreAuthorizer implements PolarisAuthorizer {
Review Comment:
Also: wondering if this shouldn't be moved to `polaris-service` as well?
--
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]
