GUACAMOLE-220: Add base extension API support for user groups. #274

mike-jumper · 2018-04-19T23:12:11Z

This change adds the base classes necessary for exposing user groups via extensions, and for declaring the permissions which apply to a user but are not necessarily directly granted to that user.

Overall, this involves:

A new UserGroup interface, as well as the usual DelegatingUserGroup, and Directory<UserGroup> on UserContext.
A new base interface, Permissions, which User and UserGroup share, defining the interface common to any object which can be granted permissions.
A new getEffectivePermissions() function on User which returns a Permissions that represents all permissions which apply to the user, even if those permissions are inherited through some arbitrary means (such as user groups). The traditional getUserPermissions(), getConnectionPermissions(), etc. directly on the User return permission sets which describe permissions which are directly granted only.
A new getEffectiveUserGroups() function on AuthenticatedUser which allows implementations to expose the identity of a user's group memberships in addition to the user's own identity. This forms the means by which membership in groups can be shared across extensions (for example, LDAP group membership can affect a user in MySQL so long as an identical MySQL group exists, even if that user does not exist in MySQL).
A new RelatedObjectSet interface, similar to PermissionSet, which abstracts batch add/remove operations on a group of objects sharing some arbitrary relation, such as the member users of a user group, the parent user groups of a user, etc. This allows relations between specific objects to be established or removed without affecting the existence of those objects (as Directory would require) nor necessarily other relations to those objects.

The user groups themselves are exposed via a Directory<UserGroup> at the UserContext level, like all other objects. A new ObjectPermissionSet for user group permissions is exposed within Permissions. User membership within groups, group membership within groups, the set of groups containing a particular user, and the set of groups containing a particular group can all be maintained through the various getters returning RelatedObjectSet instances.

Other than (1) updating the REST services and JavaScript to use effective permissions rather than directly-granted permissions and (2) any stubs necessary to allow the database auth to continue working, no changes outside the API are made here.

Assuming these changes move forward, I will open further pull requests for the REST API changes, interface changes, and finally database auth changes.

…mally necessary.

…rces.

necouchman

Took a first pass at it. Looks really good to me - just minor questions/comments.

necouchman · 2018-04-20T08:35:45Z

guacamole-ext/src/main/java/org/apache/guacamole/net/auth/UserGroup.java

+     * @throws GuacamoleException
+     *     If an error occurs while retrieving the user groups.
+     */
+    RelatedObjectSet getMemberUserGroups() throws GuacamoleException;


Is there a reason that getUserGroups() and getMemberUserGroups() are both necessary? Based on the stubs and descriptions, here, they look identical? I see that they're both implemented in DelegatingUserGroup, but they have identical implementations, there, and getMemberUserGroups() does not appear to be used anywhere outside of there?

getUserGroups() returns the user groups of which the current group is a member, while getMemberUserGroups() returns the user groups which are members of the current group.

Okay, in that case, the comments above the code need to be updated. The comment above getUserGroups() says:

Returns a set of all readable user groups that are members of this user group.

and the comment above getMemberUserGroups() starts with:

Returns a set of all readable user groups that are members of this user group.

Also, the @return comment seems to be identical for both of them?

OK - this should now be corrected.

necouchman · 2018-04-20T08:39:38Z

guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleRelatedObjectSet.java

