Skip to content

Commit

Permalink
fix: access checker config readonly
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jrassa
jrassa committed Feb 16, 2024
1 parent b90634c commit 7eaf370
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 97 deletions.
30 changes: 15 additions & 15 deletions config/default.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: '[email protected]',
// 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: '[email protected]',
// username: 'username',
// roles: ['ROLE']
// }
}
}
},

autoLogin: false,
autoCreateAccounts: false,
Expand Down
72 changes: 18 additions & 54 deletions src/app/core/access-checker/access-checker.service.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import config from 'config';
import _ from 'lodash';
import { DateTime } from 'luxon';
import should from 'should';
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
28 changes: 22 additions & 6 deletions src/app/core/access-checker/access-checker.service.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -77,7 +79,7 @@ class AccessCheckerService {
}

getCacheExpire() {
return config.auth?.accessChecker?.cacheExpire ?? 1000 * 60 * 60 * 24;
return config.get('auth.accessChecker.cacheExpire');
}

/**
Expand All @@ -86,18 +88,32 @@ class AccessCheckerService {
*/
async getProvider(): Promise<AccessCheckerProvider> {
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<string>(
'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.');
}
}
Expand Down
48 changes: 26 additions & 22 deletions src/app/core/user/auth/user-authentication.controller.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -71,6 +71,7 @@ function cacheSpec(key): Partial<ICacheEntry> {
*/
describe('User Auth Controller:', () => {
let res;
let sandbox;

before(() => {
return clearDatabase();
Expand All @@ -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 = {
Expand All @@ -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();
});

Expand Down Expand Up @@ -299,7 +305,7 @@ describe('User Auth Controller:', () => {
const cache = {};
const user = {};

before(async () => {
beforeEach(async () => {
await clearDatabase();
let defers = [];
defers = defers.concat(
Expand All @@ -314,26 +320,24 @@ describe('User Auth Controller:', () => {
);
await Promise.all(defers);

const accessCheckerConfig = {
userbypassed: {
name: 'Invalid Name',
organization: 'Invalid Org',
email: '[email protected]',
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: '[email protected]',
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();
});

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
};
Expand Down

0 comments on commit 7eaf370

Please sign in to comment.