Skip to content

Commit ef6da24

Browse files
y-oksakuy-oksaku
authoredJun 21, 2023
fix(2892): API tokens are correctly authenticated with new sd admins format (#2890)
·
v8.0.9v6.0.30
1 parent 1d241da commit ef6da24

File tree

16 files changed

+122
-103
lines changed

16 files changed

+122
-103
lines changed
 

‎plugins/auth/index.js

+20-1
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ const authPlugin = {
107107
*/
108108
server.expose('generateProfile', config => {
109109
const { username, scmUserId, scmContext, scope, metadata } = config;
110-
const profile = { username, scmContext, scope, ...(metadata || {}) };
110+
const profile = { username, scmContext, scmUserId, scope, ...(metadata || {}) };
111111

112112
if (pluginOptions.jwtEnvironment) {
113113
profile.environment = pluginOptions.jwtEnvironment;
@@ -190,6 +190,7 @@ const authPlugin = {
190190
try {
191191
const { tokenFactory, userFactory, pipelineFactory, collectionFactory } = request.server.app;
192192
const token = await tokenFactory.get({ value: tokenValue });
193+
const { scm } = pipelineFactory;
193194

194195
if (!token) {
195196
return { isValid: false, credentials: {} };
@@ -204,10 +205,28 @@ const authPlugin = {
204205
return { isValid: false, credentials: {} };
205206
}
206207

208+
let scmUser = {};
209+
210+
try {
211+
scmUser = await scm.decorateAuthor({
212+
username: user.username,
213+
scmContext: user.scmContext,
214+
token: await user.unsealToken()
215+
});
216+
} catch (err) {
217+
request.log(
218+
['auth', 'error'],
219+
`Fails to find the user "${user.username}" in ${user.scmContext}.`
220+
);
221+
222+
return { isValid: false, credentials: {} };
223+
}
224+
207225
await createDefaultCollection(user, collectionFactory);
208226

209227
profile = {
210228
username: user.username,
229+
scmUserId: scmUser.id,
211230
scmContext: user.scmContext,
212231
scope: ['user']
213232
};

‎plugins/banners/create.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,16 @@ module.exports = () => ({
1818

1919
handler: async (request, h) => {
2020
const { bannerFactory } = request.server.app;
21-
const { username } = request.auth.credentials;
22-
const { scmContext } = request.auth.credentials;
21+
const { username, scmContext, scmUserId } = request.auth.credentials;
2322
const { scm } = bannerFactory;
2423
const scmDisplayName = scm.getDisplayName({ scmContext });
2524

2625
// lookup whether user is admin
27-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
26+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
27+
username,
28+
scmDisplayName,
29+
scmUserId
30+
);
2831

2932
// verify user is authorized to create banners
3033
// return unauthorized if not system admin

‎plugins/banners/index.js

+7-8
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,20 @@ const bannerPlugin = {
2323
* @param {String} scmDisplayName Scm display name of the person
2424
* @return {Object} Details including the display name and admin status of user
2525
*/
26-
server.expose('screwdriverAdminDetails', (username, scmDisplayName) => {
26+
server.expose('screwdriverAdminDetails', (username, scmDisplayName, scmUserId) => {
2727
// construct object with defaults to store details
2828
const adminDetails = {
2929
isAdmin: false
3030
};
3131

3232
if (scmDisplayName) {
33-
const adminsList = options.admins;
33+
const userDisplayName = options.authCheckById
34+
? `${scmDisplayName}:${username}:${scmUserId}`
35+
: `${scmDisplayName}:${username}`;
36+
const admins = options.authCheckById ? options.sdAdmins : options.admins;
3437

35-
// construct displayable username string
36-
adminDetails.userDisplayName = `${scmDisplayName}:${username}`;
37-
38-
// Check system configuration for list of system admins
39-
// set admin status true if current user is identified to be a system admin
40-
if (adminsList.length > 0 && adminsList.includes(adminDetails.userDisplayName)) {
38+
// Check admin
39+
if (admins.length > 0 && admins.includes(userDisplayName)) {
4140
adminDetails.isAdmin = true;
4241
}
4342
}

‎plugins/banners/remove.js

+6-4
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,16 @@ module.exports = () => ({
2020
handler: async (request, h) => {
2121
const { bannerFactory } = request.server.app;
2222
const { id } = request.params; // id of banner to delete
23-
24-
const { username } = request.auth.credentials;
25-
const { scmContext } = request.auth.credentials;
23+
const { username, scmContext, scmUserId } = request.auth.credentials;
2624
const { scm } = bannerFactory;
2725
const scmDisplayName = scm.getDisplayName({ scmContext });
2826

2927
// lookup whether user is admin
30-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
28+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
29+
username,
30+
scmDisplayName,
31+
scmUserId
32+
);
3133

3234
// verify user is authorized to remove banners
3335
// return unauthorized if not system admin

‎plugins/banners/update.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,16 @@ module.exports = () => ({
2020
handler: async (request, h) => {
2121
const { bannerFactory } = request.server.app;
2222
const { id } = request.params; // id of banner to update
23-
const { username } = request.auth.credentials;
24-
const { scmContext } = request.auth.credentials;
23+
const { username, scmContext, scmUserId } = request.auth.credentials;
2524
const { scm } = bannerFactory;
2625
const scmDisplayName = scm.getDisplayName({ scmContext });
2726

2827
// lookup whether user is admin
29-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
28+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
29+
username,
30+
scmDisplayName,
31+
scmUserId
32+
);
3033

3134
// verify user is authorized to update banners
3235
// return unauthorized if not system admin

‎plugins/buildClusters/create.js

+6-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ module.exports = () => ({
1818

1919
handler: async (request, h) => {
2020
const { buildClusterFactory, bannerFactory } = request.server.app;
21-
const { username, scmContext: userContext } = request.auth.credentials;
21+
const { username, scmContext: userContext, scmUserId } = request.auth.credentials;
2222
const { payload } = request;
2323
const { managedByScrewdriver, name, scmOrganizations } = payload;
2424

@@ -27,7 +27,11 @@ module.exports = () => ({
2727
// Check permissions
2828
// Must be Screwdriver admin to add Screwdriver build cluster
2929
const scmDisplayName = bannerFactory.scm.getDisplayName({ scmContext: payload.scmContext });
30-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
30+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
31+
username,
32+
scmDisplayName,
33+
scmUserId
34+
);
3135

3236
if (!adminDetails.isAdmin) {
3337
return boom.forbidden(

‎plugins/buildClusters/remove.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ module.exports = () => ({
2020
handler: async (request, h) => {
2121
const { buildClusterFactory, userFactory, bannerFactory } = request.server.app;
2222
const { name } = request.params;
23-
const { username, scmContext } = request.auth.credentials;
23+
const { username, scmContext, scmUserId } = request.auth.credentials;
2424
const scmDisplayName = bannerFactory.scm.getDisplayName({ scmContext });
2525

2626
// Fetch the buildCluster and user models
@@ -46,7 +46,8 @@ module.exports = () => ({
4646

4747
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
4848
username,
49-
scmDisplayName
49+
scmDisplayName,
50+
scmUserId
5051
);
5152

5253
if (!adminDetails.isAdmin) {

‎plugins/buildClusters/update.js

+6-2
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ module.exports = () => ({
2020
handler: async (request, h) => {
2121
const { buildClusterFactory, bannerFactory } = request.server.app;
2222
const { name } = request.params; // name of build cluster to update
23-
const { username, scmContext: userContext } = request.auth.credentials;
23+
const { username, scmContext: userContext, scmUserId } = request.auth.credentials;
2424
const { payload } = request;
2525
const { managedByScrewdriver, scmOrganizations } = payload;
2626

@@ -29,7 +29,11 @@ module.exports = () => ({
2929
// Check permissions
3030
// Must be Screwdriver admin to update Screwdriver build cluster
3131
const scmDisplayName = bannerFactory.scm.getDisplayName({ scmContext: userContext });
32-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
32+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
33+
username,
34+
scmDisplayName,
35+
scmUserId
36+
);
3337

3438
if (!adminDetails.isAdmin) {
3539
return boom.forbidden(

‎plugins/builds/artifacts/unzip.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,11 @@ module.exports = config => ({
2626
return h.response(data).code(200);
2727
}
2828
const buildId = req.params.id;
29-
const { username, scope, scmContext } = req.auth.credentials;
29+
const { username, scope, scmContext, scmUserId } = req.auth.credentials;
3030
const isBuild = scope.includes('build');
3131
const { buildFactory } = req.server.app;
3232
const scmDisplayName = buildFactory.scm.getDisplayName({ scmContext })
33-
const adminDetails = req.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
33+
const adminDetails = req.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName, scmUserId);
3434

3535
if (scope.includes('user') && !adminDetails.isAdmin) {
3636
return boom.forbidden(`User ${adminDetails.userDisplayName} does not have Screwdriver administrative privileges.`)

‎plugins/builds/update.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,13 @@ async function getBuildToUpdate(id, buildFactory) {
9393
*/
9494
async function validateUserPermission(build, request) {
9595
const { jobFactory, userFactory, bannerFactory, pipelineFactory } = request.server.app;
96-
const { username, scmContext } = request.auth.credentials;
96+
const { username, scmContext, scmUserId } = request.auth.credentials;
9797

9898
const { status: desiredStatus } = request.payload;
9999

100100
const scmDisplayName = bannerFactory.scm.getDisplayName({ scmContext });
101101
// Check if Screwdriver admin
102-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
102+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName, scmUserId);
103103

104104
// Check desired status
105105
if (adminDetails.isAdmin) {

‎plugins/events/stopBuilds.js

+6-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ module.exports = () => ({
2222

2323
handler: async (request, h) => {
2424
const { eventFactory, pipelineFactory, userFactory } = request.server.app;
25-
const { username, scmContext } = request.auth.credentials;
25+
const { username, scmContext, scmUserId } = request.auth.credentials;
2626
const { isValidToken } = request.server.plugins.pipelines;
2727
const eventId = request.params.id;
2828
const { updateAdmins } = request.server.plugins.events;
@@ -47,7 +47,11 @@ module.exports = () => ({
4747

4848
// Check permissions
4949
const permissions = await user.getPermissions(pipeline.scmUri);
50-
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmContext);
50+
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
51+
username,
52+
scmContext,
53+
scmUserId
54+
);
5155
const isPrOwner = hoek.reach(event, 'commit.author.username') === username;
5256

5357
// PR author should be able to stop their own PR event

‎plugins/pipelines/index.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ const pipelinesPlugin = {
104104
* @return {Object} pipeline
105105
*/
106106
server.expose('canAccessPipeline', (credentials, pipelineId, permission, app) => {
107-
const { username, scmContext, scope } = credentials;
107+
const { username, scmContext, scope, scmUserId } = credentials;
108108
const { userFactory, pipelineFactory } = app;
109109

110110
return pipelineFactory.get(pipelineId).then(pipeline => {
@@ -141,7 +141,8 @@ const pipelinesPlugin = {
141141
const scmDisplayName = pipelineFactory.scm.getDisplayName({ scmContext });
142142
const adminDetails = server.plugins.banners.screwdriverAdminDetails(
143143
username,
144-
scmDisplayName
144+
scmDisplayName,
145+
scmUserId
145146
);
146147

147148
if (adminDetails.isAdmin) {

‎plugins/pipelines/list.js

+6-2
Original file line numberDiff line numberDiff line change
@@ -81,13 +81,17 @@ module.exports = () => ({
8181
return Promise.all(pipelineArray)
8282
.then(pipelineArrays => [].concat(...pipelineArrays))
8383
.then(allPipelines => {
84-
const { username, scope, scmContext } = request.auth.credentials;
84+
const { username, scope, scmContext, scmUserId } = request.auth.credentials;
8585
let adminDetails;
8686

8787
if (scmContext) {
8888
const scmDisplayName = pipelineFactory.scm.getDisplayName({ scmContext });
8989

90-
adminDetails = request.server.plugins.banners.screwdriverAdminDetails(username, scmDisplayName);
90+
adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
91+
username,
92+
scmDisplayName,
93+
scmUserId
94+
);
9195
}
9296

9397
if (scope.includes('user') && adminDetails && adminDetails.isAdmin) {

‎plugins/pipelines/remove.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,7 @@ module.exports = () => ({
2121
handler: async (request, h) => {
2222
const { pipelineFactory, bannerFactory } = request.server.app;
2323
const { userFactory } = request.server.app;
24-
const { username } = request.auth.credentials;
25-
const { scmContext } = request.auth.credentials;
24+
const { username, scmContext, scmUserId } = request.auth.credentials;
2625

2726
// Fetch the pipeline and user models
2827
return Promise.all([pipelineFactory.get(request.params.id), userFactory.get({ username, scmContext })])
@@ -57,7 +56,8 @@ module.exports = () => ({
5756
// Lookup whether user is admin
5857
const adminDetails = request.server.plugins.banners.screwdriverAdminDetails(
5958
username,
60-
scmDisplayName
59+
scmDisplayName,
60+
scmUserId
6161
);
6262

6363
// Allow cluster admins to remove pipeline

‎test/plugins/auth.test.js

+38-65
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,12 @@ function getUserMock(user) {
2525
const result = {
2626
update: sinon.stub(),
2727
sealToken: sinon.stub(),
28+
unsealToken: sinon.stub().returns('token'),
2829
getDisplayName: sinon.stub(),
2930
id: user.id,
3031
username: user.username,
31-
token: user.token
32+
token: user.token,
33+
scmContext: user.scmContext
3234
};
3335

3436
return result;
@@ -93,7 +95,8 @@ describe('auth plugin test', () => {
9395
scope: ['admin:repo_hook', 'read:org', 'repo:status']
9496
}
9597
},
96-
autoDeployKeyGenerationEnabled: sinon.stub().returns(true)
98+
autoDeployKeyGenerationEnabled: sinon.stub().returns(true),
99+
decorateAuthor: sinon.stub()
97100
};
98101
userFactoryMock = {
99102
get: sinon.stub(),
@@ -335,6 +338,7 @@ describe('auth plugin test', () => {
335338
});
336339

337340
expect(profile.username).to.contain('batman');
341+
expect(profile.scmUserId).to.equal(1312);
338342
expect(profile.scmContext).to.contain('github:github.com');
339343
expect(profile.scope).to.contain('user');
340344
expect(profile.scope).to.contain('admin');
@@ -344,12 +348,14 @@ describe('auth plugin test', () => {
344348
it('does not add admin scope for non-admins', () => {
345349
const profile = server.plugins.auth.generateProfile({
346350
username: 'robin',
351+
scmUserId: 1357,
347352
scmContext: 'github:mygithub.com',
348353
scope: ['user'],
349354
metadata: {}
350355
});
351356

352357
expect(profile.username).to.contain('robin');
358+
expect(profile.scmUserId).to.equal(1357);
353359
expect(profile.scmContext).to.contain('github:mygithub.com');
354360
expect(profile.scope).to.contain('user');
355361
expect(profile.scope).to.not.contain('admin');
@@ -359,12 +365,14 @@ describe('auth plugin test', () => {
359365
it('does not add admin scope for admins without SCM user id', () => {
360366
const profile = server.plugins.auth.generateProfile({
361367
username: 'batman',
368+
scmUserId: 1359,
362369
scmContext: 'github:mygithub.com',
363370
scope: ['user'],
364371
metadata: {}
365372
});
366373

367374
expect(profile.username).to.contain('batman');
375+
expect(profile.scmUserId).to.equal(1359);
368376
expect(profile.scmContext).to.contain('github:mygithub.com');
369377
expect(profile.scope).to.contain('user');
370378
expect(profile.scope).to.not.contain('admin');
@@ -405,6 +413,7 @@ describe('auth plugin test', () => {
405413
});
406414

407415
expect(profile.username).to.contain('batman');
416+
expect(profile.scmUserId).to.equal(1312);
408417
expect(profile.scmContext).to.contain('github:github.com');
409418
expect(profile.scope).to.contain('user');
410419
expect(profile.scope).to.contain('admin');
@@ -414,12 +423,14 @@ describe('auth plugin test', () => {
414423
it('does not add admin scope for non-admins', () => {
415424
const profile = server.plugins.auth.generateProfile({
416425
username: 'robin',
426+
scmUserId: 1357,
417427
scmContext: 'github:mygithub.com',
418428
scope: ['user'],
419429
metadata: {}
420430
});
421431

422432
expect(profile.username).to.contain('robin');
433+
expect(profile.scmUserId).to.equal(1357);
423434
expect(profile.scmContext).to.contain('github:mygithub.com');
424435
expect(profile.scope).to.contain('user');
425436
expect(profile.scope).to.not.contain('admin');
@@ -429,12 +440,14 @@ describe('auth plugin test', () => {
429440
it('adds admin scope for admins without SCM user id', () => {
430441
const profile = server.plugins.auth.generateProfile({
431442
username: 'batman',
443+
scmUserId: 1359,
432444
scmContext: 'github:mygithub.com',
433445
scope: ['user'],
434446
metadata: {}
435447
});
436448

437449
expect(profile.username).to.contain('batman');
450+
expect(profile.scmUserId).to.equal(1359);
438451
expect(profile.scmContext).to.contain('github:mygithub.com');
439452
expect(profile.scope).to.contain('user');
440453
expect(profile.scope).to.contain('admin');
@@ -1029,7 +1042,8 @@ describe('auth plugin test', () => {
10291042
{
10301043
username: 'robin',
10311044
scope: ['user'],
1032-
environment: 'beta'
1045+
environment: 'beta',
1046+
scmUserId: 1579
10331047
},
10341048
jwtPrivateKey,
10351049
{
@@ -1046,6 +1060,7 @@ describe('auth plugin test', () => {
10461060
assert.equal(reply.statusCode, 200, 'Login route should be available');
10471061
assert.ok(reply.result.token, 'Token not returned');
10481062
expect(reply.result.token).to.be.a.jwt.and.deep.include({
1063+
scmUserId: 1579,
10491064
username: 'robin',
10501065
scope: ['user'],
10511066
environment: 'beta'
@@ -1054,75 +1069,33 @@ describe('auth plugin test', () => {
10541069

10551070
it('returns user signed token given an API access token', () => {
10561071
tokenMock.userId = id;
1057-
server
1058-
.inject({
1059-
url: `/auth/token?api_token=${apiKey}`,
1060-
auth: {
1061-
credentials: {
1062-
username: 'robin',
1063-
scope: ['user'],
1064-
token: jwt.sign(
1065-
{
1066-
username: 'robin',
1067-
scope: ['user']
1068-
},
1069-
jwtPrivateKey,
1070-
{
1071-
algorithm: 'RS256',
1072-
expiresIn: '2h',
1073-
jwtid: 'abc'
1074-
}
1075-
)
1076-
},
1077-
strategy: ['token']
1078-
}
1079-
})
1080-
.then(reply => {
1081-
assert.equal(reply.statusCode, 200, 'Login route should be available');
1082-
assert.ok(reply.result.token, 'Token not returned');
1083-
expect(reply.result.token).to.be.a.jwt.and.deep.include({
1084-
username: 'robin',
1085-
scope: ['user']
1086-
});
1072+
scm.decorateAuthor.resolves({ id: 1315 });
1073+
collectionFactoryMock.list.resolves([[1], [2]]);
1074+
1075+
return server.inject({ url: `/auth/token?api_token=${apiKey}` }).then(reply => {
1076+
assert.equal(reply.statusCode, 200, 'Login route should be available');
1077+
assert.ok(reply.result.token, 'Token not returned');
1078+
expect(reply.result.token).to.be.a.jwt.and.deep.include({
1079+
scmUserId: 1315,
1080+
username: 'batman',
1081+
scope: ['user'],
1082+
scmContext: 'github:github.com'
10871083
});
1084+
});
10881085
});
10891086

10901087
it('returns pipeline signed token given an API access token', () => {
10911088
tokenMock.pipelineId = pipelineId;
10921089

1093-
server
1094-
.inject({
1095-
url: `/auth/token?api_token=${apiKey}`,
1096-
auth: {
1097-
credentials: {
1098-
username: 'robin',
1099-
scope: ['pipeline'],
1100-
token: jwt.sign(
1101-
{
1102-
username: 'robin',
1103-
pipelineId: 1,
1104-
scope: ['pipeline']
1105-
},
1106-
jwtPrivateKey,
1107-
{
1108-
algorithm: 'RS256',
1109-
expiresIn: '2h',
1110-
jwtid: 'abc'
1111-
}
1112-
)
1113-
},
1114-
strategy: ['token']
1115-
}
1116-
})
1117-
.then(reply => {
1118-
assert.equal(reply.statusCode, 200, 'Login route should be available');
1119-
assert.ok(reply.result.token, 'Token not returned');
1120-
expect(reply.result.token).to.be.a.jwt.and.deep.include({
1121-
username: 'robin',
1122-
scope: ['pipeline'],
1123-
pipelineId: 1
1124-
});
1090+
return server.inject({ url: `/auth/token?api_token=${apiKey}` }).then(reply => {
1091+
assert.equal(reply.statusCode, 200, 'Login route should be available');
1092+
assert.ok(reply.result.token, 'Token not returned');
1093+
expect(reply.result.token).to.be.a.jwt.and.deep.include({
1094+
username: 'batman',
1095+
scope: ['pipeline'],
1096+
pipelineId: 12345
11251097
});
1098+
});
11261099
});
11271100

11281101
it('fails to issue a jwt given an invalid application auth token', () => {

‎test/plugins/buildClusters.test.js

+2
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,14 @@ const getMockBuildClusters = buildClusters => {
3131

3232
describe('buildCluster plugin test', () => {
3333
const username = 'myself';
34+
const scmUserId = 123;
3435
const scmContext = 'github:github.com';
3536
const scmDisplayName = 'github';
3637
const buildClusterId = 12345;
3738
const name = 'iOS';
3839
const credentials = {
3940
scope: ['user'],
41+
scmUserId,
4042
username,
4143
scmContext
4244
};

0 commit comments

Comments
 (0)
Please sign in to comment.