Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/319#discussion_r221458463
--- Diff:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/permission/ModeledObjectPermissionService.java
---
@@ -155,49 +161,47 @@ public void
deletePermissions(ModeledAuthenticatedUser user, ModeledUser targetU
}
@Override
- public ObjectPermission retrievePermission(ModeledAuthenticatedUser
user,
- ModeledUser targetUser, ObjectPermission.Type type,
- String identifier) throws GuacamoleException {
+ public boolean hasPermission(ModeledAuthenticatedUser user,
+ ModeledPermissions<? extends EntityModel> targetEntity,
+ ObjectPermission.Type type, String identifier,
+ Set<String> effectiveGroups) throws GuacamoleException {
// Retrieve permissions only if allowed
- if (canReadPermissions(user, targetUser)) {
+ if (canReadPermissions(user, targetEntity))
+ return getPermissionMapper().selectOne(targetEntity.getModel(),
+ type, identifier, effectiveGroups) != null;
- // Read permission from database, return null if not found
- ObjectPermissionModel model =
getPermissionMapper().selectOne(targetUser.getModel(), type, identifier);
- if (model == null)
- return null;
-
- return getPermissionInstance(model);
-
- }
-
- // User cannot read this user's permissions
+ // User cannot read this entity's permissions
throw new GuacamoleSecurityException("Permission denied.");
}
@Override
public Collection<String>
retrieveAccessibleIdentifiers(ModeledAuthenticatedUser user,
- ModeledUser targetUser, Collection<ObjectPermission.Type>
permissions,
- Collection<String> identifiers) throws GuacamoleException {
+ ModeledPermissions<? extends EntityModel> targetEntity,
+ Collection<ObjectPermission.Type> permissions,
+ Collection<String> identifiers, Set<String> effectiveGroups)
+ throws GuacamoleException {
// Nothing is always accessible
if (identifiers.isEmpty())
return identifiers;
// Retrieve permissions only if allowed
- if (canReadPermissions(user, targetUser)) {
+ if (canReadPermissions(user, targetEntity)) {
// If user is an admin, everything is accessible
if (user.getUser().isAdministrator())
--- End diff --
Does it make sense to have this check outside the parent `if()` statement?
Are there situations where `canReadPermissions()` would return false, but
`user.getUser().isAdministrator()` would return true?
Actually, looking at some of the code below, there seem to be places where
`canReadPermissions()` is called without the `isAdministrator()` check at all -
does (or should?) `canReadPermissions()` already do the check internally, in
which case this could be eliminated entirely, here?
---
