[GitHub] [guacamole-client] necouchman commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.

[GitBox <gi...@apache.org>]

necouchman commented on a change in pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523#discussion_r441458483



##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionPermissionService.java
##########
@@ -82,21 +82,22 @@ public boolean hasPermission(ModeledAuthenticatedUser user,
         // Retrieve permissions only if allowed
         if (canReadPermissions(user, targetEntity)) {
 
-            // Only administrators may access active connections
-            boolean isAdmin = targetEntity.isAdministrator();
+            // Administrators may always access active connections
+            boolean isAdmin = targetEntity.isPrivileged();
 
             // Get all active connections
             Collection<ActiveConnectionRecord> records = tunnelService.getActiveConnections(user);
 
             // We have READ, and possibly DELETE, on all active connections
-            Set<ObjectPermission> permissions = new HashSet<ObjectPermission>();
+            Set<ObjectPermission> permissions = new HashSet<>();
             for (ActiveConnectionRecord record : records) {
 
                 // Add implicit READ
                 String identifier = record.getUUID().toString();
                 permissions.add(new ObjectPermission(ObjectPermission.Type.READ, identifier));
 
-                // If we're an admin, or the connection is ours, then we can DELETE
+                // If the target use is an admin, or the connection belongs to

Review comment:
       > If the target *user* is an admin...

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/RelatedObjectSet.java
##########
@@ -141,7 +141,7 @@ private boolean canAlterRelation(Collection<String> identifiers)
             throws GuacamoleException {
 
         // System administrators may alter any relations

Review comment:
       Not sure if this comment (and the others like it throughout this code) should be updated to say "Privileged entities" to cover both the user and privileged `UserContext` cases?  Or will this always be a `User` of some sort?

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUserContext.java
##########
@@ -141,15 +151,40 @@ public void init(ModeledAuthenticatedUser currentUser) {
         sharingProfileDirectory.init(currentUser);
         activeConnectionDirectory.init(currentUser);
 
+    }
+
+    /**
+     * Records that the user associated with this UserContext has logged in,
+     * creating a partial activity record. The resulting activity record will
+     * contain a start date only, with the end date being automatically
+     * populated when this UserContext is invalidated. If this function is
+     * invoked more than once for the same UserContext, only the first
+     * invocation has any effect. If this function is never invoked, no
+     * activity record will be recorded, including when this UserContext is
+     * invalidated.
+     */
+    public void recordUserLogin(){

Review comment:
       Style nitpick...space between the `()` and the `{`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org