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/10/12 13:47:12 UTC
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/197
GUACAMOLE-412: Prevent exception propagation for null UserContext
Changes to event listeners from GUACAMOLE-364 (#184) cause modules that do not have a UserContext object to fail at login. This is a potential fix for that issue - preventing the GuacamoleResourceNotFoundException from propagating down and instead just throwing a warning and debug message into the log file. Not sure if this is the preferred way to go, or if there are other implications to this?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-412
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/197.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 #197
----
commit 3445cb739eeba721d0fc17a0254d02270e2f3389
Author: Nick Couchman <vn...@apache.org>
Date: 2017-10-11T14:34:56Z
GUACAMOLE-412: Prevent exception propagation from getUserContext during event handling.
----
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/197#discussion_r146000493
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java ---
@@ -43,19 +44,19 @@
/**
* The credentials which passed authentication.
*/
- private Credentials credentials;
+ private AuthenticatedUser authenticatedUser;
/**
* Creates a new AuthenticationSuccessEvent which represents a successful
* authentication attempt with the given credentials.
*
* @param context The UserContext created as a result of successful
* authentication.
- * @param credentials The credentials which passed authentication.
+ * @param authenticatedUser The user which passed authentication.
*/
- public AuthenticationSuccessEvent(UserContext context, Credentials credentials) {
+ public AuthenticationSuccessEvent(UserContext context, AuthenticatedUser authenticatedUser) {
--- End diff --
Since `getUserContext` looks like an accessor method, it was an (obviously incorrect) assumption that a provider would create it once and return the same reference for a given authenticated user thereafter
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
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/197#discussion_r145992717
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java ---
@@ -43,19 +44,19 @@
/**
* The credentials which passed authentication.
*/
- private Credentials credentials;
+ private AuthenticatedUser authenticatedUser;
/**
* Creates a new AuthenticationSuccessEvent which represents a successful
* authentication attempt with the given credentials.
*
* @param context The UserContext created as a result of successful
* authentication.
- * @param credentials The credentials which passed authentication.
+ * @param authenticatedUser The user which passed authentication.
*/
- public AuthenticationSuccessEvent(UserContext context, Credentials credentials) {
+ public AuthenticationSuccessEvent(UserContext context, AuthenticatedUser authenticatedUser) {
--- End diff --
Is there an expectation that this getUserContext() method in the event handler will get called repeatedly?
I can revert back to the original method of storing it there - but, if we do that, we have to handle the exception that happens when it is null. I'm happy to put back my code for just logging the exception and not passing it through in this instance, if that's okay, or I'm happy to investigate other routes if those are preferred?
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
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/197#discussion_r145810320
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java ---
@@ -43,19 +44,19 @@
/**
* The credentials which passed authentication.
*/
- private Credentials credentials;
+ private AuthenticatedUser authenticatedUser;
/**
* Creates a new AuthenticationSuccessEvent which represents a successful
* authentication attempt with the given credentials.
*
* @param context The UserContext created as a result of successful
* authentication.
- * @param credentials The credentials which passed authentication.
+ * @param authenticatedUser The user which passed authentication.
*/
- public AuthenticationSuccessEvent(UserContext context, Credentials credentials) {
+ public AuthenticationSuccessEvent(UserContext context, AuthenticatedUser authenticatedUser) {
--- End diff --
Works for me. Done.
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
Posted by ceharris <gi...@git.apache.org>.
Github user ceharris commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/197#discussion_r145804691
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java ---
@@ -43,19 +44,19 @@
/**
* The credentials which passed authentication.
*/
- private Credentials credentials;
+ private AuthenticatedUser authenticatedUser;
/**
* Creates a new AuthenticationSuccessEvent which represents a successful
* authentication attempt with the given credentials.
*
* @param context The UserContext created as a result of successful
* authentication.
- * @param credentials The credentials which passed authentication.
+ * @param authenticatedUser The user which passed authentication.
*/
- public AuthenticationSuccessEvent(UserContext context, Credentials credentials) {
+ public AuthenticationSuccessEvent(UserContext context, AuthenticatedUser authenticatedUser) {
--- End diff --
You could drop `UserContext` in the constructor here and drop the associated field. Then you could implement `getUserContext` as:
```
public UserContext getUserContext() {
return authenticatedUser.getAuthenticationProvider().getUserContext(authenticatedUser);
}
```
This will avoid the need to handle the exception for the missing userContext when the event object is created.
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/197
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
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/197#discussion_r145886535
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java ---
@@ -43,19 +44,19 @@
/**
* The credentials which passed authentication.
*/
- private Credentials credentials;
+ private AuthenticatedUser authenticatedUser;
/**
* Creates a new AuthenticationSuccessEvent which represents a successful
* authentication attempt with the given credentials.
*
* @param context The UserContext created as a result of successful
* authentication.
- * @param credentials The credentials which passed authentication.
+ * @param authenticatedUser The user which passed authentication.
*/
- public AuthenticationSuccessEvent(UserContext context, Credentials credentials) {
+ public AuthenticationSuccessEvent(UserContext context, AuthenticatedUser authenticatedUser) {
--- End diff --
> `authenticatedUser.getAuthenticationProvider().getUserContext(authenticatedUser)`
I'm worried about that approach.
The `UserContext` is effectively the data side of a user's session, and typically is only created once, with that instance existing until the user logs out. If an extension implementation needed to persist some sort of state throughout the user's session, the lifecycle of that state would be tied to the `UserContext`. There's also the new `invalidate()` function to consider, which will need to be properly invoked for that short-lived `UserContext`.
This could have performance consequences down the line.
---
[GitHub] incubator-guacamole-client pull request #197: GUACAMOLE-412: Prevent excepti...
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/197#discussion_r146261874
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java ---
@@ -43,19 +44,19 @@
/**
* The credentials which passed authentication.
*/
- private Credentials credentials;
+ private AuthenticatedUser authenticatedUser;
/**
* Creates a new AuthenticationSuccessEvent which represents a successful
* authentication attempt with the given credentials.
*
* @param context The UserContext created as a result of successful
* authentication.
- * @param credentials The credentials which passed authentication.
+ * @param authenticatedUser The user which passed authentication.
*/
- public AuthenticationSuccessEvent(UserContext context, Credentials credentials) {
+ public AuthenticationSuccessEvent(UserContext context, AuthenticatedUser authenticatedUser) {
--- End diff --
Okay, reverted to having the objects in here, but took a slightly different angle on it by having the `AuthenticationSuccessEvent` constructor handle pulling all of the objects out of the `AuthenticatedUser` object, and handled catching the `GuacamoleResourceNotFoundException` exception there, instead of in `AuthenticationService`. Let me know if this looks okay, or if it should go back to the way I originally had it, catching the error in `AuthenticationService`, instead. Or if there's some other way that it needs to be done?
---