From f67bbbd200ece2be4df3f44cc9d46d660fcc8aa6 Mon Sep 17 00:00:00 2001 From: Dao Lam Date: Thu, 13 Jul 2017 16:35:03 -0700 Subject: [PATCH 1/2] feat: Add create and remove template tag routes --- bin/server | 4 + lib/server.js | 1 + plugins/templates/README.md | 29 ++++- plugins/templates/createTag.js | 79 ++++++++++++++ plugins/templates/index.js | 6 +- plugins/templates/removeTag.js | 65 +++++++++++ test/plugins/templates.test.js | 190 ++++++++++++++++++++++++++++++--- 7 files changed, 354 insertions(+), 20 deletions(-) create mode 100644 plugins/templates/createTag.js create mode 100644 plugins/templates/removeTag.js diff --git a/bin/server b/bin/server index aee338254..fed8701f4 100755 --- a/bin/server +++ b/bin/server @@ -76,6 +76,9 @@ const Models = require('screwdriver-models'); const templateFactory = Models.TemplateFactory.getInstance({ datastore }); +const templateTagFactory = Models.TemplateTagFactory.getInstance({ + datastore +}); const pipelineFactory = Models.PipelineFactory.getInstance({ datastore, scm @@ -120,6 +123,7 @@ datastore.setup() notifications: notificationConfig, ecosystem, templateFactory, + templateTagFactory, pipelineFactory, jobFactory, userFactory, diff --git a/lib/server.js b/lib/server.js index 248476bd5..a4b96bf2e 100644 --- a/lib/server.js +++ b/lib/server.js @@ -88,6 +88,7 @@ module.exports = (config) => { // Instantiating the server with the factories will apply a shallow copy server.app = { templateFactory: config.templateFactory, + templateTagFactory: config.templateTagFactory, pipelineFactory: config.pipelineFactory, jobFactory: config.jobFactory, userFactory: config.userFactory, diff --git a/plugins/templates/README.md b/plugins/templates/README.md index 5424b4ed7..745383bdc 100644 --- a/plugins/templates/README.md +++ b/plugins/templates/README.md @@ -32,7 +32,7 @@ server.register({ `GET /templates?page={pageNumber}&count={countNumber}` -#### Get single template +#### Get a single template You can get a single template by providing the template name and the specific version or the tag. @@ -47,13 +47,13 @@ You can get a single template by providing the template name and the specific ve * `version` - Version of the template #### Create a template -Create a template will store the template data (`config`, `name`, `version`, `description`, `maintainer`, `labels`) into the datastore. +Creating a template will store the template data (`config`, `name`, `version`, `description`, `maintainer`, `labels`) into the datastore. If the exact template and version already exist, the only thing that can be changed is `labels`. If the template already exists but not the version, the new version will be stored provided that the build has correct permissions. -This endpoint is only accessible in `build` scope. +*Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that first creates the template.* `POST /templates` @@ -83,3 +83,26 @@ Example payload: } } ``` + +#### Create/Update a tag for a template version + +Tagging a template version allows fetching on template version by tag. For example, tag `mytemplate@1.1.0` as `stable`. + +*Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that creates the template.* + +`PUT /templates/tags` with the following payload + +* `name` - Name of the template (ex: `mytemplate`) +* `version` - Version of the template (ex: `1.1.0`) +* `tag` - Name of the tag (ex: `stable`) + +#### Delete a template tag + +Delete the template tag. This does not delete the template itself. + +*Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that creates the template.* + +`DELETE /templates/tags` with the following payload + +* `name` - Name of the template (ex: `mytemplate`) +* `tag` - Name of the tag (ex: `stable`) diff --git a/plugins/templates/createTag.js b/plugins/templates/createTag.js new file mode 100644 index 000000000..df7745fef --- /dev/null +++ b/plugins/templates/createTag.js @@ -0,0 +1,79 @@ +'use strict'; + +const boom = require('boom'); +const schema = require('screwdriver-data-schema'); +const urlLib = require('url'); + +/* Currently, only build scope is allowed to tag template due to security reasons. + * The same pipeline that publishes the template has the permission to tag it. + */ +module.exports = () => ({ + method: 'PUT', + path: '/templates/tags', + config: { + description: 'Add or update a template tag', + notes: 'Add or update a specific template', + tags: ['api', 'templates'], + auth: { + strategies: ['token', 'session'], + scope: ['build'] + }, + plugins: { + 'hapi-swagger': { + security: [{ token: [] }] + } + }, + handler: (request, reply) => { + const pipelineFactory = request.server.app.pipelineFactory; + const templateFactory = request.server.app.templateFactory; + const templateTagFactory = request.server.app.templateTagFactory; + const pipelineId = request.auth.credentials.pipelineId; + const config = request.payload; + + return Promise.all([ + pipelineFactory.get(pipelineId), + templateFactory.get({ + name: config.name, + version: config.version + }), + templateTagFactory.get({ + name: config.name, + tag: config.tag + }) + ]).then(([pipeline, template, templateTag]) => { + // If template doesn't exist, throw error + if (!template) { + throw boom.notFound(`Template ${config.name}@${config.version} not found`); + } + + // If template exists, but this build's pipelineId is not the same as template's pipelineId + // Then this build does not have permission to tag the template + if (pipeline.id !== template.pipelineId) { + throw boom.unauthorized('Not allowed to tag this template'); + } + + // If template tag exists, then the only thing it can update is the version + if (templateTag) { + templateTag.version = config.version; + + return templateTag.update().then(tag => reply(tag.toJson()).code(200)); + } + + // If template exists, then create the tag + return templateTagFactory.create(config).then((tag) => { + const location = urlLib.format({ + host: request.headers.host, + port: request.headers.port, + protocol: request.server.info.protocol, + pathname: `${request.path}/${tag.id}` + }); + + return reply(tag.toJson()).header('Location', location).code(201); + }); + }).catch(err => reply(boom.wrap(err))); + }, + validate: { + payload: schema.models.templateTag.create + } + } +}); diff --git a/plugins/templates/index.js b/plugins/templates/index.js index 67cdb0868..01702e7c1 100644 --- a/plugins/templates/index.js +++ b/plugins/templates/index.js @@ -1,9 +1,11 @@ 'use strict'; const createRoute = require('./create'); +const createTagRoute = require('./createTag'); const getRoute = require('./get'); const listRoute = require('./list'); const listVersionsRoute = require('./listVersions'); +const removeTagRoute = require('./removeTag'); /** * Template API Plugin @@ -15,9 +17,11 @@ const listVersionsRoute = require('./listVersions'); exports.register = (server, options, next) => { server.route([ createRoute(), + createTagRoute(), getRoute(), listRoute(), - listVersionsRoute() + listVersionsRoute(), + removeTagRoute() ]); next(); diff --git a/plugins/templates/removeTag.js b/plugins/templates/removeTag.js new file mode 100644 index 000000000..42ac58760 --- /dev/null +++ b/plugins/templates/removeTag.js @@ -0,0 +1,65 @@ +'use strict'; + +const boom = require('boom'); +const schema = require('screwdriver-data-schema'); + +/* Currently, only build scope is allowed to tag template due to security reasons. + * The same pipeline that publishes the template has the permission to tag it. + */ +module.exports = () => ({ + method: 'DELETE', + path: '/templates/tags', + config: { + description: 'Delete a template tag', + notes: 'Delete a specific template', + tags: ['api', 'templates'], + auth: { + strategies: ['token', 'session'], + scope: ['build'] + }, + plugins: { + 'hapi-swagger': { + security: [{ token: [] }] + } + }, + handler: (request, reply) => { + const pipelineFactory = request.server.app.pipelineFactory; + const templateFactory = request.server.app.templateFactory; + const templateTagFactory = request.server.app.templateTagFactory; + const pipelineId = request.auth.credentials.pipelineId; + const config = request.payload; + + return templateTagFactory.get({ + name: config.name, + tag: config.tag + }) + .then((templateTag) => { + if (!templateTag) { + throw boom.notFound('Template tag does not exist'); + } + + return Promise.all([ + pipelineFactory.get(pipelineId), + templateFactory.get({ + name: config.name, + version: templateTag.version + }) + ]) + .then(([pipeline, template]) => { + // Check for permission + if (pipeline.id !== template.pipelineId) { + throw boom.unauthorized('Not allowed to delete this template tag'); + } + + // Remove the template tag, not the template + return templateTag.remove(); + }); + }) + .then(() => reply().code(204)) + .catch(err => reply(boom.wrap(err))); + }, + validate: { + payload: schema.models.templateTag.remove + } + } +}); diff --git a/test/plugins/templates.test.js b/test/plugins/templates.test.js index 463814650..6d3133e31 100644 --- a/test/plugins/templates.test.js +++ b/test/plugins/templates.test.js @@ -20,36 +20,28 @@ const TEMPLATE_DESCRIPTION = [ sinon.assert.expose(assert, { prefix: '' }); -const decorateTemplateMock = (template) => { - const mock = hoek.clone(template); +const decorateObj = (obj) => { + const mock = hoek.clone(obj); - mock.toJson = sinon.stub().returns(template); - - return mock; -}; - -const decoratePipelineMock = (template) => { - const mock = hoek.clone(template); - - mock.toJson = sinon.stub().returns(template); + mock.toJson = sinon.stub().returns(obj); return mock; }; const getTemplateMocks = (templates) => { if (Array.isArray(templates)) { - return templates.map(decorateTemplateMock); + return templates.map(decorateObj); } - return decorateTemplateMock(templates); + return decorateObj(templates); }; const getPipelineMocks = (pipelines) => { if (Array.isArray(pipelines)) { - return pipelines.map(decoratePipelineMock); + return pipelines.map(decorateObj); } - return decoratePipelineMock(pipelines); + return decorateObj(pipelines); }; describe('template plugin test', () => { @@ -70,7 +62,13 @@ describe('template plugin test', () => { templateFactoryMock = { create: sinon.stub(), list: sinon.stub(), - getTemplate: sinon.stub() + getTemplate: sinon.stub(), + get: sinon.stub() + }; + templateTagFactoryMock = { + create: sinon.stub(), + get: sinon.stub(), + remove: sinon.stub() }; templateTagFactoryMock = { get: sinon.stub() @@ -401,4 +399,164 @@ describe('template plugin test', () => { }); }); }); + + describe('DELETE /templates/tags', () => { + let options; + let templateMock; + let pipelineMock; + const payload = { + name: 'testtemplate', + tag: 'stable' + }; + const testTemplateTag = decorateObj(hoek.merge({ + id: 1, + remove: sinon.stub().resolves(null) + }, payload)); + + beforeEach(() => { + options = { + method: 'DELETE', + url: '/templates/tags', + payload, + credentials: { + scope: ['build'] + } + }; + + templateMock = getTemplateMocks(testtemplate); + templateFactoryMock.get.resolves(templateMock); + + templateTagFactoryMock.get.resolves(testTemplateTag); + + pipelineMock = getPipelineMocks(testpipeline); + pipelineFactoryMock.get.resolves(pipelineMock); + }); + + it('returns 401 when pipelineId does not match', () => { + templateMock.pipelineId = 8888; + + return server.inject(options).then((reply) => { + assert.equal(reply.statusCode, 401); + }); + }); + + it('returns 404 when template tag does not exist', () => { + templateTagFactoryMock.get.resolves(null); + + return server.inject(options).then((reply) => { + assert.equal(reply.statusCode, 404); + }); + }); + + it('deletes template tag if has good permission and tag exists', () => + server.inject(options).then((reply) => { + assert.calledOnce(testTemplateTag.remove); + assert.equal(reply.statusCode, 204); + })); + }); + + describe('PUT /templates/tags', () => { + let options; + let templateMock; + let pipelineMock; + const payload = { + name: 'testtemplate', + tag: 'stable', + version: '1.2.0' + }; + const testTemplateTag = decorateObj(hoek.merge({ id: 1 }, payload)); + + beforeEach(() => { + options = { + method: 'PUT', + url: '/templates/tags', + payload, + credentials: { + scope: ['build'] + } + }; + + templateMock = getTemplateMocks(testtemplate); + templateFactoryMock.get.resolves(templateMock); + + templateTagFactoryMock.get.resolves(null); + + pipelineMock = getPipelineMocks(testpipeline); + pipelineFactoryMock.get.resolves(pipelineMock); + }); + + it('returns 401 when pipelineId does not match', () => { + templateMock.pipelineId = 8888; + + return server.inject(options).then((reply) => { + assert.equal(reply.statusCode, 401); + }); + }); + + it('returns 404 when template does not exist', () => { + templateFactoryMock.get.resolves(null); + + return server.inject(options).then((reply) => { + assert.equal(reply.statusCode, 404); + }); + }); + + it('creates template tag if has good permission and tag does not exist', () => { + templateTagFactoryMock.create.resolves(testTemplateTag); + + return server.inject(options).then((reply) => { + const expectedLocation = { + host: reply.request.headers.host, + port: reply.request.headers.port, + protocol: reply.request.server.info.protocol, + pathname: `${options.url}/1` + }; + + assert.deepEqual(reply.result, hoek.merge({ id: 1 }, payload)); + assert.strictEqual(reply.headers.location, urlLib.format(expectedLocation)); + assert.calledWith(templateFactoryMock.get, { + name: 'testtemplate', + version: '1.2.0' + }); + assert.calledWith(templateTagFactoryMock.get, { + name: 'testtemplate', + tag: 'stable' + }); + assert.calledWith(templateTagFactoryMock.create, payload); + assert.equal(reply.statusCode, 201); + }); + }); + + it('update template tag if has good permission and tag exists', () => { + const template = hoek.merge({ + update: sinon.stub().resolves(testTemplateTag) + }, testTemplateTag); + + templateTagFactoryMock.get.resolves(template); + + return server.inject(options).then((reply) => { + assert.calledWith(templateFactoryMock.get, { + name: 'testtemplate', + version: '1.2.0' + }); + assert.calledWith(templateTagFactoryMock.get, { + name: 'testtemplate', + tag: 'stable' + }); + assert.calledOnce(template.update); + assert.notCalled(templateTagFactoryMock.create); + assert.equal(reply.statusCode, 200); + }); + }); + + it('returns 500 when the template tag model fails to create', () => { + const testError = new Error('templateModelCreateError'); + + templateTagFactoryMock.create.rejects(testError); + + return server.inject(options).then((reply) => { + assert.equal(reply.statusCode, 500); + }); + }); + }); }); From 144b7254160ef15f33a09f61d2a25064e90d7ce6 Mon Sep 17 00:00:00 2001 From: Dao Lam Date: Wed, 19 Jul 2017 11:35:13 -0700 Subject: [PATCH 2/2] fix: change endpoints for create and remove tags --- plugins/templates/README.md | 40 +++++++++++++++------------------- plugins/templates/createTag.js | 39 ++++++++++++++++++--------------- plugins/templates/removeTag.js | 19 +++++++++------- test/plugins/templates.test.js | 26 ++++++++++------------ 4 files changed, 62 insertions(+), 62 deletions(-) diff --git a/plugins/templates/README.md b/plugins/templates/README.md index 745383bdc..275db75dd 100644 --- a/plugins/templates/README.md +++ b/plugins/templates/README.md @@ -27,18 +27,18 @@ server.register({ ### Routes -#### Get all templates -`page` and `count` optional +#### Template +##### Get all templates -`GET /templates?page={pageNumber}&count={countNumber}` +`GET /templates` -#### Get a single template +##### Get a single template You can get a single template by providing the template name and the specific version or the tag. `GET /templates/{name}/{tag}` or `GET /templates/{name}/{version}` -**Arguments** +###### Arguments 'name', 'tag' or 'version' @@ -46,18 +46,16 @@ You can get a single template by providing the template name and the specific ve * `tag` - Tag of the template (e.g. `stable`, `latest`, etc) * `version` - Version of the template -#### Create a template -Creating a template will store the template data (`config`, `name`, `version`, `description`, `maintainer`, `labels`) into the datastore. +##### Create a template +Creating a template will store the template data (`config`, `name`, `version`, `description`, `maintainer`) into the datastore. -If the exact template and version already exist, the only thing that can be changed is `labels`. - -If the template already exists but not the version, the new version will be stored provided that the build has correct permissions. +`version` will be auto-bumped. For example, if `mytemplate@1.0.0` already exists and the version passed in is `1.0.0`, the newly created template will be version `1.0.1`. *Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that first creates the template.* `POST /templates` -**Arguments** +###### Arguments 'name', 'version', 'description', 'maintainer', labels @@ -84,25 +82,23 @@ Example payload: } ``` -#### Create/Update a tag for a template version +#### Template Tag +Template tag allows fetching on template version by tag. For example, tag `mytemplate@1.1.0` as `stable`. + +##### Create/Update a tag -Tagging a template version allows fetching on template version by tag. For example, tag `mytemplate@1.1.0` as `stable`. +If the template tag already exists, it will update the tag with the new version. If the template tag doesn't exist yet, this endpoint will create the tag. *Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that creates the template.* -`PUT /templates/tags` with the following payload +`PUT /templates/{templateName}/tags/{tagName}` with the following payload -* `name` - Name of the template (ex: `mytemplate`) -* `version` - Version of the template (ex: `1.1.0`) -* `tag` - Name of the tag (ex: `stable`) +* `version` - Exact version of the template (ex: `1.1.0`) -#### Delete a template tag +##### Delete a tag Delete the template tag. This does not delete the template itself. *Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that creates the template.* -`DELETE /templates/tags` with the following payload - -* `name` - Name of the template (ex: `mytemplate`) -* `tag` - Name of the tag (ex: `stable`) +`DELETE /templates/{templateName}/tags/{tagName}` diff --git a/plugins/templates/createTag.js b/plugins/templates/createTag.js index df7745fef..7f28475d1 100644 --- a/plugins/templates/createTag.js +++ b/plugins/templates/createTag.js @@ -1,7 +1,9 @@ 'use strict'; const boom = require('boom'); +const joi = require('joi'); const schema = require('screwdriver-data-schema'); +const baseSchema = schema.models.templateTag.base; const urlLib = require('url'); /* Currently, only build scope is allowed to tag template due to security reasons. @@ -9,7 +11,7 @@ const urlLib = require('url'); */ module.exports = () => ({ method: 'PUT', - path: '/templates/tags', + path: '/templates/{templateName}/tags/{tagName}', config: { description: 'Add or update a template tag', notes: 'Add or update a specific template', @@ -28,22 +30,18 @@ module.exports = () => ({ const templateFactory = request.server.app.templateFactory; const templateTagFactory = request.server.app.templateTagFactory; const pipelineId = request.auth.credentials.pipelineId; - const config = request.payload; + const name = request.params.templateName; + const tag = request.params.tagName; + const version = request.payload.version; return Promise.all([ pipelineFactory.get(pipelineId), - templateFactory.get({ - name: config.name, - version: config.version - }), - templateTagFactory.get({ - name: config.name, - tag: config.tag - }) + templateFactory.get({ name, version }), + templateTagFactory.get({ name, tag }) ]).then(([pipeline, template, templateTag]) => { // If template doesn't exist, throw error if (!template) { - throw boom.notFound(`Template ${config.name}@${config.version} not found`); + throw boom.notFound(`Template ${name}@${version} not found`); } // If template exists, but this build's pipelineId is not the same as template's pipelineId @@ -54,26 +52,33 @@ module.exports = () => ({ // If template tag exists, then the only thing it can update is the version if (templateTag) { - templateTag.version = config.version; + templateTag.version = version; - return templateTag.update().then(tag => reply(tag.toJson()).code(200)); + return templateTag.update().then(newTag => reply(newTag.toJson()).code(200)); } // If template exists, then create the tag - return templateTagFactory.create(config).then((tag) => { + return templateTagFactory.create({ name, tag, version }) + .then((newTag) => { const location = urlLib.format({ host: request.headers.host, port: request.headers.port, protocol: request.server.info.protocol, - pathname: `${request.path}/${tag.id}` + pathname: `${request.path}/${newTag.id}` }); - return reply(tag.toJson()).header('Location', location).code(201); + return reply(newTag.toJson()).header('Location', location).code(201); }); }).catch(err => reply(boom.wrap(err))); }, validate: { - payload: schema.models.templateTag.create + params: { + templateName: joi.reach(baseSchema, 'name'), + tagName: joi.reach(baseSchema, 'tag') + }, + payload: { + version: joi.reach(baseSchema, 'version') + } } } }); diff --git a/plugins/templates/removeTag.js b/plugins/templates/removeTag.js index 42ac58760..e2c8c895a 100644 --- a/plugins/templates/removeTag.js +++ b/plugins/templates/removeTag.js @@ -1,14 +1,16 @@ 'use strict'; const boom = require('boom'); +const joi = require('joi'); const schema = require('screwdriver-data-schema'); +const baseSchema = schema.models.templateTag.base; /* Currently, only build scope is allowed to tag template due to security reasons. * The same pipeline that publishes the template has the permission to tag it. */ module.exports = () => ({ method: 'DELETE', - path: '/templates/tags', + path: '/templates/{templateName}/tags/{tagName}', config: { description: 'Delete a template tag', notes: 'Delete a specific template', @@ -27,12 +29,10 @@ module.exports = () => ({ const templateFactory = request.server.app.templateFactory; const templateTagFactory = request.server.app.templateTagFactory; const pipelineId = request.auth.credentials.pipelineId; - const config = request.payload; + const name = request.params.templateName; + const tag = request.params.tagName; - return templateTagFactory.get({ - name: config.name, - tag: config.tag - }) + return templateTagFactory.get({ name, tag }) .then((templateTag) => { if (!templateTag) { throw boom.notFound('Template tag does not exist'); @@ -41,7 +41,7 @@ module.exports = () => ({ return Promise.all([ pipelineFactory.get(pipelineId), templateFactory.get({ - name: config.name, + name, version: templateTag.version }) ]) @@ -59,7 +59,10 @@ module.exports = () => ({ .catch(err => reply(boom.wrap(err))); }, validate: { - payload: schema.models.templateTag.remove + params: { + templateName: joi.reach(baseSchema, 'name'), + tagName: joi.reach(baseSchema, 'tag') + } } } }); diff --git a/test/plugins/templates.test.js b/test/plugins/templates.test.js index 6d3133e31..e686d2813 100644 --- a/test/plugins/templates.test.js +++ b/test/plugins/templates.test.js @@ -70,9 +70,6 @@ describe('template plugin test', () => { get: sinon.stub(), remove: sinon.stub() }; - templateTagFactoryMock = { - get: sinon.stub() - }; pipelineFactoryMock = { get: sinon.stub() }; @@ -404,20 +401,17 @@ describe('template plugin test', () => { let options; let templateMock; let pipelineMock; - const payload = { - name: 'testtemplate', - tag: 'stable' - }; - const testTemplateTag = decorateObj(hoek.merge({ + const testTemplateTag = decorateObj({ id: 1, + name: 'testtemplate', + tag: 'stable', remove: sinon.stub().resolves(null) - }, payload)); + }); beforeEach(() => { options = { method: 'DELETE', - url: '/templates/tags', - payload, + url: '/templates/testtemplate/tags/stable', credentials: { scope: ['build'] } @@ -460,8 +454,6 @@ describe('template plugin test', () => { let templateMock; let pipelineMock; const payload = { - name: 'testtemplate', - tag: 'stable', version: '1.2.0' }; const testTemplateTag = decorateObj(hoek.merge({ id: 1 }, payload)); @@ -469,7 +461,7 @@ describe('template plugin test', () => { beforeEach(() => { options = { method: 'PUT', - url: '/templates/tags', + url: '/templates/testtemplate/tags/stable', payload, credentials: { scope: ['build'] @@ -522,7 +514,11 @@ describe('template plugin test', () => { name: 'testtemplate', tag: 'stable' }); - assert.calledWith(templateTagFactoryMock.create, payload); + assert.calledWith(templateTagFactoryMock.create, { + name: 'testtemplate', + tag: 'stable', + version: '1.2.0' + }); assert.equal(reply.statusCode, 201); }); });