Skip to content

Commit 60bfa32

Browse files
JamieSlomeJamieSlome
authored
Merge pull request #1024 from jescalada/improve-auth-test-coverage
test: improve auth test coverage
2 parents 1d30ab5 + 5f04f33 commit 60bfa32

5 files changed

Lines changed: 208 additions & 49 deletions

File tree

‎src/service/passport/activeDirectory.js‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,9 @@ const configure = (passport) => {
4343
const message = `User it not a member of ${userGroup}`;
4444
return done(message, null);
4545
}
46-
} catch (e) {
47-
const message = `An error occurred while checking if the user is a member of the user group: ${JSON.stringify(e)}`;
46+
} catch (err) {
47+
console.log('ad test (isUser): e', err);
48+
const message = `An error occurred while checking if the user is a member of the user group: ${err.message}`;
4849
return done(message, null);
4950
}
5051

@@ -53,9 +54,9 @@ const configure = (passport) => {
5354
try {
5455
isAdmin = await ldaphelper.isUserInAdGroup(req, profile, ad, domain, adminGroup);
5556

56-
} catch (e) {
57-
const message = `An error occurred while checking if the user is a member of the admin group: ${JSON.stringify(e)}`;
58-
console.error(message, e); // don't return an error for this case as you may still be a user
57+
} catch (err) {
58+
const message = `An error occurred while checking if the user is a member of the admin group: ${err.message}`;
59+
console.error(message, err); // don't return an error for this case as you may still be a user
5960
}
6061

6162
profile.admin = isAdmin;

‎src/service/routes/auth.js‎

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -66,29 +66,6 @@ router.get('/oidc/callback', (req, res, next) => {
6666
})(req, res, next);
6767
});
6868

