fix: Screwdriver admins should be able to remove pipelines (#1734)

screwdriver-cd · Sep 5, 2019 · ccead74 · ccead74
1 parent ca0cccd
commit ccead74
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 36 deletions.
diff --git a/package.json b/package.json
@@ -61,13 +61,13 @@
     "Tiffany Kyi <tiffanykyi@gmail.com>"
   ],
   "dependencies": {
-    "async": "^2.6.2",
+    "async": "^2.6.3",
     "bell": "^8.0.0",
     "boom": "^7.3.0",
     "config": "^1.31.0",
     "crumb": "^6.1.0",
     "date-fns": "^1.30.1",
-    "dayjs": "^1.8.14",
+    "dayjs": "^1.8.16",
     "good": "^7.0.0",
     "good-console": "^6.1.1",
     "good-squeeze": "^5.1.0",
@@ -93,24 +93,24 @@
     "screwdriver-config-parser": "^4.11.1",
     "screwdriver-coverage-bookend": "^1.0.2",
     "screwdriver-coverage-sonar": "^1.0.22",
-    "screwdriver-data-schema": "^18.46.3",
+    "screwdriver-data-schema": "^18.46.10",
     "screwdriver-datastore-sequelize": "^5.7.2",
     "screwdriver-executor-docker": "^4.2.0",
     "screwdriver-executor-k8s": "^13.6.1",
     "screwdriver-executor-k8s-vm": "^2.10.0",
     "screwdriver-executor-queue": "^2.4.22",
     "screwdriver-executor-router": "^1.0.11",
-    "screwdriver-models": "^27.34.2",
+    "screwdriver-models": "^27.34.8",
     "screwdriver-notifications-email": "^1.1.9",
     "screwdriver-notifications-slack": "^2.3.0",
     "screwdriver-scm-github": "^9.3.2",
     "screwdriver-scm-gitlab": "^1.3.1",
     "screwdriver-scm-router": "^4.1.0",
     "screwdriver-template-validator": "^3.0.6",
-    "screwdriver-workflow-parser": "^1.8.6",
-    "sqlite3": "^4.0.7",
+    "screwdriver-workflow-parser": "^1.8.8",
+    "sqlite3": "^4.1.0",
     "tinytim": "^0.1.1",
-    "uuid": "^3.3.2",
+    "uuid": "^3.3.3",
     "verror": "^1.6.1",
     "vision": "^4.1.0",
     "winston": "^2.4.4"
@@ -124,15 +124,15 @@
     "cucumber": "2.3.1",
     "eslint": "^4.19.1",
     "eslint-config-screwdriver": "^3.0.1",
-    "form-data": "^2.3.3",
+    "form-data": "^2.5.1",
     "jenkins-mocha": "^7.0.0",
     "mockery": "^2.0.0",
     "mz": "^2.6.0",
     "nock": "^9.6.1",
     "node-plantuml": "^0.5.0",
     "npm-auto-version": "^1.0.0",
     "rewire": "^4.0.1",
-    "sinon": "^7.3.2",
+    "sinon": "^7.4.2",
     "stream-to-promise": "^2.2.0"
   }
 }
diff --git a/plugins/pipelines/remove.js b/plugins/pipelines/remove.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const boom = require('boom');
-const hoek = require('hoek');
 const joi = require('joi');
 const schema = require('screwdriver-data-schema');
 const idSchema = joi.reach(schema.models.pipeline.base, 'id');
@@ -27,8 +26,6 @@ module.exports = () => ({
             const userFactory = request.server.app.userFactory;
             const username = request.auth.credentials.username;
             const scmContext = request.auth.credentials.scmContext;
-            const scms = hoek.reach(pipelineFactory, 'scm.scms') || {};
-            const isPrivateRepo = hoek.reach(scms[scmContext], 'config.privateRepo') || false;
 
             // Fetch the pipeline and user models
             return Promise.all([
@@ -60,8 +57,8 @@ module.exports = () => ({
                         const adminDetails = request.server.plugins.banners
                             .screwdriverAdminDetails(username, scmContext);
 
-                        // Allow cluster admins to remove pipeline if the repository does not exist
-                        if (error.code === 404 && !isPrivateRepo && adminDetails.isAdmin) {
+                        // Allow cluster admins to remove pipeline
+                        if (adminDetails.isAdmin) {
                             return Promise.resolve(null);
                         }
 

diff --git a/test/plugins/pipelines.test.js b/test/plugins/pipelines.test.js
@@ -207,7 +207,7 @@ describe('pipeline plugin test', () => {
             name: 'banners'
         };
 
-        screwdriverAdminDetailsMock = sinon.stub().returns({ isAdmin: true });
+        screwdriverAdminDetailsMock = sinon.stub();
 
         /* eslint-disable global-require */
         plugin = require('../../plugins/pipelines');
@@ -522,6 +522,7 @@ describe('pipeline plugin test', () => {
 
         afterEach(() => {
             pipelineFactoryMock.get.withArgs(id).reset();
+            screwdriverAdminDetailsMock.reset();
         });
 
         it('returns 204 when delete successfully', () =>
@@ -531,22 +532,28 @@ describe('pipeline plugin test', () => {
             })
         );
 
-        it('returns 204 when repository does not exist and user is admin', () => {
+        it('returns 204 when repository does not exist and user is Screwdriver admin', () => {
             userMock.getPermissions.withArgs(scmUri).rejects({ code: 404 });
+            screwdriverAdminDetailsMock.returns({ isAdmin: true });
 
             return server.inject(options).then((reply) => {
                 assert.equal(reply.statusCode, 204);
                 assert.calledOnce(pipeline.remove);
             });
         });
 
-        it('returns 403 when user does not have admin permission', () => {
+        it('returns 403 when user does not have admin permission and is not ' +
+            'Screwdriver admin', () => {
             const error = {
                 statusCode: 403,
                 error: 'Forbidden',
-                message: 'User myself does not have admin permission for this repo'
+                message: 'User d2lam does not have admin permission for this repo'
             };
 
+            screwdriverAdminDetailsMock.returns({ isAdmin: false });
+            options.credentials.username = 'd2lam';
+            userMock = getUserMock({ username: 'd2lam', scmContext });
+            userFactoryMock.get.withArgs({ username: 'd2lam', scmContext }).resolves(userMock);
             userMock.getPermissions.withArgs(scmUri).resolves({ admin: false });
 
             return server.inject(options).then((reply) => {
@@ -555,7 +562,7 @@ describe('pipeline plugin test', () => {
             });
         });
 
-        it('returns 403 when the pipeline is child piepline', () => {
+        it('returns 403 when the pipeline is child pipeline', () => {
             pipeline.configPipelineId = 123;
 
             return server.inject(options).then((reply) => {
@@ -600,23 +607,6 @@ describe('pipeline plugin test', () => {
                 assert.equal(reply.statusCode, 500);
             });
         });
-
-        it('returns 500 when repository does not exist and private repo is enabled', () => {
-            const scms = {
-                'github:github.com': {
-                    config: {
-                        privateRepo: true
-                    }
-                }
-            };
-
-            pipelineFactoryMock.scm.scms = scms;
-            userMock.getPermissions.withArgs(scmUri).rejects({ code: 404 });
-
-            return server.inject(options).then((reply) => {
-                assert.equal(reply.statusCode, 500);
-            });
-        });
     });
 
     describe('GET /pipelines/{id}/jobs', () => {