From 7eaf370e2687ab138430a2837591035277510c75 Mon Sep 17 00:00:00 2001 From: John Rassa Date: Fri, 16 Feb 2024 15:44:02 -0500 Subject: [PATCH] fix: access checker config readonly Access checker providers expect to be able to modify the config that is passed to them. With the switch to node-config, changing the configs is not allowed. Access checker now creates a copy of the access checker config to pass to the provider. Also added some additional logging and converted the config access to use `config.get` Had to update a few tests to use sinon to stub config. These tests were previously directly mutating the config and not returning it properly to its original state. --- config/default.js | 30 ++++---- .../access-checker.service.spec.ts | 72 +++++-------------- .../access-checker/access-checker.service.ts | 28 ++++++-- .../user-authentication.controller.spec.ts | 48 +++++++------ 4 files changed, 81 insertions(+), 97 deletions(-) diff --git a/config/default.js b/config/default.js index 1da853af..b460464a 100644 --- a/config/default.js +++ b/config/default.js @@ -114,21 +114,21 @@ module.exports = { */ // strategy: 'proxy-pki', - // accessChecker: { - // provider: { - // file: 'src/app/core/access-checker/providers/example.provider', - // config: { - // 'user cn string': { - // name: 'User Name', - // profileOrganization: 'User Organization', - // email: 'user@email.com', - // username: 'username', - // roles: [ 'ROLE' ] - // } - // } - // }, - // cacheExpire: 1000*60*60*24 // expiration of cache entries - // }, + accessChecker: { + cacheExpire: 1000 * 60 * 60 * 24, // expiration of cache entries + provider: { + // file: 'src/app/core/access-checker/providers/example.provider', + config: { + // 'user cn string': { + // name: 'User Name', + // profileOrganization: 'User Organization', + // email: 'user@email.com', + // username: 'username', + // roles: ['ROLE'] + // } + } + } + }, autoLogin: false, autoCreateAccounts: false, diff --git a/src/app/core/access-checker/access-checker.service.spec.ts b/src/app/core/access-checker/access-checker.service.spec.ts index a0e99728..1a7c9917 100644 --- a/src/app/core/access-checker/access-checker.service.spec.ts +++ b/src/app/core/access-checker/access-checker.service.spec.ts @@ -1,3 +1,4 @@ +import config from 'config'; import _ from 'lodash'; import { DateTime } from 'luxon'; import should from 'should'; @@ -6,7 +7,6 @@ import { createSandbox } from 'sinon'; import accessChecker from './access-checker.service'; import { CacheEntry, ICacheEntry } from './cache/cache-entry.model'; import cacheEntryService from './cache/cache-entry.service'; -import { config } from '../../../dependencies'; /** * Helpers @@ -121,25 +121,16 @@ describe('Access Checker Service:', () => { * Test functionality with the access checker provider fails */ describe('Broken Access Checker', () => { - let originalAuth; - before(() => { - originalAuth = config.auth; - // All the data is loaded, so initialize proxy-pki - config.auth.accessChecker = { - provider: { - file: 'src/app/core/access-checker/providers/failure-provider.service', - config: {} - } - }; + beforeEach(() => { + sandbox.stub(config.auth.accessChecker, 'provider').value({ + file: 'src/app/core/access-checker/providers/failure-provider.service', + config: {} + }); // Need to clear cached provider from service to ensure proper test run. accessChecker.provider = null; }); - after(() => { - config.auth = originalAuth; - }); - // Provider fails on get it('should not update the cache when the access checker provider fails', async () => { let err; @@ -197,26 +188,16 @@ describe('Access Checker Service:', () => { * Test basic functionality of a working provider */ describe('Working Access Checker', () => { - let originalAuth; - before(() => { - originalAuth = config.auth; - - // All the data is loaded, so initialize proxy-pki - config.auth.accessChecker = { - provider: { - file: 'src/app/core/access-checker/providers/example.provider', - config: provider - } - }; + beforeEach(() => { + sandbox.stub(config.auth.accessChecker, 'provider').value({ + file: 'src/app/core/access-checker/providers/example.provider', + config: provider + }); // Need to clear cached provider from service to ensure proper test run. accessChecker.provider = null; }); - after(() => { - config.auth = originalAuth; - }); - // Pull from cache it('should fail when no key is specified', async () => { let err; @@ -289,22 +270,13 @@ describe('Access Checker Service:', () => { * Test functionality with missing access checker config */ describe('Missing Access Checker Config', () => { - let originalAuth; - before(() => { - originalAuth = config.auth; - // All the data is loaded, so initialize proxy-pki - config.auth.accessChecker = { - provider: {} - }; + beforeEach(() => { + sandbox.stub(config.auth.accessChecker, 'provider').value({}); // Need to clear cached provider from service to ensure proper test run. accessChecker.provider = null; }); - after(() => { - config.auth = originalAuth; - }); - // Provider fails on get it('should throw error when no provider is configured', async () => { let err; @@ -326,24 +298,16 @@ describe('Access Checker Service:', () => { * Test functionality with missing access checker provider file */ describe('Missing Access Checker Config', () => { - let originalAuth; - before(() => { - originalAuth = config.auth; - // All the data is loaded, so initialize proxy-pki - config.auth.accessChecker = { - provider: { - file: 'invalid/path/to/provider' - } - }; + beforeEach(() => { + sandbox.stub(config.auth.accessChecker, 'provider').value({ + file: 'invalid/path/to/provider', + config: {} + }); // Need to clear cached provider from service to ensure proper test run. accessChecker.provider = null; }); - after(() => { - config.auth = originalAuth; - }); - // Provider fails on get it('should throw error when provider is configured with invalid file path', async () => { let err; diff --git a/src/app/core/access-checker/access-checker.service.ts b/src/app/core/access-checker/access-checker.service.ts index 4c1921a4..760274bf 100644 --- a/src/app/core/access-checker/access-checker.service.ts +++ b/src/app/core/access-checker/access-checker.service.ts @@ -1,9 +1,11 @@ import path from 'path'; +import config from 'config'; + import { AccessCheckerProvider } from './access-checker.provider'; import { CacheEntryDocument } from './cache/cache-entry.model'; import cacheEntryService from './cache/cache-entry.service'; -import { config } from '../../../dependencies'; +import { logger } from '../../../dependencies'; class AccessCheckerService { provider: AccessCheckerProvider; @@ -77,7 +79,7 @@ class AccessCheckerService { } getCacheExpire() { - return config.auth?.accessChecker?.cacheExpire ?? 1000 * 60 * 60 * 24; + return config.get('auth.accessChecker.cacheExpire'); } /** @@ -86,18 +88,32 @@ class AccessCheckerService { */ async getProvider(): Promise { if (!this.provider) { - const acConfig = config.auth?.accessChecker; - if (!acConfig.provider?.file) { + if (!config.has('auth.accessChecker.provider.file')) { + logger.error( + 'Invalid accessChecker provider configuration. No `provider.file` specified' + ); return Promise.reject( new Error('Invalid accessChecker provider configuration.') ); } + const providerFile = config.get( + 'auth.accessChecker.provider.file' + ); try { const { default: Provider } = await import( - path.posix.resolve(acConfig.provider.file) + path.posix.resolve(providerFile) + ); + this.provider = new Provider( + config.util.cloneDeep( + config.get('auth.accessChecker.provider.config') + ) ); - this.provider = new Provider(acConfig.provider.config); } catch (err) { + logger.error( + err, + 'Failed to load access checker provider: %s', + providerFile + ); throw new Error('Failed to load access checker provider.'); } } diff --git a/src/app/core/user/auth/user-authentication.controller.spec.ts b/src/app/core/user/auth/user-authentication.controller.spec.ts index fec3bff8..3cc87e58 100644 --- a/src/app/core/user/auth/user-authentication.controller.spec.ts +++ b/src/app/core/user/auth/user-authentication.controller.spec.ts @@ -2,7 +2,7 @@ import _ from 'lodash'; import { DateTime } from 'luxon'; import passport from 'passport'; import should from 'should'; -import { assert } from 'sinon'; +import { assert, createSandbox } from 'sinon'; import * as userAuthenticationController from './user-authentication.controller'; import { config } from '../../../../dependencies'; @@ -71,6 +71,7 @@ function cacheSpec(key): Partial { */ describe('User Auth Controller:', () => { let res; + let sandbox; before(() => { return clearDatabase(); @@ -81,9 +82,14 @@ describe('User Auth Controller:', () => { }); beforeEach(() => { + sandbox = createSandbox(); res = getResponseSpy(); }); + afterEach(() => { + sandbox.restore(); + }); + describe('signout', () => { it('should successfully redirect after logout', () => { const req = { @@ -104,16 +110,16 @@ describe('User Auth Controller:', () => { const spec = { user: localUserSpec('user1') }; let user; - before(async () => { + beforeEach(async () => { await clearDatabase(); user = await new User(spec.user).save(); //setup to use local passport - config.auth.strategy = 'local'; + sandbox.stub(config.auth, 'strategy').value('local'); passport.use(local); }); - after(() => { + afterEach(() => { return clearDatabase(); }); @@ -299,7 +305,7 @@ describe('User Auth Controller:', () => { const cache = {}; const user = {}; - before(async () => { + beforeEach(async () => { await clearDatabase(); let defers = []; defers = defers.concat( @@ -314,26 +320,24 @@ describe('User Auth Controller:', () => { ); await Promise.all(defers); - const accessCheckerConfig = { - userbypassed: { - name: 'Invalid Name', - organization: 'Invalid Org', - email: 'invalid@invalid.org', - username: 'invalid' + sandbox.stub(config.auth, 'strategy').value('proxy-pki'); + sandbox.stub(config.auth.accessChecker, 'provider').value({ + file: 'src/app/core/access-checker/providers/example.provider', + config: { + userbypassed: { + name: 'Invalid Name', + organization: 'Invalid Org', + email: 'invalid@invalid.org', + username: 'invalid' + } } - }; + }); + // All of the data is loaded, so initialize proxy-pki - config.auth.strategy = 'proxy-pki'; - config.auth.accessChecker = { - provider: { - file: 'src/app/core/access-checker/providers/example.provider', - config: accessCheckerConfig - } - }; passport.use(proxyPki); }); - after(() => { + afterEach(() => { return clearDatabase(); }); @@ -393,7 +397,7 @@ describe('User Auth Controller:', () => { // Unknown DN header it('should fail when the dn is unknown and auto create is disabled', async () => { - config.auth.autoCreateAccounts = false; + sandbox.stub(config.auth, 'autoCreateAccounts').value(false); req.headers = { [config.proxyPkiPrimaryUserHeader]: 'unknown' }; let err; try { @@ -606,7 +610,7 @@ describe('User Auth Controller:', () => { }; it('should create a new account from access checker information', async () => { - config.auth.autoCreateAccounts = true; + sandbox.stub(config.auth, 'autoCreateAccounts').value(true); req.headers = { [config.proxyPkiPrimaryUserHeader]: spec.cache.cacheOnly.key };