Rework PolarisAuthorizer to use self-contained manifest on input #499
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * 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.PolarisConfigurationStore; | ||
|
|
||
| @SuppressWarnings("unused") // configured in polaris-server.yml | ||
| public class DefaultPolarisAuthorizerFactory implements PolarisAuthorizerFactory { | ||
| @Override | ||
| public PolarisAuthorizer createAuthorizer(PolarisConfigurationStore config) { | ||
| return new DefaultPolarisAuthorizer(config); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,26 +19,28 @@ | |
| package org.apache.polaris.core.auth; | ||
|
|
||
| import jakarta.annotation.Nonnull; | ||
| import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; | ||
|
|
||
| /** Interface for invoking authorization checks. */ | ||
| public interface PolarisAuthorizer { | ||
|
|
||
| /** | ||
| * Validates whether the requested operation is permitted based on the collection of entities | ||
| * (including principals, roles, and catalog objects) that are affected by the operation. | ||
| * | ||
| * <p>"activated" entities, "targets" and "secondaries" are contained within the provided | ||
| * manifest. The extra selector parameters merely define what sub-set of objects from the manifest | ||
|
There was a problem hiding this comment. I think this sentence is obsolete now: "The extra selector parameters merely define what sub-set of objects from the manifest should be considered as "targets", etc." |
||
| * should be considered as "targets", etc. | ||
| * | ||
| * <p>The effective principal information is also provided in the manifest. | ||
| * | ||
| * @param manifest defines the input for authorization checks. | ||
| * @param operation the operation being authorized. | ||
| * @param considerCatalogRoles whether catalog roles should be considered ({@code true}) or only | ||
| * principal roles ({@code false}). | ||
| */ | ||
| void authorizeOrThrow( | ||
| @Nonnull PolarisResolutionManifest manifest, | ||
|
There was a problem hiding this comment. My intention in #465 is to decouple the PolarisAuthorizer logic from the Resolver code. IMO, the Authorizer should take in a list of entities (principal and authz targets) without regard for how those entities were retrieved. I kept the There was a problem hiding this comment. I do believe that this change actually decouples the Authorizer implementation from entity resolution. That is because Authrorizer inputs in this PR are self-sufficient when the authrorization SPI is invoked. The implementation does not have to access the model. The default implementation certainly can function purely on the data from the authrorization SPI method parameters. I suppose you meant it to be approached the other way, when the Authorizer SPI is decoupled from entity resolution, but the Authorizer implementation resolves what is needs to resolve against the storage data model inside the authorization calls. The latter approach is certainly possible, but it will lose strong correlation with public API inputs. My reading of current code made me think, that the Authorizer was expected to act exactly on the same state of data that the API implementations use for producing API outputs. Is that so? In that case, and if the Authorizer has to perform extra resolutions, it will complicate the contract of the Persistence layer, which will then have to ensure consistency across calls from multiple services. WDYT? |
||
| @Nonnull PolarisAuthorizableOperation operation, | ||
| boolean considerCatalogRoles); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * 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 com.fasterxml.jackson.annotation.JsonTypeInfo; | ||
| import org.apache.polaris.core.PolarisConfigurationStore; | ||
|
|
||
| /** | ||
| * A factory for creating {@link PolarisAuthorizer} instances configured via a {@link | ||
| * PolarisConfigurationStore} | ||
| */ | ||
| @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, property = "factory") | ||
| public interface PolarisAuthorizerFactory { | ||
| PolarisAuthorizer createAuthorizer(PolarisConfigurationStore config); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would an
OptionalLongwould be better here?