You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/06/17 04:02:22 UTC
[GitHub] [guacamole-client] mike-jumper opened a new pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
mike-jumper opened a new pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523
To facilitate collaborative storage of arbitrary attributes between extensions, this change adds a new `getPrivileged()` function to the `UserContext` interface, allowing an internal and privileged `UserContext` to be retrieved. As only an extension may invoke this function (it is not called within the REST API), this allows extensions to expose privileged access to other extensions without requiring that users logging into Guacamole via those extensions have the same privileges.
For example, Guacamole's TOTP support has historically required that each user have `UPDATE` privileges on themselves, since the database authentication would otherwise deny the TOTP extension's attempt to store additional attributes for that user. With these changes, the TOTP support uses a privileged `UserContext` to store its attributes, and users of TOTP need not be granted additional privileges.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by 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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523#discussion_r441838667
##########
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:
Oops.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523#discussion_r441838573
##########
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:
Sure - I'll look through and correct the cases that still imply that `isPrivileged()` indicates a system administrator.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523#discussion_r441838990
##########
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:
Oops++
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523#discussion_r441965138
##########
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:
Fixed by amending the relevant commit.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523#discussion_r441965138
##########
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:
Fixed as part of the comment update for privileged vs. admin.
##########
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:
Fixed by amending the relevant commit.
##########
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:
I believe this is now fixed.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman merged pull request #523: GUACAMOLE-708: Add API for exposing privileged access to extensions.
Posted by GitBox <gi...@apache.org>.
necouchman merged pull request #523:
URL: https://github.com/apache/guacamole-client/pull/523
----------------------------------------------------------------
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