Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#6116] fix(authz): Fix the check of duplicated privileges #6117

Merged
merged 6 commits into from
Jan 7, 2025
Merged

[#6116] fix(authz): Fix the check of duplicated privileges #6117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bff7f8a
[#6116] fix(authz): Fix the check of duplicated privileges
jerqi Jan 6, 2025
d72174a
address comment
jerqi Jan 6, 2025
e958b10
refactor
jerqi Jan 6, 2025
ba3c686
format
jerqi Jan 6, 2025
f61efec
fix style
jerqi Jan 6, 2025
7b1bc96
remove deprecated methods
jerqi Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.apache.gravitino.authorization.ranger.integration.test.RangerITEnv.currentFunName;

import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -905,7 +906,7 @@ void testAllowUseSchemaPrivilege() throws InterruptedException {
MetadataObject catalogObject =
MetadataObjects.of(null, catalogName, MetadataObject.Type.CATALOG);
metalake.revokePrivilegesFromRole(
roleName, catalogObject, Lists.newArrayList(Privileges.CreateSchema.allow()));
roleName, catalogObject, Sets.newHashSet(Privileges.CreateSchema.allow()));
waitForUpdatingPolicies();

// Use Spark to show this databases(schema)
Expand All @@ -920,7 +921,7 @@ void testAllowUseSchemaPrivilege() throws InterruptedException {
MetadataObject schemaObject =
MetadataObjects.of(catalogName, schemaName, MetadataObject.Type.SCHEMA);
metalake.grantPrivilegesToRole(
roleName, schemaObject, Lists.newArrayList(Privileges.UseSchema.allow()));
roleName, schemaObject, Sets.newHashSet(Privileges.UseSchema.allow()));
waitForUpdatingPolicies();

// Use Spark to show this databases(schema) again
Expand Down Expand Up @@ -1020,7 +1021,7 @@ void testGrantPrivilegesForMetalake() throws InterruptedException {
metalake.grantPrivilegesToRole(
roleName,
MetadataObjects.of(null, metalakeName, MetadataObject.Type.METALAKE),
Lists.newArrayList(Privileges.CreateSchema.allow()));
Sets.newHashSet(Privileges.CreateSchema.allow()));

// Fail to create a schema
Assertions.assertThrows(
Expand Down
9 changes: 5 additions & 4 deletions ...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerFilesetIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import java.io.IOException;
import java.security.PrivilegedExceptionAction;
import java.util.Arrays;
Expand Down Expand Up @@ -262,7 +263,7 @@ void testReadWritePath() throws IOException, RangerServiceException {
String.format("%s.%s", catalogName, schemaName),
fileset.name(),
MetadataObject.Type.FILESET),
Lists.newArrayList(Privileges.WriteFileset.allow()));
Sets.newHashSet(Privileges.WriteFileset.allow()));

policies = rangerClient.getPoliciesInService(RangerITEnv.RANGER_HDFS_REPO_NAME);
Assertions.assertEquals(1, policies.size());
Expand Down Expand Up @@ -320,7 +321,7 @@ void testReadWritePath() throws IOException, RangerServiceException {
String.format("%s.%s", catalogName, schemaName),
fileset.name(),
MetadataObject.Type.FILESET),
Lists.newArrayList(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow()));
Sets.newHashSet(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow()));
policies = rangerClient.getPoliciesInService(RangerITEnv.RANGER_HDFS_REPO_NAME);
Assertions.assertEquals(1, policies.size());
Assertions.assertEquals(3, policies.get(0).getPolicyItems().size());
Expand Down Expand Up @@ -460,7 +461,7 @@ void testReadWritePathE2E() throws IOException, RangerServiceException, Interrup
fileset.name(),
MetadataObject.Type.FILESET);
metalake.grantPrivilegesToRole(
filesetRole, filesetObject, Lists.newArrayList(Privileges.WriteFileset.allow()));
filesetRole, filesetObject, Sets.newHashSet(Privileges.WriteFileset.allow()));
RangerBaseE2EIT.waitForUpdatingPolicies();
UserGroupInformation.createProxyUser(userName, UserGroupInformation.getCurrentUser())
.doAs(
Expand Down Expand Up @@ -488,7 +489,7 @@ void testReadWritePathE2E() throws IOException, RangerServiceException, Interrup
metalake.revokePrivilegesFromRole(
filesetRole,
filesetObject,
Lists.newArrayList(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow()));
Sets.newHashSet(Privileges.ReadFileset.allow(), Privileges.WriteFileset.allow()));
RangerBaseE2EIT.waitForUpdatingPolicies();
UserGroupInformation.createProxyUser(userName, UserGroupInformation.getCurrentUser())
.doAs(
Expand Down
10 changes: 5 additions & 5 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

package org.apache.gravitino.cli.commands;

import java.util.ArrayList;
import java.util.List;
import java.util.HashSet;
import java.util.Set;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.cli.ErrorMessages;
Expand Down Expand Up @@ -69,7 +69,7 @@ public GrantPrivilegesToRole(
public void handle() {
try {
GravitinoClient client = buildClient(metalake);
List<Privilege> privilegesList = new ArrayList<>();
Set<Privilege> privilegesSet = new HashSet<>();

for (String privilege : privileges) {
if (!Privileges.isValid(privilege)) {
Expand All @@ -81,11 +81,11 @@ public void handle() {
.withName(Privileges.toName(privilege))
.withCondition(Privilege.Condition.ALLOW)
.build();
privilegesList.add(privilegeDTO);
privilegesSet.add(privilegeDTO);
}

MetadataObject metadataObject = constructMetadataObject(entity, client);
client.grantPrivilegesToRole(role, metadataObject, privilegesList);
client.grantPrivilegesToRole(role, metadataObject, privilegesSet);
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
return;
Expand Down
10 changes: 5 additions & 5 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

package org.apache.gravitino.cli.commands;

import java.util.ArrayList;
import java.util.List;
import java.util.HashSet;
import java.util.Set;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.authorization.Privilege;
import org.apache.gravitino.cli.ErrorMessages;
Expand Down Expand Up @@ -69,7 +69,7 @@ public RevokePrivilegesFromRole(
public void handle() {
try {
GravitinoClient client = buildClient(metalake);
List<Privilege> privilegesList = new ArrayList<>();
Set<Privilege> privilegesSet = new HashSet<>();

for (String privilege : privileges) {
if (!Privileges.isValid(privilege)) {
Expand All @@ -81,11 +81,11 @@ public void handle() {
.withName(Privileges.toName(privilege))
.withCondition(Privilege.Condition.DENY)
.build();
privilegesList.add(privilegeDTO);
privilegesSet.add(privilegeDTO);
}

MetadataObject metadataObject = constructMetadataObject(entity, client);
client.revokePrivilegesFromRole(role, metadataObject, privilegesList);
client.revokePrivilegesFromRole(role, metadataObject, privilegesSet);
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
return;
Expand Down
3 changes: 2 additions & 1 deletion clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.apache.gravitino.dto.util.DTOConverters.toFunctionArg;

import java.util.Collection;
import java.util.List;
import java.util.stream.Collectors;
import org.apache.gravitino.Catalog;
Expand Down Expand Up @@ -321,7 +322,7 @@ static SecurableObjectDTO toSecurableObject(SecurableObject securableObject) {
.build();
}

static List<PrivilegeDTO> toPrivileges(List<Privilege> privileges) {
static List<PrivilegeDTO> toPrivileges(Collection<Privilege> privileges) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collection<Privilege> or Set<Privilege>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collection. Other places I used some collections which are not sets.

return privileges.stream()
.map(
privilege ->
Expand Down
45 changes: 45 additions & 0 deletions clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
package org.apache.gravitino.client;

import com.google.common.base.Preconditions;
import com.google.common.collect.Sets;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import org.apache.gravitino.Catalog;
import org.apache.gravitino.CatalogChange;
import org.apache.gravitino.MetadataObject;
Expand Down Expand Up @@ -420,12 +422,33 @@ public String[] listRoleNames() throws NoSuchMetalakeException {
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If granting roles to a role encounters storage issues.
*/
@Deprecated
public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
return getMetalake().grantPrivilegesToRole(role, object, privileges);
}

/**
* Grant privileges to a role.
*
* @param role The name of the role.
* @param privileges The privileges to grant.
* @param object The object is associated with privileges to grant.
* @return The role after granted.
* @throws NoSuchRoleException If the role with the given name does not exist.
* @throws NoSuchMetadataObjectException If the metadata object with the given name does not
* exist.
* @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If granting roles to a role encounters storage issues.
*/
public Role grantPrivilegesToRole(String role, MetadataObject object, Set<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
return getMetalake().grantPrivilegesToRole(role, object, privileges);
}

/**
* Revoke privileges from a role.
*
Expand All @@ -440,10 +463,32 @@ public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privi
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If revoking privileges from a role encounters storage issues.
*/
@Deprecated
public Role revokePrivilegesFromRole(
String role, MetadataObject object, List<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
return getMetalake().revokePrivilegesFromRole(role, object, Sets.newHashSet(privileges));
}

/**
* Revoke privileges from a role.
*
* @param role The name of the role.
* @param privileges The privileges to revoke.
* @param object The object is associated with privileges to revoke.
* @return The role after revoked.
* @throws NoSuchRoleException If the role with the given name does not exist.
* @throws NoSuchMetadataObjectException If the metadata object with the given name does not
* exist.
* @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If revoking privileges from a role encounters storage issues.
*/
public Role revokePrivilegesFromRole(
String role, MetadataObject object, Set<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
return getMetalake().revokePrivilegesFromRole(role, object, privileges);
}

Expand Down
46 changes: 46 additions & 0 deletions clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Catalog;
Expand Down Expand Up @@ -1018,6 +1020,27 @@ public Group revokeRolesFromGroup(List<String> roles, String group)
public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
Set<Privilege> privilegeSet = Sets.newHashSet(privileges);
return grantPrivilegesToRole(role, object, privilegeSet);
}

/**
* Grant privileges to a role.
*
* @param role The name of the role.
* @param privileges The privileges to grant.
* @param object The object is associated with privileges to grant.
* @return The role after granted.
* @throws NoSuchRoleException If the role with the given name does not exist.
* @throws NoSuchMetadataObjectException If the metadata object with the given name does not
* exist.
* @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If granting privileges to a role encounters storage issues.
*/
public Role grantPrivilegesToRole(String role, MetadataObject object, Set<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
PrivilegeGrantRequest request =
new PrivilegeGrantRequest(DTOConverters.toPrivileges(privileges));
request.validate();
Expand Down Expand Up @@ -1056,10 +1079,33 @@ public Role grantPrivilegesToRole(String role, MetadataObject object, List<Privi
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If revoking privileges from a role encounters storage issues.
*/
@Deprecated
public Role revokePrivilegesFromRole(
String role, MetadataObject object, List<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
Set<Privilege> privilegeSet = Sets.newHashSet(privileges);
return revokePrivilegesFromRole(role, object, privilegeSet);
}

/**
* Revoke privileges from a role.
*
* @param role The name of the role.
* @param privileges The privileges to revoke.
* @param object The object is associated with privileges to revoke.
* @return The role after revoked.
* @throws NoSuchRoleException If the role with the given name does not exist.
* @throws NoSuchMetadataObjectException If the metadata object with the given name does not
* exist.
* @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
* @throws IllegalPrivilegeException If any privilege can't be bind to the metadata object.
* @throws RuntimeException If revoking privileges from a role encounters storage issues.
*/
public Role revokePrivilegesFromRole(
String role, MetadataObject object, Set<Privilege> privileges)
throws NoSuchRoleException, NoSuchMetadataObjectException, NoSuchMetalakeException,
IllegalPrivilegeException {
PrivilegeRevokeRequest request =
new PrivilegeRevokeRequest(DTOConverters.toPrivileges(privileges));
request.validate();
Expand Down
9 changes: 5 additions & 4 deletions clients/client-java/src/test/java/org/apache/gravitino/client/TestPermission.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.apache.hc.core5.http.HttpStatus.SC_SERVER_ERROR;

import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import java.time.Instant;
import java.util.List;
import org.apache.gravitino.MetadataObject;
Expand Down Expand Up @@ -231,7 +232,7 @@ public void testGrantPrivilegeToRole() throws Exception {
MetadataObject object = MetadataObjects.of(null, metalakeName, MetadataObject.Type.METALAKE);
Role grantedRole =
gravitinoClient.grantPrivilegesToRole(
role, object, Lists.newArrayList(Privileges.CreateTable.allow()));
role, object, Sets.newHashSet(Privileges.CreateTable.allow()));
Assertions.assertEquals(grantedRole.name(), role);
Assertions.assertEquals(1, grantedRole.securableObjects().size());
SecurableObject securableObject = grantedRole.securableObjects().get(0);
Expand All @@ -249,7 +250,7 @@ public void testGrantPrivilegeToRole() throws Exception {
RuntimeException.class,
() ->
gravitinoClient.grantPrivilegesToRole(
role, object, Lists.newArrayList(Privileges.CreateTable.allow())));
role, object, Sets.newHashSet(Privileges.CreateTable.allow())));
}

@Test
Expand Down Expand Up @@ -280,7 +281,7 @@ public void testRevokePrivilegeFromRole() throws Exception {
MetadataObject object = MetadataObjects.of(null, metalakeName, MetadataObject.Type.METALAKE);
Role revokedRole =
gravitinoClient.revokePrivilegesFromRole(
role, object, Lists.newArrayList(Privileges.CreateTable.allow()));
role, object, Sets.newHashSet(Privileges.CreateTable.allow()));
Assertions.assertEquals(revokedRole.name(), role);
Assertions.assertTrue(revokedRole.securableObjects().isEmpty());

Expand All @@ -291,6 +292,6 @@ public void testRevokePrivilegeFromRole() throws Exception {
RuntimeException.class,
() ->
gravitinoClient.revokePrivilegesFromRole(
role, object, Lists.newArrayList(Privileges.CreateTable.allow())));
role, object, Sets.newHashSet(Privileges.CreateTable.allow())));
}
}
Loading
Loading