Skip to content

Commit c7d94a9

Browse files
jescaladajescalada
authored
Merge pull request #1098 from andypols/fix-additional-user-api-leaks
fix: additional user api leaks
2 parents 4956b73 + 40df0d1 commit c7d94a9

File tree

5 files changed

+137
-46
lines changed

5 files changed

+137
-46
lines changed

‎src/service/routes/auth.js

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const passportLocal = require('../passport/local');
66
const passportAD = require('../passport/activeDirectory');
77
const authStrategies = require('../passport').authStrategies;
88
const db = require('../../db');
9+
const { toPublicUser } = require('./publicApi');
910
const { GIT_PROXY_UI_HOST: uiHost = 'http://localhost', GIT_PROXY_UI_PORT: uiPort = 3000 } =
1011
process.env;
1112

@@ -46,6 +47,25 @@ const getLoginStrategy = () => {
4647
return enabledAppropriateLoginStrategies[0].type.toLowerCase();
4748
};
4849

50+
const loginSuccessHandler = () => async (req, res) => {
51+
try {
52+
const currentUser = { ...req.user };
53+
delete currentUser.password;
54+
console.log(
55+
`serivce.routes.auth.login: user logged in, username=${
56+
currentUser.username
57+
} profile=${JSON.stringify(currentUser)}`,
58+
);
59+
res.send({
60+
message: 'success',
61+
user: toPublicUser(currentUser),
62+
});
63+
} catch (e) {
64+
console.log(`service.routes.auth.login: Error logging user in ${JSON.stringify(e)}`);
65+
res.status(500).send('Failed to login').end();
66+
}
67+
};
68+
4969
// TODO: provide separate auth endpoints for each auth strategy or chain compatibile auth strategies
5070
// TODO: if providing separate auth methods, inform the frontend so it has relevant UI elements and appropriate client-side behavior
5171
router.post(
@@ -59,25 +79,7 @@ router.post(
5979
console.log('going to auth with', authType);
6080
return passport.authenticate(authType)(req, res, next);
6181
},
62-
async (req, res) => {
63-
try {
64-
const currentUser = { ...req.user };
65-
delete currentUser.password;
66-
console.log(
67-
`serivce.routes.auth.login: user logged in, username=${
68-
currentUser.username
69-
} profile=${JSON.stringify(currentUser)}`,
70-
);
71-
res.send({
72-
message: 'success',
73-
user: currentUser,
74-
});
75-
} catch (e) {
76-
console.log(`service.routes.auth.login: Error logging user in ${JSON.stringify(e)}`);
77-
res.status(500).send('Failed to login').end();
78-
return;
79-
}
80-
},
82+
loginSuccessHandler(),
8183
);
8284

