Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BIOMAGE-1747] Expired token #335

Merged
merged 7 commits into from
Apr 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/utils/authMiddlewares.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,13 @@ const checkAuthExpiredMiddleware = (req, res, next) => {
* Authentication middleware for Socket.IO requests. Resolves with
* the JWT claim if the authentication was successful, or rejects with
* the error otherwise.
* If ignoreExpiration is set to true, jwt.verify will not return an error
* for expired tokens.
*
* @param {*} authHeader The bearer-encoded JWT token.
* @returns Promise that resolves or rejects based on authentication status.
*/
const authenticationMiddlewareSocketIO = async (authHeader) => {
const authenticationMiddlewareSocketIO = async (authHeader, ignoreExpiration = false) => {
const poolId = await config.awsUserPoolIdPromise;
const cache = CacheSingleton.get();

Expand Down Expand Up @@ -187,6 +189,7 @@ const authenticationMiddlewareSocketIO = async (authHeader) => {
});
},
{
ignoreExpiration,
algorithms: ['RS256'],
issuer,
},
Expand Down
42 changes: 25 additions & 17 deletions src/utils/hooks/hookRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,36 @@ class hookRunner {

// run requires taskName to be present as a key in the payload
async run(payload) {
const { taskName } = payload;
this.results[taskName] = [];
// run all hooks inside a try catch because they are side-effects and as such, they should
// not break the main program execution
try {
const { taskName } = payload;
this.results[taskName] = [];

// Manual looping is done to prevent passing function in hooks[taskName] into a callback,
// which might cause scoping issues
// Manual looping is done to prevent passing function in hooks[taskName] into a callback,
// which might cause scoping issues

// Runs task specific hooks
for (let i = 0; this.hooks[taskName] !== undefined && i < this.hooks[taskName].length; i += 1) {
// calling the hooks sequentially since they may depend on each other
// eslint-disable-next-line no-await-in-loop
this.results[taskName].push(await this.hooks[taskName][i](payload));
}
// Runs task specific hooks
// eslint-disable-next-line max-len
for (let i = 0; this.hooks[taskName] !== undefined && i < this.hooks[taskName].length; i += 1) {
// calling the hooks sequentially since they may depend on each other
// eslint-disable-next-line no-await-in-loop
this.results[taskName].push(await this.hooks[taskName][i](payload));
}

// Runs hooks that apply to all tasks (assigning the results to current task)
for (let i = 0; this.hooks[ALL] !== undefined && i < this.hooks[ALL].length; i += 1) {
// eslint-disable-next-line no-await-in-loop
this.results[taskName].push(await this.hooks[ALL][i](payload));
}
// Runs hooks that apply to all tasks (assigning the results to current task)
for (let i = 0; this.hooks[ALL] !== undefined && i < this.hooks[ALL].length; i += 1) {
// eslint-disable-next-line no-await-in-loop
this.results[taskName].push(await this.hooks[ALL][i](payload));
}

logger.log(`Completed ${this.results[taskName].length} hooks for pipeline task ${taskName}`);
logger.log(`Completed ${this.results[taskName].length} hooks for pipeline task ${taskName}`);

return this.results;
return this.results;
} catch (e) {
logger.error('Error running hooks: ', e);
}
return [];
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/utils/hooks/send-notification.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ const sendNotification = async (message) => {
logger.log('No authJWT token in message, skipping status check for notifications...');
return;
}
const user = await authenticationMiddlewareSocketIO(authJWT);

const user = await authenticationMiddlewareSocketIO(authJWT, true);

const { experimentId } = message;
const statusRes = await getPipelineStatus(experimentId, process);
Expand Down
14 changes: 14 additions & 0 deletions tests/utils/hookRunner.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,18 @@ describe('HookRunner', () => {
expect(runner.results['event-2']).toEqual([fn1()]);
expect(runner.results['event-3']).toEqual([fn1()]);
});

it('does not crash when a hook throws an exception ', async () => {
const fn1 = jest.fn(() => { throw new Error(); });

const runner = new HookRunner();

runner.registerAll([fn1]);

await runner.run({ taskName: 'event-1' });

expect(fn1).toHaveBeenCalledTimes(1);
// functions that throw exceptions return an empty list
expect(runner.results['event-1']).toEqual([]);
});
});