+ * A read-only implementation of RelatedObjectSet which uses a backing Set
+ * of identifiers to determine which objects are present.
+ */
+public class SimpleRelatedObjectSet implements RelatedObjectSet {


Do you think there's any value to implementing a AbstractRelatedObjectSet class like you've done for several of the other Simple classes, that implements a bunch of these default behaviors? Perhaps not initially useful for the JDBC extension, but is it something that would be helpful as other extensions implement the RelatedObjectSet class without having to rely on the behavior of SimpleRelatedObjectSet?

In principle, yes, but I'm not sure what that would be. There are only two functions that are effectively unimplemented by SimpleRelatedObjectSet:

addObjects()

removeObjects()

both of which make changes to the RelatedObjectSet in a way that would be implementation dependent. It does make sense to provide default behaviors, but I don't think there are any sensible default behaviors we can provide here.

Make sense.

necouchman · 2018-04-20T08:43:54Z

guacamole/src/main/java/org/apache/guacamole/rest/connection/ConnectionResource.java

        User self = userContext.self();
+        Permissions effective = self.getEffectivePermissions();


In the ConnectionGroupTree class you did:

Permissions effective = userContext.self().getEffectivePermissions();

eliminating the need for the existence of the self object. Any reason not to do the same, here?

Nope. Sounds fine to me. I'll make the change.

necouchman · 2018-04-20T08:45:19Z

guacamole/src/main/java/org/apache/guacamole/rest/directory/DirectoryResource.java

@@ -143,13 +144,14 @@ public DirectoryResource(UserContext userContext, Directory<InternalType> direct

        // An admin user has access to all objects
        User self = userContext.self();
-        SystemPermissionSet systemPermissions = self.getSystemPermissions();
+        Permissions effective = self.getEffectivePermissions();


Same note as above - any reason not to do

Permissions effective = userContext.self().getEffectivePermissions()

?

Nope. No reason. Will do.

necouchman · 2018-04-20T08:47:30Z

guacamole/src/main/java/org/apache/guacamole/rest/sharingprofile/SharingProfileResource.java

        User self = userContext.self();
+        Permissions effective = self.getEffectivePermissions();


As with previous places, guessing these two lines could be collapsed...

Yup, I agree.

necouchman

A few more style/grammar nitpicks.

necouchman · 2018-04-21T00:13:43Z

guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticatedUser.java

@@ -19,6 +19,9 @@

 package org.apache.guacamole.net.auth;

+import java.util.Collections;
+import java.util.Set;
+


Extra line added, here - seems like most files have a single space between import and class documentation.

necouchman · 2018-04-21T00:15:34Z

guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AuthenticatedUser.java

+     * Note that, as with user identifiers, user group identifiers form the
+     * basis of identity which applies across authentication providers. It is
+     * expected that any two user groups having the same identifier represent
+     * the same group, even if defined by different authentication providers


Missing period at the end of this sentence.

necouchman · 2018-04-21T00:21:58Z

guacamole-ext/src/main/java/org/apache/guacamole/net/auth/User.java

-     * Returns all user permissions given to this user.
+     * Returns a read-only view of all permissions granted to this user. The
+     * exact semantics of what permissions are granted are up to the
+     * implementation, and the permissions within this view may implied,


"may be implied"

…permissions.

…period.

…BE implied, not "may implied".

mike-jumper · 2018-04-21T21:28:30Z

OK, I believe I've addressed the issues above, with the exception of adding a AbstractRelatedObjectSet (as I am unable to define a set of default behaviors that is not already provided by SimpleRelatedObjectSet).

necouchman · 2018-04-21T23:03:10Z

Cool - will try to review one last time tonight and get it merged.

necouchman

Looks great, about to merge.

mike-jumper added 2 commits April 19, 2018 15:29

GUACAMOLE-220: Add base API support for user groups. Refactor as mini…

dfb07ed

…mally necessary.

GUACAMOLE-220: Use effective permissions to test user access to resou…

393e1ab

…rces.

necouchman reviewed Apr 20, 2018

View reviewed changes

necouchman reviewed Apr 21, 2018

View reviewed changes

mike-jumper added 5 commits April 21, 2018 14:11

GUACAMOLE-220: Correct documentation of getUserGroups() behavior.

b43aa59

GUACAMOLE-220: Collapse / clean up retrieval of self() and effective …

6db10c1

…permissions.

GUACAMOLE-220: Remove erroneously-added blank line.

b35e494

GUACAMOLE-220: Correct JavaDoc of getEffectiveUserGroups() - missing …

c8564f5

…period.

GUACAMOLE-220: Permissions returned by getEffectivePermissions() may …

e75e971

…BE implied, not "may implied".

necouchman approved these changes Apr 22, 2018

View reviewed changes

asfgit merged commit e75e971 into apache:master Apr 22, 2018

mike-jumper deleted the user-group-ext branch April 23, 2018 15:12

mike-jumper mentioned this pull request May 4, 2018

GUACAMOLE-220: Expose user groups, group membership, and group permissions via REST. #285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUACAMOLE-220: Add base extension API support for user groups. #274

GUACAMOLE-220: Add base extension API support for user groups. #274

mike-jumper commented Apr 19, 2018

necouchman left a comment

necouchman Apr 20, 2018

mike-jumper Apr 20, 2018

necouchman Apr 20, 2018

mike-jumper Apr 21, 2018

mike-jumper Apr 21, 2018

necouchman Apr 20, 2018

mike-jumper Apr 21, 2018

necouchman Apr 21, 2018

necouchman Apr 20, 2018

mike-jumper Apr 21, 2018

necouchman Apr 20, 2018

mike-jumper Apr 21, 2018

necouchman Apr 20, 2018

mike-jumper Apr 21, 2018

necouchman left a comment

necouchman Apr 21, 2018

necouchman Apr 21, 2018

necouchman Apr 21, 2018

mike-jumper commented Apr 21, 2018

necouchman commented Apr 21, 2018

necouchman left a comment

		User self = userContext.self();
		Permissions effective = self.getEffectivePermissions();

GUACAMOLE-220: Add base extension API support for user groups. #274

GUACAMOLE-220: Add base extension API support for user groups. #274

Conversation

mike-jumper commented Apr 19, 2018

necouchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necouchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-jumper commented Apr 21, 2018

necouchman commented Apr 21, 2018

necouchman left a comment

Choose a reason for hiding this comment