This is an automated email from the ASF dual-hosted git repository. bereng pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/cassandra.git
The following commit(s) were added to refs/heads/trunk by this push: new e780b5a Error out on noop GRANT/REVOKE e780b5a is described below commit e780b5a45b829f89049ad358a36be3055dbcd344 Author: Bereng <berenguerbl...@gmail.com> AuthorDate: Wed Feb 2 09:52:49 2022 +0100 Error out on noop GRANT/REVOKE patch by Berenguer Blasi; reviewed by Andres de la Peña for CASSANDRA-17333 --- .../apache/cassandra/auth/AllowAllAuthorizer.java | 4 +- .../apache/cassandra/auth/CassandraAuthorizer.java | 68 ++++++++++++++++++++-- .../org/apache/cassandra/auth/IAuthorizer.java | 8 ++- .../cql3/statements/GrantPermissionsStatement.java | 23 +++++++- .../statements/RevokePermissionsStatement.java | 23 +++++++- .../apache/cassandra/auth/GrantAndRevokeTest.java | 28 +++++++++ .../org/apache/cassandra/auth/StubAuthorizer.java | 48 ++++++++------- test/unit/org/apache/cassandra/cql3/CQLTester.java | 24 +++++++- 8 files changed, 189 insertions(+), 37 deletions(-) diff --git a/src/java/org/apache/cassandra/auth/AllowAllAuthorizer.java b/src/java/org/apache/cassandra/auth/AllowAllAuthorizer.java index 3b40979..a943db0 100644 --- a/src/java/org/apache/cassandra/auth/AllowAllAuthorizer.java +++ b/src/java/org/apache/cassandra/auth/AllowAllAuthorizer.java @@ -33,12 +33,12 @@ public class AllowAllAuthorizer implements IAuthorizer return resource.applicablePermissions(); } - public void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource to) + public Set<Permission> grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource to) { throw new UnsupportedOperationException("GRANT operation is not supported by AllowAllAuthorizer"); } - public void revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource from) + public Set<Permission> revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource from) { throw new UnsupportedOperationException("REVOKE operation is not supported by AllowAllAuthorizer"); } diff --git a/src/java/org/apache/cassandra/auth/CassandraAuthorizer.java b/src/java/org/apache/cassandra/auth/CassandraAuthorizer.java index c5e1cca..b3f85e8 100644 --- a/src/java/org/apache/cassandra/auth/CassandraAuthorizer.java +++ b/src/java/org/apache/cassandra/auth/CassandraAuthorizer.java @@ -28,12 +28,15 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Table; +import com.google.common.collect.Sets; + import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.*; +import org.apache.cassandra.cql3.UntypedResultSet.Row; import org.apache.cassandra.cql3.statements.BatchStatement; import org.apache.cassandra.cql3.statements.ModificationStatement; import org.apache.cassandra.db.ConsistencyLevel; @@ -92,18 +95,37 @@ public class CassandraAuthorizer implements IAuthorizer } } - public void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee) + public Set<Permission> grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee) throws RequestValidationException, RequestExecutionException { - modifyRolePermissions(permissions, resource, grantee, "+"); - addLookupEntry(resource, grantee); + String roleName = escape(grantee.getRoleName()); + String resourceName = escape(resource.getName()); + Set<Permission> existingPermissions = getExistingPermissions(roleName, resourceName, permissions); + Set<Permission> nonExistingPermissions = Sets.difference(permissions, existingPermissions); + + if (!nonExistingPermissions.isEmpty()) + { + modifyRolePermissions(nonExistingPermissions, resource, grantee, "+"); + addLookupEntry(resource, grantee); + } + + return nonExistingPermissions; } - public void revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee) + public Set<Permission> revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee) throws RequestValidationException, RequestExecutionException { - modifyRolePermissions(permissions, resource, revokee, "-"); - removeLookupEntry(resource, revokee); + String roleName = escape(revokee.getRoleName()); + String resourceName = escape(resource.getName()); + Set<Permission> existingPermissions = getExistingPermissions(roleName, resourceName, permissions); + + if (!existingPermissions.isEmpty()) + { + modifyRolePermissions(existingPermissions, resource, revokee, "-"); + removeLookupEntry(resource, revokee); + } + + return existingPermissions; } // Called when deleting a role with DROP ROLE query. @@ -186,6 +208,40 @@ public class CassandraAuthorizer implements IAuthorizer } } + /** + * Checks that the specified role has at least one of the expected permissions on the resource. + * + * @param roleName the role name + * @param resourceName the resource name + * @param expectedPermissions the permissions to check for + * @return The existing permissions + */ + private Set<Permission> getExistingPermissions(String roleName, + String resourceName, + Set<Permission> expectedPermissions) + { + UntypedResultSet rs = process(String.format("SELECT permissions FROM %s.%s WHERE role = '%s' AND resource = '%s'", + SchemaConstants.AUTH_KEYSPACE_NAME, + AuthKeyspace.ROLE_PERMISSIONS, + roleName, + resourceName), + ConsistencyLevel.LOCAL_ONE); + + if (rs.isEmpty()) + return Collections.emptySet(); + + Row one = rs.one(); + + Set<Permission> existingPermissions = Sets.newHashSetWithExpectedSize(expectedPermissions.size()); + for (String permissionName : one.getSet("permissions", UTF8Type.instance)) + { + Permission permission = Permission.valueOf(permissionName); + if (expectedPermissions.contains(permission)) + existingPermissions.add(permission); + } + return existingPermissions; + } + private void executeLoggedBatch(List<CQLStatement> statements) throws RequestExecutionException, RequestValidationException { diff --git a/src/java/org/apache/cassandra/auth/IAuthorizer.java b/src/java/org/apache/cassandra/auth/IAuthorizer.java index a6c5eff..b39e315 100644 --- a/src/java/org/apache/cassandra/auth/IAuthorizer.java +++ b/src/java/org/apache/cassandra/auth/IAuthorizer.java @@ -62,12 +62,14 @@ public interface IAuthorizer extends AuthCache.BulkLoader<Pair<AuthenticatedUser * @param permissions Set of permissions to grant. * @param resource Resource on which to grant the permissions. * @param grantee Role to which the permissions are to be granted. + * @return the permissions that have been successfully granted, comprised by the requested permissions excluding + * those permissions that were already granted. * * @throws RequestValidationException * @throws RequestExecutionException * @throws java.lang.UnsupportedOperationException */ - void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee) + Set<Permission> grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee) throws RequestValidationException, RequestExecutionException; /** @@ -80,12 +82,14 @@ public interface IAuthorizer extends AuthCache.BulkLoader<Pair<AuthenticatedUser * @param permissions Set of permissions to revoke. * @param revokee Role from which to the permissions are to be revoked. * @param resource Resource on which to revoke the permissions. + * @return the permissions that have been successfully revoked, comprised by the requested permissions excluding + * those permissions that were already not granted. * * @throws RequestValidationException * @throws RequestExecutionException * @throws java.lang.UnsupportedOperationException */ - void revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee) + Set<Permission> revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee) throws RequestValidationException, RequestExecutionException; /** diff --git a/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java b/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java index 3db20e3..824c485 100644 --- a/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java @@ -18,9 +18,11 @@ package org.apache.cassandra.cql3.statements; import java.util.Set; +import java.util.stream.Collectors; import org.apache.cassandra.audit.AuditLogContext; import org.apache.cassandra.audit.AuditLogEntryType; +import org.apache.cassandra.auth.IAuthorizer; import org.apache.cassandra.auth.IResource; import org.apache.cassandra.auth.Permission; import org.apache.cassandra.config.DatabaseDescriptor; @@ -28,6 +30,7 @@ import org.apache.cassandra.cql3.RoleName; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.exceptions.RequestValidationException; import org.apache.cassandra.service.ClientState; +import org.apache.cassandra.service.ClientWarn; import org.apache.cassandra.transport.messages.ResultMessage; public class GrantPermissionsStatement extends PermissionsManagementStatement @@ -39,7 +42,25 @@ public class GrantPermissionsStatement extends PermissionsManagementStatement public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException { - DatabaseDescriptor.getAuthorizer().grant(state.getUser(), permissions, resource, grantee); + IAuthorizer authorizer = DatabaseDescriptor.getAuthorizer(); + Set<Permission> granted = authorizer.grant(state.getUser(), permissions, resource, grantee); + + // We want to warn the client if all the specified permissions have not been granted and the client did + // not specify ALL in the query. + if (!granted.equals(permissions) && !permissions.equals(Permission.ALL)) + { + String permissionsStr = permissions.stream() + .filter(permission -> !granted.contains(permission)) + .sorted(Permission::compareTo) // guarantee the order for testing + .map(Permission::name) + .collect(Collectors.joining(", ")); + + ClientWarn.instance.warn(String.format("Role '%s' was already granted %s on %s", + grantee.getRoleName(), + permissionsStr, + resource)); + } + return null; } diff --git a/src/java/org/apache/cassandra/cql3/statements/RevokePermissionsStatement.java b/src/java/org/apache/cassandra/cql3/statements/RevokePermissionsStatement.java index 57d0631..4262285 100644 --- a/src/java/org/apache/cassandra/cql3/statements/RevokePermissionsStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/RevokePermissionsStatement.java @@ -18,9 +18,11 @@ package org.apache.cassandra.cql3.statements; import java.util.Set; +import java.util.stream.Collectors; import org.apache.cassandra.audit.AuditLogContext; import org.apache.cassandra.audit.AuditLogEntryType; +import org.apache.cassandra.auth.IAuthorizer; import org.apache.cassandra.auth.IResource; import org.apache.cassandra.auth.Permission; import org.apache.cassandra.config.DatabaseDescriptor; @@ -28,6 +30,7 @@ import org.apache.cassandra.cql3.RoleName; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.exceptions.RequestValidationException; import org.apache.cassandra.service.ClientState; +import org.apache.cassandra.service.ClientWarn; import org.apache.cassandra.transport.messages.ResultMessage; import org.apache.commons.lang3.builder.ToStringBuilder; import org.apache.commons.lang3.builder.ToStringStyle; @@ -41,7 +44,25 @@ public class RevokePermissionsStatement extends PermissionsManagementStatement public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException { - DatabaseDescriptor.getAuthorizer().revoke(state.getUser(), permissions, resource, grantee); + IAuthorizer authorizer = DatabaseDescriptor.getAuthorizer(); + Set<Permission> revoked = authorizer.revoke(state.getUser(), permissions, resource, grantee); + + // We want to warn the client if all the specified permissions have not been revoked and the client did + // not specify ALL in the query. + if (!revoked.equals(permissions) && !permissions.equals(Permission.ALL)) + { + String permissionsStr = permissions.stream() + .filter(permission -> !revoked.contains(permission)) + .sorted(Permission::compareTo) // guarantee the order for testing + .map(Permission::name) + .collect(Collectors.joining(", ")); + + ClientWarn.instance.warn(String.format("Role '%s' was not granted %s on %s", + grantee.getRoleName(), + permissionsStr, + resource)); + } + return null; } diff --git a/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java b/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java index 41ae8d2..5c1c2a0 100644 --- a/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java +++ b/test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java @@ -21,10 +21,13 @@ import org.junit.After; import org.junit.BeforeClass; import org.junit.Test; +import com.datastax.driver.core.ResultSet; import org.apache.cassandra.Util; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.CQLTester; +import static org.junit.Assert.assertTrue; + public class GrantAndRevokeTest extends CQLTester { private static final String user = "user"; @@ -357,4 +360,29 @@ public class GrantAndRevokeTest extends CQLTester assertUnauthorizedQuery("User user has no ALTER permission on <table " + table + "> or any of its parents", "DROP INDEX " + index); } + + @Test + public void testWarnings() throws Throwable + { + useSuperUser(); + + executeNet("CREATE KEYSPACE revoke_yeah WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}"); + executeNet("CREATE TABLE revoke_yeah.t1 (id int PRIMARY KEY, val text)"); + executeNet("CREATE USER '" + user + "' WITH PASSWORD '" + pass + "'"); + + ResultSet res = executeNet("REVOKE CREATE ON KEYSPACE revoke_yeah FROM " + user); + assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted CREATE on <keyspace revoke_yeah>"); + + res = executeNet("GRANT SELECT ON KEYSPACE revoke_yeah TO " + user); + assertTrue(res.getExecutionInfo().getWarnings().isEmpty()); + + res = executeNet("GRANT SELECT ON KEYSPACE revoke_yeah TO " + user); + assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was already granted SELECT on <keyspace revoke_yeah>"); + + res = executeNet("REVOKE SELECT ON TABLE revoke_yeah.t1 FROM " + user); + assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted SELECT on <table revoke_yeah.t1>"); + + res = executeNet("REVOKE SELECT, MODIFY ON KEYSPACE revoke_yeah FROM " + user); + assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted MODIFY on <keyspace revoke_yeah>"); + } } diff --git a/test/unit/org/apache/cassandra/auth/StubAuthorizer.java b/test/unit/org/apache/cassandra/auth/StubAuthorizer.java index 8e0d141..e9f7d22 100644 --- a/test/unit/org/apache/cassandra/auth/StubAuthorizer.java +++ b/test/unit/org/apache/cassandra/auth/StubAuthorizer.java @@ -21,6 +21,8 @@ package org.apache.cassandra.auth; import java.util.*; import java.util.stream.Collectors; +import com.google.common.collect.Sets; + import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.exceptions.RequestValidationException; @@ -42,34 +44,36 @@ public class StubAuthorizer implements IAuthorizer return perms != null ? perms : Collections.emptySet(); } - public void grant(AuthenticatedUser performer, - Set<Permission> permissions, - IResource resource, - RoleResource grantee) throws RequestValidationException, RequestExecutionException + public Set<Permission> grant(AuthenticatedUser performer, + Set<Permission> permissions, + IResource resource, + RoleResource grantee) throws RequestValidationException, RequestExecutionException { Pair<String, IResource> key = Pair.create(grantee.getRoleName(), resource); - Set<Permission> perms = userPermissions.get(key); - if (null == perms) - { - perms = new HashSet<>(); - userPermissions.put(key, perms); - } - perms.addAll(permissions); + Set<Permission> oldPermissions = userPermissions.computeIfAbsent(key, k -> Collections.emptySet()); + Set<Permission> nonExisting = Sets.difference(permissions, oldPermissions); + + if (!nonExisting.isEmpty()) + userPermissions.put(key, Sets.union(oldPermissions, nonExisting)); + + return nonExisting; } - public void revoke(AuthenticatedUser performer, - Set<Permission> permissions, - IResource resource, - RoleResource revokee) throws RequestValidationException, RequestExecutionException + public Set<Permission> revoke(AuthenticatedUser performer, + Set<Permission> permissions, + IResource resource, + RoleResource revokee) throws RequestValidationException, RequestExecutionException { Pair<String, IResource> key = Pair.create(revokee.getRoleName(), resource); - Set<Permission> perms = userPermissions.get(key); - if (null != perms) - { - perms.removeAll(permissions); - if (perms.isEmpty()) - userPermissions.remove(key); - } + Set<Permission> oldPermissions = userPermissions.computeIfAbsent(key, k -> Collections.emptySet()); + Set<Permission> existing = Sets.intersection(permissions, oldPermissions); + + if (existing.isEmpty()) + userPermissions.remove(key); + else + userPermissions.put(key, Sets.difference(oldPermissions, existing)); + + return existing; } public Set<PermissionDetails> list(AuthenticatedUser performer, diff --git a/test/unit/org/apache/cassandra/cql3/CQLTester.java b/test/unit/org/apache/cassandra/cql3/CQLTester.java index da35189..6844cb8 100644 --- a/test/unit/org/apache/cassandra/cql3/CQLTester.java +++ b/test/unit/org/apache/cassandra/cql3/CQLTester.java @@ -1160,15 +1160,33 @@ public abstract class CQLTester protected static void assertWarningsContain(Message.Response response, String message) { - List<String> warnings = response.getWarnings(); + assertWarningsContain(response.getWarnings(), message); + } + + protected static void assertWarningsContain(List<String> warnings, String message) + { Assert.assertNotNull(warnings); assertTrue(warnings.stream().anyMatch(s -> s.contains(message))); } + + protected static void assertWarningsEquals(ResultSet rs, String... messages) + { + assertWarningsEquals(rs.getExecutionInfo().getWarnings(), messages); + } + + protected static void assertWarningsEquals(List<String> warnings, String... messages) + { + Assert.assertNotNull(warnings); + Assertions.assertThat(messages).hasSameElementsAs(warnings); + } protected static void assertNoWarningContains(Message.Response response, String message) { - List<String> warnings = response.getWarnings(); - + assertNoWarningContains(response.getWarnings(), message); + } + + protected static void assertNoWarningContains(List<String> warnings, String message) + { if (warnings != null) { assertFalse(warnings.stream().anyMatch(s -> s.contains(message))); --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org