From 5e4216e672c0fd95917b8d20b24a81001798c56a Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 11:59:11 +0900 Subject: [PATCH 01/11] test: add preliminary test setup for AD auth --- test/testActiveDirectoryAuth.test.js | 66 ++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 test/testActiveDirectoryAuth.test.js diff --git a/test/testActiveDirectoryAuth.test.js b/test/testActiveDirectoryAuth.test.js new file mode 100644 index 000000000..338bada2b --- /dev/null +++ b/test/testActiveDirectoryAuth.test.js @@ -0,0 +1,66 @@ +const chai = require('chai'); +const sinon = require('sinon'); +const proxyquire = require('proxyquire'); +const expect = chai.expect; + +describe('ActiveDirectory auth method', () => { + let ldapStub, dbStub, passportStub, strategyCallback; + + const newConfig = JSON.stringify({ + authentication: [ + { + type: 'ActiveDirectory', + enabled: true, + adminGroup: 'test-admin-group', + userGroup: 'test-user-group', + domain: 'test.com', + adConfig: { + url: 'ldap://test-url', + baseDN: 'dc=test,dc=com', + searchBase: 'ou=users,dc=test,dc=com', + }, + }, + ], + }); + + beforeEach(() => { + ldapStub = { + isUserInAdGroup: sinon.stub(), + }; + + dbStub = { + updateUser: sinon.stub(), + }; + + passportStub = { + use: sinon.stub(), + serializeUser: sinon.stub(), + deserializeUser: sinon.stub(), + }; + + const fsStub = { + existsSync: sinon.stub().returns(true), + readFileSync: sinon.stub().returns(newConfig), + }; + + const config = proxyquire('../src/config', { + fs: fsStub, + }); + + const { configure } = proxyquire('../src/service/passport/activeDirectory', { + './ldaphelper': ldapStub, + '../../db': dbStub, + '../../config': config, + 'passport-activedirectory': function (options, callback) { + strategyCallback = callback; + return { + name: 'ActiveDirectory', + authenticate: () => {}, + }; + }, + }); + + configure(passportStub); + }); + +}); From 9f752da613a73b4833d9f6932c5f6dbe94045158 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 12:00:42 +0900 Subject: [PATCH 02/11] test: ad auth admin --- test/testActiveDirectoryAuth.test.js | 32 ++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/testActiveDirectoryAuth.test.js b/test/testActiveDirectoryAuth.test.js index 338bada2b..ad8c5b0dd 100644 --- a/test/testActiveDirectoryAuth.test.js +++ b/test/testActiveDirectoryAuth.test.js @@ -63,4 +63,36 @@ describe('ActiveDirectory auth method', () => { configure(passportStub); }); + it('should authenticate a valid user and mark them as admin', async () => { + const mockReq = {}; + const mockProfile = { + _json: { + sAMAccountName: 'test-user', + mail: 'test@test.com', + userPrincipalName: 'test@test.com', + title: 'Test User', + }, + displayName: 'Test User', + }; + + ldapStub.isUserInAdGroup + .onCall(0).resolves(true) + .onCall(1).resolves(true); + + const done = sinon.spy(); + + await strategyCallback(mockReq, mockProfile, {}, done); + + expect(done.calledOnce).to.be.true; + const [err, user] = done.firstCall.args; + expect(err).to.be.null; + expect(user).to.have.property('username', 'test-user'); + expect(user).to.have.property('email', 'test@test.com'); + expect(user).to.have.property('displayName', 'Test User'); + expect(user).to.have.property('admin', true); + expect(user).to.have.property('title', 'Test User'); + + expect(dbStub.updateUser.calledOnce).to.be.true; + }); + }); From c495d36e0b93c8937a2c76d750f91490fb765037 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 12:01:08 +0900 Subject: [PATCH 03/11] test: add more ad tests --- test/testActiveDirectoryAuth.test.js | 49 ++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/testActiveDirectoryAuth.test.js b/test/testActiveDirectoryAuth.test.js index ad8c5b0dd..76614d197 100644 --- a/test/testActiveDirectoryAuth.test.js +++ b/test/testActiveDirectoryAuth.test.js @@ -95,4 +95,53 @@ describe('ActiveDirectory auth method', () => { expect(dbStub.updateUser.calledOnce).to.be.true; }); + it('should fail if user is not in user group', async () => { + const mockReq = {}; + const mockProfile = { + _json: { + sAMAccountName: 'bad-user', + mail: 'bad@test.com', + userPrincipalName: 'bad@test.com', + title: 'Bad User' + }, + displayName: 'Bad User' + }; + + ldapStub.isUserInAdGroup.onCall(0).resolves(false); + + const done = sinon.spy(); + + await strategyCallback(mockReq, mockProfile, {}, done); + + expect(done.calledOnce).to.be.true; + const [err, user] = done.firstCall.args; + expect(err).to.include('not a member'); + expect(user).to.be.null; + + expect(dbStub.updateUser.notCalled).to.be.true; + }); + + it('should handle LDAP errors gracefully', async () => { + const mockReq = {}; + const mockProfile = { + _json: { + sAMAccountName: 'error-user', + mail: 'err@test.com', + userPrincipalName: 'err@test.com', + title: 'Whoops' + }, + displayName: 'Error User' + }; + + ldapStub.isUserInAdGroup.rejects(new Error('LDAP error')); + + const done = sinon.spy(); + + await strategyCallback(mockReq, mockProfile, {}, done); + + expect(done.calledOnce).to.be.true; + const [err, user] = done.firstCall.args; + expect(err.message).to.equal('LDAP error'); + expect(user).to.be.null; + }); }); From 7a4ffee56fed98afb8d99d8878cae1049ec2c90b Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 12:01:28 +0900 Subject: [PATCH 04/11] test: add preliminary oidc auth tests --- test/testOidcAuth.test.js | 131 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 test/testOidcAuth.test.js diff --git a/test/testOidcAuth.test.js b/test/testOidcAuth.test.js new file mode 100644 index 000000000..cff45eea9 --- /dev/null +++ b/test/testOidcAuth.test.js @@ -0,0 +1,131 @@ +const chai = require('chai'); +const expect = chai.expect; +const sinon = require('sinon'); +const proxyquire = require('proxyquire'); + +describe('OIDC auth method', () => { + let dbStub, passportStub, discoveryStub, fetchUserInfoStub, StrategyStub, strategyCallback; + + const configStub = { + getAuthMethods: () => ([ + { + type: 'openidconnect', + oidcConfig: { + issuer: 'https://test.com', + clientID: 'test-client-id', + clientSecret: 'test-client-secret', + callbackURL: 'http://localhost:3000/auth/callback', + scope: 'openid profile email', + } + } + ]) + }; + + const mockUserInfo = { + sub: 'test-sub', + email: 'test@test.com', + name: 'Test User', + }; + + beforeEach(async () => { + dbStub = { + findUserByOIDC: sinon.stub(), + createUser: sinon.stub(), + }; + + passportStub = { + use: sinon.stub(), + serializeUser: sinon.stub(), + deserializeUser: sinon.stub(), + }; + + discoveryStub = sinon.stub().resolves({}); + fetchUserInfoStub = sinon.stub().resolves(mockUserInfo); + + StrategyStub = class { + constructor(options, callback) { + strategyCallback = callback; + this.options = options; + } + + name = 'oidc'; + authenticate = () => {}; + }; + + const { configure } = proxyquire('../src/service/passport/oidc', { + '../../db': dbStub, + '../../config': configStub, + 'openid-client': { + discovery: discoveryStub, + fetchUserInfo: fetchUserInfoStub, + }, + 'openid-client/passport': { + Strategy: StrategyStub, + } + }); + + await configure(passportStub); + }) + + it('should authenticate a new user and create them in the DB', async () => { + if (!strategyCallback) { + throw new Error('strategyCallback is not defined'); + } + + dbStub.findUserByOIDC.resolves(null); // same result as new user flow + dbStub.createUser.resolves({ + username: 'test', + email: 'test@test.com', + oidcId: 'test-sub' + }); + + const done = sinon.spy(); + + // Workaround for the async callback + await new Promise((resolve, reject) => { + strategyCallback({ + claims: () => ({ sub: 'test-sub' }), + email: 'test@test.com', + name: 'Test User' + }, (...args) => { + done(...args); + resolve(); + }); + }); + + expect(done.calledOnce).to.be.true; + + const [err, user] = done.firstCall.args; + expect(err).to.be.null; + expect(user).to.have.property('username', 'test'); + expect(user).to.have.property('email', 'test@test.com'); + expect(user).to.have.property('oidcId', 'test-sub'); + + expect(dbStub.createUser.calledOnce).to.be.true; + expect(dbStub.createUser.firstCall.args[0]).to.equal('test'); + }); + + it('should return an existing user from the DB', async () => { + if (!strategyCallback) { + throw new Error('strategyCallback is not defined'); + } + + const existingUser = { + username: 'existing', + email: 'existing@example.com', + oidcId: 'test-sub' + }; + dbStub.findUserByOIDC.resolves(existingUser); + + const done = sinon.spy(); + + await strategyCallback({ claims: () => ({ sub: 'test-sub' }), access_token: 'access' }, done); + + expect(done.calledOnce).to.be.true; + const [err, user] = done.firstCall.args; + expect(err).to.be.null; + expect(user).to.deep.equal(existingUser); + + expect(dbStub.createUser.notCalled).to.be.true; + }); +}); From f3755f2734c21e3ee524e082fd381a057b00c301 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 12:02:25 +0900 Subject: [PATCH 05/11] fix: ad auth method undefined bug --- src/service/passport/activeDirectory.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/service/passport/activeDirectory.js b/src/service/passport/activeDirectory.js index 372868133..4ccc68527 100644 --- a/src/service/passport/activeDirectory.js +++ b/src/service/passport/activeDirectory.js @@ -7,7 +7,7 @@ const configure = (passport) => { // We can refactor this by normalizing auth strategy config and pass it directly into the configure() function, // ideally when we convert this to TS. const authMethods = require('../../config').getAuthMethods(); - const config = authMethods.find((method) => method.type.toLowerCase() === "activeDirectory"); + const config = authMethods.find((method) => method.type.toLowerCase() === "activedirectory"); const adConfig = config.adConfig; const { userGroup, adminGroup, domain } = config; From bd94aba15cac27fec2d26aea213f5143bbf2fb58 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 12:18:25 +0900 Subject: [PATCH 06/11] test: temporarily remove oidc tests --- test/testOidcAuth.test.js | 131 -------------------------------------- 1 file changed, 131 deletions(-) delete mode 100644 test/testOidcAuth.test.js diff --git a/test/testOidcAuth.test.js b/test/testOidcAuth.test.js deleted file mode 100644 index cff45eea9..000000000 --- a/test/testOidcAuth.test.js +++ /dev/null @@ -1,131 +0,0 @@ -const chai = require('chai'); -const expect = chai.expect; -const sinon = require('sinon'); -const proxyquire = require('proxyquire'); - -describe('OIDC auth method', () => { - let dbStub, passportStub, discoveryStub, fetchUserInfoStub, StrategyStub, strategyCallback; - - const configStub = { - getAuthMethods: () => ([ - { - type: 'openidconnect', - oidcConfig: { - issuer: 'https://test.com', - clientID: 'test-client-id', - clientSecret: 'test-client-secret', - callbackURL: 'http://localhost:3000/auth/callback', - scope: 'openid profile email', - } - } - ]) - }; - - const mockUserInfo = { - sub: 'test-sub', - email: 'test@test.com', - name: 'Test User', - }; - - beforeEach(async () => { - dbStub = { - findUserByOIDC: sinon.stub(), - createUser: sinon.stub(), - }; - - passportStub = { - use: sinon.stub(), - serializeUser: sinon.stub(), - deserializeUser: sinon.stub(), - }; - - discoveryStub = sinon.stub().resolves({}); - fetchUserInfoStub = sinon.stub().resolves(mockUserInfo); - - StrategyStub = class { - constructor(options, callback) { - strategyCallback = callback; - this.options = options; - } - - name = 'oidc'; - authenticate = () => {}; - }; - - const { configure } = proxyquire('../src/service/passport/oidc', { - '../../db': dbStub, - '../../config': configStub, - 'openid-client': { - discovery: discoveryStub, - fetchUserInfo: fetchUserInfoStub, - }, - 'openid-client/passport': { - Strategy: StrategyStub, - } - }); - - await configure(passportStub); - }) - - it('should authenticate a new user and create them in the DB', async () => { - if (!strategyCallback) { - throw new Error('strategyCallback is not defined'); - } - - dbStub.findUserByOIDC.resolves(null); // same result as new user flow - dbStub.createUser.resolves({ - username: 'test', - email: 'test@test.com', - oidcId: 'test-sub' - }); - - const done = sinon.spy(); - - // Workaround for the async callback - await new Promise((resolve, reject) => { - strategyCallback({ - claims: () => ({ sub: 'test-sub' }), - email: 'test@test.com', - name: 'Test User' - }, (...args) => { - done(...args); - resolve(); - }); - }); - - expect(done.calledOnce).to.be.true; - - const [err, user] = done.firstCall.args; - expect(err).to.be.null; - expect(user).to.have.property('username', 'test'); - expect(user).to.have.property('email', 'test@test.com'); - expect(user).to.have.property('oidcId', 'test-sub'); - - expect(dbStub.createUser.calledOnce).to.be.true; - expect(dbStub.createUser.firstCall.args[0]).to.equal('test'); - }); - - it('should return an existing user from the DB', async () => { - if (!strategyCallback) { - throw new Error('strategyCallback is not defined'); - } - - const existingUser = { - username: 'existing', - email: 'existing@example.com', - oidcId: 'test-sub' - }; - dbStub.findUserByOIDC.resolves(existingUser); - - const done = sinon.spy(); - - await strategyCallback({ claims: () => ({ sub: 'test-sub' }), access_token: 'access' }, done); - - expect(done.calledOnce).to.be.true; - const [err, user] = done.firstCall.args; - expect(err).to.be.null; - expect(user).to.deep.equal(existingUser); - - expect(dbStub.createUser.notCalled).to.be.true; - }); -}); From 88fd8957ed2b082d628623f735c2e4b8fb1098f0 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Fri, 23 May 2025 12:42:56 +0900 Subject: [PATCH 07/11] chore: fix linter --- test/testActiveDirectoryAuth.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/testActiveDirectoryAuth.test.js b/test/testActiveDirectoryAuth.test.js index 76614d197..813cbc2bb 100644 --- a/test/testActiveDirectoryAuth.test.js +++ b/test/testActiveDirectoryAuth.test.js @@ -4,7 +4,10 @@ const proxyquire = require('proxyquire'); const expect = chai.expect; describe('ActiveDirectory auth method', () => { - let ldapStub, dbStub, passportStub, strategyCallback; + let ldapStub; + let dbStub; + let passportStub; + let strategyCallback; const newConfig = JSON.stringify({ authentication: [ From 9a0276eabce50c91dd710b9a54744993779f5ea9 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Mon, 26 May 2025 23:09:30 +0900 Subject: [PATCH 08/11] test: improve coverage for service/routes/auth and fix bug --- src/service/routes/auth.js | 14 +++++++++----- test/testLogin.test.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/service/routes/auth.js b/src/service/routes/auth.js index aaf2efa26..5dc8bb412 100644 --- a/src/service/routes/auth.js +++ b/src/service/routes/auth.js @@ -109,24 +109,28 @@ router.get('/profile', async (req, res) => { router.post('/gitAccount', async (req, res) => { if (req.user) { try { - let login = + let username = req.body.username == null || req.body.username == 'undefined' ? req.body.id : req.body.username; + username = username?.split('@')[0]; - login = login.split('@')[0]; + if (!username) { + res.status(400).send('Error: Missing username. Git account not updated').end(); + return; + } - const user = await db.findUser(login); + const user = await db.findUser(username); console.log('Adding gitAccount' + req.body.gitAccount); user.gitAccount = req.body.gitAccount; db.updateUser(user); res.status(200).end(); - } catch { + } catch (e) { res .status(500) .send({ - message: 'An error occurred', + message: `Error updating git account: ${e.message}`, }) .end(); } diff --git a/test/testLogin.test.js b/test/testLogin.test.js index 107bb7256..ebcf7563f 100644 --- a/test/testLogin.test.js +++ b/test/testLogin.test.js @@ -52,6 +52,27 @@ describe('auth', async () => { res.should.have.status(200); }); + it('should be able to set the git account', async function () { + console.log(`cookie: ${cookie}`); + const res = await chai.request(app).post('/api/auth/gitAccount') + .set('Cookie', `${cookie}`) + .send({ + username: 'admin', + gitAccount: 'new-account', + }); + res.should.have.status(200); + }); + + it('should throw an error if the username is not provided when setting the git account', async function () { + const res = await chai.request(app).post('/api/auth/gitAccount') + .set('Cookie', `${cookie}`) + .send({ + gitAccount: 'new-account', + }); + console.log(`res: ${JSON.stringify(res)}`); + res.should.have.status(400); + }); + it('should now be able to logout', async function () { const res = await chai.request(app).post('/api/auth/logout').set('Cookie', `${cookie}`); res.should.have.status(200); @@ -78,6 +99,19 @@ describe('auth', async () => { }); res.should.have.status(401); }); + + it('should fail to set the git account if the user is not logged in', async function () { + const res = await chai.request(app).post('/api/auth/gitAccount').send({ + username: 'admin', + gitAccount: 'new-account', + }); + res.should.have.status(401); + }); + + it('should fail to get the current user metadata if not logged in', async function () { + const res = await chai.request(app).get('/api/auth/me'); + res.should.have.status(401); + }); }); after(async function () { From e6f0cfe0c3a1e0fe903679c151902b4607eae129 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Mon, 26 May 2025 23:50:06 +0900 Subject: [PATCH 09/11] chore: remove unused auth endpoints and function The getUser function on git-push.js (which used the api/auth/success endpoint) was exported but not used anywhere else --- src/service/routes/auth.js | 23 ----------------------- src/ui/services/git-push.js | 15 --------------- test/testLogin.test.js | 8 ++++++++ 3 files changed, 8 insertions(+), 38 deletions(-) diff --git a/src/service/routes/auth.js b/src/service/routes/auth.js index 5dc8bb412..6a7852267 100644 --- a/src/service/routes/auth.js +++ b/src/service/routes/auth.js @@ -65,29 +65,6 @@ router.get('/oidc/callback', (req, res, next) => { })(req, res, next); }); -// when login is successful, retrieve user info -router.get('/success', (req, res) => { - console.log('authenticated' + JSON.stringify(req.user)); - if (req.user) { - res.json({ - success: true, - message: 'user has successfully authenticated', - user: req.user, - cookies: req.cookies, - }); - } else { - res.status(401).end(); - } -}); - -// when login failed, send failed msg -router.get('failed', (req, res) => { - res.status(401).json({ - success: false, - message: 'user failed to authenticate.', - }); -}); - router.post('/logout', (req, res, next) => { req.logout(req.user, (err) => { if (err) return next(err); diff --git a/src/ui/services/git-push.js b/src/ui/services/git-push.js index df8f6f354..414411fa3 100644 --- a/src/ui/services/git-push.js +++ b/src/ui/services/git-push.js @@ -9,21 +9,6 @@ const config = { withCredentials: true, }; -const getUser = async (setIsLoading, setData, setAuth, setIsError) => { - const url = new URL(`${location.origin}/api/auth/success`); - await axios(url.toString(), config) - .then((response) => { - const data = response.data; - setData(data); - setIsLoading(false); - }) - .catch((error) => { - if (error.response && error.response.status === 401) setAuth(false); - else setIsError(true); - setIsLoading(false); - }); -}; - const getPush = async (id, setIsLoading, setData, setAuth, setIsError) => { const url = `${baseUrl}/push/${id}`; await axios(url, config) diff --git a/test/testLogin.test.js b/test/testLogin.test.js index ebcf7563f..dea0cfc75 100644 --- a/test/testLogin.test.js +++ b/test/testLogin.test.js @@ -112,6 +112,14 @@ describe('auth', async () => { const res = await chai.request(app).get('/api/auth/me'); res.should.have.status(401); }); + + it('should fail to login with invalid credentials', async function () { + const res = await chai.request(app).post('/api/auth/login').send({ + username: 'admin', + password: 'invalid', + }); + res.should.have.status(401); + }); }); after(async function () { From ecb83cc61feb21004b91a08dea947a3323c2db4d Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Mon, 26 May 2025 23:54:50 +0900 Subject: [PATCH 10/11] chore: fix linter issue --- src/ui/services/git-push.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/services/git-push.js b/src/ui/services/git-push.js index 414411fa3..66875f1a5 100644 --- a/src/ui/services/git-push.js +++ b/src/ui/services/git-push.js @@ -111,4 +111,4 @@ const cancelPush = async (id, setAuth, setIsError) => { }); }; -export { getPush, getPushes, authorisePush, rejectPush, cancelPush, getUser }; +export { getPush, getPushes, authorisePush, rejectPush, cancelPush }; From 4d742f6458bca849e42d227c7e3e05e511126686 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Thu, 19 Jun 2025 15:54:00 +0900 Subject: [PATCH 11/11] chore: improve AD error messages and fix failing test --- src/service/passport/activeDirectory.js | 11 ++++++----- test/testActiveDirectoryAuth.test.js | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/service/passport/activeDirectory.js b/src/service/passport/activeDirectory.js index e5eb6f8a2..8f681c823 100644 --- a/src/service/passport/activeDirectory.js +++ b/src/service/passport/activeDirectory.js @@ -43,8 +43,9 @@ const configure = (passport) => { const message = `User it not a member of ${userGroup}`; return done(message, null); } - } catch (e) { - const message = `An error occurred while checking if the user is a member of the user group: ${JSON.stringify(e)}`; + } catch (err) { + console.log('ad test (isUser): e', err); + const message = `An error occurred while checking if the user is a member of the user group: ${err.message}`; return done(message, null); } @@ -53,9 +54,9 @@ const configure = (passport) => { try { isAdmin = await ldaphelper.isUserInAdGroup(req, profile, ad, domain, adminGroup); - } catch (e) { - const message = `An error occurred while checking if the user is a member of the admin group: ${JSON.stringify(e)}`; - console.error(message, e); // don't return an error for this case as you may still be a user + } catch (err) { + const message = `An error occurred while checking if the user is a member of the admin group: ${err.message}`; + console.error(message, err); // don't return an error for this case as you may still be a user } profile.admin = isAdmin; diff --git a/test/testActiveDirectoryAuth.test.js b/test/testActiveDirectoryAuth.test.js index 813cbc2bb..4d50dbb45 100644 --- a/test/testActiveDirectoryAuth.test.js +++ b/test/testActiveDirectoryAuth.test.js @@ -144,7 +144,7 @@ describe('ActiveDirectory auth method', () => { expect(done.calledOnce).to.be.true; const [err, user] = done.firstCall.args; - expect(err.message).to.equal('LDAP error'); + expect(err).to.contain('LDAP error'); expect(user).to.be.null; }); });