From 7ce857f216692aa27cace484293db01187933fee Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Mon, 5 Jun 2023 22:08:43 -0700 Subject: [PATCH 1/2] Fixed middleware auth logs and code smells --- src/middleware/auth.js | 32 +++++++++++++++++++------------- src/services/metric.js | 6 +++--- tests/metric.test.js | 13 +++++++++++++ 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/middleware/auth.js b/src/middleware/auth.js index 2b78607..6df2f96 100644 --- a/src/middleware/auth.js +++ b/src/middleware/auth.js @@ -5,6 +5,7 @@ import { getComponentById } from '../services/component'; import Admin from '../models/admin'; import Component from '../models/component'; import { getRateLimit } from '../external/switcher-api-facade'; +import Logger from '../helpers/logger'; export async function auth(req, res, next) { try { @@ -12,18 +13,19 @@ export async function auth(req, res, next) { const decoded = jwt.verify(token, process.env.JWT_SECRET); const admin = await getAdminById(decoded._id); - if (!admin || !admin.active) { - throw new Error(); + if (!admin?.active) { + throw new Error('User not active'); } if (admin.token !== Admin.extractTokenPart(token)) { - throw new Error(); + throw new Error(`Invalid token for ${admin.email}`); } req.token = token; req.admin = admin; next(); - } catch (e) { + } catch (err) { + Logger.httpError('auth', 401, err.message, err); res.status(401).send({ error: 'Please authenticate.' }); } } @@ -35,20 +37,21 @@ export async function authRefreshToken(req, res, next) { const decodedRefreshToken = jwt.verify(refreshToken, process.env.JWT_SECRET); if (decodedRefreshToken.subject !== Admin.extractTokenPart(token)) { - throw new Error(); + throw new Error('Refresh code does not match'); } const decoded = await jwt.decode(token); const admin = await getAdmin({ _id: decoded._id, token: decodedRefreshToken.subject }); - if (!admin || !admin.active) { - throw new Error(); + if (!admin?.active) { + throw new Error('User not active'); } const newTokenPair = await admin.generateAuthToken(); res.jwt = newTokenPair; next(); - } catch (e) { + } catch (err) { + Logger.httpError('authRefreshToken', 401, err.message, err); res.status(401).send({ error: 'Unable to refresh token.' }); } } @@ -60,7 +63,7 @@ export async function appAuth(req, res, next) { const component = await getComponentById(decoded.component); if (component?.apihash.substring(50, component.apihash.length - 1) !== decoded.vc) { - throw new Error(); + throw new Error('Invalid API token'); } req.token = token; @@ -70,7 +73,8 @@ export async function appAuth(req, res, next) { req.environment = decoded.environment; req.rate_limit = decoded.rate_limit; next(); - } catch (e) { + } catch (err) { + Logger.httpError('appAuth', 401, err.message, err); res.status(401).send({ error: 'Invalid API token.' }); } } @@ -80,7 +84,8 @@ export async function slackAuth(req, res, next) { const token = req.header('Authorization').replace('Bearer ', ''); jwt.verify(token, process.env.SWITCHER_SLACK_JWT_SECRET); next(); - } catch (e) { + } catch (err) { + Logger.httpError('slackAuth', 401, err.message, err); res.status(401).send({ error: 'Invalid API token.' }); } } @@ -100,7 +105,7 @@ export async function appGenerateCredentials(req, res, next) { const { component, domain } = await Component.findByCredentials(req.body.domain, req.body.component, key); if (!component) { - throw new Error(); + throw new Error('Component not found'); } const rate_limit = await getRateLimit(key, component); @@ -111,7 +116,8 @@ export async function appGenerateCredentials(req, res, next) { req.environment = req.body.environment; req.rate_limit = rate_limit; next(); - } catch (e) { + } catch (err) { + Logger.httpError('appGenerateCredentials', 401, err.message, err); res.status(401).send({ error: 'Invalid token request' }); } } \ No newline at end of file diff --git a/src/services/metric.js b/src/services/metric.js index e53a3d1..9391710 100644 --- a/src/services/metric.js +++ b/src/services/metric.js @@ -46,11 +46,11 @@ export async function getStatistics(req) { let result = {}; const options = String(req.query.statistics); await Promise.all([ - options.match(/(switchers)|(all)/) ? + RegExp(/(switchers)|(all)/).exec(options) ? aggregateSwitchers(switcher.aggregate, dateGroupPattern, result) : Promise.resolve(), - options.match(/(components)|(all)/) ? + RegExp(/(components)|(all)/).exec(options) ? aggregateComponents(components.aggregate, dateGroupPattern, result) : Promise.resolve(), - options.match(/(reasons)|(all)/) ? + RegExp(/(reasons)|(all)/).exec(options) ? aggreagateReasons(reasons.aggregate, result) : Promise.resolve() ]); diff --git a/tests/metric.test.js b/tests/metric.test.js index 70395cc..4ac86dc 100644 --- a/tests/metric.test.js +++ b/tests/metric.test.js @@ -54,6 +54,19 @@ describe('Fetch overall statistics', () => { expect(response.body?.reasons).toEqual(undefined); }); + test('METRIC_SUITE - Should return only reason statistics from a given Domain', async () => { + const response = await request(app) + .get(`/metric/statistics?domainid=${domainId}&statistics=reasons`) + .set('Authorization', `Bearer ${adminMasterAccountToken}`) + .send().expect(200); + + // Response validation + expect(response.body).not.toBeNull(); + expect(response.body?.reasons.length > 0).toEqual(true); + expect(response.body?.switchers).toEqual(undefined); + expect(response.body?.components).toEqual(undefined); + }); + test('METRIC_SUITE - Should return statistics filtered by Switcher KEY', async () => { const response = await request(app) .get(`/metric/statistics?domainid=${domainId}&key=KEY_1&statistics=all`) From a936c93f36e325dbd4ee9e2eac99d0185f7373d0 Mon Sep 17 00:00:00 2001 From: petruki <31597636+petruki@users.noreply.github.com> Date: Mon, 5 Jun 2023 23:41:32 -0700 Subject: [PATCH 2/2] Removed dead code - improved logs --- src/exceptions/index.js | 16 +++++++++++----- src/middleware/auth.js | 23 +++++++---------------- tests/admin.test.js | 26 ++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 21 deletions(-) diff --git a/src/exceptions/index.js b/src/exceptions/index.js index 6c0d95e..7b75265 100644 --- a/src/exceptions/index.js +++ b/src/exceptions/index.js @@ -37,15 +37,21 @@ export class FeatureUnavailableError extends Error { } export function responseException(res, err, code, feature = undefined) { + if (process.env.SWITCHER_API_LOGGER == 'true' && feature) { + Logger.info(`Feature [${feature}]`, { log: Switcher.getLogger(feature) }); + } + + responseExceptionSilent(res, err, code, err.message); +} + +export function responseExceptionSilent(res, err, code, message) { if (process.env.SWITCHER_API_LOGGER == 'true') { Logger.httpError(err.constructor.name, err.code, err.message, err); - if (feature) { - Logger.info(`Feature [${feature}]`, { log: Switcher.getLogger(feature) }); - } } if (err.code) { - return res.status(err.code).send({ error: err.message }); + return res.status(err.code).send({ error: message }); } - res.status(code).send({ error: err.message }); + + res.status(code).send({ error: message }); } \ No newline at end of file diff --git a/src/middleware/auth.js b/src/middleware/auth.js index 6df2f96..5c419d1 100644 --- a/src/middleware/auth.js +++ b/src/middleware/auth.js @@ -5,11 +5,11 @@ import { getComponentById } from '../services/component'; import Admin from '../models/admin'; import Component from '../models/component'; import { getRateLimit } from '../external/switcher-api-facade'; -import Logger from '../helpers/logger'; +import { responseExceptionSilent } from '../exceptions'; export async function auth(req, res, next) { try { - const token = req.header('Authorization').replace('Bearer ', ''); + const token = req.header('Authorization')?.replace('Bearer ', ''); const decoded = jwt.verify(token, process.env.JWT_SECRET); const admin = await getAdminById(decoded._id); @@ -25,8 +25,7 @@ export async function auth(req, res, next) { req.admin = admin; next(); } catch (err) { - Logger.httpError('auth', 401, err.message, err); - res.status(401).send({ error: 'Please authenticate.' }); + responseExceptionSilent(res, err, 401, 'Please authenticate.'); } } @@ -51,8 +50,7 @@ export async function authRefreshToken(req, res, next) { res.jwt = newTokenPair; next(); } catch (err) { - Logger.httpError('authRefreshToken', 401, err.message, err); - res.status(401).send({ error: 'Unable to refresh token.' }); + responseExceptionSilent(res, err, 401, 'Unable to refresh token.'); } } @@ -74,8 +72,7 @@ export async function appAuth(req, res, next) { req.rate_limit = decoded.rate_limit; next(); } catch (err) { - Logger.httpError('appAuth', 401, err.message, err); - res.status(401).send({ error: 'Invalid API token.' }); + responseExceptionSilent(res, err, 401, 'Invalid API token.'); } } @@ -85,8 +82,7 @@ export async function slackAuth(req, res, next) { jwt.verify(token, process.env.SWITCHER_SLACK_JWT_SECRET); next(); } catch (err) { - Logger.httpError('slackAuth', 401, err.message, err); - res.status(401).send({ error: 'Invalid API token.' }); + responseExceptionSilent(res, err, 401, 'Invalid API token.'); } } @@ -104,10 +100,6 @@ export async function appGenerateCredentials(req, res, next) { const key = req.header('switcher-api-key'); const { component, domain } = await Component.findByCredentials(req.body.domain, req.body.component, key); - if (!component) { - throw new Error('Component not found'); - } - const rate_limit = await getRateLimit(key, component); const token = await component.generateAuthToken(req.body.environment, rate_limit); @@ -117,7 +109,6 @@ export async function appGenerateCredentials(req, res, next) { req.rate_limit = rate_limit; next(); } catch (err) { - Logger.httpError('appGenerateCredentials', 401, err.message, err); - res.status(401).send({ error: 'Invalid token request' }); + responseExceptionSilent(res, err, 401, 'Invalid token request.'); } } \ No newline at end of file diff --git a/tests/admin.test.js b/tests/admin.test.js index db85ce7..082a9ab 100644 --- a/tests/admin.test.js +++ b/tests/admin.test.js @@ -140,6 +140,32 @@ describe('Testing Admin insertion', () => { }).expect(200); }); + test('ADMIN_SUITE - Should NOT access routes - Admin not active', async () => { + // given + let admin = await Admin.findById(signedupUser).exec(); + let response = await request(app) + .post('/admin/login') + .send({ + email: admin.email, + password: '12312312312' + }).expect(200); + + // admin not active + admin.active = false; + await admin.save(); + const token = response.body.jwt.token; + + // test + await request(app) + .get('/admin/me') + .set('Authorization', `Bearer ${token}`) + .send().expect(401); + + // teardown - restore admin + admin.active = true; + await admin.save(); + }); + test('ADMIN_SUITE - Should NOT confirm access to a new Admin - Invalid access code', async () => { await request(app) .post('/admin/signup/authorization?code=INVALID_CODE')