Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(1079): integrate deploy keys features with auth and pipeline routes [5] #2157

Merged
merged 16 commits into from Aug 6, 2020
Merged

feat(1079): integrate deploy keys features with auth and pipeline routes [5] #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
eac6bdc
feat(): add autoKeysGeneration key value in auth contexts and pipelin…
supra08 Jul 20, 2020
8ea65eb
feat(): add tests for autoKeysGeneration in auth contexts and pipelin…
supra08 Jul 20, 2020
070e7c6
fix(): fix deploy key secret name
supra08 Jul 20, 2020
8e5320d
refactor(): change function names to autoDeployKeyGenerationEnabled
supra08 Jul 23, 2020
7db30b5
refactor(): Update plugins/pipelines/create.js
supra08 Jul 25, 2020
2293cd6
refactor(): Add return statements
supra08 Jul 28, 2020
8c02a17
cleanup(): Remove commented code
supra08 Jul 28, 2020
d4cbc25
lint(): Add return statement corresponding to arrow function
supra08 Aug 4, 2020
e8898b3
fix(): Make autoDeployKeyGenerationEnabled to that of non promise fun…
supra08 Aug 4, 2020
4727675
fix(): Fix secret value in test
supra08 Aug 4, 2020
a05200a
fix(): Remove redundant usage of pipelineFactory
supra08 Aug 5, 2020
a075490
refactor(): remove unreachable paths and change variable names
supra08 Aug 5, 2020
371d072
cleanup(): remove straw console logs
supra08 Aug 5, 2020
d55ba14
fix(): Remove unnecessary get secret test assertion
supra08 Aug 5, 2020
fd87e14
fix(): Add necessary catch block
supra08 Aug 5, 2020
9f11578
fix(): remove unnecessary catch block
supra08 Aug 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions plugins/auth/contexts.js
View file
Open in desktop
Expand Up @@ -24,7 +24,10 @@ module.exports = config => ({
scmContexts.forEach(scmContext => {
const context = {
context: scmContext,
displayName: scm.getDisplayName({ scmContext })
displayName: scm.getDisplayName({ scmContext }),
autoDeployKeyGeneration: scm.autoDeployKeyGenerationEnabled({
scmContext
jithine marked this conversation as resolved.
Show resolved Hide resolved
})
};

contexts.push(context);
Expand All @@ -33,7 +36,8 @@ module.exports = config => ({
if (config.allowGuestAccess) {
contexts.push({
context: 'guest',
displayName: 'Guest Access'
displayName: 'Guest Access',
autoDeployKeyGeneration: false
});
}

Expand Down
45 changes: 38 additions & 7 deletions plugins/pipelines/create.js
View file
Open in desktop
Expand Up @@ -24,9 +24,11 @@ module.exports = () => ({
handler: (request, reply) => {
const checkoutUrl = helper.formatCheckoutUrl(request.payload.checkoutUrl);
const rootDir = helper.sanitizeRootDir(request.payload.rootDir);
const { pipelineFactory, userFactory, collectionFactory } = request.server.app;
const { username } = request.auth.credentials;
const { scmContext } = request.auth.credentials;
const { autoKeysGeneration } = request.payload;
const { pipelineFactory, userFactory, collectionFactory, secretFactory } = request.server.app;
const { username, scmContext } = request.auth.credentials;
let pipelineToken = '';
const deployKeySecret = 'SD_SCM_DEPLOY_KEY';

// fetch the user
return (
Expand All @@ -35,6 +37,11 @@ module.exports = () => ({
.then(user =>
user
.unsealToken()
.then(token => {
pipelineToken = token;

return token;
})
.then(token =>
pipelineFactory.scm.parseUrl({
scmContext,
Expand Down Expand Up @@ -78,8 +85,8 @@ module.exports = () => ({
return pipelineFactory.create(pipelineConfig);
})
// get the default collection for current user
.then(pipeline =>
collectionFactory
.then(pipeline => {
return collectionFactory
.list({
params: {
userId: user.id,
Expand Down Expand Up @@ -113,6 +120,30 @@ module.exports = () => ({

return defaultCollection.update();
})
.then(() => {
if (autoKeysGeneration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This boolean check if scm, such as github has ability to do autoKeysGeneration.

What if such scm, say bitbucket does not have such feature and autoKeysGeneration = true from the payload. How do we handle that case? Would there be:

  1. add a catch block
  2. add another if condition to check both payload sent from ui has autoKeysGeneration: true, as well as check if given SCM has such ability.

In UI, we can prevent user giving input for if scm does not have autoKeysGeneration feature, but we also need to sanitize and guard on API. I can see some power users would call this API directly.

Just a thought, what do you think?

Note:

From: #1079 (comment)

If we apply this feature to all SCMs: scm-github, scm-bitbucket, then we dont have such issue, just need to catch the errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I get the point. I didn't take the case of direct API calls.
Catch is definitely needed. And it will be handled by the not implemented promise rejection in scm base.
I am wondering if the 2nd point is needed because if autoKeysGeneration is set to true in the payload, it will be rejected in the addDeployKey method and eventually sent to the catch block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I meant either 1) or 2).

Since handled that case in 1), 2) is no longer necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright cool 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we already have a catch at end of Promise chain ?

return pipelineFactory.scm
.addDeployKey({
scmContext,
checkoutUrl,
token: pipelineToken
})
.then(privateDeployKey => {
const privateDeployKeyB64 = Buffer.from(
privateDeployKey
).toString('base64');

return secretFactory.create({
pipelineId: pipeline.id,
name: deployKeySecret,
value: privateDeployKeyB64,
allowInPR: false
});
});
}

return null;
})
.then(() => {
// TODO: decide to put this outside or inside
Promise.all([
Expand All @@ -130,8 +161,8 @@ module.exports = () => ({
.header('Location', location)
.code(201);
});
})
)
});
})
)
)
// something broke, respond with error
Expand Down
21 changes: 16 additions & 5 deletions test/plugins/auth.test.js
View file
Open in desktop
Expand Up @@ -82,7 +82,8 @@ describe('auth plugin test', () => {
provider: 'github',
scope: ['admin:repo_hook', 'read:org', 'repo:status']
}
})
}),
autoDeployKeyGenerationEnabled: sinon.stub().returns(true)
};
userFactoryMock = {
get: sinon.stub(),
Expand Down Expand Up @@ -1049,6 +1050,7 @@ describe('auth plugin test', () => {
scm.getScmContexts.returns(['github:github.com', 'github:mygithub.com']);
scm.getDisplayName.withArgs({ scmContext: 'github:github.com' }).returns('github');
scm.getDisplayName.withArgs({ scmContext: 'github:mygithub.com' }).returns('mygithub');
scm.autoDeployKeyGenerationEnabled.withArgs({ scmContext: 'github:mygithub.com' }).returns(true);
});

it('lists the contexts', () =>
Expand All @@ -1062,9 +1064,9 @@ describe('auth plugin test', () => {
assert.deepEqual(
reply.result,
[
{ context: 'github:github.com', displayName: 'github' },
{ context: 'github:mygithub.com', displayName: 'mygithub' },
{ context: 'guest', displayName: 'Guest Access' }
{ context: 'github:github.com', displayName: 'github', autoDeployKeyGeneration: true },
{ context: 'github:mygithub.com', displayName: 'mygithub', autoDeployKeyGeneration: true },
{ context: 'guest', displayName: 'Guest Access', autoDeployKeyGeneration: false }
],
'Contexts returns data'
);
Expand All @@ -1074,6 +1076,8 @@ describe('auth plugin test', () => {
scm.getScmContexts.returns(['github:github.com']);
server = new hapi.Server();
server.app.userFactory = userFactoryMock;
server.app.pipelineFactory = pipelineFactoryMock;
scm.autoDeployKeyGenerationEnabled.withArgs({ scmContext: 'github:mygithub.com' }).returns(true);

server.connection({
port: 1234
Expand Down Expand Up @@ -1101,10 +1105,17 @@ describe('auth plugin test', () => {
url: '/auth/contexts'
})
.then(reply => {
console.log(reply.result[0]);
assert.equal(reply.statusCode, 200, 'Contexts should be available');
assert.deepEqual(
reply.result,
[{ context: 'github:github.com', displayName: 'github' }],
[
{
context: 'github:github.com',
displayName: 'github',
autoDeployKeyGeneration: true
}
],
'Contexts returns data'
);
})
Expand Down
21 changes: 20 additions & 1 deletion test/plugins/pipelines.test.js
View file
Open in desktop
Expand Up @@ -166,6 +166,7 @@ describe('pipeline plugin test', () => {
let tokenFactoryMock;
let jobFactoryMock;
let triggerFactoryMock;
let secretFactoryMock;
let bannerMock;
let screwdriverAdminDetailsMock;
let scmMock;
Expand All @@ -191,7 +192,8 @@ describe('pipeline plugin test', () => {
getScmContexts: sinon.stub(),
parseUrl: sinon.stub(),
decorateUrl: sinon.stub(),
getCommitSha: sinon.stub().resolves('sha')
getCommitSha: sinon.stub().resolves('sha'),
addDeployKey: sinon.stub()
}
};
userFactoryMock = {
Expand All @@ -218,6 +220,10 @@ describe('pipeline plugin test', () => {
triggerFactoryMock = {
getTriggers: sinon.stub()
};
secretFactoryMock = {
create: sinon.stub(),
get: sinon.stub()
};
bannerMock = {
register: (s, o, next) => {
s.expose('screwdriverAdminDetails', screwdriverAdminDetailsMock);
Expand All @@ -242,6 +248,7 @@ describe('pipeline plugin test', () => {
userFactory: userFactoryMock,
collectionFactory: collectionFactoryMock,
tokenFactory: tokenFactoryMock,
secretFactory: secretFactoryMock,
ecosystem: {
badges: '{{subject}}/{{status}}/{{color}}'
}
Expand Down Expand Up @@ -1385,6 +1392,8 @@ describe('pipeline plugin test', () => {
const testId = '123';
const username = 'd2lam';
const userId = '34';
const privateKey = 'testkey';
const privateKeyB64 = Buffer.from(privateKey).toString('base64');

beforeEach(() => {
options = {
Expand Down Expand Up @@ -1415,12 +1424,16 @@ describe('pipeline plugin test', () => {
pipelineFactoryMock.create.resolves(pipelineMock);

pipelineFactoryMock.scm.parseUrl.resolves(scmUri);
pipelineFactoryMock.scm.addDeployKey.resolves(privateKey);

secretFactoryMock.get.withArgs({ pipelineId: 123, name: 'SD_SCM_DEPLOY_KEY' }).resolves(null);
});

it('returns 201 and correct pipeline data', () => {
let expectedLocation;
const testDefaultCollection = Object.assign(testCollection, { type: 'default' });

options.payload.autoKeysGeneration = true;
collectionFactoryMock.list
.withArgs({
params: {
Expand Down Expand Up @@ -1452,6 +1465,12 @@ describe('pipeline plugin test', () => {
type: 'default'
}
});
assert.calledWith(secretFactoryMock.create, {
pipelineId: 123,
name: 'SD_SCM_DEPLOY_KEY',
value: privateKeyB64,
allowInPR: false
});
assert.equal(reply.statusCode, 201);
});
});
Expand Down