roryqi commented on code in PR #14326:
URL: https://github.com/apache/gravitino/pull/14326#discussion_r4481069188
##########
core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java:
##########
@@ -67,6 +67,7 @@ public OwnerManager(EntityStore store) {
@Override
public void setOwner(
String metalake, MetadataObject metadataObject, String ownerName,
Owner.Type ownerType) {
+ AuthorizationUtils.checkVisibleTag(store, metalake, metadataObject);
Review Comment:
Owner writes bypass TagManager, so they need their own check against the
persisted hidden flag. Otherwise a caller can assign a user owner directly to
an internal tag. The helper returns immediately for non-tag objects; I added a
comment explaining this boundary.
##########
core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java:
##########
@@ -408,6 +408,7 @@ User revokeRolesFromUser(String metalake, List<String>
roles, String user) {
Role grantPrivilegesToRole(
String metalake, String role, MetadataObject object, Set<Privilege>
privileges) {
+ AuthorizationUtils.checkVisibleTag(store, metalake, object);
Review Comment:
Privilege grants also bypass TagManager. This check prevents a hidden tag
from becoming a user-managed securable object through a role grant, while
leaving visible tags, including legacy reserved_* names, accessible.
##########
core/src/main/java/org/apache/gravitino/policy/HiddenPolicyTag.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * 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.policy;
+
+import com.google.common.base.Preconditions;
+import java.util.Collections;
+import java.util.Locale;
+import javax.annotation.Nullable;
+import org.apache.gravitino.Audit;
+import org.apache.gravitino.exceptions.NoSuchTagException;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.meta.TagEntity;
+import org.apache.gravitino.utils.NamespaceUtil;
+
+/** Internal naming convention for server-managed policy tags. */
+public final class HiddenPolicyTag {
+
+ private static final String PREFIX = "reserved_";
+ private static final String POLICY_ID_PROPERTY =
"gravitino.internal.policy-id";
+
+ private HiddenPolicyTag() {}
+
+ /**
+ * Builds the server-managed tag for a policy using Gravitino's tag entity
model.
+ *
+ * @param policy The policy that owns the hidden tag.
+ * @param tagId The generated tag entity ID.
+ * @return An unassigned, value-less tag with the policy's namespace and
audit information.
+ */
+ public static TagEntity create(PolicyEntity policy, long tagId) {
+ return create(policy.id(), tagId, policy.namespace().level(0),
policy.auditInfo());
+ }
+
+ /**
+ * Builds a hidden tag from persisted policy identity and audit metadata.
+ *
+ * @param policyId The owning policy ID.
+ * @param tagId The generated tag ID.
+ * @param metalake The policy's metalake name.
+ * @param audit The creation audit information.
+ * @return The hidden tag entity.
+ */
+ public static TagEntity create(long policyId, long tagId, String metalake,
Audit audit) {
+ return TagEntity.builder()
+ .withId(tagId)
+ .withName(name(policyId))
+ .withNamespace(NamespaceUtil.ofTag(metalake))
+ .withComment("Server-managed policy binding")
+ .withProperties(Collections.singletonMap(POLICY_ID_PROPERTY,
Long.toString(policyId)))
+ .withAllowedValues(new String[0])
+ .withHidden(true)
+ .withAuditInfo(audit)
+ .build();
+ }
+
+ /**
+ * Returns a stable tag name that survives policy renames.
+ *
+ * @param policyId The policy entity ID.
+ * @return The reserved tag name.
+ */
+ public static String name(long policyId) {
+ Preconditions.checkArgument(policyId >= 0, "Policy ID must be
non-negative");
+ return PREFIX + policyId;
+ }
+
+ /**
+ * Returns whether a tag name belongs to the reserved internal namespace.
+ *
+ * @param name The tag name.
+ * @return Whether the name is reserved.
+ */
+ public static boolean isHidden(@Nullable String name) {
+ return name != null && name.toLowerCase(Locale.ROOT).startsWith(PREFIX);
+ }
+
+ /**
+ * Parses the policy identity from a canonical hidden tag name.
+ *
+ * @param tagName The internal tag name.
+ * @return The policy entity ID.
+ */
+ public static long policyId(String tagName) {
+ Preconditions.checkArgument(isHidden(tagName), "Not a hidden policy tag");
+ long id = Long.parseLong(tagName.substring(PREFIX.length()));
+ Preconditions.checkArgument(name(id).equals(tagName), "Invalid hidden
policy tag name");
+ return id;
+ }
+
+ /**
+ * Returns the persisted ownership marker for a hidden tag.
+ *
+ * @param policyId The policy entity ID.
+ * @return The internal properties as JSON.
+ */
+ public static String properties(long policyId) {
+ return "{\"" + POLICY_ID_PROPERTY + "\":\"" + policyId + "\"}";
+ }
+
+ /**
+ * Rejects an ordinary operation on a persisted hidden tag.
+ *
+ * @param tag The resolved tag entity.
+ * @throws NoSuchTagException If the tag is hidden.
+ */
+ public static void checkVisible(TagEntity tag) {
+ if (tag.hidden()) {
+ throw new NoSuchTagException(
+ "Tag with name %s under metalake %s does not exist",
+ tag.name(), tag.namespace().level(0));
+ }
+ }
+
+ /**
+ * Rejects use of the internal namespace through ordinary tag operations.
+ *
+ * @param name The tag name to check.
+ */
+ public static void checkVisible(String name) {
+ Preconditions.checkArgument(
+ !isHidden(name), "Tag name is reserved for internal policy bindings");
+ }
Review Comment:
Renamed the name validator to `checkNameNotReserved` and the prefix
predicate to `isReservedName`. `checkVisible(TagEntity)` now has a single
purpose: checking the persisted hidden flag. Updated callers and tests.
##########
core/src/main/java/org/apache/gravitino/policy/HiddenPolicyTag.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * 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.policy;
+
+import com.google.common.base.Preconditions;
+import java.util.Collections;
+import java.util.Locale;
+import javax.annotation.Nullable;
+import org.apache.gravitino.Audit;
+import org.apache.gravitino.exceptions.NoSuchTagException;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.meta.TagEntity;
+import org.apache.gravitino.utils.NamespaceUtil;
+
+/** Internal naming convention for server-managed policy tags. */
+public final class HiddenPolicyTag {
+
+ private static final String PREFIX = "reserved_";
+ private static final String POLICY_ID_PROPERTY =
"gravitino.internal.policy-id";
+
+ private HiddenPolicyTag() {}
+
+ /**
+ * Builds the server-managed tag for a policy using Gravitino's tag entity
model.
+ *
+ * @param policy The policy that owns the hidden tag.
+ * @param tagId The generated tag entity ID.
+ * @return An unassigned, value-less tag with the policy's namespace and
audit information.
+ */
+ public static TagEntity create(PolicyEntity policy, long tagId) {
+ return create(policy.id(), tagId, policy.namespace().level(0),
policy.auditInfo());
+ }
+
+ /**
+ * Builds a hidden tag from persisted policy identity and audit metadata.
+ *
+ * @param policyId The owning policy ID.
+ * @param tagId The generated tag ID.
+ * @param metalake The policy's metalake name.
+ * @param audit The creation audit information.
+ * @return The hidden tag entity.
+ */
+ public static TagEntity create(long policyId, long tagId, String metalake,
Audit audit) {
+ return TagEntity.builder()
+ .withId(tagId)
+ .withName(name(policyId))
+ .withNamespace(NamespaceUtil.ofTag(metalake))
+ .withComment("Server-managed policy binding")
+ .withProperties(Collections.singletonMap(POLICY_ID_PROPERTY,
Long.toString(policyId)))
+ .withAllowedValues(new String[0])
+ .withHidden(true)
+ .withAuditInfo(audit)
+ .build();
+ }
+
+ /**
+ * Returns a stable tag name that survives policy renames.
+ *
+ * @param policyId The policy entity ID.
+ * @return The reserved tag name.
+ */
+ public static String name(long policyId) {
+ Preconditions.checkArgument(policyId >= 0, "Policy ID must be
non-negative");
+ return PREFIX + policyId;
+ }
+
+ /**
+ * Returns whether a tag name belongs to the reserved internal namespace.
+ *
+ * @param name The tag name.
+ * @return Whether the name is reserved.
+ */
+ public static boolean isHidden(@Nullable String name) {
+ return name != null && name.toLowerCase(Locale.ROOT).startsWith(PREFIX);
+ }
Review Comment:
Visibility is determined by the persisted `hidden` boolean. The name prefix
only reserves names for new tags and rename targets. I renamed
`isHidden(String)` to `isReservedName` to make that distinction explicit, and
added a test showing that an existing visible reserved_* tag remains visible.
##########
core/src/main/java/org/apache/gravitino/policy/HiddenPolicyTag.java:
##########
@@ -0,0 +1,139 @@
+/*
+ * 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.policy;
+
+import com.google.common.base.Preconditions;
+import java.util.Collections;
+import java.util.Locale;
+import javax.annotation.Nullable;
+import org.apache.gravitino.Audit;
+import org.apache.gravitino.exceptions.NoSuchTagException;
+import org.apache.gravitino.meta.PolicyEntity;
+import org.apache.gravitino.meta.TagEntity;
+import org.apache.gravitino.utils.NamespaceUtil;
+
+/** Internal naming convention for server-managed policy tags. */
+public final class HiddenPolicyTag {
+
+ private static final String PREFIX = "reserved_";
+ private static final String POLICY_ID_PROPERTY =
"gravitino.internal.policy-id";
+
+ private HiddenPolicyTag() {}
+
+ /**
+ * Builds the server-managed tag for a policy using Gravitino's tag entity
model.
+ *
+ * @param policy The policy that owns the hidden tag.
+ * @param tagId The generated tag entity ID.
+ * @return An unassigned, value-less tag with the policy's namespace and
audit information.
+ */
+ public static TagEntity create(PolicyEntity policy, long tagId) {
+ return create(policy.id(), tagId, policy.namespace().level(0),
policy.auditInfo());
+ }
+
+ /**
+ * Builds a hidden tag from persisted policy identity and audit metadata.
+ *
+ * @param policyId The owning policy ID.
+ * @param tagId The generated tag ID.
+ * @param metalake The policy's metalake name.
+ * @param audit The creation audit information.
+ * @return The hidden tag entity.
+ */
+ public static TagEntity create(long policyId, long tagId, String metalake,
Audit audit) {
+ return TagEntity.builder()
+ .withId(tagId)
+ .withName(name(policyId))
+ .withNamespace(NamespaceUtil.ofTag(metalake))
+ .withComment("Server-managed policy binding")
+ .withProperties(Collections.singletonMap(POLICY_ID_PROPERTY,
Long.toString(policyId)))
+ .withAllowedValues(new String[0])
+ .withHidden(true)
+ .withAuditInfo(audit)
+ .build();
+ }
+
+ /**
+ * Returns a stable tag name that survives policy renames.
+ *
+ * @param policyId The policy entity ID.
+ * @return The reserved tag name.
+ */
+ public static String name(long policyId) {
+ Preconditions.checkArgument(policyId >= 0, "Policy ID must be
non-negative");
Review Comment:
The current RandomIdGenerator clears the sign bit, but the IdGenerator
interface does not guarantee that. I removed the unnecessary non-negative
restriction: canonical tag names now round-trip the full long range, with tests
for negative IDs, zero, Long.MIN_VALUE and Long.MAX_VALUE.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -73,6 +79,114 @@ public PolicyManager(IdGenerator idGenerator, EntityStore
entityStore) {
this.objectPolicyResolver = new ObjectPolicyResolver(entityStore);
}
+ /**
+ * Applies a policy to an object through its hidden tag using the entity
store.
+ *
+ * <p>This internal operation is idempotent. Migration callers can group it
with legacy cleanup in
+ * their storage transaction. It is not exposed through the policy
dispatcher or client APIs.
+ *
+ * @param metalake The policy and object's metalake.
+ * @param policyName The policy name.
+ * @param object The metadata object to associate.
+ * @param auditInfo The audit information to preserve on newly created
relations.
+ * @return The policy's hidden tag ID.
+ * @throws IOException If an entity or relation cannot be persisted.
+ */
+ public long addPolicyToObject(
+ String metalake, String policyName, MetadataObject object, Audit
auditInfo)
+ throws IOException {
+ Preconditions.checkNotNull(auditInfo, "Policy binding audit information is
required");
+ Preconditions.checkArgument(
+ SUPPORTED_METADATA_OBJECT_TYPES_FOR_TAGS.contains(object.type()),
+ "Unsupported policy binding object type: %s",
+ object.type());
+ checkMetalake(NameIdentifier.of(metalake), entityStore);
+ NameIdentifier policyIdent = NameIdentifierUtil.ofPolicy(metalake,
policyName);
+ PolicyEntity policy =
+ entityStore.get(policyIdent, Entity.EntityType.POLICY,
PolicyEntity.class);
+ NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake,
object);
+ Entity.EntityType objectType = MetadataObjectUtil.toEntityType(object);
+ // Relation lookup validates the endpoint, including columns stored as
part of a table.
+ entityStore
+ .relationOperations()
+ .listEntitiesByRelation(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
objectIdent, objectType);
+ NameIdentifier tagIdent = NameIdentifierUtil.ofTag(metalake,
HiddenPolicyTag.name(policy.id()));
+ TagEntity tag;
+ try {
+ tag = entityStore.get(tagIdent, Entity.EntityType.TAG, TagEntity.class);
+ } catch (NoSuchEntityException e) {
+ tag = HiddenPolicyTag.create(policy, idGenerator.nextId());
+ try {
+ entityStore.put(tag, false);
+ } catch (EntityAlreadyExistsException concurrentCreate) {
+ tag = entityStore.get(tagIdent, Entity.EntityType.TAG,
TagEntity.class);
+ }
+ }
+ Preconditions.checkArgument(
+ tag.hidden()
+ && tag.valueConstraint().allowedValues() != null
+ && tag.valueConstraint().allowedValues().length == 0
+ && HiddenPolicyTag.create(policy,
tag.id()).properties().equals(tag.properties()),
+ "Reserved tag name collision for policy %s",
+ policy.id());
+ List<RelationalEntity<?>> policies =
+ entityStore
+ .relationOperations()
+ .batchListEntitiesByRelation(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ Collections.singletonList(tagIdent),
+ Entity.EntityType.TAG);
+ if (policies.isEmpty()) {
+ try {
+ entityStore
+ .relationOperations()
+ .updateEntityRelations(
+ RelationUpdate.of(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ tagIdent,
+ Entity.EntityType.TAG,
+ new RelationEdgeTarget[] {
+ RelationEdgeTarget.of(policyIdent,
Entity.EntityType.POLICY, null)
+ },
+ null)
+ .withAuditInfo(auditInfo));
+ } catch (EntityAlreadyExistsException | OptimisticLockException
concurrentCreate) {
+ policies =
+ entityStore
+ .relationOperations()
+ .batchListEntitiesByRelation(
+ SupportsRelationOperations.Type.POLICY_TAG_REL,
+ Collections.singletonList(tagIdent),
+ Entity.EntityType.TAG);
+ if (policies.isEmpty()) {
+ throw concurrentCreate;
+ }
+ }
+ }
+ if (!policies.isEmpty()) {
+ Preconditions.checkArgument(
+ policies.size() == 1
+ &&
policies.get(0).targetEntity().nameIdentifier().equals(policyIdent)
+ && !policies.get(0).relationValue().isPresent(),
+ "Invalid policy relation for hidden tag %s",
+ tagIdent);
+ }
+ entityStore
+ .relationOperations()
+ .updateEntityRelations(
+ RelationUpdate.of(
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL,
+ objectIdent,
+ objectType,
+ new RelationEdgeTarget[] {
+ RelationEdgeTarget.of(tagIdent, Entity.EntityType.TAG,
null)
+ },
+ null)
+ .withAuditInfo(auditInfo));
+ return tag.id();
+ }
Review Comment:
Split the method into hidden-tag loading/initialization, policy-tag relation
validation/initialization, and object assignment helpers. Added blank lines and
comments around endpoint validation and concurrent initialization. The existing
concurrency and rollback tests are retained.
--
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]
