You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/06/12 15:21:18 UTC
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-client/pull/301
GUACAMOLE-360: Allow users to kill their own sessions
This is the first of a few changes related to GUACAMOLE-360 - this one makes a few minor changes that allow users to kill their own sessions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-client jira/360
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/301.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #301
----
commit 1fa5f22ba71cb5f6ad14c1831d4f7ccfed596d86
Author: Nick Couchman <ni...@...>
Date: 2018-06-12T13:43:16Z
GUACAMOLE-360: Allow all users access to the session management page.
commit ba1bd535eaa929e2db2f4f14eff99676ab46542b
Author: Nick Couchman <ni...@...>
Date: 2018-06-12T14:55:28Z
GUACAMOLE-360: Allow user to kill their own active sessions.
----
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r194937148
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionService.java ---
@@ -110,21 +110,21 @@ public TrackedActiveConnection retrieveObject(ModeledAuthenticatedUser user,
@Override
public void deleteObject(ModeledAuthenticatedUser user, String identifier)
throws GuacamoleException {
-
- // Only administrators may delete active connections
- if (!user.getUser().isAdministrator())
- throw new GuacamoleSecurityException("Permission denied.");
-
+
// Close connection, if it exists (and we have permission)
ActiveConnection activeConnection = retrieveObject(user, identifier);
- if (activeConnection != null) {
+ if (activeConnection != null &&
+ (user.getUser().isAdministrator()
+ || user.getIdentifier().equals(activeConnection.getUsername()))) {
--- End diff --
> Also, not sure if there is anywhere in the JS code that I should modify to make use of these permisisons...
Updated the JS code to only show session management if the permission exists. Answered my own question.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196296995
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -312,6 +305,12 @@ angular.module('navigation').factory('userPageService', ['$injector',
url : '/settings/preferences'
}));
+ // Add link to Session management (always accessible)
+ pages.push(new PageDefinition({
--- End diff --
Order matters within the `pages` array. Moving the session management tab definition to the bottom effectively reorders the tabs of the settings screen such that the session management tab is last. Unless there is a reason to change the tab order, we should keep it consistent.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196296859
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,25 +259,16 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
});
- // If user can manage sessions, add link to sessions management page
- if (canManageSessions.length) {
--- End diff --
With these deletions, is `canManageSessions` used anywhere else anymore?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196265369
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
Always displaying it is fine with me. I actually originally implemented it that way, but swapped over to checking for active connections and permissions.
The only side-effect that I see to this is dealing with the issue of behavior when only a single connection is present and going to the home screen vs. automatically connecting. In general, this wouldn't be an issue - we can adjust the number of pages it looks at up one since we're adding one more that is there for every user - however, there's a corner-case, here, that should at least be considered, even if we ignore it.
Say the user only has access to a single connection, and that connection is open on another system. The user wants to log in and kill that session, but when they log in they get put into the connection automatically, disconnecting the one on the other client, which starts the loop of connect/disconnect between clients.
Again, it's a corner-case, but worth at least figuring out what we want to do overall with automatic connections, whether this tab is always displayed, etc.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196266846
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
For that case, I think we're relatively stuck until something like the proposed automatic-joining functionality is implemented.
I don't think there should be a disconnect loop, though. IIRC, the recent improvements to RDP error handling will cause Guacamole to not automatically reconnect when the user is kicked off specifically because another user has started a session.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196569580
--- Diff: guacamole/src/main/webapp/translations/fr.json ---
@@ -624,7 +624,9 @@
"FORMAT_STARTDATE" : "@:APP.FORMAT_DATE_TIME_PRECISE",
- "HELP_SESSIONS" : "Toutes les connexions actives Guacamole sont listées ici. Si vous souhaitez en fermer une ou plusieurs, sélectionner les et cliquer sur \"Fermer Sessions\". La fermeture d'une session déconnectera immédiatement l'utilisateur.",
+ "HELP_SESSIONS" : "
+137/5000
--- End diff --
What happened here?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196366531
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -312,6 +305,12 @@ angular.module('navigation').factory('userPageService', ['$injector',
url : '/settings/preferences'
}));
+ // Add link to Session management (always accessible)
+ pages.push(new PageDefinition({
--- End diff --
Ah, yeah, that makes sense. Should be ordered correctly, now.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196264500
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
I'm not sure. Only displaying the tab if manageable connections exist definitely makes sense in theory, but the behavior in practice feels inconsistent to me:
https://youtu.be/yiVlfRtZRK4
What do you think about always displaying that tab?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196613490
--- Diff: guacamole/src/main/webapp/translations/fr.json ---
@@ -624,7 +624,9 @@
"FORMAT_STARTDATE" : "@:APP.FORMAT_DATE_TIME_PRECISE",
- "HELP_SESSIONS" : "Toutes les connexions actives Guacamole sont listées ici. Si vous souhaitez en fermer une ou plusieurs, sélectionner les et cliquer sur \"Fermer Sessions\". La fermeture d'une session déconnectera immédiatement l'utilisateur.",
+ "HELP_SESSIONS" : "
+137/5000
--- End diff --
Copypasta. From Google Translate. Will fix when I fix the wording.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r194934397
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionService.java ---
@@ -110,21 +110,21 @@ public TrackedActiveConnection retrieveObject(ModeledAuthenticatedUser user,
@Override
public void deleteObject(ModeledAuthenticatedUser user, String identifier)
throws GuacamoleException {
-
- // Only administrators may delete active connections
- if (!user.getUser().isAdministrator())
- throw new GuacamoleSecurityException("Permission denied.");
-
+
// Close connection, if it exists (and we have permission)
ActiveConnection activeConnection = retrieveObject(user, identifier);
- if (activeConnection != null) {
+ if (activeConnection != null &&
+ (user.getUser().isAdministrator()
+ || user.getIdentifier().equals(activeConnection.getUsername()))) {
--- End diff --
Okay, I've taken a stab at implementing this, though it was slightly different from the `ConnectionService` and `UserService` object types. Those seem to extend the `ModeledChildDirectoryObjectService`, which has abstracts in it for the `hasObjectPermissions()` and `getPermissionSet()` methods. `ActiveConnectionService`, on the other hand, extends `DirectoryObjectService`, which does not have such methods. So, I wasn't sure if just implementing them in `ActiveConnectionService` as `private` methods was okay, if `ActiveConnectionService` should somehow be reworked to extend the `ModeledChildDirectoryObjectService` class (this doesn't seem right since `ActiveConnection` objects aren't modeling/modeled by anything), or if `DirectoryObjectService` should add some method declarations for the permission checks?
Also, not sure if there is anywhere in the JS code that I should modify to make use of these permisisons...
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196366478
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,25 +259,16 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
});
- // If user can manage sessions, add link to sessions management page
- if (canManageSessions.length) {
--- End diff --
Yep, you're right. Removed.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196234222
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionService.java ---
@@ -111,20 +113,19 @@ public TrackedActiveConnection retrieveObject(ModeledAuthenticatedUser user,
public void deleteObject(ModeledAuthenticatedUser user, String identifier)
throws GuacamoleException {
- // Only administrators may delete active connections
- if (!user.getUser().isAdministrator())
- throw new GuacamoleSecurityException("Permission denied.");
-
- // Close connection, if it exists (and we have permission)
+ // Close connection, if it exists and we have permission
ActiveConnection activeConnection = retrieveObject(user, identifier);
- if (activeConnection != null) {
+ if (activeConnection != null
+ && hasObjectPermissions(user, identifier, ObjectPermission.Type.DELETE)) {
// Close connection if not already closed
GuacamoleTunnel tunnel = activeConnection.getTunnel();
if (tunnel != null && tunnel.isOpen())
tunnel.close();
}
+ else
+ throw new GuacamoleSecurityException("Permission denied.");
--- End diff --
This technically goes against [the definition of `deleteObject()` on the interface](https://github.com/apache/guacamole-client/blob/984ab48ce8dbbb5b9949ce1f5e5f774168b4830b/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/DirectoryObjectService.java#L103-L104) which states (emphasis added):
> Deletes the object having the given identifier. **If no such object exists, this function has no effect.**
The `GuacamoleSecurityException` should only be thrown if permission is denied to delete the object. If no such object exists at all, the function should silently do nothing.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196613262
--- Diff: guacamole/src/main/webapp/translations/en.json ---
@@ -750,7 +750,7 @@
"FORMAT_STARTDATE" : "@:APP.FORMAT_DATE_TIME_PRECISE",
- "HELP_SESSIONS" : "All currently-active Guacamole sessions are listed here. If you wish to kill one or more sessions, check the box next to those sessions and click \"Kill Sessions\". Killing a session will immediately disconnect the user from the associated connection.",
+ "HELP_SESSIONS" : "Your currently-active Guacamole sessions are listed here, and Administrators will be able to see currently-active sessions for all users. If you wish to kill one or more sessions, check the box next to those sessions and click \"Kill Sessions\". Killing a session will immediately disconnect the user from the associated connection.",
--- End diff --
How about:
> This page will be populated with currently-active connections. The connections listed and the ability to kill those connections is dependent upon your access level. ...
That sound okay?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196572846
--- Diff: guacamole/src/main/webapp/translations/en.json ---
@@ -750,7 +750,7 @@
"FORMAT_STARTDATE" : "@:APP.FORMAT_DATE_TIME_PRECISE",
- "HELP_SESSIONS" : "All currently-active Guacamole sessions are listed here. If you wish to kill one or more sessions, check the box next to those sessions and click \"Kill Sessions\". Killing a session will immediately disconnect the user from the associated connection.",
+ "HELP_SESSIONS" : "Your currently-active Guacamole sessions are listed here, and Administrators will be able to see currently-active sessions for all users. If you wish to kill one or more sessions, check the box next to those sessions and click \"Kill Sessions\". Killing a session will immediately disconnect the user from the associated connection.",
--- End diff --
This is certainly true for the database authentication extensions, but not necessarily true in the general case. Extensions are free to define their own systems for determining who has permission to view/kill a particular session, so long as they properly expose that permission through the relevant permission sets.
For other parts of the administrative interface, the wording of the help text both describes what may be done and that those actions depend on access level. For example, from the connection interface:
> Click or tap on a connection below to manage that connection. Depending on your access level, connections can be added and deleted, and their properties (protocol, hostname, port, etc.) can be changed.
The new wording is a step in the right direction, but I don't think we should assert that the permissions work a specific way, even if it's the most common implementation of those permissions. Perhaps similar "depending on access level", etc. wording would be useful here?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196258355
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
That's what I was going for, yes, but I don't really have a strong attraction to that behavior if you think it should act some other way?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196257189
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
This will result in the session management tab only appearing if at least one killable connection is active. Once that connection is killed, the tab will cease to appear (after any cached permissions have been refreshed).
Is this the intended behavior?
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-client/pull/301
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196270568
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
Okay, changed so it will always show the session management page under Settings.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r194894400
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionService.java ---
@@ -110,21 +110,21 @@ public TrackedActiveConnection retrieveObject(ModeledAuthenticatedUser user,
@Override
public void deleteObject(ModeledAuthenticatedUser user, String identifier)
throws GuacamoleException {
-
- // Only administrators may delete active connections
- if (!user.getUser().isAdministrator())
- throw new GuacamoleSecurityException("Permission denied.");
-
+
// Close connection, if it exists (and we have permission)
ActiveConnection activeConnection = retrieveObject(user, identifier);
- if (activeConnection != null) {
+ if (activeConnection != null &&
+ (user.getUser().isAdministrator()
+ || user.getIdentifier().equals(activeConnection.getUsername()))) {
--- End diff --
This same check will need to be done within `ActiveConnectionPermissionService` for permission queries to correctly declare the actions available to the user:
https://github.com/apache/guacamole-client/blob/bf3d27611db6df6549c337ddc6063ad6a195d9ad/extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionPermissionService.java#L99-L101
Rather than duplicating the checks, though, I suggest testing the permissions using the permission set (similar to the checks performed for users and connections).
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196253739
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionService.java ---
@@ -111,20 +113,19 @@ public TrackedActiveConnection retrieveObject(ModeledAuthenticatedUser user,
public void deleteObject(ModeledAuthenticatedUser user, String identifier)
throws GuacamoleException {
- // Only administrators may delete active connections
- if (!user.getUser().isAdministrator())
- throw new GuacamoleSecurityException("Permission denied.");
-
- // Close connection, if it exists (and we have permission)
+ // Close connection, if it exists and we have permission
ActiveConnection activeConnection = retrieveObject(user, identifier);
- if (activeConnection != null) {
+ if (activeConnection != null
+ && hasObjectPermissions(user, identifier, ObjectPermission.Type.DELETE)) {
// Close connection if not already closed
GuacamoleTunnel tunnel = activeConnection.getTunnel();
if (tunnel != null && tunnel.isOpen())
tunnel.close();
}
+ else
+ throw new GuacamoleSecurityException("Permission denied.");
--- End diff --
Adjusted to behave as defined.
---
[GitHub] guacamole-client pull request #301: GUACAMOLE-360: Allow users to kill their...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/301#discussion_r196267141
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -257,15 +257,23 @@ angular.module('navigation').factory('userPageService', ['$injector',
canManageConnections.push(dataSource);
}
- // Determine whether the current user needs access to the session management UI or view connection history
+ // Determine whether the current user needs access to view connection history
if (
- // A user must be a system administrator to manage sessions
+ // A user must be a system administrator to view connection records
PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
) {
- canManageSessions.push(dataSource);
canViewConnectionRecords.push(dataSource);
}
+ // Determine whether the current user needs access to view session management
+ if (
+ // Permission to manage active sessions.
+ PermissionSet.hasSystemPermission(permissions, PermissionSet.SystemPermissionType.ADMINISTER)
+ || PermissionSet.hasActiveConnectionPermission(permissions, PermissionSet.ObjectPermissionType.DELETE)
--- End diff --
Also regarding that corner case, such a connection could be configured to allow only one concurrent user. The user attempting to connect would be disallowed with a warning that the connection is in use. If they wish to kick off the existing session (and have permission to do so), they could then open the menu with Ctrl+Alt+Shift, go to Settings, and manually kill it.
---