This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new c3172952c [#6060] fix(core): Add the check of requests related to
authorization (#6065)
c3172952c is described below
commit c3172952c7d70671641eb36a020c3cd12a84f0a8
Author: roryqi <[email protected]>
AuthorDate: Fri Jan 3 17:36:57 2025 +0800
[#6060] fix(core): Add the check of requests related to authorization
(#6065)
### What changes were proposed in this pull request?
Add the check of requests related to authorization
### Why are the changes needed?
Fix: #6060
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added UT.
---
.../gravitino/dto/requests/RoleCreateRequest.java | 9 ++++
.../gravitino/server/web/rest/GroupOperations.java | 12 +++--
.../gravitino/server/web/rest/OwnerOperations.java | 1 +
.../server/web/rest/PermissionOperations.java | 60 +++++++++++++---------
.../gravitino/server/web/rest/RoleOperations.java | 1 +
.../gravitino/server/web/rest/UserOperations.java | 12 +++--
.../server/web/rest/TestGroupOperations.java | 9 ++++
.../server/web/rest/TestOwnerOperations.java | 8 +++
.../server/web/rest/TestPermissionOperations.java | 47 ++++++++++++++++-
.../server/web/rest/TestRoleOperations.java | 31 ++++++++++-
.../server/web/rest/TestUserOperations.java | 9 ++++
11 files changed, 163 insertions(+), 36 deletions(-)
diff --git
a/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java
b/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java
index 9d85c0c6e..0466d7d31 100644
---
a/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java
+++
b/common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java
@@ -79,5 +79,14 @@ public class RoleCreateRequest implements RESTRequest {
Preconditions.checkArgument(
StringUtils.isNotBlank(name), "\"name\" field is required and cannot
be empty");
Preconditions.checkArgument(securableObjects != null,
"\"securableObjects\" can't null ");
+ for (SecurableObjectDTO objectDTO : securableObjects) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(objectDTO.name()), "\" securable object
name\" can't be blank");
+ Preconditions.checkArgument(
+ objectDTO.type() != null, "\" securable object type\" can't be
null");
+ Preconditions.checkArgument(
+ objectDTO.privileges() != null && !objectDTO.privileges().isEmpty(),
+ "\"securable object privileges\" can't be null or empty");
+ }
}
}
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 12cf76993..95db0ca67 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
@@ -103,11 +103,13 @@ public class GroupOperations {
TreeLockUtils.doWithTreeLock(
NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()),
LockType.WRITE,
- () ->
- Utils.ok(
- new GroupResponse(
- DTOConverters.toDTO(
- accessControlManager.addGroup(metalake,
request.getName()))))));
+ () -> {
+ request.validate();
+ return Utils.ok(
+ new GroupResponse(
+ DTOConverters.toDTO(
+ accessControlManager.addGroup(metalake,
request.getName()))));
+ }));
} catch (Exception e) {
return ExceptionHandlers.handleGroupException(
OperationType.ADD, request.getName(), metalake, e);
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java
index ea5684b55..7dcfcfd06 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java
@@ -113,6 +113,7 @@ public class OwnerOperations {
return Utils.doAs(
httpRequest,
() -> {
+ request.validate();
MetadataObjectUtil.checkMetadataObject(metalake, object);
NameIdentifier objectIdent =
MetadataObjectUtil.toEntityIdent(metalake, object);
TreeLockUtils.doWithTreeLock(
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
index 38fcd7380..3ce1517a4 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
@@ -87,12 +87,14 @@ public class PermissionOperations {
TreeLockUtils.doWithTreeLock(
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
LockType.READ,
- () ->
- Utils.ok(
- new UserResponse(
- DTOConverters.toDTO(
-
accessControlManager.grantRolesToUser(
- metalake,
request.getRoleNames(), user)))))));
+ () -> {
+ request.validate();
+ return Utils.ok(
+ new UserResponse(
+ DTOConverters.toDTO(
+ accessControlManager.grantRolesToUser(
+ metalake, request.getRoleNames(),
user))));
+ })));
} catch (Exception e) {
return ExceptionHandlers.handleUserPermissionOperationException(
OperationType.GRANT, StringUtils.join(request.getRoleNames(), ","),
user, e);
@@ -119,12 +121,14 @@ public class PermissionOperations {
TreeLockUtils.doWithTreeLock(
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
LockType.READ,
- () ->
- Utils.ok(
- new GroupResponse(
- DTOConverters.toDTO(
-
accessControlManager.grantRolesToGroup(
- metalake,
request.getRoleNames(), group)))))));
+ () -> {
+ request.validate();
+ return Utils.ok(
+ new GroupResponse(
+ DTOConverters.toDTO(
+ accessControlManager.grantRolesToGroup(
+ metalake, request.getRoleNames(),
group))));
+ })));
} catch (Exception e) {
return ExceptionHandlers.handleGroupPermissionOperationException(
OperationType.GRANT, StringUtils.join(request.getRoleNames(), ","),
group, e);
@@ -151,12 +155,14 @@ public class PermissionOperations {
TreeLockUtils.doWithTreeLock(
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
LockType.READ,
- () ->
- Utils.ok(
- new UserResponse(
- DTOConverters.toDTO(
-
accessControlManager.revokeRolesFromUser(
- metalake,
request.getRoleNames(), user)))))));
+ () -> {
+ request.validate();
+ return Utils.ok(
+ new UserResponse(
+ DTOConverters.toDTO(
+
accessControlManager.revokeRolesFromUser(
+ metalake, request.getRoleNames(),
user))));
+ })));
} catch (Exception e) {
return ExceptionHandlers.handleUserPermissionOperationException(
OperationType.REVOKE, StringUtils.join(request.getRoleNames(), ","),
user, e);
@@ -183,12 +189,14 @@ public class PermissionOperations {
TreeLockUtils.doWithTreeLock(
NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
LockType.READ,
- () ->
- Utils.ok(
- new GroupResponse(
- DTOConverters.toDTO(
-
accessControlManager.revokeRolesFromGroup(
- metalake,
request.getRoleNames(), group)))))));
+ () -> {
+ request.validate();
+ return Utils.ok(
+ new GroupResponse(
+ DTOConverters.toDTO(
+
accessControlManager.revokeRolesFromGroup(
+ metalake, request.getRoleNames(),
group))));
+ })));
} catch (Exception e) {
return ExceptionHandlers.handleGroupPermissionOperationException(
OperationType.REVOKE, StringUtils.join(request.getRoleNames()),
group, e);
@@ -214,6 +222,8 @@ public class PermissionOperations {
return Utils.doAs(
httpRequest,
() -> {
+ privilegeGrantRequest.validate();
+
for (PrivilegeDTO privilegeDTO :
privilegeGrantRequest.getPrivileges()) {
AuthorizationUtils.checkPrivilege(privilegeDTO, object,
metalake);
}
@@ -259,6 +269,8 @@ public class PermissionOperations {
return Utils.doAs(
httpRequest,
() -> {
+ privilegeRevokeRequest.validate();
+
for (PrivilegeDTO privilegeDTO :
privilegeRevokeRequest.getPrivileges()) {
AuthorizationUtils.checkPrivilege(privilegeDTO, object,
metalake);
}
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 bf82d78b6..9690afe13 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
@@ -127,6 +127,7 @@ public class RoleOperations {
return Utils.doAs(
httpRequest,
() -> {
+ request.validate();
Set<MetadataObject> metadataObjects = Sets.newHashSet();
for (SecurableObjectDTO object : request.getSecurableObjects()) {
MetadataObject metadataObject =
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 24f34d652..518178cd3 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
@@ -129,11 +129,13 @@ public class UserOperations {
TreeLockUtils.doWithTreeLock(
NameIdentifier.of(AuthorizationUtils.ofGroupNamespace(metalake).levels()),
LockType.WRITE,
- () ->
- Utils.ok(
- new UserResponse(
- DTOConverters.toDTO(
- accessControlManager.addUser(metalake,
request.getName()))))));
+ () -> {
+ request.validate();
+ return Utils.ok(
+ new UserResponse(
+ DTOConverters.toDTO(
+ accessControlManager.addUser(metalake,
request.getName()))));
+ }));
} catch (Exception e) {
return ExceptionHandlers.handleUserException(
OperationType.ADD, request.getName(), metalake, e);
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
index 77f0cf979..ac4f8c66a 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
@@ -116,6 +116,15 @@ public class TestGroupOperations extends JerseyTest {
when(manager.addGroup(any(), any())).thenReturn(group);
+ // test with IllegalRequest
+ GroupAddRequest illegalReq = new GroupAddRequest("");
+ Response illegalResp =
+ target("/metalakes/metalake1/groups")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
Response resp =
target("/metalakes/metalake1/groups")
.request(MediaType.APPLICATION_JSON_TYPE)
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java
index 0643ed9bf..dc7451a53 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java
@@ -202,6 +202,14 @@ class TestOwnerOperations extends JerseyTest {
@Test
void testSetOwnerForObject() {
when(metalakeDispatcher.metalakeExists(any())).thenReturn(true);
+ OwnerSetRequest invalidRequest = new OwnerSetRequest(null,
Owner.Type.USER);
+ Response invalidResp =
+ target("/metalakes/metalake1/owners/metalake/metalake1")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(invalidRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
invalidResp.getStatus());
+
OwnerSetRequest request = new OwnerSetRequest("test", Owner.Type.USER);
Response resp =
target("/metalakes/metalake1/owners/metalake/metalake1")
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
index 8876e9035..1f507cbbc 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
@@ -135,8 +135,15 @@ public class TestPermissionOperations extends JerseyTest {
.build();
when(manager.grantRolesToUser(any(), any(), any())).thenReturn(userEntity);
- RoleGrantRequest request = new
RoleGrantRequest(Lists.newArrayList("role1"));
+ RoleGrantRequest illegalReq = new RoleGrantRequest(null);
+ Response illegalResp =
+ target("/metalakes/metalake1/permissions/users/user/grant")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+ RoleGrantRequest request = new
RoleGrantRequest(Lists.newArrayList("role1"));
Response resp =
target("/metalakes/metalake1/permissions/users/user/grant")
.request(MediaType.APPLICATION_JSON_TYPE)
@@ -232,6 +239,15 @@ public class TestPermissionOperations extends JerseyTest {
.build();
when(manager.grantRolesToGroup(any(), any(),
any())).thenReturn(groupEntity);
+ // Test with Illegal request
+ RoleGrantRequest illegalReq = new RoleGrantRequest(null);
+ Response illegalResp =
+ target("/metalakes/metalake1/permissions/groups/group/grant")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
RoleGrantRequest request = new
RoleGrantRequest(Lists.newArrayList("role1"));
Response resp =
@@ -331,6 +347,16 @@ public class TestPermissionOperations extends JerseyTest {
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
.build();
when(manager.revokeRolesFromUser(any(), any(),
any())).thenReturn(userEntity);
+
+ // Test with illegal request
+ RoleRevokeRequest illegalReq = new RoleRevokeRequest(null);
+ Response illegalResp =
+ target("/metalakes/metalake1/permissions/users/user1/revoke")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
RoleRevokeRequest request = new
RoleRevokeRequest(Lists.newArrayList("role1"));
Response resp =
@@ -393,6 +419,15 @@ public class TestPermissionOperations extends JerseyTest {
AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build())
.build();
when(manager.revokeRolesFromGroup(any(), any(),
any())).thenReturn(groupEntity);
+ // Test with illegal request
+ RoleRevokeRequest illegalReq = new RoleRevokeRequest(null);
+ Response illegalResp =
+ target("/metalakes/metalake1/permissions/groups/group1/revoke")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
RoleRevokeRequest request = new
RoleRevokeRequest(Lists.newArrayList("role1"));
Response resp =
@@ -538,6 +573,16 @@ public class TestPermissionOperations extends JerseyTest {
.build();
when(manager.revokePrivilegesFromRole(any(), any(), any(),
any())).thenReturn(roleEntity);
when(metalakeDispatcher.metalakeExists(any())).thenReturn(true);
+
+ // Test with illegal request
+ PrivilegeRevokeRequest illegalReq = new PrivilegeRevokeRequest(null);
+ Response illegalResp =
+
target("/metalakes/metalake1/permissions/roles/role1/metalake/metalake1/revoke")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
PrivilegeRevokeRequest request =
new PrivilegeRevokeRequest(
Lists.newArrayList(
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
index 6edd33393..06d9fcc27 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
@@ -29,6 +29,7 @@ import static org.mockito.Mockito.when;
import com.google.common.collect.Lists;
import java.io.IOException;
+import java.lang.reflect.Field;
import java.time.Instant;
import java.util.Collections;
import java.util.concurrent.atomic.AtomicReference;
@@ -52,6 +53,7 @@ import org.apache.gravitino.catalog.FilesetDispatcher;
import org.apache.gravitino.catalog.SchemaDispatcher;
import org.apache.gravitino.catalog.TableDispatcher;
import org.apache.gravitino.catalog.TopicDispatcher;
+import org.apache.gravitino.dto.authorization.PrivilegeDTO;
import org.apache.gravitino.dto.authorization.RoleDTO;
import org.apache.gravitino.dto.authorization.SecurableObjectDTO;
import org.apache.gravitino.dto.requests.RoleCreateRequest;
@@ -142,7 +144,7 @@ public class TestRoleOperations extends JerseyTest {
}
@Test
- public void testCreateRole() {
+ public void testCreateRole() throws IllegalAccessException,
NoSuchFieldException {
SecurableObject securableObject =
SecurableObjects.ofCatalog("catalog",
Lists.newArrayList(Privileges.UseCatalog.allow()));
SecurableObject anotherSecurableObject =
@@ -161,6 +163,33 @@ public class TestRoleOperations extends JerseyTest {
when(manager.createRole(any(), any(), any(), any())).thenReturn(role);
when(catalogDispatcher.catalogExists(any())).thenReturn(true);
+ // Test with IllegalRequest
+ RoleCreateRequest illegalRequest = new RoleCreateRequest("role",
Collections.emptyMap(), null);
+ Response illegalResp =
+ target("/metalakes/metalake1/roles")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(Entity.entity(illegalRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
+ SecurableObjectDTO illegalObject =
+ DTOConverters.toDTO(
+ SecurableObjects.ofCatalog(
+ "illegal_catalog",
Lists.newArrayList(Privileges.CreateSchema.deny())));
+ Field field = illegalObject.getClass().getDeclaredField("privileges");
+ field.setAccessible(true);
+ field.set(illegalObject, new PrivilegeDTO[] {});
+
+ illegalRequest =
+ new RoleCreateRequest(
+ "role", Collections.emptyMap(), new SecurableObjectDTO[]
{illegalObject});
+ illegalResp =
+ target("/metalakes/metalake1/roles")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(Entity.entity(illegalRequest,
MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
Response resp =
target("/metalakes/metalake1/roles")
.request(MediaType.APPLICATION_JSON_TYPE)
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java
index 7f570e779..82bc59155 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java
@@ -115,6 +115,15 @@ public class TestUserOperations extends JerseyTest {
when(manager.addUser(any(), any())).thenReturn(user);
+ // test with IllegalRequest
+ UserAddRequest illegalReq = new UserAddRequest("");
+ Response illegalResp =
+ target("/metalakes/metalake1/users")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(Entity.entity(illegalReq, MediaType.APPLICATION_JSON_TYPE));
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
illegalResp.getStatus());
+
Response resp =
target("/metalakes/metalake1/users")
.request(MediaType.APPLICATION_JSON_TYPE)
