Skip to content

Commit 34d68bb

Browse files
berengtiagomlalves
bereng
authored and
tiagomlalves
committed
HCD-93 Back-ports Error out on noop GRANT/REVOKE
patch by Berenguer Blasi; reviewed by Andres de la Peña for CASSANDRA-17333
1 parent 384ba23 commit 34d68bb

File tree

8 files changed

+237
-36
lines changed

8 files changed

+237
-36
lines changed

‎src/java/org/apache/cassandra/auth/AllowAllAuthorizer.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ public Set<Permission> authorize(AuthenticatedUser user, IResource resource)
3333
return resource.applicablePermissions();
3434
}
3535

36-
public void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource to)
36+
public Set<Permission> grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource to)
3737
{
3838
throw new UnsupportedOperationException("GRANT operation is not supported by AllowAllAuthorizer");
3939
}
4040

41-
public void revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource from)
41+
public Set<Permission> revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource from)
4242
{
4343
throw new UnsupportedOperationException("REVOKE operation is not supported by AllowAllAuthorizer");
4444
}

‎src/java/org/apache/cassandra/auth/CassandraAuthorizer.java

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,15 @@
2222
import com.google.common.collect.ImmutableSet;
2323
import com.google.common.collect.Iterables;
2424
import com.google.common.collect.Lists;
25+
import com.google.common.collect.Sets;
26+
2527
import org.apache.commons.lang3.StringUtils;
2628
import org.slf4j.Logger;
2729
import org.slf4j.LoggerFactory;
2830

2931
import org.apache.cassandra.config.DatabaseDescriptor;
3032
import org.apache.cassandra.cql3.*;
33+
import org.apache.cassandra.cql3.UntypedResultSet.Row;
3134
import org.apache.cassandra.cql3.statements.BatchStatement;
3235
import org.apache.cassandra.cql3.statements.ModificationStatement;
3336
import org.apache.cassandra.cql3.statements.SelectStatement;
@@ -83,18 +86,37 @@ public Set<Permission> authorize(AuthenticatedUser user, IResource resource)
8386
}
8487
}
8588

86-
public void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee)
89+
public Set<Permission> grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee)
8790
throws RequestValidationException, RequestExecutionException
8891
{
89-
modifyRolePermissions(permissions, resource, grantee, "+");
90-
addLookupEntry(resource, grantee);
92+
String roleName = escape(grantee.getRoleName());
93+
String resourceName = escape(resource.getName());
94+
Set<Permission> existingPermissions = getExistingPermissions(roleName, resourceName, permissions);
95+
Set<Permission> nonExistingPermissions = Sets.difference(permissions, existingPermissions);
96+
97+
if (!nonExistingPermissions.isEmpty())
98+
{
99+
modifyRolePermissions(nonExistingPermissions, resource, grantee, "+");
100+
addLookupEntry(resource, grantee);
101+
}
102+
103+
return nonExistingPermissions;
91104
}
92105

93-
public void revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee)
106+
public Set<Permission> revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee)
94107
throws RequestValidationException, RequestExecutionException
95108
{
96-
modifyRolePermissions(permissions, resource, revokee, "-");
97-
removeLookupEntry(resource, revokee);
109+
String roleName = escape(revokee.getRoleName());
110+
String resourceName = escape(resource.getName());
111+
Set<Permission> existingPermissions = getExistingPermissions(roleName, resourceName, permissions);
112+
113+
if (!existingPermissions.isEmpty())
114+
{
115+
modifyRolePermissions(existingPermissions, resource, revokee, "-");
116+
removeLookupEntry(resource, revokee);
117+
}
118+
119+
return existingPermissions;
98120
}
99121

100122
// Called when deleting a role with DROP ROLE query.
@@ -181,6 +203,39 @@ public Set<RoleResource> revokeAllOn(IResource droppedResource)
181203
return affectedRoles;
182204
}
183205

