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 2017/08/03 15:09:31 UTC
[GitHub] incubator-guacamole-client pull request #179: GUACAMOLE-358: Fix issue with ...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/179
GUACAMOLE-358: Fix issue with 404 errors from REST API
This pull request adds a new SimpleUserContext to the CAS extension, instead of returning null, which seems to resolve the issue reported in this JIRA issue with errors trying to access the Settings page in Guacamole.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-358
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/179.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 #179
----
commit 3f19d8d54c5c678d00e673165f61a5716a3b80d7
Author: Nick Couchman <vn...@apache.org>
Date: 2017-08-03T15:06:59Z
GUACAMOLE-358: Add a SimpleUserContext object to CAS to fix session errors thrown when userContext is null.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #179: GUACAMOLE-358: Fix issue with ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/179#discussion_r135919819
--- Diff: guacamole/src/main/webapp/app/settings/directives/guacSettingsPreferences.js ---
@@ -192,6 +192,9 @@ angular.module('settings').directive('guacSettingsPreferences', [function guacSe
$scope.canChangePassword = PermissionSet.hasUserPermission(permissions,
PermissionSet.ObjectPermissionType.UPDATE, username);
+ })
+ .error(function permissionsFailed(error) {
+ $scope.canChangePassword = false;
--- End diff --
Eh ... unless @jmuehlner disagrees, I withdraw my concerns. I think these changes are fine.
There are ways to explicitly check for and handle the error response, as done in other parts of the admin interface, such that the user will see some useful error message describing the failure... but in this case, I can't think of any such message which would be appropriate or helpful. It's just the password edit screen.
If the rest of the webapp's interface handled such errors and always warned the user when the service is unavailable (for example), that would be another matter, but that's not currently done, and doing so here would thus be out of scope.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #179: GUACAMOLE-358: Fix issue with ...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/179#discussion_r134768825
--- Diff: guacamole/src/main/webapp/app/settings/directives/guacSettingsPreferences.js ---
@@ -192,6 +192,9 @@ angular.module('settings').directive('guacSettingsPreferences', [function guacSe
$scope.canChangePassword = PermissionSet.hasUserPermission(permissions,
PermissionSet.ObjectPermissionType.UPDATE, username);
+ })
+ .error(function permissionsFailed(error) {
+ $scope.canChangePassword = false;
--- End diff --
The more I think on this, the more I think that 404 response is indeed the correct response, and that what you have here is the most correct approach. My original thoughts were to modify the REST API to ensure that the authenticated user can always be queried, but if an extension does not expose data at all, even data related to the authenticated user, 404 is really exactly what should be seen. It's the client-side which needs to be modified to properly understand and deal with that response. So ... looks good.
My only concern with these changes as they stand is that absolutely all errors from the REST API here will be interpreted as successful indications that the user lacks permission.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #179: GUACAMOLE-358: Fix issue with ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/179
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #179: GUACAMOLE-358: Fix issue with ...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/179#discussion_r134770947
--- Diff: guacamole/src/main/webapp/app/settings/directives/guacSettingsPreferences.js ---
@@ -192,6 +192,9 @@ angular.module('settings').directive('guacSettingsPreferences', [function guacSe
$scope.canChangePassword = PermissionSet.hasUserPermission(permissions,
PermissionSet.ObjectPermissionType.UPDATE, username);
+ })
+ .error(function permissionsFailed(error) {
+ $scope.canChangePassword = false;
--- End diff --
So is there some additional checking I should do on the contents of the error object to see what's happening there, and do some interpretation? Are there other specific errors that you'd expect to see coming out of this particular REST call?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---