You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by be...@apache.org on 2022/02/28 06:19:14 UTC
[cassandra] branch trunk updated: Error out on noop GRANT/REVOKE
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 <be...@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-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org