206+
/**
207+
* Checks that the specified role has at least one of the expected permissions on the resource.
208+
*
209+
* @param roleName the role name
210+
* @param resourceName the resource name
211+
* @param expectedPermissions the permissions to check for
212+
* @return The existing permissions
213+
*/
214+
private Set<Permission> getExistingPermissions(String roleName,
215+
String resourceName,
216+
Set<Permission> expectedPermissions)
217+
{
218+
UntypedResultSet rs = process(String.format("SELECT permissions FROM %s.%s WHERE role = '%s' AND resource = '%s'",
219+
SchemaConstants.AUTH_KEYSPACE_NAME,
220+
AuthKeyspace.ROLE_PERMISSIONS,
221+
roleName,
222+
resourceName));
223+
224+
if (rs.isEmpty())
225+
return Collections.emptySet();
226+
227+
Row one = rs.one();
228+
229+
Set<Permission> existingPermissions = Sets.newHashSetWithExpectedSize(expectedPermissions.size());
230+
for (String permissionName : one.getSet("permissions", UTF8Type.instance))
231+
{
232+
Permission permission = Permission.valueOf(permissionName);
233+
if (expectedPermissions.contains(permission))
234+
existingPermissions.add(permission);
235+
}
236+
return existingPermissions;
237+
}
238+
184239
private void executeLoggedBatch(List<CQLStatement> statements)
185240
throws RequestExecutionException, RequestValidationException
186241
{

‎src/java/org/apache/cassandra/auth/IAuthorizer.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,14 @@ default boolean requireAuthorization()
6161
* @param permissions Set of permissions to grant.
6262
* @param resource Resource on which to grant the permissions.
6363
* @param grantee Role to which the permissions are to be granted.
64+
* @return the permissions that have been successfully granted, comprised by the requested permissions excluding
65+
* those permissions that were already granted.
6466
*
6567
* @throws RequestValidationException
6668
* @throws RequestExecutionException
6769
* @throws java.lang.UnsupportedOperationException
6870
*/
69-
void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee)
71+
Set<Permission> grant(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource grantee)
7072
throws RequestValidationException, RequestExecutionException;
7173

7274
/**
@@ -79,12 +81,14 @@ void grant(AuthenticatedUser performer, Set<Permission> permissions, IResource r
7981
* @param permissions Set of permissions to revoke.
8082
* @param revokee Role from which to the permissions are to be revoked.
8183
* @param resource Resource on which to revoke the permissions.
84+
* @return the permissions that have been successfully revoked, comprised by the requested permissions excluding
85+
* those permissions that were already not granted.
8286
*
8387
* @throws RequestValidationException
8488
* @throws RequestExecutionException
8589
* @throws java.lang.UnsupportedOperationException
8690
*/
87-
void revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee)
91+
Set<Permission> revoke(AuthenticatedUser performer, Set<Permission> permissions, IResource resource, RoleResource revokee)
8892
throws RequestValidationException, RequestExecutionException;
8993

