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 2021/02/21 06:22:01 UTC
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #573: GUACAMOLE-680: Implement a guaranteed logout handler hook
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