diff --git a/features/banner.feature b/features/banner.feature index 6d2f59bbb..82f5f4ebc 100644 --- a/features/banner.feature +++ b/features/banner.feature @@ -19,18 +19,25 @@ Feature: Banner Scenario: Banner with global scope When they create new banner with message "Hello World" and "GLOBAL" scope - Then they can see that the banner is created with "GLOBAL" scope + Then they "can" see that the banner is created with "GLOBAL" scope And banner is "updated" when they update the banner with "message" "Some Random Message" And banner is "not updated" when they update the banner with "scopeId" "1234" And banner is "not updated" when they update the banner with "scope" "PIPELINE" + Then calvin has expired token + And they "can" see that the banner is created with "GLOBAL" scope + Then "calvin" is logged in Then banner is deleted Scenario: Banner with pipeline scope Given an existing pipeline And there is no banner associated to that pipeline When they create new banner with message "Hello World" and "PIPELINE" scope - Then they can see that the banner is created with "PIPELINE" scope + Then they "can" see that the banner is created with "PIPELINE" scope And they can get the banner associated to that pipeline And banner is "updated" when they update the banner with "isActive" "false" And banner is "not updated" when they update the banner with "scope" "GLOBAL" - Then banner is deleted + Then calvin has expired token + Then they "cannot" see that the banner is created with "PIPELINE" scope + And they cannot see any banner + Then "calvin" is logged in + Then banner is deleted \ No newline at end of file diff --git a/features/step_definitions/banner.js b/features/step_definitions/banner.js index 9e833bc18..9021cb18a 100644 --- a/features/step_definitions/banner.js +++ b/features/step_definitions/banner.js @@ -52,25 +52,37 @@ When( ); Then( - /^they can see that the banner is created with "(GLOBAL|PIPELINE)" scope$/, + /^they "(can|cannot)" see that the banner is created with "(GLOBAL|PIPELINE)" scope$/, { timeout: TIMEOUT }, - function step(scope) { + function step(ok, scope) { return request({ url: `${this.instance}/${this.namespace}/banners/${this.bannerId}`, method: 'GET', context: { token: this.jwt } - }).then(resp => { - Assert.equal(resp.statusCode, 200); - if (scope === 'PIPELINE') { - Assert.equal(resp.body.scope, scope.toUpperCase()); - Assert.equal(resp.body.scopeId, this.pipelineId); + }) + .then(resp => { + if (ok === 'can') { + Assert.equal(resp.statusCode, 200); + if (scope === 'PIPELINE') { + Assert.equal(resp.body.scope, scope.toUpperCase()); + Assert.equal(resp.body.scopeId, this.pipelineId); - return; - } - Assert.equal(resp.body.scope, 'GLOBAL'); - }); + return; + } + Assert.equal(resp.body.scope, 'GLOBAL'); + } else { + throw new Error('User should not be able to see the banner'); + } + }) + .catch(err => { + if (ok === 'can') { + console.log(err); + throw new Error('User should be able to see the banner'); + } + Assert.equal(err.statusCode, 401); + }); } ); @@ -148,3 +160,23 @@ Then(/^they can get the banner associated to that pipeline$/, { timeout: TIMEOUT Assert.equal(resp.body[0].scopeId, this.pipelineId); }); }); + +Then(/^calvin has expired token$/, { timeout: TIMEOUT }, function step() { + this.jwt = null; +}); + +Then(/^they cannot see any banner$/, { timeout: TIMEOUT }, function step() { + return request({ + url: `${this.instance}/${this.namespace}/banners`, + method: 'GET', + context: { + token: this.jwt + } + }) + .then(() => { + throw new Error('User should not be able to see any banners'); + }) + .catch(err => { + Assert.equal(err.statusCode, 401); + }); +}); diff --git a/package.json b/package.json index 08ad5d5a0..b56047947 100644 --- a/package.json +++ b/package.json @@ -7,10 +7,12 @@ "pretest": "eslint . --quiet", "test": "nyc --report-dir ./artifacts/coverage --reporter=lcov mocha --reporter mocha-multi-reporters --reporter-options configFile=./mocha.config.json --recursive --timeout 10000 --retries 1 --exit --allow-uncaught true --color true", "test-debug": "mocha --inspect-brk ./test/**/*.js", + "test-banner": "mocha ./test/**/banner.test.js", "start": "./bin/server", "debug": "node --nolazy ./bin/server", "profile": "node --prof ./bin/server", "functional": "cucumber-js --format=progress --tags '(not @ignore) and @prod' --retry 2 --fail-fast --exit", + "functional-banner": "cucumber-js --format=progress --tags '(not @ignore) and (not @prod) and @banner' --fail-fast --exit", "functional-beta": "cucumber-js --format=progress --tags '(not @ignore) and (not @prod) and (not @x1) and (not @parallel)' --retry 2 --fail-fast --exit", "functional-beta-parallel": "cucumber-js --format=progress --tags '(not @ignore) and (not @prod) and (not @x1) and @parallel' --retry 2 --fail-fast --exit --parallel 4", "functional-beta-x1-parallel": "cucumber-js --format=progress --tags '(not @ignore) and (not @prod) and @x1 and @parallel' --retry 2 --fail-fast --exit --parallel 4", diff --git a/plugins/banners/get.js b/plugins/banners/get.js index aeb8934e7..a01b1ad17 100644 --- a/plugins/banners/get.js +++ b/plugins/banners/get.js @@ -18,6 +18,11 @@ module.exports = () => ({ enabled: false } }, + auth: { + strategies: ['token'], + scope: ['user'], + mode: 'try' // This allows unauthenticated requests but still runs the auth check + }, handler: async (request, h) => { const { bannerFactory } = request.server.app; const { id } = request.params; @@ -28,6 +33,11 @@ module.exports = () => ({ if (!banner) { throw boom.notFound(`Banner ${id} does not exist`); } + if (banner.scope !== 'GLOBAL') { + if (!request.auth.isAuthenticated) { + throw boom.unauthorized('Authentication required'); + } + } return h.response(banner.toJson()); }) diff --git a/plugins/banners/list.js b/plugins/banners/list.js index 96678251e..ce073e621 100644 --- a/plugins/banners/list.js +++ b/plugins/banners/list.js @@ -2,6 +2,7 @@ const schema = require('screwdriver-data-schema'); const listSchema = schema.models.banner.list; +const boom = require('@hapi/boom'); module.exports = () => ({ method: 'GET', @@ -10,6 +11,11 @@ module.exports = () => ({ description: 'Get banners', notes: 'Returns all banner records', tags: ['api', 'banners'], + auth: { + strategies: ['token'], + scope: ['user'], + mode: 'try' // This allows unauthenticated requests but still runs the auth check + }, plugins: { 'hapi-rate-limit': { enabled: false @@ -17,6 +23,13 @@ module.exports = () => ({ }, handler: async (request, h) => { const { bannerFactory } = request.server.app; + const { scope } = request.query; + + if (scope !== 'GLOBAL') { + if (!request.auth.isAuthenticated) { + throw boom.unauthorized('Authentication required'); + } + } // list params defaults to empty object in models if undefined return bannerFactory diff --git a/test/plugins/banner.test.js b/test/plugins/banner.test.js index 81c0018ba..80a207f37 100644 --- a/test/plugins/banner.test.js +++ b/test/plugins/banner.test.js @@ -3,7 +3,9 @@ const { assert } = require('chai'); const sinon = require('sinon'); const hapi = require('@hapi/hapi'); +const boom = require('@hapi/boom'); const testBanner = require('./data/banner.json'); +const testBannerPipeline = require('./data/banner-pipeline.json'); const testBanners = require('./data/banners.json'); const testBannersActive = require('./data/banners-active.json'); const updatedBanner = require('./data/updatedBanner.json'); @@ -72,12 +74,17 @@ describe('banner plugin test', () => { }; server.auth.scheme('custom', () => ({ - authenticate: (request, h) => - h.authenticated({ + authenticate: (request, h) => { + if (request.headers['x-mock-auth'] === 'false') { + return h.unauthenticated(boom.unauthorized('Authentication required')); + } + + return h.authenticated({ credentials: { scope: ['user'] } - }) + }); + } })); server.auth.strategy('token', 'custom'); @@ -192,18 +199,17 @@ describe('banner plugin test', () => { url: '/banners' }; }); - - it('returns 200 for listing banners', () => { + it('returns 401 for listing banners', () => { + options.headers = { 'x-mock-auth': 'false' }; // Force authentication failure bannerFactoryMock.list.resolves(getBannerMock(testBanners)); return server.inject(options).then(reply => { - assert.equal(reply.statusCode, 200); - assert.deepEqual(reply.result, testBanners); + assert.equal(reply.statusCode, 401); }); }); - it('returns 200 for listing banners with query params', () => { - options.url = '/banners?isActive=true'; + it('returns 200 for listing active banners with GLOBAL scope', () => { + options.url = '/banners?scope=GLOBAL&isActive=true'; bannerFactoryMock.list.resolves(getBannerMock(testBannersActive)); return server.inject(options).then(reply => { @@ -234,7 +240,7 @@ describe('banner plugin test', () => { }; }); - it('returns 200 for get banner', () => { + it('returns 200 for get banner with GLOBAL scope', () => { bannerFactoryMock.get.withArgs(id).resolves(bannerMock); return server.inject(options).then(reply => { @@ -248,6 +254,22 @@ describe('banner plugin test', () => { }); }); + it('returns 200 for get banner with PIPELINE scope', () => { + const pipelineBannerMock = getMock(testBannerPipeline); + + bannerFactoryMock.get.withArgs(id).resolves(pipelineBannerMock); + + return server.inject(options).then(reply => { + const expected = { ...testBannerPipeline }; + + delete expected.id; + delete expected.createdBy; + delete expected.createTime; + assert.equal(reply.statusCode, 200); + assert.deepEqual(reply.result, testBannerPipeline); + }); + }); + it('returns 404 when banner does not exist', () => { bannerFactoryMock.get.withArgs(id).resolves(null); diff --git a/test/plugins/data/banner-pipeline.json b/test/plugins/data/banner-pipeline.json new file mode 100644 index 000000000..0d2afd04a --- /dev/null +++ b/test/plugins/data/banner-pipeline.json @@ -0,0 +1,10 @@ +{ + "id": 123, + "message": "Test banner example", + "isActive": true, + "type": "info", + "scope": "PIPELINE", + "scopeId": 1, + "createdBy": "jimgrund", + "createTime": "2017-01-06T01:49:50.384359267Z" +}