9094
/**

‎src/java/org/apache/cassandra/cql3/statements/GrantPermissionsStatement.java

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,19 @@
1818
package org.apache.cassandra.cql3.statements;
1919

2020
import java.util.Set;
21+
import java.util.stream.Collectors;
2122

2223
import org.apache.cassandra.audit.AuditLogContext;
2324
import org.apache.cassandra.audit.AuditLogEntryType;
25+
import org.apache.cassandra.auth.IAuthorizer;
2426
import org.apache.cassandra.auth.IResource;
2527
import org.apache.cassandra.auth.Permission;
2628
import org.apache.cassandra.config.DatabaseDescriptor;
2729
import org.apache.cassandra.cql3.RoleName;
2830
import org.apache.cassandra.exceptions.RequestExecutionException;
2931
import org.apache.cassandra.exceptions.RequestValidationException;
3032
import org.apache.cassandra.service.ClientState;
33+
import org.apache.cassandra.service.ClientWarn;
3134
import org.apache.cassandra.transport.messages.ResultMessage;
3235

3336
public class GrantPermissionsStatement extends PermissionsManagementStatement
@@ -39,7 +42,25 @@ public GrantPermissionsStatement(Set<Permission> permissions, IResource resource
3942

4043
public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException
4144
{
42-
DatabaseDescriptor.getAuthorizer().grant(state.getUser(), permissions, resource, grantee);
45+
IAuthorizer authorizer = DatabaseDescriptor.getAuthorizer();
46+
Set<Permission> granted = authorizer.grant(state.getUser(), permissions, resource, grantee);
47+
48+
// We want to warn the client if all the specified permissions have not been granted and the client did
49+
// not specify ALL in the query.
50+
if (!granted.equals(permissions) && !permissions.equals(Permission.ALL))
51+
{
52+
String permissionsStr = permissions.stream()
53+
.filter(permission -> !granted.contains(permission))
54+
.sorted(Permission::compareTo) // guarantee the order for testing
55+
.map(Permission::name)
56+
.collect(Collectors.joining(", "));
57+
58+
ClientWarn.instance.warn(String.format("Role '%s' was already granted %s on %s",
59+
grantee.getRoleName(),
60+
permissionsStr,
61+
resource));
62+
}
63+
4364
return null;
4465
}
4566

‎src/java/org/apache/cassandra/cql3/statements/RevokePermissionsStatement.java

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,19 @@
1818
package org.apache.cassandra.cql3.statements;
1919

2020
import java.util.Set;
21+
import java.util.stream.Collectors;
2122

2223
import org.apache.cassandra.audit.AuditLogContext;
2324
import org.apache.cassandra.audit.AuditLogEntryType;
25+
import org.apache.cassandra.auth.IAuthorizer;
2426
import org.apache.cassandra.auth.IResource;
2527
import org.apache.cassandra.auth.Permission;
2628
import org.apache.cassandra.config.DatabaseDescriptor;
2729
import org.apache.cassandra.cql3.RoleName;
2830
import org.apache.cassandra.exceptions.RequestExecutionException;
2931
import org.apache.cassandra.exceptions.RequestValidationException;
3032
import org.apache.cassandra.service.ClientState;
33+
import org.apache.cassandra.service.ClientWarn;
3134
import org.apache.cassandra.transport.messages.ResultMessage;
3235
import org.apache.commons.lang3.builder.ToStringBuilder;
3336
import org.apache.commons.lang3.builder.ToStringStyle;
@@ -41,7 +44,25 @@ public RevokePermissionsStatement(Set<Permission> permissions, IResource resourc
4144

4245
public ResultMessage execute(ClientState state) throws RequestValidationException, RequestExecutionException
4346
{
44-
DatabaseDescriptor.getAuthorizer().revoke(state.getUser(), permissions, resource, grantee);
47+
IAuthorizer authorizer = DatabaseDescriptor.getAuthorizer();
48+
Set<Permission> revoked = authorizer.revoke(state.getUser(), permissions, resource, grantee);
49+
50+
// We want to warn the client if all the specified permissions have not been revoked and the client did
51+
// not specify ALL in the query.
52+
if (!revoked.equals(permissions) && !permissions.equals(Permission.ALL))
53+
{
54+
String permissionsStr = permissions.stream()
55+
.filter(permission -> !revoked.contains(permission))
56+
.sorted(Permission::compareTo) // guarantee the order for testing
57+
.map(Permission::name)
58+
.collect(Collectors.joining(", "));
59+
60+
ClientWarn.instance.warn(String.format("Role '%s' was not granted %s on %s",
61+
grantee.getRoleName(),
62+
permissionsStr,
63+
resource));
64+
}
65+
4566
return null;
4667
}
4768

‎test/unit/org/apache/cassandra/auth/GrantAndRevokeTest.java

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.apache.cassandra.auth;
19+
20+
import org.junit.After;
21+
import org.junit.BeforeClass;
22+
import org.junit.Test;
23+
24+
import com.datastax.driver.core.ResultSet;
25+
import org.apache.cassandra.Util;
26+
import org.apache.cassandra.config.DatabaseDescriptor;
27+
import org.apache.cassandra.cql3.CQLTester;
28+
import org.apache.cassandra.service.StorageService;
29+
30+
import static org.junit.Assert.assertTrue;
31+
32+
public class GrantAndRevokeTest extends CQLTester
33+
{
34+
private static final String user = "user";
35+
private static final String pass = "12345";
36+
37+
@BeforeClass
38+
public static void setUpClass()
39+
{
40+
DatabaseDescriptor.setPermissionsValidity(0);
41+
CQLTester.setUpClass();
42+
requireAuthentication();
43+
requireNetwork();
44+
StorageService.instance.doAuthSetup(true);
45+
}
46+
47+
@After
48+
public void tearDown() throws Throwable
49+
{
50+
useSuperUser();
51+
executeNet("DROP ROLE " + user);
52+
}
53+
54+
@Test
55+
public void testWarnings() throws Throwable
56+
{
57+
useSuperUser();
58+
59+
executeNet("CREATE KEYSPACE revoke_yeah WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}");
60+
executeNet("CREATE TABLE revoke_yeah.t1 (id int PRIMARY KEY, val text)");
61+
executeNet("CREATE USER '" + user + "' WITH PASSWORD '" + pass + "'");
62+
63+
ResultSet res = executeNet("REVOKE CREATE ON KEYSPACE revoke_yeah FROM " + user);
64+
assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted CREATE on <keyspace revoke_yeah>");
65+
66+
res = executeNet("GRANT SELECT ON KEYSPACE revoke_yeah TO " + user);
67+
assertTrue(res.getExecutionInfo().getWarnings().isEmpty());
68+
69+
res = executeNet("GRANT SELECT ON KEYSPACE revoke_yeah TO " + user);
70+
assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was already granted SELECT on <keyspace revoke_yeah>");
71+
72+
res = executeNet("REVOKE SELECT ON TABLE revoke_yeah.t1 FROM " + user);
73+
assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted SELECT on <table revoke_yeah.t1>");
74+
75+
res = executeNet("REVOKE MODIFY ON KEYSPACE revoke_yeah FROM " + user);
76+
assertWarningsContain(res.getExecutionInfo().getWarnings(), "Role '" + user + "' was not granted MODIFY on <keyspace revoke_yeah>");
77+
}
78+
}

‎test/unit/org/apache/cassandra/auth/StubAuthorizer.java

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import java.util.*;
2222
import java.util.stream.Collectors;
2323

24+
import com.google.common.collect.Sets;
25+
2426
import org.apache.cassandra.exceptions.ConfigurationException;
2527
import org.apache.cassandra.exceptions.RequestExecutionException;
2628
import org.apache.cassandra.exceptions.RequestValidationException;
@@ -42,34 +44,36 @@ public Set<Permission> authorize(AuthenticatedUser user, IResource resource)
4244
return perms != null ? perms : Collections.emptySet();
4345
}
4446

45-
public void grant(AuthenticatedUser performer,
46-
Set<Permission> permissions,
47-
IResource resource,
48-
RoleResource grantee) throws RequestValidationException, RequestExecutionException
47+
public Set<Permission> grant(AuthenticatedUser performer,
48+
Set<Permission> permissions,
49+
IResource resource,
50+
RoleResource grantee) throws RequestValidationException, RequestExecutionException
4951
{
5052
Pair<String, IResource> key = Pair.create(grantee.getRoleName(), resource);
51-
Set<Permission> perms = userPermissions.get(key);
52-
if (null == perms)
53-
{
54-
perms = new HashSet<>();
55-
userPermissions.put(key, perms);
56-
}
57-
perms.addAll(permissions);
53+
Set<Permission> oldPermissions = userPermissions.computeIfAbsent(key, k -> Collections.emptySet());
54+
Set<Permission> nonExisting = Sets.difference(permissions, oldPermissions);
55+
56+
if (!nonExisting.isEmpty())
57+
userPermissions.put(key, Sets.union(oldPermissions, nonExisting));
58+
59+
return nonExisting;
5860
}
5961

60-
public void revoke(AuthenticatedUser performer,
61-
Set<Permission> permissions,
62-
IResource resource,
63-
RoleResource revokee) throws RequestValidationException, RequestExecutionException
62+
public Set<Permission> revoke(AuthenticatedUser performer,
63+
Set<Permission> permissions,
64+
IResource resource,
65+
RoleResource revokee) throws RequestValidationException, RequestExecutionException
6466
{
6567
Pair<String, IResource> key = Pair.create(revokee.getRoleName(), resource);
66-
Set<Permission> perms = userPermissions.get(key);
67-
if (null != perms)
68-
{
69-
perms.removeAll(permissions);
70-
if (perms.isEmpty())
71-
userPermissions.remove(key);
72-
}
68+
Set<Permission> oldPermissions = userPermissions.computeIfAbsent(key, k -> Collections.emptySet());
69+
Set<Permission> existing = Sets.intersection(permissions, oldPermissions);
70+
71+
if (existing.isEmpty())
72+
userPermissions.remove(key);
73+
else
74+
userPermissions.put(key, Sets.difference(oldPermissions, existing));
75+
76+
return existing;
7377
}
7478

7579
public Set<PermissionDetails> list(AuthenticatedUser performer,

0 commit comments

Comments
 (0)