69-
// when login is successful, retrieve user info
70-
router.get('/success', (req, res) => {
71-
console.log('authenticated' + JSON.stringify(req.user));
72-
if (req.user) {
73-
res.json({
74-
success: true,
75-
message: 'user has successfully authenticated',
76-
user: req.user,
77-
cookies: req.cookies,
78-
});
79-
} else {
80-
res.status(401).end();
81-
}
82-
});
83-
84-
// when login failed, send failed msg
85-
router.get('failed', (req, res) => {
86-
res.status(401).json({
87-
success: false,
88-
message: 'user failed to authenticate.',
89-
});
90-
});
91-
9269
router.post('/logout', (req, res, next) => {
9370
req.logout(req.user, (err) => {
9471
if (err) return next(err);
@@ -110,24 +87,28 @@ router.get('/profile', async (req, res) => {
11087
router.post('/gitAccount', async (req, res) => {
11188
if (req.user) {
11289
try {
113-
let login =
90+
let username =
11491
req.body.username == null || req.body.username == 'undefined'
11592
? req.body.id
11693
: req.body.username;
94+
username = username?.split('@')[0];
11795

118-
login = login.split('@')[0];
96+
if (!username) {
97+
res.status(400).send('Error: Missing username. Git account not updated').end();
98+
return;
99+
}
119100

120-
const user = await db.findUser(login);
101+
const user = await db.findUser(username);
121102

122103
console.log('Adding gitAccount' + req.body.gitAccount);
123104
user.gitAccount = req.body.gitAccount;
124105
db.updateUser(user);
125106
res.status(200).end();
126-
} catch {
107+
} catch (e) {
127108
res
128109
.status(500)
129110
.send({
130-
message: 'An error occurred',
111+
message: `Error updating git account: ${e.message}`,
131112
})
132113
.end();
133114
}

‎src/ui/services/git-push.js‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,21 +9,6 @@ const config = {
99
withCredentials: true,
1010
};
1111

12-
const getUser = async (setIsLoading, setData, setAuth, setIsError) => {
13-
const url = new URL(`${location.origin}/api/auth/success`);
14-
await axios(url.toString(), config)
15-
.then((response) => {
16-
const data = response.data;
17-
setData(data);
18-
setIsLoading(false);
19-
})
20-
.catch((error) => {
21-
if (error.response && error.response.status === 401) setAuth(false);
22-
else setIsError(true);
23-
setIsLoading(false);
24-
});
25-
};
26-
2712
const getPush = async (id, setIsLoading, setData, setAuth, setIsError) => {
2813
const url = `${baseUrl}/push/${id}`;
2914
await axios(url, config)
@@ -128,4 +113,4 @@ const cancelPush = async (id, setAuth, setIsError) => {
128113
});
129114
};
130115

131-
export { getPush, getPushes, authorisePush, rejectPush, cancelPush, getUser };
116+
export { getPush, getPushes, authorisePush, rejectPush, cancelPush };

‎test/testActiveDirectoryAuth.test.js‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
const chai = require('chai');
2+
const sinon = require('sinon');
3+
const proxyquire = require('proxyquire');
4+
const expect = chai.expect;
5+
6+
describe('ActiveDirectory auth method', () => {
7+
let ldapStub;
8+
let dbStub;
9+
let passportStub;
10+
let strategyCallback;
11+
12+
const newConfig = JSON.stringify({
13+
authentication: [
14+
{
15+
type: 'ActiveDirectory',
16+
enabled: true,
17+
adminGroup: 'test-admin-group',
18+
userGroup: 'test-user-group',
19+
domain: 'test.com',
20+
adConfig: {
21+
url: 'ldap://test-url',
22+
baseDN: 'dc=test,dc=com',
23+
searchBase: 'ou=users,dc=test,dc=com',
24+
},
25+
},
26+
],
27+
});
28+
29+
beforeEach(() => {
30+
ldapStub = {
31+
isUserInAdGroup: sinon.stub(),
32+
};
33+
34+
dbStub = {
35+
updateUser: sinon.stub(),
36+
};
37+
38+
passportStub = {
39+
use: sinon.stub(),
40+
serializeUser: sinon.stub(),
41+
deserializeUser: sinon.stub(),
42+
};
43+
44+
const fsStub = {
45+
existsSync: sinon.stub().returns(true),
46+
readFileSync: sinon.stub().returns(newConfig),
47+
};
48+
49+
const config = proxyquire('../src/config', {
50+
fs: fsStub,
51+
});
52+
53+
const { configure } = proxyquire('../src/service/passport/activeDirectory', {
54+
'./ldaphelper': ldapStub,
55+
'../../db': dbStub,
56+
'../../config': config,
57+
'passport-activedirectory': function (options, callback) {
58+
strategyCallback = callback;
59+
return {
60+
name: 'ActiveDirectory',
61+
authenticate: () => {},
62+
};
63+
},
64+
});
65+
66+
configure(passportStub);
67+
});
68+
69+
it('should authenticate a valid user and mark them as admin', async () => {
70+
const mockReq = {};
71+
const mockProfile = {
72+
_json: {
73+
sAMAccountName: 'test-user',
74+
mail: 'test@test.com',
75+
userPrincipalName: 'test@test.com',
76+
title: 'Test User',
77+
},
78+
displayName: 'Test User',
79+
};
80+
81+
ldapStub.isUserInAdGroup
82+
.onCall(0).resolves(true)
83+
.onCall(1).resolves(true);
84+
85+
const done = sinon.spy();
86+
87+
await strategyCallback(mockReq, mockProfile, {}, done);
88+
89+
expect(done.calledOnce).to.be.true;
90+
const [err, user] = done.firstCall.args;
91+
expect(err).to.be.null;
92+
expect(user).to.have.property('username', 'test-user');
93+
expect(user).to.have.property('email', 'test@test.com');
94+
expect(user).to.have.property('displayName', 'Test User');
95+
expect(user).to.have.property('admin', true);
96+
expect(user).to.have.property('title', 'Test User');
97+
98+
expect(dbStub.updateUser.calledOnce).to.be.true;
99+
});
100+
101+
it('should fail if user is not in user group', async () => {
102+
const mockReq = {};
103+
const mockProfile = {
104+
_json: {
105+
sAMAccountName: 'bad-user',
106+
mail: 'bad@test.com',
107+
userPrincipalName: 'bad@test.com',
108+
title: 'Bad User'
109+
},
110+
displayName: 'Bad User'
111+
};
112+
113+
ldapStub.isUserInAdGroup.onCall(0).resolves(false);
114+
115+
const done = sinon.spy();
116+
117+
await strategyCallback(mockReq, mockProfile, {}, done);
118+
119+
expect(done.calledOnce).to.be.true;
120+
const [err, user] = done.firstCall.args;
121+
expect(err).to.include('not a member');
122+
expect(user).to.be.null;
123+
124+
expect(dbStub.updateUser.notCalled).to.be.true;
125+
});
126+
127+
it('should handle LDAP errors gracefully', async () => {
128+
const mockReq = {};
129+
const mockProfile = {
130+
_json: {
131+
sAMAccountName: 'error-user',
132+
mail: 'err@test.com',
133+
userPrincipalName: 'err@test.com',
134+
title: 'Whoops'
135+
},
136+
displayName: 'Error User'
137+
};
138+
139+
ldapStub.isUserInAdGroup.rejects(new Error('LDAP error'));
140+
141+
const done = sinon.spy();
142+
143+
await strategyCallback(mockReq, mockProfile, {}, done);
144+
145+
expect(done.calledOnce).to.be.true;
146+
const [err, user] = done.firstCall.args;
147+
expect(err).to.contain('LDAP error');
148+
expect(user).to.be.null;
149+
});
150+
});

‎test/testLogin.test.js‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,27 @@ describe('auth', async () => {
5252
res.should.have.status(200);
5353
});
5454

55+
it('should be able to set the git account', async function () {
56+
console.log(`cookie: ${cookie}`);
57+
const res = await chai.request(app).post('/api/auth/gitAccount')
58+
.set('Cookie', `${cookie}`)
59+
.send({
60+
username: 'admin',
61+
gitAccount: 'new-account',
62+
});
63+
res.should.have.status(200);
64+
});
65+
66+
it('should throw an error if the username is not provided when setting the git account', async function () {
67+
const res = await chai.request(app).post('/api/auth/gitAccount')
68+
.set('Cookie', `${cookie}`)
69+
.send({
70+
gitAccount: 'new-account',
71+
});
72+
console.log(`res: ${JSON.stringify(res)}`);
73+
res.should.have.status(400);
74+
});
75+
5576
it('should now be able to logout', async function () {
5677
const res = await chai.request(app).post('/api/auth/logout').set('Cookie', `${cookie}`);
5778
res.should.have.status(200);
@@ -78,6 +99,27 @@ describe('auth', async () => {
7899
});
79100
res.should.have.status(401);
80101
});
102+
103+
it('should fail to set the git account if the user is not logged in', async function () {
104+
const res = await chai.request(app).post('/api/auth/gitAccount').send({
105+
username: 'admin',
106+
gitAccount: 'new-account',
107+
});
108+
res.should.have.status(401);
109+
});
110+
111+
it('should fail to get the current user metadata if not logged in', async function () {
112+
const res = await chai.request(app).get('/api/auth/me');
113+
res.should.have.status(401);
114+
});
115+
116+
it('should fail to login with invalid credentials', async function () {
117+
const res = await chai.request(app).post('/api/auth/login').send({
118+
username: 'admin',
119+
password: 'invalid',
120+
});
121+
res.should.have.status(401);
122+
});
81123
});
82124

83125
after(async function () {

0 commit comments

Comments
 (0)