You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2021/09/24 12:14:49 UTC
[GitHub] [cassandra] azotcsit commented on a change in pull request #1179: CASSANDRA-16902 trunk: A user should be able to view permissions of role they created
azotcsit commented on a change in pull request #1179:
URL: https://github.com/apache/cassandra/pull/1179#discussion_r715141670
##########
File path: src/java/org/apache/cassandra/service/ClientState.java
##########
@@ -404,6 +404,19 @@ public void ensurePermission(Permission perm, IResource resource)
ensurePermissionOnResourceChain(perm, resource);
}
+ public boolean hasPermission(Permission required, RoleResource resource) throws UnauthorizedException
Review comment:
It does not throw `UnauthorizedException`
##########
File path: src/java/org/apache/cassandra/cql3/statements/ListPermissionsStatement.java
##########
@@ -78,6 +79,17 @@ public void validate(ClientState state) throws RequestValidationException
if ((grantee != null) && !DatabaseDescriptor.getRoleManager().isExistingRole(grantee))
throw new InvalidRequestException(String.format("%s doesn't exist", grantee));
+
+ // If the user requesting 'LIST PERMISSIONS' is not a superuser OR their username doesn't match 'grantee' OR
+ // has no DESCRIBE permission on the role or all roles, we throw UnauthorizedException.
+ // So only a superuser, system user can view everybody's permissions. Regular users are only
+ // allowed to see their own permissions and those of roles for which they have DESCRIBE permission.
+ if (!state.getUser().isSuper()
Review comment:
I guess the right place to this logic is `authorize` method.
##########
File path: src/java/org/apache/cassandra/cql3/statements/ListPermissionsStatement.java
##########
@@ -78,6 +79,17 @@ public void validate(ClientState state) throws RequestValidationException
if ((grantee != null) && !DatabaseDescriptor.getRoleManager().isExistingRole(grantee))
throw new InvalidRequestException(String.format("%s doesn't exist", grantee));
+
+ // If the user requesting 'LIST PERMISSIONS' is not a superuser OR their username doesn't match 'grantee' OR
+ // has no DESCRIBE permission on the role or all roles, we throw UnauthorizedException.
+ // So only a superuser, system user can view everybody's permissions. Regular users are only
+ // allowed to see their own permissions and those of roles for which they have DESCRIBE permission.
+ if (!state.getUser().isSuper()
+ && !state.getUser().isSystem()
+ && !state.getUser().getRoles().contains(grantee)
+ && !state.hasPermission(Permission.DESCRIBE, grantee == null ? RoleResource.root() : grantee))
Review comment:
Would it be easier to not create a `hasPermission` method and just check everything right here?
```
&& !state.getUser().getPermissions(RoleResource.root()).contains(Permission.DESCRIBE)
&& !(grantee != null && state.getUser().getPermissions(grantee).contains(Permission.DESCRIBE)))
```
`hasPermission` is slightly "non-concise" to `ClientState` class because everything there is "ensured" (meaning throwing exceptions).
##########
File path: src/java/org/apache/cassandra/auth/RoleResource.java
##########
@@ -49,7 +49,8 @@
// permissions which may be granted on role level resources
private static final Set<Permission> ROLE_LEVEL_PERMISSIONS = Sets.immutableEnumSet(Permission.ALTER,
Permission.DROP,
- Permission.AUTHORIZE);
+ Permission.AUTHORIZE,
+ Permission.DESCRIBE);
Review comment:
Please, update `CreateRoleStatement`, see https://github.com/azotcsit/cassandra/commit/4ee78c216c1f4e03f55174c9f2d7b86385bbbd3d#diff-6fa1c95412982ddb8451f332084d0d8d59a38d3a4c3c4b8bd8d955f71d55554aR98
##########
File path: src/java/org/apache/cassandra/auth/CassandraAuthorizer.java
##########
@@ -240,19 +240,12 @@ private void addLookupEntry(IResource resource, RoleResource role) throws Reques
}
// 'of' can be null - in that case everyone's permissions have been requested. Otherwise only single user's.
- // If the user requesting 'LIST PERMISSIONS' is not a superuser OR their username doesn't match 'of', we
- // throw UnauthorizedException. So only a superuser can view everybody's permissions. Regular users are only
- // allowed to see their own permissions.
public Set<PermissionDetails> list(AuthenticatedUser performer,
Set<Permission> permissions,
IResource resource,
RoleResource grantee)
throws RequestValidationException, RequestExecutionException
{
- if (!(performer.isSuper() || performer.isSystem()) && !performer.getRoles().contains(grantee))
Review comment:
Basically we could achieve everything we need right here. Please, check a prototype code I wrote: https://github.com/azotcsit/cassandra/commit/4ee78c216c1f4e03f55174c9f2d7b86385bbbd3d#diff-a7b54023248dbee7fbdcb09e9336fabeddeede6276dd226850cb29e79eb5e7f2R252
##########
File path: src/java/org/apache/cassandra/cql3/statements/ListPermissionsStatement.java
##########
@@ -78,6 +79,17 @@ public void validate(ClientState state) throws RequestValidationException
if ((grantee != null) && !DatabaseDescriptor.getRoleManager().isExistingRole(grantee))
throw new InvalidRequestException(String.format("%s doesn't exist", grantee));
+
+ // If the user requesting 'LIST PERMISSIONS' is not a superuser OR their username doesn't match 'grantee' OR
+ // has no DESCRIBE permission on the role or all roles, we throw UnauthorizedException.
+ // So only a superuser, system user can view everybody's permissions. Regular users are only
+ // allowed to see their own permissions and those of roles for which they have DESCRIBE permission.
+ if (!state.getUser().isSuper()
+ && !state.getUser().isSystem()
+ && !state.getUser().getRoles().contains(grantee)
+ && !state.hasPermission(Permission.DESCRIBE, grantee == null ? RoleResource.root() : grantee))
Review comment:
Actually we can even write it like this:
```
AuthenticatedUser performer = state.getUser();
if (performer.isSuper() || performer.isSystem() || performer.getRoles().contains(grantee))
return;
state.ensurePermission(Permission.DESCRIBE, grantee == null ? RoleResource.root() : grantee);
```
I feel this logic is even more clear. However, the downside is that it slightly changes error message. From:
```
"You are not authorized to view %s's permissions", grantee == null ? "everyone" : grantee.getRoleName()
```
To:
```
"User %s has no %s permission on %s or any of its parents", user.getName(), perm, resource)
```
that does not seem to be a problem.
Please, share your thoughts.
##########
File path: src/java/org/apache/cassandra/service/ClientState.java
##########
@@ -404,6 +404,19 @@ public void ensurePermission(Permission perm, IResource resource)
ensurePermissionOnResourceChain(perm, resource);
}
+ public boolean hasPermission(Permission required, RoleResource resource) throws UnauthorizedException
Review comment:
In general I'm not sure we really need to introduce this methods, please, check another comment and let me know your thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org