bharos commented on code in PR #14364: URL: https://github.com/apache/gravitino/pull/14364#discussion_r4461722325
########## core/src/test/java/org/apache/gravitino/authorization/TagPolicyTestUtils.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.gravitino.authorization; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.meta.AuditInfo; +import org.apache.gravitino.meta.PolicyEntity; +import org.apache.gravitino.meta.TagEntity; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyAssociationSelector; +import org.apache.gravitino.policy.PolicyContents; +import org.apache.gravitino.tag.TagAssignment; +import org.apache.gravitino.utils.NamespaceUtil; + +/** Shared tag-policy entities and a controllable reader for authorization tests. */ +final class TagPolicyTestUtils { + static final String METALAKE = "metalake"; + + private TagPolicyTestUtils() {} + + static TagEntity tag(long id, String name, TagAssignment assignment) { + return TagEntity.builder() + .withId(id) + .withName(name) + .withNamespace(NamespaceUtil.ofTag(METALAKE)) + .withProperties(Collections.emptyMap()) + .withAuditInfo(audit()) + .withAssignment(assignment) + .build(); + } + + static PolicyEntity policy( + long id, boolean enabled, List<Privilege.Name> privileges, List<String> roles) { + return PolicyEntity.builder() + .withId(id) + .withName("policy_" + id) + .withNamespace(NamespaceUtil.ofPolicy(METALAKE)) + .withPolicyType(Policy.BuiltInType.ACCESS_CONTROL) + .withEnabled(enabled) + .withContent(PolicyContents.accessControl(privileges, roles)) + .withAuditInfo(audit()) + .build(); + } + + static AuditInfo audit() { + return AuditInfo.builder().withCreator("tester").withCreateTime(Instant.EPOCH).build(); + } + + static PolicyEntity customPolicy() { + return PolicyEntity.builder() + .withId(99L) + .withName("custom") + .withNamespace(NamespaceUtil.ofPolicy(METALAKE)) + .withPolicyType(Policy.BuiltInType.CUSTOM) + .withEnabled(true) + .withContent( + PolicyContents.custom( + Collections.singletonMap("rule", "value"), + Collections.singleton(MetadataObject.Type.TABLE), + Collections.emptyMap())) + .withAuditInfo(audit()) + .build(); + } + + /** Counts reads and allows tests to inject assignment or policy-read failures. */ + static final class InMemoryReader implements TagPolicyReader { + final Map<MetadataObject, List<TagEntity>> assignments = new HashMap<>(); + final Map<String, List<ResolvedTagPolicy>> bindings = new HashMap<>(); + final AtomicInteger tagReads = new AtomicInteger(); + final AtomicInteger policyReads = new AtomicInteger(); + RuntimeException tagFailure; + RuntimeException policyFailure; + + void bind(TagEntity tag, PolicyEntity policy, PolicyAssociationSelector selector) { + bindings + .computeIfAbsent(tag.name(), ignored -> new ArrayList<>()) + .add(new ResolvedTagPolicy(tag, policy, selector)); + } + + /** {@inheritDoc} */ + @Override + public List<TagEntity> loadTags(String metalake, MetadataObject object) { + tagReads.incrementAndGet(); + if (tagFailure != null) { + throw tagFailure; + } + return assignments.getOrDefault(object, Collections.emptyList()); + } + + /** {@inheritDoc} */ + @Override + public List<ResolvedTagPolicy> loadPolicyBindings(String metalake, List<TagEntity> tags) { + policyReads.incrementAndGet(); + if (policyFailure != null) { + throw policyFailure; + } + List<ResolvedTagPolicy> result = new ArrayList<>(); + for (TagEntity tag : tags) { Review Comment: nit: duplicate tag names are silently collapsed. A precondition would catch it. ########## core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java: ########## @@ -333,6 +336,36 @@ public Optional<OwnerInfo> computeOwnerIfAbsent( id -> Objects.requireNonNull(loader.apply(id), "Owner loader must not return null")); } + /** + * Resolves tag-policy data at most once per target, independently of principal and privilege. + * + * @param metalake the containing metalake + * @param target the original canonical target, not an ancestor visited by the RBAC walk + * @param loader the resolver, returning a complete non-null list or throwing + * @return the immutable resolved bindings + * @throws RuntimeException if resolution fails + */ + public List<ResolvedTagPolicy> computeTagPoliciesIfAbsent( + String metalake, MetadataObject target, Supplier<List<ResolvedTagPolicy>> loader) { + TagPolicyKey key = new TagPolicyKey(metalake, target); Review Comment: The cache key is (metalake, target), so the ancestors’ direct assignments are read again for every target. So a list over N tables re-reads the schema and catalog tags N times. Could we cache direct assignments per object, or batch them? ########## core/src/main/java/org/apache/gravitino/authorization/EntityStoreTagPolicyReader.java: ########## @@ -0,0 +1,110 @@ +/* + * 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.gravitino.authorization; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import org.apache.gravitino.Entity; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.RelationalEntity; +import org.apache.gravitino.SupportsRelationOperations; +import org.apache.gravitino.json.PolicyAssociationSelectorSerde; +import org.apache.gravitino.meta.PolicyEntity; +import org.apache.gravitino.meta.TagEntity; +import org.apache.gravitino.policy.AllValuesSelector; +import org.apache.gravitino.policy.PolicyAssociationSelector; +import org.apache.gravitino.utils.MetadataObjectUtil; +import org.apache.gravitino.utils.NameIdentifierUtil; + +/** Reads direct tag assignments and policy bindings through the entity store's relation API. */ +public final class EntityStoreTagPolicyReader implements TagPolicyReader { + private final EntityStore entityStore; + + /** + * Creates a reader backed by the relation store. + * + * @param entityStore the store to read + */ + public EntityStoreTagPolicyReader(EntityStore entityStore) { + this.entityStore = Objects.requireNonNull(entityStore, "entityStore must not be null"); + } + + /** {@inheritDoc} */ + @Override + public List<TagEntity> loadTags(String metalake, MetadataObject object) { + try { + return entityStore + .relationOperations() + .listEntitiesByRelation( + SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL, + MetadataObjectUtil.toEntityIdent(metalake, object), + MetadataObjectUtil.toEntityType(object)); + } catch (IOException e) { + throw new UncheckedIOException("Failed to read tag assignments for " + object, e); + } + } + + /** {@inheritDoc} */ + @Override + public List<ResolvedTagPolicy> loadPolicyBindings( + String metalake, List<TagEntity> effectiveTags) { + if (effectiveTags.isEmpty()) { + return Collections.emptyList(); + } + Map<NameIdentifier, TagEntity> tagsByIdentifier = new LinkedHashMap<>(); + for (TagEntity tag : effectiveTags) { + tagsByIdentifier.put(NameIdentifierUtil.ofTag(metalake, tag.name()), tag); + } + try { + List<RelationalEntity<?>> relations = + entityStore + .relationOperations() + .batchListEntitiesByRelation( + SupportsRelationOperations.Type.POLICY_TAG_REL, + new ArrayList<>(tagsByIdentifier.keySet()), + Entity.EntityType.TAG); + List<ResolvedTagPolicy> bindings = new ArrayList<>(); + for (RelationalEntity<?> relation : relations) { + TagEntity tag = + Objects.requireNonNull( + tagsByIdentifier.get(relation.source()), + "Policy binding has an unexpected source tag"); + // SQL NULL matches this tag with any assigned value or no value. + PolicyAssociationSelector selector = + relation + .relationValue() + .map(PolicyAssociationSelectorSerde::deserialize) Review Comment: If one stored selector fails to parse (e.g. a newer selector type during a rolling upgrade), this throws for all bindings, so tag resolution fails for every object under that tag, and the IllegalArgumentException surfaces as a 400. Could we either skip and log the bad binding , or throw a server-side error with the tag/policy IDs ########## core/src/main/java/org/apache/gravitino/authorization/TagPolicyMatcher.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.gravitino.authorization; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import org.apache.gravitino.policy.AccessControlContent; +import org.apache.gravitino.policy.AllValuesSelector; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyAssociationSelector; +import org.apache.gravitino.policy.TagValueSelector; +import org.apache.gravitino.tag.TagAssignment; + +/** Matches requested privileges and active roles against resolved tag-policy bindings. */ +public final class TagPolicyMatcher { + private TagPolicyMatcher() {} + + /** + * Finds the first binding that confers a privilege on an effective active role. + * + * @param privilege the requested privilege + * @param activeRoleNames the caller's held role names after active-role narrowing + * @param bindings the target's resolved bindings in deterministic order + * @return the first matching binding, carrying the tag and policy IDs, or empty + * @throws IllegalArgumentException if a relevant binding has an unsupported selector + */ + public static Optional<ResolvedTagPolicy> match( + Privilege.Name privilege, Set<String> activeRoleNames, List<ResolvedTagPolicy> bindings) { + Objects.requireNonNull(privilege, "privilege must not be null"); + Objects.requireNonNull(activeRoleNames, "activeRoleNames must not be null"); + Objects.requireNonNull(bindings, "bindings must not be null"); + if (activeRoleNames.isEmpty() + || !AccessControlContent.PERMITTED_PRIVILEGES.contains(privilege)) { + return Optional.empty(); + } + for (ResolvedTagPolicy binding : bindings) { + if (!binding.policy().enabled() + || binding.policy().policyType() != Policy.BuiltInType.ACCESS_CONTROL) { + continue; + } + AccessControlContent content = (AccessControlContent) binding.policy().content(); + if (content.privileges().contains(privilege) + && !Collections.disjoint(content.applicableRoles(), activeRoleNames) + && matches( + binding.selector(), binding.tag().assignment().orElseGet(TagAssignment::noValue))) { + return Optional.of(binding); + } + } + return Optional.empty(); + } + + private static boolean matches(PolicyAssociationSelector selector, TagAssignment assignment) { Review Comment: This is identical to the private ObjectPolicyResolver.matches(...). Could we move it to a shared helper (e.g. a static method next to PolicyAssociationSelector, or a small PolicySelectorMatcher) and use it from both? ########## core/src/main/java/org/apache/gravitino/tag/EffectiveTagResolver.java: ########## @@ -280,4 +265,41 @@ public Map<MetadataObject, ResolvedTag[]> resolveWithSources( }); return result; } + + private static ResolvedTag[] resolveWithSources( + MetadataObject metadataObject, Function<MetadataObject, List<TagEntity>> reader) { + List<MetadataObject> resolutionOrder = new ArrayList<>(); + resolutionOrder.add(metadataObject); + resolutionOrder.addAll(MetadataObjectUtil.getParentMetadataObjects(metadataObject)); + + Map<String, ResolvedTag> effectiveTags = new LinkedHashMap<>(); + for (int index = 0; index < resolutionOrder.size(); index++) { + MetadataObject object = resolutionOrder.get(index); + boolean inherited = index > 0; + reader + .apply(object) + .forEach( + tag -> + effectiveTags.putIfAbsent( + tag.name(), new ResolvedTag(tag.copyWithInherited(inherited), object))); + } + return effectiveTags.values().toArray(new ResolvedTag[0]); + } + + private List<TagEntity> loadTags(String metalake, MetadataObject object) { + NameIdentifier identifier = MetadataObjectUtil.toEntityIdent(metalake, object); + Entity.EntityType entityType = MetadataObjectUtil.toEntityType(object); + try { + return entityStore + .relationOperations() + .listEntitiesByRelation( + SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL, identifier, entityType); Review Comment: If the target or an ancestor isn’t in the entity store yet, this throws NoSuchEntityException, and that gets cached as a failure for the request. Should a missing object just mean no tags? Please add a test either way. -- 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]
