diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java b/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java index abc6421d955..ecf1dbff4c7 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java @@ -55,6 +55,7 @@ public class ErrorMessages { public static final String MISSING_GROUP = "Missing --group option."; public static final String MISSING_METALAKE = "Missing --metalake option."; public static final String MISSING_NAME = "Missing --name option."; + public static final String MISSING_PRIVILEGES = "Missing --privilege option."; public static final String MISSING_PROPERTY = "Missing --property option."; public static final String MISSING_PROPERTY_AND_VALUE = "Missing --property and --value options."; public static final String MISSING_ROLE = "Missing --role option."; @@ -63,6 +64,8 @@ public class ErrorMessages { public static final String MISSING_USER = "Missing --user option."; public static final String MISSING_VALUE = "Missing --value option."; + public static final String MULTIPLE_ROLE_COMMAND_ERROR = + "This command only supports one --role option."; public static final String MULTIPLE_TAG_COMMAND_ERROR = "This command only supports one --tag option."; public static final String MISSING_PROVIDER = "Missing --provider option."; diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java b/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java index 07a1ecd5b7f..1ac8a420441 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java @@ -20,7 +20,6 @@ package org.apache.gravitino.cli; import com.google.common.base.Joiner; -import com.google.common.base.Preconditions; import com.google.common.collect.Lists; import java.io.BufferedReader; import java.io.IOException; @@ -767,34 +766,34 @@ protected void handleRoleCommand() { switch (command) { case CommandActions.DETAILS: if (line.hasOption(GravitinoOptions.AUDIT)) { - newRoleAudit(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle(); + newRoleAudit(url, ignore, metalake, getOneRole(roles)).validate().handle(); } else { - newRoleDetails(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle(); + newRoleDetails(url, ignore, metalake, getOneRole(roles)).validate().handle(); } break; case CommandActions.LIST: - newListRoles(url, ignore, metalake).handle(); + newListRoles(url, ignore, metalake).validate().handle(); break; case CommandActions.CREATE: - newCreateRole(url, ignore, metalake, roles).handle(); + newCreateRole(url, ignore, metalake, roles).validate().handle(); break; case CommandActions.DELETE: boolean forceDelete = line.hasOption(GravitinoOptions.FORCE); - newDeleteRole(url, ignore, forceDelete, metalake, roles).handle(); + newDeleteRole(url, ignore, forceDelete, metalake, roles).validate().handle(); break; case CommandActions.GRANT: - newGrantPrivilegesToRole( - url, ignore, metalake, getOneRole(roles, CommandActions.GRANT), name, privileges) + newGrantPrivilegesToRole(url, ignore, metalake, getOneRole(roles), name, privileges) + .validate() .handle(); break; case CommandActions.REVOKE: - newRevokePrivilegesFromRole( - url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privileges) + newRevokePrivilegesFromRole(url, ignore, metalake, getOneRole(roles), name, privileges) + .validate() .handle(); break; @@ -805,9 +804,12 @@ url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privilege } } - private String getOneRole(String[] roles, String command) { - Preconditions.checkArgument( - roles.length == 1, command + " requires only one role, but multiple are currently passed."); + private String getOneRole(String[] roles) { + if (roles == null || roles.length != 1) { + System.err.println(ErrorMessages.MULTIPLE_ROLE_COMMAND_ERROR); + Main.exit(-1); + } + return roles[0]; } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java index 584e073beac..8630282ea60 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java @@ -103,4 +103,10 @@ public void handle() { String all = String.join(",", privileges); System.out.println(role + " granted " + all + " on " + entity.getName()); } + + @Override + public Command validate() { + if (privileges == null) exitWithError(ErrorMessages.MISSING_PRIVILEGES); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java index a62e977a2fb..3bfa7cd4526 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java @@ -103,4 +103,10 @@ public void handle() { String all = String.join(",", privileges); System.out.println(role + " revoked " + all + " on " + entity.getName()); } + + @Override + public Command validate() { + if (privileges == null) exitWithError(ErrorMessages.MISSING_PRIVILEGES); + return super.validate(); + } } diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java index 0e671067e3f..529979582ff 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java @@ -80,6 +80,7 @@ void testListRolesCommand() { doReturn(mockList) .when(commandLine) .newListRoles(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo"); + doReturn(mockList).when(mockList).validate(); commandLine.handleCommandLine(); verify(mockList).handle(); } @@ -98,12 +99,14 @@ void testRoleDetailsCommand() { doReturn(mockDetails) .when(commandLine) .newRoleDetails(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin"); + doReturn(mockDetails).when(mockDetails).validate(); commandLine.handleCommandLine(); verify(mockDetails).handle(); } @Test void testRoleDetailsCommandWithMultipleRoles() { + Main.useExit = false; when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); when(mockCommandLine.hasOption(GravitinoOptions.ROLE)).thenReturn(true); @@ -114,7 +117,7 @@ void testRoleDetailsCommandWithMultipleRoles() { new GravitinoCommandLine( mockCommandLine, mockOptions, CommandEntities.ROLE, CommandActions.DETAILS)); - assertThrows(IllegalArgumentException.class, commandLine::handleCommandLine); + assertThrows(RuntimeException.class, commandLine::handleCommandLine); verify(commandLine, never()) .newRoleDetails( eq(GravitinoCommandLine.DEFAULT_URL), eq(false), eq("metalake_demo"), any()); @@ -135,6 +138,7 @@ void testRoleAuditCommand() { doReturn(mockAudit) .when(commandLine) .newRoleAudit(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "group"); + doReturn(mockAudit).when(mockAudit).validate(); commandLine.handleCommandLine(); verify(mockAudit).handle(); } @@ -154,6 +158,7 @@ void testCreateRoleCommand() { .when(commandLine) .newCreateRole( GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", new String[] {"admin"}); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -178,6 +183,7 @@ void testCreateRolesCommand() { eq(false), eq("metalake_demo"), eq(new String[] {"admin", "engineer", "scientist"})); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -201,6 +207,7 @@ void testDeleteRoleCommand() { false, "metalake_demo", new String[] {"admin"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -227,6 +234,7 @@ void testDeleteRolesCommand() { eq(false), eq("metalake_demo"), eq(new String[] {"admin", "engineer", "scientist"})); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -247,6 +255,7 @@ void testDeleteRoleForceCommand() { .when(commandLine) .newDeleteRole( GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo", new String[] {"admin"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -276,10 +285,24 @@ void testGrantPrivilegesToRole() { eq("admin"), any(), eq(privileges)); + doReturn(mockGrant).when(mockGrant).validate(); commandLine.handleCommandLine(); verify(mockGrant).handle(); } + @Test + void testGrantPrivilegesToRoleWithoutPrivileges() { + Main.useExit = false; + GrantPrivilegesToRole spyGrantRole = + spy( + new GrantPrivilegesToRole( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin", null, null)); + assertThrows(RuntimeException.class, spyGrantRole::validate); + verify(spyGrantRole, never()).handle(); + String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + assertEquals(ErrorMessages.MISSING_PRIVILEGES, errOutput); + } + @Test void testRevokePrivilegesFromRole() { RevokePrivilegesFromRole mockRevoke = mock(RevokePrivilegesFromRole.class); @@ -305,10 +328,24 @@ void testRevokePrivilegesFromRole() { eq("admin"), any(), eq(privileges)); + doReturn(mockRevoke).when(mockRevoke).validate(); commandLine.handleCommandLine(); verify(mockRevoke).handle(); } + @Test + void testRevokePrivilegesFromRoleWithoutPrivileges() { + Main.useExit = false; + RevokePrivilegesFromRole spyGrantRole = + spy( + new RevokePrivilegesFromRole( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin", null, null)); + assertThrows(RuntimeException.class, spyGrantRole::validate); + verify(spyGrantRole, never()).handle(); + String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + assertEquals(ErrorMessages.MISSING_PRIVILEGES, errOutput); + } + @Test void testDeleteRoleCommandWithoutRole() { Main.useExit = false;