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/11 01:35:58 UTC
[GitHub] incubator-guacamole-client pull request #180: GUACAMOLE-355: Catch CAS Error...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/180
GUACAMOLE-355: Catch CAS Errors and Display Them
This PR catches the errors thrown by the CAS module and rethrows them as Guacmaole exceptions, and displays an error message in the login box.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-355
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/180.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 #180
----
commit 0c33d2da7d745223295c87ebbb80574774c490e6
Author: Nick Couchman <vn...@apache.org>
Date: 2017-08-11T01:34:23Z
GUACAMOLE-355: Catch CAS errors and throw them to Guacamole, and display error message in login dialog.
----
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r147494242
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -116,9 +116,6 @@
}
.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
--- End diff --
For this particular request, no, it doesn't matter, so I'll pull that change. However, it may make it into the RADIUS change (#122), as having these entries here makes the subsequent login dialogs look a little odd.
---
[GitHub] incubator-guacamole-client pull request #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r134079492
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -75,6 +84,10 @@ public String processUsername(String ticket) throws GuacamoleException {
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
+ catch (Throwable t) {
+ logger.error("Error validating ticket with CAS server: {}", t.getMessage());
+ throw new GuacamoleInsufficientCredentialsException("Error validating ticket with CAS server.", t, CredentialsInfo.USERNAME_PASSWORD);
--- End diff --
Okay, I can make the error more generic. However, throwing GuacamoleInvalidCredentialsException does not display the red "Invalid Login" error message when the username/password field is displayed - I think it has something to do with the fact that the CAS authentication redirects away from the Guacamole page and then back to it. So, unless there's some trick to getting that to display, I have to stick with the InsufficientCredentials exception.
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r134079501
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -115,13 +115,6 @@
background-image: url("images/guac-tricolor.png");
}
-.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
- max-width: 6in;
-}
-
--- End diff --
I'll give it a try...
---
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 #180: GUACAMOLE-355: Catch CAS Error...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/180
---
[GitHub] incubator-guacamole-client pull request #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r134063008
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -75,6 +84,10 @@ public String processUsername(String ticket) throws GuacamoleException {
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
+ catch (Throwable t) {
+ logger.error("Error validating ticket with CAS server: {}", t.getMessage());
+ throw new GuacamoleInsufficientCredentialsException("Error validating ticket with CAS server.", t, CredentialsInfo.USERNAME_PASSWORD);
--- End diff --
Internal errors really shouldn't be exposed to users of the system. If the ticket can't be validated, then that should definitely be logged for the administrator's sake, but such internals shouldn't make their way to the interface.
Why not let the interface continue rendering all failures as "Invalid login"?
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r132605562
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -75,6 +84,10 @@ public String processUsername(String ticket) throws GuacamoleException {
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
+ catch (Throwable t) {
+ logger.error("Error validating ticket with CAS server: {}", t.getMessage());
+ throw new GuacamoleInsufficientCredentialsException("Error validating ticket with CAS server.", t, CredentialsInfo.USERNAME_PASSWORD);
--- End diff --
Still not sure this is the right exception to throw. It's the one that works - that actually displays an error on the dialog box - but still seems a little weird to throw Insufficient instead of Invalid. However, throwing Invalid and getting the error would require some additional reworking of the index controller.
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r135427156
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -115,13 +115,6 @@
background-image: url("images/guac-tricolor.png");
}
-.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
- max-width: 6in;
-}
-
--- End diff --
Added back the max-width tag so that the message isn't quite so squished, but kept the shadow/box around it.
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r132605710
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -115,13 +115,6 @@
background-image: url("images/guac-tricolor.png");
}
-.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
- max-width: 6in;
-}
-
--- End diff --
So, I removed all of this because I wasn't sure that the continuation piece of this actually does anything other than make the dialog box look a little goofy. I can understand hiding the logo and version, which I did leave in place; however, this just takes off half of the borders, the shadow, and makes the box show up as an odd width (relative to the "normal" login box). I also noticed this when working on the RADIUS module - when it prompts for the additional credentials from RADIUS.
Any reason not to pull this out?
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r134062628
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -115,13 +115,6 @@
background-image: url("images/guac-tricolor.png");
}
-.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
- max-width: 6in;
-}
-
--- End diff --
The main other piece of code which leverages the authentication continuation process is the database password reset. Does the password reset still look OK with these changes?
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r134752718
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -115,13 +115,6 @@
background-image: url("images/guac-tricolor.png");
}
-.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
- max-width: 6in;
-}
-
--- End diff --
Ah, OK. Well, this doesn't look bad to me (though that text is a bit squished). Based on the screenshot, I don't have any major objection to the style changes if they are necessary.
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r134753808
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -115,13 +115,6 @@
background-image: url("images/guac-tricolor.png");
}
-.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
- max-width: 6in;
-}
-
--- End diff --
I could leave in the max-width option there and that would un-squish things a bit, if that's preferable. I think the main thing was the shadow/box effect.
---
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 #180: GUACAMOLE-355: Catch CAS Error...
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/180#discussion_r147493674
--- Diff: guacamole/src/main/webapp/app/login/styles/dialog.css ---
@@ -116,9 +116,6 @@
}
.login-ui.continuation div.login-dialog {
- border-right: none;
- border-left: none;
- box-shadow: none;
--- End diff --
Is this still necessary?
---