Skip to content

Commit

Permalink
[apache#6060] fix(core): Add the check of requests related to authori…
Browse files Browse the repository at this point in the history 
…zation (apache#6065)

### What changes were proposed in this pull request?

Add the check of requests related to authorization

### Why are the changes needed?

Fix: apache#6060 
### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?
Added UT.
  • Loading branch information
@jerqi @Abyss-lord
jerqi authored and Abyss-lord committed Jan 4, 2025
1 parent 1c01f97 commit 2bb614e
Show file tree
Hide file tree
Showing 11 changed files with 163 additions and 36 deletions.
9 changes: 9 additions & 0 deletions common/src/main/java/org/apache/gravitino/dto/requests/RoleCreateRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,14 @@ public void validate() throws IllegalArgumentException {
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");
}
}
}
12 changes: 7 additions & 5 deletions server/src/main/java/org/apache/gravitino/server/web/rest/GroupOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,13 @@ public Response addGroup(@PathParam("metalake") String metalake, GroupAddRequest
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);
Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/org/apache/gravitino/server/web/rest/OwnerOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ public Response setOwnerForObject(
return Utils.doAs(
httpRequest,
() -> {
request.validate();
MetadataObjectUtil.checkMetadataObject(metalake, object);
NameIdentifier objectIdent = MetadataObjectUtil.toEntityIdent(metalake, object);
TreeLockUtils.doWithTreeLock(
Expand Down
60 changes: 36 additions & 24 deletions server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,14 @@ public Response grantRolesToUser(
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);
Expand All @@ -119,12 +121,14 @@ public Response grantRolesToGroup(
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);
Expand All @@ -151,12 +155,14 @@ public Response revokeRolesFromUser(
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);
Expand All @@ -183,12 +189,14 @@ public Response revokeRolesFromGroup(
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);
Expand All @@ -214,6 +222,8 @@ public Response grantPrivilegeToRole(
return Utils.doAs(
httpRequest,
() -> {
privilegeGrantRequest.validate();

for (PrivilegeDTO privilegeDTO : privilegeGrantRequest.getPrivileges()) {
AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake);
}
Expand Down Expand Up @@ -259,6 +269,8 @@ public Response revokePrivilegeFromRole(
return Utils.doAs(
httpRequest,
() -> {
privilegeRevokeRequest.validate();

for (PrivilegeDTO privilegeDTO : privilegeRevokeRequest.getPrivileges()) {
AuthorizationUtils.checkPrivilege(privilegeDTO, object, metalake);
}
Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq
return Utils.doAs(
httpRequest,
() -> {
request.validate();
Set<MetadataObject> metadataObjects = Sets.newHashSet();
for (SecurableObjectDTO object : request.getSecurableObjects()) {
MetadataObject metadataObject =
Expand Down
12 changes: 7 additions & 5 deletions server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,13 @@ public Response addUser(@PathParam("metalake") String metalake, UserAddRequest r
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);
Expand Down
9 changes: 9 additions & 0 deletions server/src/test/java/org/apache/gravitino/server/web/rest/TestGroupOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,15 @@ public void testAddGroup() {

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)
Expand Down
8 changes: 8 additions & 0 deletions server/src/test/java/org/apache/gravitino/server/web/rest/TestOwnerOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ public Type type() {
@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")
Expand Down
47 changes: 46 additions & 1 deletion server/src/test/java/org/apache/gravitino/server/web/rest/TestPermissionOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,15 @@ public void testGrantRolesToUser() {
.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)
Expand Down Expand Up @@ -232,6 +239,15 @@ public void testGrantRolesToGroup() {
.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 =
Expand Down Expand Up @@ -331,6 +347,16 @@ public void testRevokeRolesFromUser() {
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 =
Expand Down Expand Up @@ -393,6 +419,15 @@ public void testRevokeRolesFromGroup() {
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 =
Expand Down Expand Up @@ -538,6 +573,16 @@ public void testRevokePrivilegesFromRole() {
.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(
Expand Down
31 changes: 30 additions & 1 deletion server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

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;
Expand All @@ -52,6 +53,7 @@
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;
Expand Down Expand Up @@ -142,7 +144,7 @@ protected void configure() {
}

@Test
public void testCreateRole() {
public void testCreateRole() throws IllegalAccessException, NoSuchFieldException {
SecurableObject securableObject =
SecurableObjects.ofCatalog("catalog", Lists.newArrayList(Privileges.UseCatalog.allow()));
SecurableObject anotherSecurableObject =
Expand All @@ -161,6 +163,33 @@ public void testCreateRole() {
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)
Expand Down
9 changes: 9 additions & 0 deletions server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,15 @@ public void testAddUser() {

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)
Expand Down

0 comments on commit 2bb614e

Please sign in to comment.