8385
router.get('/oidc', passport.authenticate(authStrategies['openidconnect'].type));
@@ -114,8 +116,7 @@ router.post('/logout', (req, res, next) => {
114116
router.get('/profile', async (req, res) => {
115117
if (req.user) {
116118
const userVal = await db.findUser(req.user.username);
117-
delete userVal.password;
118-
res.send(userVal);
119+
res.send(toPublicUser(userVal));
119120
} else {
120121
res.status(401).end();
121122
}
@@ -160,14 +161,14 @@ router.post('/gitAccount', async (req, res) => {
160161

161162
router.get('/me', async (req, res) => {
162163
if (req.user) {
163-
const user = JSON.parse(JSON.stringify(req.user));
164-
if (user && user.password) delete user.password;
165-
const login = user.username;
166-
const userVal = await db.findUser(login);
167-
if (userVal && userVal.password) delete userVal.password;
168-
res.send(userVal);
164+
const userVal = await db.findUser(req.user.username);
165+
res.send(toPublicUser(userVal));
169166
} else {
170167
res.status(401).end();
171168
}
172169
});
173-
module.exports = router;
170+
171+
module.exports = {
172+
router,
173+
loginSuccessHandler
174+
};

‎src/service/routes/index.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ const jwtAuthHandler = require('../passport/jwtAuthHandler');
1010
const router = new express.Router();
1111

1212
router.use('/api', home);
13-
router.use('/api/auth', auth);
13+
router.use('/api/auth', auth.router);
1414
router.use('/api/v1/healthcheck', healthcheck);
1515
router.use('/api/v1/push', jwtAuthHandler(), push);
1616
router.use('/api/v1/repo', jwtAuthHandler(), repo);

‎src/service/routes/publicApi.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
export const toPublicUser = (user) => {
2+
return {
3+
username: user.username || '',
4+
displayName: user.displayName || '',
5+
email: user.email || '',
6+
title: user.title || '',
7+
gitAccount: user.gitAccount || '',
8+
admin: user.admin || false,
9+
}
10+
}

‎src/service/routes/users.js

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,7 @@
11
const express = require('express');
22
const router = new express.Router();
33
const db = require('../../db');
4-
5-
const toPublicUser = (user) => {
6-
return {
7-
username: user.username || '',
8-
displayName: user.displayName || '',
9-
email: user.email || '',
10-
title: user.title || '',
11-
gitAccount: user.gitAccount || '',
12-
admin: user.admin || false,
13-
}
14-
}
4+
const { toPublicUser } = require('./publicApi');
155

166
router.get('/', async (req, res) => {
177
const query = {};
@@ -35,8 +25,7 @@ router.get('/', async (req, res) => {
3525
router.get('/:id', async (req, res) => {
3626
const username = req.params.id.toLowerCase();
3727
console.log(`Retrieving details for user: ${username}`);
38-
const data = await db.findUser(username);
39-
const user = JSON.parse(JSON.stringify(data));
28+
const user = await db.findUser(username);
4029
res.send(toPublicUser(user));
4130
});
4231

‎test/services/routes/auth.test.js

Lines changed: 95 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ const chai = require('chai');
22
const chaiHttp = require('chai-http');
33
const sinon = require('sinon');
44
const express = require('express');
5-
const authRouter = require('../../../src/service/routes/auth');
5+
const { router, loginSuccessHandler } = require('../../../src/service/routes/auth');
66
const db = require('../../../src/db');
77

88
const { expect } = chai;
@@ -19,11 +19,15 @@ const newApp = (username) => {
1919
});
2020
}
2121

22-
app.use('/auth', authRouter);
22+
app.use('/auth', router);
2323
return app;
2424
};
2525

26-
describe('Authentication Routes', () => {
26+
describe('Auth API', function () {
27+
afterEach(function () {
28+
sinon.restore();
29+
});
30+
2731
describe('/gitAccount', () => {
2832
beforeEach(() => {
2933
sinon.stub(db, 'findUser').callsFake((username) => {
@@ -112,7 +116,7 @@ describe('Authentication Routes', () => {
112116
}),
113117
).to.be.true;
114118
});
115-
119+
116120
it('POST /gitAccount allows non-admin user to update their own gitAccount', async () => {
117121
const updateUserStub = sinon.stub(db, 'updateUser').resolves();
118122

@@ -132,6 +136,93 @@ describe('Authentication Routes', () => {
132136
}),
133137
).to.be.true;
134138
});
139+
});
140+
141+
describe('loginSuccessHandler', function () {
142+
it('should log in user and return public user data', async function () {
143+
const user = {
144+
username: 'bob',
145+
password: 'secret',
146+
email: '[email protected]',
147+
displayName: 'Bob',
148+
};
135149

150+
const res = {
151+
send: sinon.spy(),
152+
};
153+
154+
await loginSuccessHandler()({ user }, res);
155+
156+
expect(res.send.calledOnce).to.be.true;
157+
expect(res.send.firstCall.args[0]).to.deep.equal({
158+
message: 'success',
159+
user: {
160+
admin: false,
161+
displayName: 'Bob',
162+
email: '[email protected]',
163+
gitAccount: '',
164+
title: '',
165+
username: 'bob',
166+
},
167+
});
168+
});
169+
});
170+
171+
describe('/me', function () {
172+
it('GET /me returns Unauthorized if authenticated user not in request', async () => {
173+
const res = await chai.request(newApp()).get('/auth/me');
174+
175+
expect(res).to.have.status(401);
176+
});
177+
178+
it('GET /me serializes public data representation of current authenticated user', async function () {
179+
sinon.stub(db, 'findUser').resolves({
180+
username: 'alice',
181+
password: 'secret-hashed-password',
182+
email: '[email protected]',
183+
displayName: 'Alice Walker',
184+
otherUserData: 'should not be returned',
185+
});
186+
187+
const res = await chai.request(newApp('alice')).get('/auth/me');
188+
expect(res).to.have.status(200);
189+
expect(res.body).to.deep.equal({
190+
username: 'alice',
191+
displayName: 'Alice Walker',
192+
email: '[email protected]',
193+
title: '',
194+
gitAccount: '',
195+
admin: false,
196+
});
197+
});
198+
});
199+
200+
describe('/profile', function () {
201+
it('GET /profile returns Unauthorized if authenticated user not in request', async () => {
202+
const res = await chai.request(newApp()).get('/auth/profile');
203+
204+
expect(res).to.have.status(401);
205+
});
206+
207+
it('GET /profile serializes public data representation of current authenticated user', async function () {
208+
sinon.stub(db, 'findUser').resolves({
209+
username: 'alice',
210+
password: 'secret-hashed-password',
211+
email: '[email protected]',
212+
displayName: 'Alice Walker',
213+
otherUserData: 'should not be returned',
214+
});
215+
216+
const res = await chai.request(newApp('alice')).get('/auth/profile');
217+
expect(res).to.have.status(200);
218+
expect(res.body).to.deep.equal({
219+
username: 'alice',
220+
displayName: 'Alice Walker',
221+
email: '[email protected]',
222+
title: '',
223+
gitAccount: '',
224+
admin: false,
225+
});
226+
});
136227
});
137228
});

0 commit comments

Comments
 (0)