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/11/05 19:24:51 UTC
[GitHub] [guacamole-client] knacktim opened a new pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
knacktim opened a new pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573
This is my attempt at a solution for GUACAMOLE-680. From the discussion in this https://github.com/apache/guacamole-client/pull/346 I wanted to create a utility 'service' that would handle this use case.
I thought about making this generic and adding handlers for other events...but I don't think that is the route that we want to go. This is a specific use case for the user logout case so it should be handled by the service that handles those actions. The AngularJS broadcast system should be good enough for other situations.
Example usage:
```javascript
angular.module('someModule').run(['authenticationService',
function logoutListener(authenticationService) {
authenticationService.registerLogoutHandler(() => {
// Do your actions that are needed here.
})
}]);
// Ensure the openid module is loaded along with the rest of the app
angular.module('index').requires.push('someModule');
```
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-816894928
> In that case, what about:
>
> * Leverage `Guacamole.Event.Target`.
> * In the event implementation that extends `Guacamole.Event`, expose the logout promise as a property.
>
> Implementations that hook into the logout event would then be able to layer their own async operations onto the promise. As long as that default reload operation is added to the promise only after dispatching the event, things would work as you describe.
I am hoping to get the time to refactor to this implementation in the next week or so!
--
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] knacktim edited a comment on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim edited a comment on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-723156343
@necouchman
> * I wonder if there is some other/better way to handle the case where a reload/redirect by a login handler causes the remaining ones to not be processed. It seems like just noting that in the documentation of the function could be confusing when it gets to the admin level and one or more modules (SSO logout, for example) causes another one to fail to actually run. I don't have any great ideas for how to handle that, so curious if we can brainstorm up some possible alternatives.
I couldn't find anything in the AngularJS API to handle 'guaranteed delivery' of events. Even using something like the Broadcast Channel API would still break on the reload. I felt that this was the most straightforward way to accomplish this.
> * I don't know if there's any level of sanity checks we should do on the registered functions to make sure that they don't get abused - purposely or inadvertently - to do bad things to the users' browsers? Again, I don't have any great ideas, here, just seems like blindly running whatever function is passed in might be asking for exploitation down the line somewhere...
Is there a fundamental difference between what I am doing here and someone implementing a guacamole extension using a `$rootScope.on('blah', nefariousFunction)`? Unless I am thinking about this incorrectly?
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-840894086
@necouchman I will try to get some more time on this next Monday :)
--
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-786280366
Yep!
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-841433723
@mike-jumper Does the Guacamole.Event need to actually know about the promises at all? I can't wrap my head around why they would be different from a normal Guacamole.Event
--
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 edited a comment on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper edited a comment on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-850779391
Waaaait a minute ...
> >
> > So, the idea is that implementations need to be able to (somehow) hook into the promise stack of the logout process to ensure that any async operations they may need to perform during logout are allowed to complete before the default behavior (reloading the page) breaks those async operations?
> >
>
> Yep!
The code in question is:
https://github.com/apache/guacamole-client/blob/754e9649f1fa0ba225ee42b56ded64bc283d17df/guacamole/src/main/webapp/app/navigation/directives/guacUserMenu.js#L145-L148
If things were updated such that:
* The `guacLogout` event behaves as documented.
* `$route.reload()` is not called.
* Upon logout, the current contents of the browser window are replaced with a generic "You have been logged out" message (rather than attempting to reload and thus reshow a login prompt).
would that solve things?
I ask because the above would avoid extensive restructuring of the webapp's current handling of events, would avoid having to define a whole new type of event, and would also happen to solve the issue with OpenID, SAML, and others where clicking "Logout" appears to have no effect because the user is almost instantly SSO'd back in.
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-786269567
So, the idea is that implementations need to be able to (somehow) hook into the promise stack of the logout process to ensure that any async operations they may need to perform during logout are allowed to complete before the default behavior (reloading the page) breaks those async operations?
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-762247676
I have been poking at this when I get time, but I haven't found a good way to do a synchronous REST call.
----------------------------------------------------------------
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] lchanouha commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
lchanouha commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-744092590
For CAS, we must assure that function is called **after** guacamole session is destroyed.
Because next step in protocol is to redirect to
$window.location.href = config['cas_authorization_endpoint"] + "/logout"
Could the logic be inverted ?
```
service.logout
=> delete cookie
=> call requestService({ method: 'DELETE', url: 'api/tokens/' + token })
=> on requestService finished, call return $q.all(getLogoutHandlerPromises())
```
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-763079018
@necouchman
> I wonder if there is some other/better way to handle the case where a reload/redirect by a login handler causes the remaining ones to not be processed. It seems like just noting that in the documentation of the function could be confusing when it gets to the admin level and one or more modules (SSO logout, for example) causes another one to fail to actually run. I don't have any great ideas for how to handle that, so curious if we can brainstorm up some possible alternatives.
I still don't have a good way to tackle this -- I am open to suggestions.
After looking more at the documentation of $q.all() is that the following updates will get this to where we want this to be. The only problem that I see is that if ANY of the promises call `reject()` then it is unclear to me if $q.all will stop processing. (at least that is how I understood [this documentation](https://docs.angularjs.org/api/ng/service/$q#all:~:text=If%20any%20of%20the%20promises%20is,rejected%20with%20the%20same%20rejection%20value))
I updated the follow implementation for [GUACAMOLE-519] to use the new handler hooks.
```javascript
angular.module('openid').run(['$routeParams', 'authenticationService',
function openIDLogoutListener($routeParams, authenticationService) {
let config = {} // TODO get these from backend
authenticationService.registerLogoutHandler((resolve, reject) => {
if ($routeParams && Object.prototype.hasOwnProperty.call($routeParams, 'id_token')) {
$.get({
url: config['openid_logout_endpoint'],
dataType: 'application/json',
async: false,
data: {
client_id: config['openid_client_id'],
id_token_hint: $routeParams.id_token
},
success: function(resp) {
resolve("Success");
},
error: function() {
reject("Error when trying to log out");
}
})
}
})
}]);
// Ensure the openid module is loaded along with the rest of the app
angular.module('index').requires.push('openid');
```
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-786011286
```js
angular.module('openid').run(['$routeParams', 'authenticationService',
function openIDLogoutListener($routeParams, authenticationService) {
let config = {} // TODO get these from backend
authenticationService.registerLogoutHandler((resolve, reject) => {
if ($routeParams && Object.prototype.hasOwnProperty.call($routeParams, 'id_token')) {
$.get({
url: config['openid_logout_endpoint'],
dataType: 'application/json',
async: false,
data: {
client_id: config['openid_client_id'],
id_token_hint: $routeParams.id_token
},
success: function(resp) {
resolve("Success");
},
error: function() {
reject("Error when trying to log out");
}
})
}
})
}]);
```
@mike-jumper
> I'd think either implementation would allow an extension to reliably hook into application logout. Is your concern that you want to be able to manually reject the promise returned by `authenticationService.logout()` from within a logout hook?
Looking at the above use case as an example. I am focusing on this part here:
https://github.com/apache/guacamole-client/blob/5319f9011a0cca5b024709adb9a512c2d1070e90/guacamole-common-js/src/main/webapp/modules/Event.js#L217-L230
If users do any async REST calls inside of a `relevantListener` I need the `dispatch` method to return a Promise.all() promise to the caller so that if needed, they can trigger a `.then()` after they are sure all of the listeners have been handled the event. Otherwise I think there is a risk that we propagate the event, then the page could get refreshed before we are sure that the handler was finished.
I don't think we care if the action was `resolve` or `reject`, we just need a hook to tell us when the listener is done.
It is totally possible that I am overthinking this :zany_face:
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-723156343
> * I wonder if there is some other/better way to handle the case where a reload/redirect by a login handler causes the remaining ones to not be processed. It seems like just noting that in the documentation of the function could be confusing when it gets to the admin level and one or more modules (SSO logout, for example) causes another one to fail to actually run. I don't have any great ideas for how to handle that, so curious if we can brainstorm up some possible alternatives.
I couldn't find anything in the AngularJS API to handle 'guaranteed delivery' of events. Even using something like the Broadcast Channel API would still break on the reload. I felt that this was the most straightforward way to accomplish this.
> * I don't know if there's any level of sanity checks we should do on the registered functions to make sure that they don't get abused - purposely or inadvertently - to do bad things to the users' browsers? Again, I don't have any great ideas, here, just seems like blindly running whatever function is passed in might be asking for exploitation down the line somewhere...
Is there a fundamental difference between what I am doing here and someone implementing a guacamole extension using a `$rootScope.on('blah', nefariousFunction)`? Unless I am thinking about this incorrectly?
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-851395829
Yes, I believe that would work!
The only risk with doing that is that we need to make sure to clean up the state of the client properly. Since the reload was doing that for us. There will also need to be a button to log in again so that a user can get back to the login page.
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-840846652
@knacktim Any further progress, here?
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-722628449
Thanks @knacktim - Overall I like this approach, but am curious what @mike-jumper has to say about it.
I do have a couple of concerns:
* I wonder if there is some other/better way to handle the case where a reload/redirect by a login handler causes the remaining ones to not be processed. It seems like just noting that in the documentation of the function could be confusing when it gets to the admin level and one or more modules (SSO logout, for example) causes another one to fail to actually run. I don't have any great ideas for how to handle that, so curious if we can brainstorm up some possible alternatives.
* I don't know if there's any level of sanity checks we should do on the registered functions to make sure that they don't get abused - purposely or inadvertently - to do bad things to the users' browsers? Again, I don't have any great ideas, here, just seems like blindly running whatever function is passed in might be asking for exploitation down the line somewhere...
----------------------------------------------------------------
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-860383224
--
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 edited a comment on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper edited a comment on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-850239126
That level of the API doesn't, no, but part of the idea behind `Guacamole.Event` is that it may be extended to produce other more specific events.
For example, #613 is partial migration toward using `Guacamole.Event` which updates `Guacamole.Mouse` to subclass `Guacamole.Event.Target` for event handling. The `Guacamole.Mouse` implementation defines its own subclass of `Guacamole.Event` (`Guacamole.Mouse.MouseEvent`) that exposes the mouse state.
`Guacamole.Event` is not itself aware of mouse state, but subclass of `Guacamole.Event` specific to the mouse is. The same could go for a subclass of `Guacamole.Event` that is defined by the webapp and intended to be associated with the webapp's various asynchronous operations.
--
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-762249963
> For CAS, we must assure that function is called **after** guacamole session is destroyed.
> Because next step in protocol is to redirect to
> $window.location.href = config['cas_authorization_endpoint"] + "/logout"
>
> Could the logic be inverted ? : do local stuff then remote SSO stuff
>
> ```
> service.logout
> => delete cookie
> => call requestService({ method: 'DELETE', url: 'api/tokens/' + token })
> => on requestService finished, call return $q.all(getLogoutHandlerPromises())
> ```
My proposed change for this is the following:
```javascript
return requestService({
method: 'DELETE',
url: 'api/tokens/' + token
}).then(() => {
return $q.all(getLogoutHandlerPromises());
})
```
But I need to finish testing on this to ensure its doing what I think it is.
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-724884087
Used this to implement [GUACAMOLE-519](https://issues.apache.org/jira/browse/GUACAMOLE-519). Here is what it looked like:
```
angular.module('openid').run(['$routeParams', 'authenticationService',
function openIDLogoutListener($routeParams, authenticationService) {
let config = {} // TODO get these from backend
authenticationService.registerLogoutHandler(() => {
if ($routeParams && Object.prototype.hasOwnProperty.call($routeParams, 'id_token')) {
$.get({
url: config['openid_logout_endpoint'],
dataType: 'application/json',
async: false,
data: {
client_id: config['openid_client_id'],
id_token_hint: $routeParams.id_token
},
success: function(resp) {
console.log("Success");
},
error: function() {
console.error("Error when trying to log out");
}
})
}
})
}]);
// Ensure the openid module is loaded along with the rest of the app
angular.module('index').requires.push('openid');
```
----------------------------------------------------------------
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-826389806
> Am I missing some other promise API that is already included in Guacamole-client?
While there _is_ a promise API included within the Guacamole webapp via AngularJS (`$q`), there isn't one included with guacamole-common-js, and the core guacamole-common-js shouldn't rely on the promise API if this would affect compatibility with older browsers.
Shouldn't the change leveraging the promise be at the webapp level rather than in guacamole-common-js? The former is a full-fledged webapp that can rely on bundling any libraries it requires, while the latter is a reusable library that should not be too tightly coupled with the needs of the webapp.
If modifying guacamole-common-js to support promises if the implementation wishes to use promises, this could make sense, but it shouldn't require them.
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-830426970
This is looking better, yes. My feedback on this latest iteration:
* It's seems odd to me to subclass `Guacamole.Event.Target` rather than `Guacamole.Event`.
* For external code to be able to hook into the dispatched `Guacamole.Event`, there will need to be some sort of global `Guacamole.Event.Target` that extensions, etc. can rely on existing.
* It's a bit disconcerting to me that guaranteed is in quotes here:
> Any event handlers that return a Promise will be 'Guaranteed' to finish before the logout refresh happens.
;)
I think it would make more sense to subclass `Guacamole.Event`, provided some mechanism on that subclass to allow implementations to associate the promises of their own async operations with the event, and then handle `$q.all()` or similar in the part of the code that owns the event after `dispatch()` has finished.
For example:
1. Magical async subclass of `Guacamole.Event` is dispatched via `dispatch()`. Perhaps the subclass has an array of associated promises. Perhaps it keeps that hidden and provides a function for associating operations. Perhaps it exposes a single promise that can be replaced with another to form an arbitrary chain. In any case, it provides some API for associating the promises of additional async operations with the event.
2. A listener associates its own additional async operation promise with the event using the above mechanism.
3. `dispatch()` returns and the code that originally invoked `dispatch()` inspects the contents of that event and deals with the relevant promises.
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-850779391
Waaaait a minute ...
> >
> > So, the idea is that implementations need to be able to (somehow) hook into the promise stack of the logout process to ensure that any async operations they may need to perform during logout are allowed to complete before the default behavior (reloading the page) breaks those async operations?
> >
>
> Yep!
The code in question is:
https://github.com/apache/guacamole-client/blob/754e9649f1fa0ba225ee42b56ded64bc283d17df/guacamole/src/main/webapp/app/navigation/directives/guacUserMenu.js#L145-L148
If things were updated such that:
* The `guacLogout` event behaves as documented.
* `$location.reload()` is not called.
* Upon logout, the current contents of the browser window are replaced with a generic "You have been logged out" message (rather than attempting to reload and thus reshow a login prompt).
would that solve things?
I ask because the above would avoid extensive restructuring of the webapp's current handling of events, would avoid having to define a whole new type of event, and would solve the issue with OpenID, SAML, and others where clicking "Logout" appears to have no effect because the user is almost instantly SSO'd back in.
--
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-744494384
> Could the logic be inverted ? : do local stuff then remote SSO stuff
I will take a stab at this
----------------------------------------------------------------
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] knacktim edited a comment on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim edited a comment on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-724884087
Used this to implement [GUACAMOLE-519](https://issues.apache.org/jira/browse/GUACAMOLE-519). Here is what it looked like:
```javascript
angular.module('openid').run(['$routeParams', 'authenticationService',
function openIDLogoutListener($routeParams, authenticationService) {
let config = {} // TODO get these from backend
authenticationService.registerLogoutHandler(() => {
if ($routeParams && Object.prototype.hasOwnProperty.call($routeParams, 'id_token')) {
$.get({
url: config['openid_logout_endpoint'],
dataType: 'application/json',
async: false,
data: {
client_id: config['openid_client_id'],
id_token_hint: $routeParams.id_token
},
success: function(resp) {
console.log("Success");
},
error: function() {
console.error("Error when trying to log out");
}
})
}
})
}]);
// Ensure the openid module is loaded along with the rest of the app
angular.module('index').requires.push('openid');
```
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-785192211
@mike-jumper I am going to try and work on these fixes soon. (Life is busy for me atm)
> Perhaps it would be better to establish a generic, non-Angular, global object defined by the webapp that could be used as the overall Guacamole web application event target? That would allow extensions to hook into and deal with various events independently of AngularJS, and would provide the webapp with a more generic and friendly hook mechanism.
I like this idea. I took a look at the Event Target that you linked above and I think I would run into the same shortcoming that I had in my use case of making a REST call in the listener function. I don't want the listener function to be marked as 'done' until the REST call out to the Identity Provider is done and I call some kind of 'resolve' or 'success' callback.
I think it is feasible for me to do some refactoring on the event target to use the [Promises API](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise). Does that sound like what you had in mind?
----------------------------------------------------------------
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 edited a comment on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper edited a comment on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-850779391
Waaaait a minute ...
> >
> > So, the idea is that implementations need to be able to (somehow) hook into the promise stack of the logout process to ensure that any async operations they may need to perform during logout are allowed to complete before the default behavior (reloading the page) breaks those async operations?
> >
>
> Yep!
The code in question is:
https://github.com/apache/guacamole-client/blob/754e9649f1fa0ba225ee42b56ded64bc283d17df/guacamole/src/main/webapp/app/navigation/directives/guacUserMenu.js#L145-L148
If things were updated such that:
* The `guacLogout` event behaves as documented.
* `$route.reload()` is not called.
* Upon logout, the current contents of the browser window are replaced with a generic "You have been logged out" message (rather than attempting to reload and thus reshow a login prompt).
would that solve things?
I ask because the above would avoid extensive restructuring of the webapp's current handling of events, would avoid having to define a whole new type of event, and would solve the issue with OpenID, SAML, and others where clicking "Logout" appears to have no effect because the user is almost instantly SSO'd back in.
--
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 closed pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper closed pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-785454021
> ...
>
> I like this idea. I took a look at the Event Target that you linked above and I think I would run into the same shortcoming that I had in my use case of making a REST call in the listener function. I don't want the listener function to be marked as 'done' until the REST call out to the Identity Provider is done and I call some kind of 'resolve' or 'success' callback.
>
> I think it is feasible for me to do some refactoring on the event target to use the [Promises API](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise). Does that sound like what you had in mind?
I'd think either implementation would allow an extension to reliably hook into application logout. Is your concern that you want to be able to manually reject the promise returned by `authenticationService.logout()` from within a logout hook?
----------------------------------------------------------------
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-850239126
That level of the API doesn't, no, but part of the idea behind `Guacamole.Event` is that it may be extended to produce other more specific events.
For example, #613 is partial migration toward using `Guacamole.Event` which updates `Guacamole.Mouse` to subclass `Guacamole.Event.Target` for event handling. The `Guacamole.Mouse` implementation defines its own subclass of `Guacamole.Event` (`Guacamole.Mouse.MouseEvent`) that exposes the mouse state.
`Guacamole.Event` is not itself aware of mouse state, but subclass of `Guacamole.Event` specific to the mouse is. The same could go for a subclass of `Guacamole.Event` intended to be associated with asynchronous operations.
--
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] lchanouha edited a comment on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
lchanouha edited a comment on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-744092590
For CAS, we must assure that function is called **after** guacamole session is destroyed.
Because next step in protocol is to redirect to
$window.location.href = config['cas_authorization_endpoint"] + "/logout"
Could the logic be inverted ? : do local stuff then remote SSO stuff
```
service.logout
=> delete cookie
=> call requestService({ method: 'DELETE', url: 'api/tokens/' + token })
=> on requestService finished, call return $q.all(getLogoutHandlerPromises())
```
----------------------------------------------------------------
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-827847417
@mike-jumper Is this implementation closer to what you had in mind? I have more testing to do, just wanted to make sure I am on the right track :)
--
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-825164502
I have a new implementation that should satisfy all of the review comments...but I am having a problem with the jasmine spec.
Error:
```
-------------------------------------------------------
J A S M I N E S P E C S
-------------------------------------------------------
[INFO]
Guacamole.Event
when an event is dispatched
should invoke the legacy handler for matching events <<< FAILURE!
* ReferenceError: Can't find variable: Promise in http://localhost:41497/src/all.min.js (line 57)
should invoke all listeners for matching events <<< FAILURE!
* ReferenceError: Can't find variable: Promise in http://localhost:41497/src/all.min.js (line 57)
should not invoke any listeners for non-matching events <<< FAILURE!
* ReferenceError: Can't find variable: Promise in http://localhost:41497/src/all.min.js (line 57)
should not invoke any listeners that have been removed <<< FAILURE!
* ReferenceError: Can't find variable: Promise in http://localhost:41497/src/all.min.js (line 57)
when listeners are removed
should return whether a listener is successfully removed
Results: 5 specs, 4 failures, 0 pending
```
I believe it may be related to PhantomJS not supporting the `Promise` API. There are some solutions on this thread here: https://github.com/ariya/phantomjs/issues/12401
Am I missing some other promise API that is already included in Guacamole-client?
--
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-826389806
> Am I missing some other promise API that is already included in Guacamole-client?
While there _is_ a promise API included within the Guacamole webapp via AngularJS (`$q`), there isn't one included with guacamole-common-js, and the core guacamole-common-js shouldn't rely on the promise API if this would affect compatibility with older browsers.
Shouldn't the change leveraging the promise be at the webapp level rather than in guacamole-common-js? The former is a full-fledged webapp that can rely on bundling any libraries it requires, while the latter is a reusable library that should not be too tightly coupled with the needs of the webapp.
If modifying guacamole-common-js to support promises if the implementation wishes to use promises, this could make sense, but it shouldn't require them.
--
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] knacktim commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
knacktim commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-826766938
@mike-jumper It appears I misunderstood what you originally asked for. I'll take another crack at this today :)
--
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] michaelmiklis commented on pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
michaelmiklis commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-762194278
Any update on this? I'm also highly interested in this feature. We need that for the SAML authentication extension to destroy the session on the idp.
----------------------------------------------------------------
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 #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#discussion_r579757092
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -68,6 +69,17 @@ angular.module('auth').factory('authenticationService', ['$injector',
*/
var AUTH_STORAGE_KEY = 'GUAC_AUTH';
+ /**
+ * List of Logout handlers that are ensured to be run before this service
+ * forces the page to be reloaded.
+ *
+ * Warning: If a handler has a page reload or redirect in it this can cause
+ * handlers not to be run.
+ *
+ * @type Array of Angular promises
Review comment:
This should be `@type Promise[]` or `@type Promise.<WhateverItResolvesWith>[]` if the type of resolution is known. See: https://github.com/apache/guacamole-client/blob/0091bb1aea14c567c8166f0ed8eadf7c31b6bd6e/guacamole/src/main/webapp/app/rest/services/schemaService.js#L45
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService', ['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback) or
+ * reject('reason') (failure callback). This will allow you to do async
+ * operations inside the handlerFunction and only trigger success/failure
+ * when your operations are finished.
+ *
+ */
+ service.registerLogoutHandler = function registerLogoutHandler(handlerFunction) {
+ logoutHandlers.push(handlerFunction)
+ }
+
+ /**
+ * This private function transforms all of the registered logout handlers into
+ * promises which will allow the use of $q.all() or Promise.all() to ensure that
+ * each handler is activated.
+ */
+ let getLogoutHandlerPromises = function getLogoutHandlerPromises() {
+ let promises = [];
+
+ for (x in logoutHandlers) {
+ let handlerFunction = logoutHandlers[x];
+ promises.push(
+ $q((resolve, reject) => {
+ try {
+ handlerFunction(resolve, reject);
+ } catch (e) {
+ reject(e);
+ }
Review comment:
Won't an object thrown within a promise handler implicitly reject the promise with that object?
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService', ['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback) or
+ * reject('reason') (failure callback). This will allow you to do async
+ * operations inside the handlerFunction and only trigger success/failure
+ * when your operations are finished.
+ *
+ */
+ service.registerLogoutHandler = function registerLogoutHandler(handlerFunction) {
+ logoutHandlers.push(handlerFunction)
+ }
+
+ /**
+ * This private function transforms all of the registered logout handlers into
+ * promises which will allow the use of $q.all() or Promise.all() to ensure that
+ * each handler is activated.
+ */
+ let getLogoutHandlerPromises = function getLogoutHandlerPromises() {
+ let promises = [];
+
+ for (x in logoutHandlers) {
Review comment:
`x` here is implicitly global.
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService', ['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback) or
+ * reject('reason') (failure callback). This will allow you to do async
+ * operations inside the handlerFunction and only trigger success/failure
+ * when your operations are finished.
+ *
Review comment:
What's this blank line about? ;)
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService', ['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback) or
+ * reject('reason') (failure callback). This will allow you to do async
+ * operations inside the handlerFunction and only trigger success/failure
+ * when your operations are finished.
+ *
+ */
+ service.registerLogoutHandler = function registerLogoutHandler(handlerFunction) {
+ logoutHandlers.push(handlerFunction)
+ }
+
+ /**
+ * This private function transforms all of the registered logout handlers into
+ * promises which will allow the use of $q.all() or Promise.all() to ensure that
+ * each handler is activated.
+ */
+ let getLogoutHandlerPromises = function getLogoutHandlerPromises() {
+ let promises = [];
+
+ for (x in logoutHandlers) {
+ let handlerFunction = logoutHandlers[x];
+ promises.push(
+ $q((resolve, reject) => {
Review comment:
Unfortunately, we can't use arrow expressions or `let` within the Guacamole source for compatibility. We'll need to rely on the more traditional `var` and `function`.
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService', ['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback) or
+ * reject('reason') (failure callback). This will allow you to do async
+ * operations inside the handlerFunction and only trigger success/failure
+ * when your operations are finished.
Review comment:
There is a handy and specific JSDoc syntax for callback functions:
https://jsdoc.app/tags-param.html#callback-functions
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService', ['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is triggered.
Review comment:
I recommend avoiding the temptation to document all functions as "This function ..." and instead write as if that has already been said. This is the general convention for documenting function behavior, and is the convention we follow elsewhere.
For example, instead of:
```js
/**
* This function accepts a duration and beeps for that amount of time.
*
* @param {Number} duration
* The number of milliseconds that the beep should last.
*/
var beep = function beep(duration) {
...
};
```
Use:
```js
/**
* Beeps for the specified amount of time.
*
* @param {Number} duration
* The number of milliseconds that the beep should last.
*/
var beep = function beep(duration) {
...
};
```
----------------------------------------------------------------
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 pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#issuecomment-786282279
In that case, what about:
* Leverage `Guacamole.Event.Target`.
* In the event implementation that extends `Guacamole.Event`, expose the logout promise as a property.
Implementations that hook into the logout event would then be able to layer their own async operations onto the promise. As long as that default reload operation is added to the promise only after dispatching the event, things would work as you describe.
----------------------------------------------------------------
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