diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java index 3b62bbf229b..f67f1168fce 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/CatalogAuthorizationIT.java @@ -22,8 +22,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import com.google.common.collect.Maps; +import java.lang.reflect.Method; import java.util.Map; import org.apache.gravitino.Catalog; +import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.dto.MetalakeDTO; import org.apache.gravitino.exceptions.ForbiddenException; import org.apache.gravitino.integration.test.container.ContainerSuite; import org.apache.gravitino.integration.test.container.HiveContainer; @@ -146,4 +149,48 @@ public void testDeleteCatalog() { catalogs = client.loadMetalake(METALAKE).listCatalogs(); assertEquals(0, catalogs.length); } + + @Test + @Order(4) + public void testListCatalogsWithNonExistentMetalake() throws Exception { + // Test that listCatalogs with @AuthorizationExpression returns 403 Forbidden + // when the metalake doesn't exist, instead of 404 response + String nonExistentMetalake = "nonExistentMetalake"; + + // Access the restClient from normalUserClient using reflection + Method restClientMethod = + normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient"); + restClientMethod.setAccessible(true); + Object restClient = restClientMethod.invoke(normalUserClient); + + // Create a MetalakeDTO for the non-existent metalake + MetalakeDTO metalakeDTO = + MetalakeDTO.builder() + .withName(nonExistentMetalake) + .withComment("test") + .withProperties(Maps.newHashMap()) + .withAudit( + org.apache.gravitino.dto.AuditDTO.builder() + .withCreator("test") + .withCreateTime(java.time.Instant.now()) + .build()) + .build(); + + // Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake + Class dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters"); + Method toMetaLakeMethod = + dtoConvertersClass.getDeclaredMethod( + "toMetaLake", + MetalakeDTO.class, + Class.forName("org.apache.gravitino.client.RESTClient")); + toMetaLakeMethod.setAccessible(true); + GravitinoMetalake nonExistentMetalakeObj = + (GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient); + + // Test listCatalogs - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listCatalogs); + + // Test listCatalogsInfo - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listCatalogsInfo); + } } diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/GroupAuthorizationIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/GroupAuthorizationIT.java index deb5d8bf445..a2790eed74c 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/GroupAuthorizationIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/GroupAuthorizationIT.java @@ -20,12 +20,15 @@ import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Maps; +import java.lang.reflect.Method; import java.util.Collections; import java.util.HashMap; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.MetadataObjects; import org.apache.gravitino.authorization.Privileges; import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.dto.MetalakeDTO; import org.apache.gravitino.exceptions.ForbiddenException; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Order; @@ -71,4 +74,48 @@ public void testRemoveGroup() { gravitinoMetalake.grantRolesToUser(ImmutableList.of("role"), NORMAL_USER); normalUserClient.loadMetalake(METALAKE).removeGroup("group2"); } + + @Test + @Order(3) + public void testListGroupsWithNonExistentMetalake() throws Exception { + // Test that listGroups with @AuthorizationExpression returns 403 Forbidden + // when the metalake doesn't exist, instead of 404 response + String nonExistentMetalake = "nonExistentMetalake"; + + // Access the restClient from normalUserClient using reflection + Method restClientMethod = + normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient"); + restClientMethod.setAccessible(true); + Object restClient = restClientMethod.invoke(normalUserClient); + + // Create a MetalakeDTO for the non-existent metalake + MetalakeDTO metalakeDTO = + MetalakeDTO.builder() + .withName(nonExistentMetalake) + .withComment("test") + .withProperties(Maps.newHashMap()) + .withAudit( + org.apache.gravitino.dto.AuditDTO.builder() + .withCreator("test") + .withCreateTime(java.time.Instant.now()) + .build()) + .build(); + + // Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake + Class dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters"); + Method toMetaLakeMethod = + dtoConvertersClass.getDeclaredMethod( + "toMetaLake", + MetalakeDTO.class, + Class.forName("org.apache.gravitino.client.RESTClient")); + toMetaLakeMethod.setAccessible(true); + GravitinoMetalake nonExistentMetalakeObj = + (GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient); + + // Test listGroups - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listGroups); + + // Test listGroupNames - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listGroupNames); + } } diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/RoleAuthorizationIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/RoleAuthorizationIT.java index 73daa1ce727..e5996515976 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/RoleAuthorizationIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/RoleAuthorizationIT.java @@ -20,12 +20,16 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertThrows; +import com.google.common.collect.Maps; +import java.lang.reflect.Method; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.MetadataObjects; import org.apache.gravitino.authorization.Privileges; +import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.dto.MetalakeDTO; import org.apache.gravitino.exceptions.ForbiddenException; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Order; @@ -151,4 +155,45 @@ public void testDeleteRole() { .createRole("role2", new HashMap<>(), Collections.emptyList()); }); } + + @Test + @Order(5) + public void testListRolesWithNonExistentMetalake() throws Exception { + // Test that listRoles with @AuthorizationExpression returns 403 Forbidden + // when the metalake doesn't exist, instead of 404 response + String nonExistentMetalake = "nonExistentMetalake"; + + // Access the restClient from normalUserClient using reflection + Method restClientMethod = + normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient"); + restClientMethod.setAccessible(true); + Object restClient = restClientMethod.invoke(normalUserClient); + + // Create a MetalakeDTO for the non-existent metalake + MetalakeDTO metalakeDTO = + MetalakeDTO.builder() + .withName(nonExistentMetalake) + .withComment("test") + .withProperties(Maps.newHashMap()) + .withAudit( + org.apache.gravitino.dto.AuditDTO.builder() + .withCreator("test") + .withCreateTime(java.time.Instant.now()) + .build()) + .build(); + + // Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake + Class dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters"); + Method toMetaLakeMethod = + dtoConvertersClass.getDeclaredMethod( + "toMetaLake", + MetalakeDTO.class, + Class.forName("org.apache.gravitino.client.RESTClient")); + toMetaLakeMethod.setAccessible(true); + GravitinoMetalake nonExistentMetalakeObj = + (GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient); + + // Test listRoleNames - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listRoleNames); + } } diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java index 3d7d5eb9615..247adb5a007 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/TagOperationsAuthorizationIT.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import java.io.IOException; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -38,6 +39,7 @@ import org.apache.gravitino.authorization.SecurableObject; import org.apache.gravitino.authorization.SecurableObjects; import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.dto.MetalakeDTO; import org.apache.gravitino.exceptions.ForbiddenException; import org.apache.gravitino.integration.test.container.ContainerSuite; import org.apache.gravitino.integration.test.container.HiveContainer; @@ -294,6 +296,82 @@ public void testDropTag() { gravitinoMetalake.deleteTag("tag1"); } + @Test + @Order(9) + public void testTagOperationsWithNonExistentMetalake() throws Exception { + // Test that all tag operations with @AuthorizationExpression return 403 Forbidden + // when the metalake doesn't exist, instead of inconsistent 404 responses + String nonExistentMetalake = "nonExistentMetalake"; + + // Access the restClient from normalUserClient using reflection + Method restClientMethod = + normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient"); + restClientMethod.setAccessible(true); + Object restClient = restClientMethod.invoke(normalUserClient); + + // Create a MetalakeDTO for the non-existent metalake + MetalakeDTO metalakeDTO = + MetalakeDTO.builder() + .withName(nonExistentMetalake) + .withComment("test") + .withProperties(Maps.newHashMap()) + .withAudit( + org.apache.gravitino.dto.AuditDTO.builder() + .withCreator("test") + .withCreateTime(java.time.Instant.now()) + .build()) + .build(); + + // Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake + Class dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters"); + Method toMetaLakeMethod = + dtoConvertersClass.getDeclaredMethod( + "toMetaLake", + MetalakeDTO.class, + Class.forName("org.apache.gravitino.client.RESTClient")); + toMetaLakeMethod.setAccessible(true); + GravitinoMetalake nonExistentMetalakeObj = + (GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient); + + // Test createTag - should return 403 ForbiddenException + assertThrows( + ForbiddenException.class, + () -> nonExistentMetalakeObj.createTag("testTag", "comment", Maps.newHashMap())); + + // Test getTag - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, () -> nonExistentMetalakeObj.getTag("testTag")); + + // Test alterTag - should return 403 ForbiddenException + assertThrows( + ForbiddenException.class, + () -> nonExistentMetalakeObj.alterTag("testTag", TagChange.rename("newName"))); + + // Test deleteTag - should return 403 ForbiddenException + assertThrows( + ForbiddenException.class, + () -> { + nonExistentMetalakeObj.deleteTag("testTag"); + }); + + // Test listTags - now has @AuthorizationExpression with empty expression, + // should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTags); + + // Test listTagsInfo - now has @AuthorizationExpression with empty expression, + // should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listTagsInfo); + + // Test associateTags for metadata object - should return 403 ForbiddenException + // when trying to associate tags to a catalog in a non-existent metalake + assertThrows( + ForbiddenException.class, + () -> + nonExistentMetalakeObj + .loadCatalog(CATALOG) + .supportsTags() + .associateTags(new String[] {"testTag"}, null)); + } + private Column[] createColumns() { return new Column[] {Column.of("col1", Types.StringType.get())}; } diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/UserAuthorizationIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/UserAuthorizationIT.java index 4e39fe559b0..f0b92daccce 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/UserAuthorizationIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/UserAuthorizationIT.java @@ -21,6 +21,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Maps; +import java.lang.reflect.Method; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; @@ -31,6 +33,7 @@ import org.apache.gravitino.authorization.User; import org.apache.gravitino.client.GravitinoAdminClient; import org.apache.gravitino.client.GravitinoMetalake; +import org.apache.gravitino.dto.MetalakeDTO; import org.apache.gravitino.exceptions.ForbiddenException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.MethodOrderer; @@ -126,6 +129,50 @@ public void testRemoveUser() { assertUserEquals(new String[] {USER, NORMAL_USER, "user1"}, users); } + @Test + @Order(4) + public void testListUsersWithNonExistentMetalake() throws Exception { + // Test that listUsers with @AuthorizationExpression returns 403 Forbidden + // when the metalake doesn't exist, instead of 404 response + String nonExistentMetalake = "nonExistentMetalake"; + + // Access the restClient from normalUserClient using reflection + Method restClientMethod = + normalUserClient.getClass().getSuperclass().getDeclaredMethod("restClient"); + restClientMethod.setAccessible(true); + Object restClient = restClientMethod.invoke(normalUserClient); + + // Create a MetalakeDTO for the non-existent metalake + MetalakeDTO metalakeDTO = + MetalakeDTO.builder() + .withName(nonExistentMetalake) + .withComment("test") + .withProperties(Maps.newHashMap()) + .withAudit( + org.apache.gravitino.dto.AuditDTO.builder() + .withCreator("test") + .withCreateTime(java.time.Instant.now()) + .build()) + .build(); + + // Use DTOConverters.toMetaLake() via reflection to create GravitinoMetalake + Class dtoConvertersClass = Class.forName("org.apache.gravitino.client.DTOConverters"); + Method toMetaLakeMethod = + dtoConvertersClass.getDeclaredMethod( + "toMetaLake", + MetalakeDTO.class, + Class.forName("org.apache.gravitino.client.RESTClient")); + toMetaLakeMethod.setAccessible(true); + GravitinoMetalake nonExistentMetalakeObj = + (GravitinoMetalake) toMetaLakeMethod.invoke(null, metalakeDTO, restClient); + + // Test listUsers - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listUsers); + + // Test listUserNames - should return 403 ForbiddenException + assertThrows(ForbiddenException.class, nonExistentMetalakeObj::listUserNames); + } + private void assertUserEquals(String[] exceptUsers, User[] actualUsers) { Arrays.sort(exceptUsers); Arrays.sort(actualUsers, Comparator.comparing(User::name)); diff --git a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java index 6146781760a..c0438a5354c 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java +++ b/server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java @@ -41,6 +41,7 @@ import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.NoSuchMetalakeException; import org.apache.gravitino.server.authorization.annotations.AuthorizationExpression; import org.apache.gravitino.server.authorization.annotations.AuthorizationRequest; import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; @@ -132,37 +133,6 @@ public Object invoke(MethodInvocation methodInvocation) throws Throwable { AuthorizationExpression expressionAnnotation = method.getAnnotation(AuthorizationExpression.class); - // Check current user exists in metalake before authorization - if (expressionAnnotation != null) { - Object[] args = methodInvocation.getArguments(); - Map metadataContext = - extractNameIdentifierFromParameters(parameters, args); - - // Check if current user exists in the metalake. - NameIdentifier metalakeIdent = metadataContext.get(Entity.EntityType.METALAKE); - - if (metalakeIdent != null) { - String currentUser = PrincipalUtils.getCurrentUserName(); - try { - AuthorizationUtils.checkCurrentUser(metalakeIdent.name(), currentUser); - } catch (ForbiddenException ex) { - LOG.warn( - "User validation failed - User: {}, Metalake: {}, Reason: {}", - currentUser, - metalakeIdent.name(), - ex.getMessage()); - return Utils.forbidden(ex.getMessage(), ex); - } catch (Exception ex) { - LOG.error( - "Unexpected error during user validation - User: {}, Metalake: {}", - currentUser, - metalakeIdent.name(), - ex); - return Utils.internalError("Failed to validate user", ex); - } - } - } - try { AuthorizationExecutor executor; if (expressionAnnotation != null) { @@ -173,22 +143,53 @@ public Object invoke(MethodInvocation methodInvocation) throws Throwable { extractNameIdentifierFromParameters(parameters, args); Map pathParams = Utils.extractPathParamsFromParameters(parameters, args); - AuthorizationExpressionEvaluator authorizationExpressionEvaluator = - new AuthorizationExpressionEvaluator(expression); - AuthorizationRequest.RequestType requestType = - extractAuthorizationRequestTypeFromParameters(parameters); - executor = - AuthorizeExecutorFactory.create( - requestType, - metadataContext, - authorizationExpressionEvaluator, - pathParams, - entityType, - parameters, - args); - boolean authorizeResult = executor.execute(); - if (!authorizeResult) { - return buildNoAuthResponse(expressionAnnotation, metadataContext, method, expression); + + // Check metalake and user existence before authorization + NameIdentifier metalakeIdent = metadataContext.get(Entity.EntityType.METALAKE); + if (metalakeIdent != null) { + String currentUser = PrincipalUtils.getCurrentUserName(); + try { + AuthorizationUtils.checkCurrentUser(metalakeIdent.name(), currentUser); + } catch (NoSuchMetalakeException e) { + LOG.warn( + "Metalake {} does not exist when validating user {}", metalakeIdent, currentUser); + return buildNoAuthResponse(expressionAnnotation, metadataContext, method, expression); + } catch (ForbiddenException ex) { + LOG.warn( + "User validation failed - User: {}, Metalake: {}, Reason: {}", + currentUser, + metalakeIdent.name(), + ex.getMessage()); + return Utils.forbidden(ex.getMessage(), ex); + } catch (Exception ex) { + LOG.error( + "Unexpected error during user validation - User: {}, Metalake: {}", + currentUser, + metalakeIdent.name(), + ex); + return Utils.internalError("Failed to validate user", ex); + } + } + + // If expression is empty, skip authorization check (method handles its own filtering) + if (StringUtils.isNotBlank(expression)) { + AuthorizationExpressionEvaluator authorizationExpressionEvaluator = + new AuthorizationExpressionEvaluator(expression); + AuthorizationRequest.RequestType requestType = + extractAuthorizationRequestTypeFromParameters(parameters); + executor = + AuthorizeExecutorFactory.create( + requestType, + metadataContext, + authorizationExpressionEvaluator, + pathParams, + entityType, + parameters, + args); + boolean authorizeResult = executor.execute(); + if (!authorizeResult) { + return buildNoAuthResponse(expressionAnnotation, metadataContext, method, expression); + } } } return methodInvocation.proceed(); diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java index 11b15809b2e..43d7a6f7794 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java @@ -84,8 +84,10 @@ public CatalogOperations(CatalogDispatcher catalogDispatcher) { @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-catalog." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-catalog", absolute = true) + @AuthorizationExpression(expression = "") public Response listCatalogs( - @PathParam("metalake") String metalake, + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake, @QueryParam("details") @DefaultValue("false") boolean verbose) { LOG.info( "Received list catalog {} request for metalake: {}, ", diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java index ae2292f26ab..08ca7b73251 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java @@ -158,8 +158,10 @@ public Response removeGroup( @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-group." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-group", absolute = true) + @AuthorizationExpression(expression = "") public Response listGroups( - @PathParam("metalake") String metalake, + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake, @QueryParam("details") @DefaultValue("false") boolean verbose) { LOG.info("Received list groups request."); try { diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java index 42578191565..49f640b9884 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java @@ -82,7 +82,10 @@ public RoleOperations() { @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-role." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-role", absolute = true) - public Response listRoles(@PathParam("metalake") String metalake) { + @AuthorizationExpression(expression = "") + public Response listRoles( + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake) { try { return Utils.doAs( httpRequest, diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java index 0813ad48629..95a2973ddf3 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java @@ -87,8 +87,10 @@ public TagOperations(TagDispatcher tagDispatcher) { @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-tags." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-tags", absolute = true) + @AuthorizationExpression(expression = "") public Response listTags( - @PathParam("metalake") String metalake, + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake, @QueryParam("details") @DefaultValue("false") boolean verbose) { LOG.info( "Received list tag {} request for metalake: {}", verbose ? "infos" : "names", metalake); @@ -261,8 +263,11 @@ public Response deleteTag( @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-objects-for-tag." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-objects-for-tag", absolute = true) + @AuthorizationExpression(expression = "") public Response listMetadataObjectsForTag( - @PathParam("metalake") String metalake, @PathParam("tag") String tagName) { + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake, + @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG) String tagName) { LOG.info("Received list objects for tag: {} under metalake: {}", tagName, metalake); try { diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java index f35c4e5f49e..2fc38033c80 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java @@ -103,8 +103,10 @@ public Response getUser( @Produces("application/vnd.gravitino.v1+json") @Timed(name = "list-user." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) @ResponseMetered(name = "list-user", absolute = true) + @AuthorizationExpression(expression = "") public Response listUsers( - @PathParam("metalake") String metalake, + @PathParam("metalake") @AuthorizationMetadata(type = Entity.EntityType.METALAKE) + String metalake, @QueryParam("details") @DefaultValue("false") boolean verbose) { try { return Utils.doAs( diff --git a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java index f508d082e4b..7a5826df7ac 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java +++ b/server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java @@ -30,19 +30,25 @@ import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; import org.apache.gravitino.Entity; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.GravitinoEnv; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.UserPrincipal; import org.apache.gravitino.authorization.AuthorizationRequestContext; +import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.authorization.GravitinoAuthorizer; import org.apache.gravitino.authorization.Privilege; import org.apache.gravitino.dto.responses.ErrorResponse; +import org.apache.gravitino.exceptions.NoSuchMetalakeException; +import org.apache.gravitino.metalake.MetalakeManager; import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider; import org.apache.gravitino.server.authorization.annotations.AuthorizationExpression; import org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata; import org.apache.gravitino.server.web.Utils; import org.apache.gravitino.utils.PrincipalUtils; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentMatchers; import org.mockito.MockedStatic; /** Test for {@link GravitinoInterceptionService}. */ @@ -52,7 +58,9 @@ public class TestGravitinoInterceptionService { public void testMetadataAuthorizationMethodInterceptor() throws Throwable { try (MockedStatic principalUtilsMocked = mockStatic(PrincipalUtils.class); MockedStatic mockStatic = - mockStatic(GravitinoAuthorizerProvider.class)) { + mockStatic(GravitinoAuthorizerProvider.class); + MockedStatic envMocked = mockStatic(GravitinoEnv.class); + MockedStatic metalakeManagerMocked = mockStatic(MetalakeManager.class)) { principalUtilsMocked .when(PrincipalUtils::getCurrentPrincipal) .thenReturn(new UserPrincipal("tester")); @@ -61,6 +69,18 @@ public void testMetadataAuthorizationMethodInterceptor() throws Throwable { GravitinoAuthorizerProvider mockedProvider = mock(GravitinoAuthorizerProvider.class); mockStatic.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider); when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new MockGravitinoAuthorizer()); + + // Mock GravitinoEnv and EntityStore + GravitinoEnv mockEnv = mock(GravitinoEnv.class); + EntityStore mockStore = mock(EntityStore.class); + envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv); + when(mockEnv.entityStore()).thenReturn(mockStore); + + // Mock MetalakeManager.checkMetalake to do nothing (metalake exists) + metalakeManagerMocked + .when(() -> MetalakeManager.checkMetalake(ArgumentMatchers.any(), ArgumentMatchers.any())) + .thenAnswer(invocation -> null); + GravitinoInterceptionService gravitinoInterceptionService = new GravitinoInterceptionService(); Class testOperationsClass = TestOperations.class; @@ -127,6 +147,109 @@ public void testSystemInternalErrorHandling() throws Throwable { } } + @Test + public void testMetalakeNotExist() throws Throwable { + try (MockedStatic principalUtilsMocked = mockStatic(PrincipalUtils.class); + MockedStatic authorizerMocked = + mockStatic(GravitinoAuthorizerProvider.class); + MockedStatic authorizationUtilsMocked = + mockStatic(AuthorizationUtils.class)) { + + principalUtilsMocked + .when(PrincipalUtils::getCurrentPrincipal) + .thenReturn(new UserPrincipal("tester")); + principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester"); + + MethodInvocation methodInvocation = mock(MethodInvocation.class); + GravitinoAuthorizerProvider mockedProvider = mock(GravitinoAuthorizerProvider.class); + authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider); + when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new MockGravitinoAuthorizer()); + + // Mock AuthorizationUtils.checkCurrentUser to throw NoSuchMetalakeException + authorizationUtilsMocked + .when( + () -> + AuthorizationUtils.checkCurrentUser( + ArgumentMatchers.any(), ArgumentMatchers.any())) + .thenThrow(new NoSuchMetalakeException("Metalake nonExistentMetalake does not exist")); + + GravitinoInterceptionService gravitinoInterceptionService = + new GravitinoInterceptionService(); + Class testOperationsClass = TestOperations.class; + Method[] methods = testOperationsClass.getMethods(); + Method testMethod = methods[0]; + List methodInterceptors = + gravitinoInterceptionService.getMethodInterceptors(testMethod); + MethodInterceptor methodInterceptor = methodInterceptors.get(0); + + // Test with non-existent metalake + when(methodInvocation.getMethod()).thenReturn(testMethod); + when(methodInvocation.getArguments()).thenReturn(new Object[] {"nonExistentMetalake"}); + Response response = (Response) methodInterceptor.invoke(methodInvocation); + + // Verify that a 403 Forbidden response is returned + assertEquals(Response.Status.FORBIDDEN.getStatusCode(), response.getStatus()); + ErrorResponse errorResponse = (ErrorResponse) response.getEntity(); + assertEquals( + "User 'tester' is not authorized to perform operation 'testMethod' on " + + "metadata 'nonExistentMetalake'", + errorResponse.getMessage()); + } + } + + @Test + public void testEmptyExpressionSkipsAuthorization() throws Throwable { + try (MockedStatic principalUtilsMocked = mockStatic(PrincipalUtils.class); + MockedStatic authorizerMocked = + mockStatic(GravitinoAuthorizerProvider.class); + MockedStatic envMocked = + mockStatic(org.apache.gravitino.GravitinoEnv.class); + MockedStatic metalakeManagerMocked = + mockStatic(org.apache.gravitino.metalake.MetalakeManager.class)) { + + principalUtilsMocked + .when(PrincipalUtils::getCurrentPrincipal) + .thenReturn(new UserPrincipal("tester")); + principalUtilsMocked.when(PrincipalUtils::getCurrentUserName).thenReturn("tester"); + + GravitinoAuthorizerProvider mockedProvider = mock(GravitinoAuthorizerProvider.class); + authorizerMocked.when(GravitinoAuthorizerProvider::getInstance).thenReturn(mockedProvider); + when(mockedProvider.getGravitinoAuthorizer()).thenReturn(new MockGravitinoAuthorizer()); + + GravitinoEnv mockEnv = mock(GravitinoEnv.class); + EntityStore mockStore = mock(EntityStore.class); + envMocked.when(GravitinoEnv::getInstance).thenReturn(mockEnv); + when(mockEnv.entityStore()).thenReturn(mockStore); + + // Mock MetalakeManager.checkMetalake to do nothing (metalake exists) + metalakeManagerMocked + .when(() -> MetalakeManager.checkMetalake(ArgumentMatchers.any(), ArgumentMatchers.any())) + .thenAnswer(invocation -> null); + + GravitinoInterceptionService gravitinoInterceptionService = + new GravitinoInterceptionService(); + Class testOperationsClass = + TestOperationsWithEmptyExpression.class; + Method[] methods = testOperationsClass.getMethods(); + Method testMethod = methods[0]; + List methodInterceptors = + gravitinoInterceptionService.getMethodInterceptors(testMethod); + MethodInterceptor methodInterceptor = methodInterceptors.get(0); + + MethodInvocation methodInvocation = mock(MethodInvocation.class); + when(methodInvocation.getMethod()).thenReturn(testMethod); + when(methodInvocation.getArguments()).thenReturn(new Object[] {"testMetalake"}); + when(methodInvocation.proceed()).thenReturn(Utils.ok("success")); + + // Test with empty expression - should skip authorization and proceed + Response response = (Response) methodInterceptor.invoke(methodInvocation); + + // Verify that the method was allowed to proceed without authorization check + assertEquals(Response.Status.OK.getStatusCode(), response.getStatus()); + assertEquals("success", response.getEntity()); + } + } + public static class TestOperations { @AuthorizationExpression( @@ -138,6 +261,15 @@ public Response testMethod( } } + public static class TestOperationsWithEmptyExpression { + + @AuthorizationExpression(expression = "", accessMetadataType = MetadataObject.Type.METALAKE) + public Response testMethodWithEmptyExpression( + @AuthorizationMetadata(type = Entity.EntityType.METALAKE) String metalake) { + return Utils.ok("success"); + } + } + private static class MockGravitinoAuthorizer implements GravitinoAuthorizer { @Override