[#8942]feat(authz): support tag access control by hdygxsj · Pull Request #9018 · apache/gravitino

hdygxsj · 2025-11-04T17:00:06Z

What changes were proposed in this pull request?

support tag access control

Why are the changes needed?

Fix: #8942

Does this PR introduce any user-facing change?

None

How was this patch tested?

org.apache.gravitino.client.integration.test.authorization.TagOperationsAuthorizationIT

hdygxsj · 2025-11-04T17:00:39Z

PHAL @jerqi

roryqi · 2025-11-05T02:20:33Z

api/src/main/java/org/apache/gravitino/authorization/Privileges.java

+
+    @Override
+    public boolean canBindTo(MetadataObject.Type type) {
+      return true;


Could the tag and role be tagged?

It seems there are no restrictions in TagManager.

cc @jerryshao WDYT？

Could the tag and role be tagged?

I misunderstood earlier—CREATE_TAG should be bound to the metalake.

roryqi · 2025-11-05T07:33:20Z

Could u add the documents for this PR?

roryqi · 2025-11-06T02:09:23Z

docs/security/access-control.md

+| set owner                         | The owner of the securable object                            |
+| list tags                         | The owner of the metalake can see all the tags. Others can see his owned tags. |
+| create tag                        | `CREATE_TAG` on the metalake or the owner of the metalake    |
+| get tag                           | The owner of the metalake or the tag.                        |


I have some concern about this. Maybe we can't use this.

roryqi · 2025-11-12T07:07:37Z

Could you add the tag to the metadata object type in the openapi.yaml
Could you confirm influence about tag, policy when adding tag to the metadata object type?

roryqi · 2025-11-12T08:21:08Z

core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java

-          metadataObject,
-          authorizationPlugin ->
-              authorizationPlugin.onOwnerSet(metadataObject, originOwner.orElse(null), newOwner));
+      if (metadataObject.type() != MetadataObject.Type.TAG) {


Why do we need this?

org.apache.gravitino.authorization.AuthorizationUtils#callAuthorizationPluginForMetadataObject can not handle tag

We should let this method handle the tag. Because tag becomes a metadata object.

We should let this method handle the tag. Because tag becomes a metadata object.

This method seems to be for handling pushdown, tags shouldn’t be in the underlying engine, right?

# Conflicts: # api/src/main/java/org/apache/gravitino/authorization/Privilege.java # api/src/main/java/org/apache/gravitino/authorization/Privileges.java # core/src/main/java/org/apache/gravitino/GravitinoEnv.java # docs/security/access-control.md

roryqi · 2025-11-17T16:43:24Z

server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java

+                  .forEach(
+                      tag -> {
+                        boolean result =
+                            new AuthorizationExpressionEvaluator(


Why do we this instead of expression?

Authorization expressions are also used here, but interceptor is not used.

Just like with list operations, it's challenging in interceptors to extract an array from the request body and perform authorization and filtering on it.

If we have the tag which we don't have privilege, we should forbid the behaviour.

You should move the logic to GravitinoInterceptionService
We can add the new annotation for the request like

AuthorizationRequest authorizeRequest = parameter.getAnnotation(AuthorizationRequest.class);

then

preAuthzRequest(authorizeRequest);

AuthorizationExpressionEvaluator and authorization expressions are not suitable for authorizing lists/arrays—or even multiple metadata objects within multiple lists/arrays(tagsToAdd, tagsToRemove). Should we consider designing a new authorization mechanism specifically for this scenario?

Maybe we should design this. We should have the semantics like scala forall.

roryqi · 2025-11-18T03:43:30Z

We would like to load a metadata object first. Then, we associate the metadata object with tags. It requires we must have the privilege to load the table. Actually, if we have the privilege BROWSE in the future, we can only list the metadata objects and know the names of the metadata objects. Maybe we need to add an interface for Tag

SupportsTagOperations {
   void associateObjects(List<MetadataObject> addedObjects,  List<MetadataObject> removeObjects)
}

@jerryshao WDYT?

roryqi · 2025-11-18T03:59:22Z

server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectTagOperations.java

  @Timed(name = "get-object-tag." + MetricNames.HTTP_PROCESS_DURATION, absolute = true)
  @ResponseMetered(name = "get-object-tag", absolute = true)
+  @AuthorizationExpression(expression = "CAN_ACCESS_METADATA && ( METALAKE::OWNER || TAG::OWNER)")
  public Response getTagForObject(


This expression is wrong

METALAKE_OWNER || (CAN_LOAD_OBJECT && CAN_LOAD_TAG)

# Conflicts: # server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java

roryqi · 2025-11-19T12:50:28Z

server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java

-                expression);
-
-            return buildNoAuthResponse(errorMessage, accessMetadataName, currentUser, methodName);
+          if (!isBatch) {


Could extract some classes to handle the conditions?

server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java

roryqi · 2025-11-20T03:12:47Z

server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java

+              extractAuthorizationRequestTypeFromParameters(parameters);
+          executor =
+              switch (requestType) {
+                case COMMON -> new CommonAuthorizerExecutor(


Could extract these classes to single files?

roryqi · 2025-11-20T15:20:28Z

...java/org/apache/gravitino/server/web/filter/authorization/AssociateTagAuthorizeExecutor.java

+
+    // Authorize both 'tagsToAdd' and 'tagsToRemove' fields.
+    // Short-circuit on first failure.
+    return authorizeTagField(request, "tagsToAdd", context, targetType)


We don't need use reflection here.

# Conflicts: # server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java

roryqi · 2025-11-21T06:24:45Z

core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java


  public static void callAuthorizationPluginForMetadataObject(
      String metalake, MetadataObject metadataObject, Consumer<AuthorizationPlugin> consumer) {
+    if (metadataObject.type() == MetadataObject.Type.TAG) {


Do we still need this?

Yes, otherwise setting the tag's owner here will throw an exception.

We have already handled this in this method.

roryqi · 2025-11-21T06:32:22Z

core/src/main/java/org/apache/gravitino/hook/TagHookDispatcher.java

+    try {
+      // Set the creator as the owner of the catalog.
+      OwnerDispatcher ownerDispatcher = GravitinoEnv.getInstance().ownerDispatcher();
+      if (ownerDispatcher != null) {


Why do we need try catch here?

roryqi · 2025-11-21T07:24:25Z

core/src/main/java/org/apache/gravitino/tag/TagDispatcher.java

   * @return The created tag.
   */
-  Tag createTag(String metalake, String name, String comment, Map<String, String> properties);
+  Tag createTag(String metalake, String name, String comment, Map<String, String> properties)


Why do need to throw Exception here?

This reverts commit 4ba420c.

roryqi · 2025-11-21T08:53:12Z

.../org/apache/gravitino/server/web/filter/authorization/AssociateTagAuthorizationExecutor.java

+      return authorizeTagField(tagsAssociateRequest.getTagsToAdd(), context, targetType)
+          && authorizeTagField(tagsAssociateRequest.getTagsToRemove(), context, targetType);
+    }
+    throw new UnsupportedOperationException("Unsupported request type: " + request.getClass());


Could you use Precondition.checkArguments(). This should throw illegalAgurmentException here.

.../org/apache/gravitino/server/web/filter/authorization/AssociateTagAuthorizationExecutor.java

roryqi

LGTM.

support tag access control

75c4540

hdygxsj requested a review from roryqi November 4, 2025 17:00

hdygxsj added 3 commits November 5, 2025 09:19

fix ci

bfd4f9d

fix ci

921eb6d

fix ci

0666785

roryqi reviewed Nov 5, 2025

View reviewed changes

hdygxsj added 3 commits November 5, 2025 10:56

fix ci

c10f74a

fix ci

a2fcd8e

fix ci

8b236c4

hdygxsj and others added 5 commits November 5, 2025 16:36

fix ci

071238e

add docs

8b2eed0

fix

c5fffff

fix ci

90bd18a

Merge branch 'main' into feat-8942

1b1e050

roryqi reviewed Nov 6, 2025

View reviewed changes

roryqi reviewed Nov 12, 2025

View reviewed changes

hdygxsj added 7 commits November 12, 2025 20:39

rebase

68c84d4

rebase

b50866b

revert policy dispatcher code

d421656

fix ci

20053d2

fix

07dd00a

Merge remote-tracking branch 'origin/feat-8942' into feat-8942

288b7d7

roryqi reviewed Nov 17, 2025

View reviewed changes

roryqi reviewed Nov 18, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat-8942

bf30006

# Conflicts: # server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java

fix blank

4192a9e

roryqi reviewed Nov 19, 2025

View reviewed changes

hdygxsj added 3 commits November 19, 2025 22:00

fix

ae3c097

fix ci

3136b73

remove batch

c31020b

roryqi reviewed Nov 20, 2025

View reviewed changes

hdygxsj added 2 commits November 20, 2025 21:06

fix comment

fc04173

java doc

37f119d

roryqi reviewed Nov 20, 2025

View reviewed changes

hdygxsj added 4 commits November 21, 2025 10:49

Merge remote-tracking branch 'upstream/main' into feat-8942

1560ccb

Merge remote-tracking branch 'upstream/main' into feat-8942

c5d60e3

# Conflicts: # server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java

merge main

797709f

fix ci

a09b702