From 9b58b8e363fd388635385085c58be3d4637eaa45 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Mon, 6 Nov 2017 22:20:44 +0900
Subject: Do not allow jobs to be erased

---
 app/controllers/projects/jobs_controller.rb |  5 ++++
 app/models/ci/build.rb                      |  4 +++
 app/policies/ci/build_policy.rb             |  5 ++++
 app/serializers/build_details_entity.rb     |  2 +-
 app/views/projects/jobs/show.html.haml      |  2 +-
 lib/api/jobs.rb                             |  2 +-
 lib/api/v3/builds.rb                        |  2 +-
 spec/policies/ci/build_policy_spec.rb       | 42 +++++++++++++++++++++++++++++
 8 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb
index 1b985ea9763..fd6708666c3 100644
--- a/app/controllers/projects/jobs_controller.rb
+++ b/app/controllers/projects/jobs_controller.rb
@@ -5,6 +5,7 @@ class Projects::JobsController < Projects::ApplicationController
     only: [:index, :show, :status, :raw, :trace]
   before_action :authorize_update_build!,
     except: [:index, :show, :status, :raw, :trace, :cancel_all]
+  before_action :authorize_erase_build!, only: [:erase]
 
   layout 'project'
 
@@ -131,6 +132,10 @@ class Projects::JobsController < Projects::ApplicationController
     return access_denied! unless can?(current_user, :update_build, build)
   end
 
+  def authorize_erase_build!
+    return access_denied! unless can?(current_user, :erase_build, build)
+  end
+
   def build
     @build ||= project.builds.find(params[:id])
       .present(current_user: current_user)
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 6ca46ae89c1..0d992c4c01f 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -192,6 +192,10 @@ module Ci
       project.build_timeout
     end
 
+    def owned_by?(current_user)
+      user == current_user
+    end
+
     # A slugified version of the build ref, suitable for inclusion in URLs and
     # domain names. Rules:
     #
diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index 984e5482288..f158cda2f0e 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -10,6 +10,11 @@ module Ci
       end
     end
 
+    condition(:owner_of_build) do
+      can?(:developer_access) && @subject.owned_by?(@user)
+    end
+
     rule { protected_ref }.prevent :update_build
+    rule { can?(:master_access) | owner_of_build }.enable :erase_build
   end
 end
diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
index 8c89eea607f..69d46f5ec14 100644
--- a/app/serializers/build_details_entity.rb
+++ b/app/serializers/build_details_entity.rb
@@ -6,7 +6,7 @@ class BuildDetailsEntity < JobEntity
   expose :pipeline, using: PipelineEntity
 
   expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity
-  expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :update_build, project) } do |build|
+  expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build|
     erase_project_job_path(project, build)
   end
 
diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml
index ce0e3872240..2abd2c9e652 100644
--- a/app/views/projects/jobs/show.html.haml
+++ b/app/views/projects/jobs/show.html.haml
@@ -71,7 +71,7 @@
                     class: 'js-raw-link-controller has-tooltip controllers-buttons' do
               = icon('file-text-o')
 
-          - if can?(current_user, :update_build, @project) && @build.erasable?
+          - if @build.erasable? && can?(current_user, :erase_build, @build)
             = link_to erase_project_job_path(@project, @build),
                       method: :post,
                       data: { confirm: 'Are you sure you want to erase this build?', placement: 'top', container: 'body' },
diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb
index 3c1c412ba42..a116ab3c9bd 100644
--- a/lib/api/jobs.rb
+++ b/lib/api/jobs.rb
@@ -136,7 +136,7 @@ module API
         authorize_update_builds!
 
         build = find_build!(params[:job_id])
-        authorize!(:update_build, build)
+        authorize!(:erase_build, build)
         return forbidden!('Job is not erasable!') unless build.erasable?
 
         build.erase(erased_by: current_user)
diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb
index f493fd7c7ec..fa0bef39602 100644
--- a/lib/api/v3/builds.rb
+++ b/lib/api/v3/builds.rb
@@ -169,7 +169,7 @@ module API
           authorize_update_builds!
 
           build = get_build!(params[:build_id])
-          authorize!(:update_build, build)
+          authorize!(:erase_build, build)
           return forbidden!('Build is not erasable!') unless build.erasable?
 
           build.erase(erased_by: current_user)
diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb
index 8e1bc3d1543..e5d5e1017cd 100644
--- a/spec/policies/ci/build_policy_spec.rb
+++ b/spec/policies/ci/build_policy_spec.rb
@@ -150,5 +150,47 @@ describe Ci::BuildPolicy do
         end
       end
     end
+
+    # TODO: Finish spec
+    describe 'rules for erase build' do
+      let(:project) { create(:project, :repository) }
+      let(:another_user) { create(:user) }
+
+      context 'when developer created a build' do
+        before do
+          project.add_developer(user)
+        end
+
+        context 'when the build was created by the user' do
+          let(:build) { create(:ci_build, user: user) }
+
+          it { expect(policy).to be_allowed :erase_build }
+        end
+
+        context 'when the build was created by others' do
+          let(:build) { create(:ci_build, user: another_user) }
+
+          it { expect(policy).to be_disallowed :erase_build }
+        end
+      end
+
+      context 'when master erases a build' do
+        before do
+          project.add_master(user)
+        end
+
+        context 'when the build was created by the user' do
+          let(:build) { create(:ci_build, user: user) }
+
+          it { expect(policy).to be_allowed :erase_build }
+        end
+
+        context 'when the build was created by others' do
+          let(:build) { create(:ci_build, user: another_user) }
+
+          it { expect(policy).to be_allowed :erase_build }
+        end
+      end
+    end
   end
 end
-- 
cgit v1.2.1


From c8eb2a914b6f9348ffa16436853964998c115085 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Mon, 6 Nov 2017 23:24:25 +0900
Subject: Fix spec. Revert update check.

---
 lib/api/jobs.rb                       |  1 +
 lib/api/v3/builds.rb                  |  1 +
 spec/models/ci/build_spec.rb          | 17 +++++++++++++++++
 spec/policies/ci/build_policy_spec.rb | 11 +++++------
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb
index a116ab3c9bd..6dcbe2ff936 100644
--- a/lib/api/jobs.rb
+++ b/lib/api/jobs.rb
@@ -136,6 +136,7 @@ module API
         authorize_update_builds!
 
         build = find_build!(params[:job_id])
+        authorize!(:update_build, build)
         authorize!(:erase_build, build)
         return forbidden!('Job is not erasable!') unless build.erasable?
 
diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb
index fa0bef39602..1c0f9f73c78 100644
--- a/lib/api/v3/builds.rb
+++ b/lib/api/v3/builds.rb
@@ -169,6 +169,7 @@ module API
           authorize_update_builds!
 
           build = get_build!(params[:build_id])
+          authorize!(:update_build, build)
           authorize!(:erase_build, build)
           return forbidden!('Build is not erasable!') unless build.erasable?
 
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 5ed2e1ca99a..88f7b1775a0 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -270,6 +270,23 @@ describe Ci::Build do
     end
   end
 
+  describe '#owned_by?' do
+    subject { build.owned_by?(user) }
+
+    context 'when user is owner' do
+      let(:build) { create(:ci_build, pipeline: pipeline, user: user) }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when user is not owner' do
+      let(:another_user) { create(:user) }
+      let(:build) { create(:ci_build, pipeline: pipeline, user: another_user) }
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe '#detailed_status' do
     it 'returns a detailed status' do
       expect(build.detailed_status(user))
diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb
index e5d5e1017cd..d8e73e4a890 100644
--- a/spec/policies/ci/build_policy_spec.rb
+++ b/spec/policies/ci/build_policy_spec.rb
@@ -151,10 +151,9 @@ describe Ci::BuildPolicy do
       end
     end
 
-    # TODO: Finish spec
     describe 'rules for erase build' do
       let(:project) { create(:project, :repository) }
-      let(:another_user) { create(:user) }
+      let(:build) { create(:ci_build, pipeline: pipeline, user: owner) }
 
       context 'when developer created a build' do
         before do
@@ -162,13 +161,13 @@ describe Ci::BuildPolicy do
         end
 
         context 'when the build was created by the user' do
-          let(:build) { create(:ci_build, user: user) }
+          let(:owner) { user }
 
           it { expect(policy).to be_allowed :erase_build }
         end
 
         context 'when the build was created by others' do
-          let(:build) { create(:ci_build, user: another_user) }
+          let(:owner) { create(:user) }
 
           it { expect(policy).to be_disallowed :erase_build }
         end
@@ -180,13 +179,13 @@ describe Ci::BuildPolicy do
         end
 
         context 'when the build was created by the user' do
-          let(:build) { create(:ci_build, user: user) }
+          let(:owner) { user }
 
           it { expect(policy).to be_allowed :erase_build }
         end
 
         context 'when the build was created by others' do
-          let(:build) { create(:ci_build, user: another_user) }
+          let(:owner) { create(:user) }
 
           it { expect(policy).to be_allowed :erase_build }
         end
-- 
cgit v1.2.1


From cb5e35d562a1bf0737c1ad3316c3723775fada01 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Mon, 6 Nov 2017 23:27:31 +0900
Subject: Add change log

---
 .../unreleased/fix-sm-31771-do-not-allow-jobs-to-be-erased-new.yml   | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/fix-sm-31771-do-not-allow-jobs-to-be-erased-new.yml

diff --git a/changelogs/unreleased/fix-sm-31771-do-not-allow-jobs-to-be-erased-new.yml b/changelogs/unreleased/fix-sm-31771-do-not-allow-jobs-to-be-erased-new.yml
new file mode 100644
index 00000000000..198116f34aa
--- /dev/null
+++ b/changelogs/unreleased/fix-sm-31771-do-not-allow-jobs-to-be-erased-new.yml
@@ -0,0 +1,5 @@
+---
+title: Only owner or master can erase jobs
+merge_request: 15216
+author:
+type: changed
-- 
cgit v1.2.1


From afef38533727cf32a7be324243a25b4db5eb5498 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Tue, 7 Nov 2017 02:47:05 +0900
Subject: Add doc. Fix spec. Add erase_build in protected_ref rule

---
 app/controllers/projects/jobs_controller.rb       |  2 +-
 app/models/ci/build.rb                            |  2 +-
 app/policies/ci/build_policy.rb                   | 12 ++++---
 doc/user/permissions.md                           |  2 ++
 lib/api/jobs.rb                                   |  1 -
 lib/api/v3/builds.rb                              |  1 -
 spec/controllers/projects/jobs_controller_spec.rb | 25 +++++++++++++-
 spec/models/ci/build_spec.rb                      |  4 +--
 spec/policies/ci/build_policy_spec.rb             | 40 ++++++++++++++++-------
 spec/requests/api/jobs_spec.rb                    |  2 ++
 spec/requests/api/v3/builds_spec.rb               |  2 ++
 11 files changed, 71 insertions(+), 22 deletions(-)

diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb
index fd6708666c3..1c4c09c772f 100644
--- a/app/controllers/projects/jobs_controller.rb
+++ b/app/controllers/projects/jobs_controller.rb
@@ -4,7 +4,7 @@ class Projects::JobsController < Projects::ApplicationController
   before_action :authorize_read_build!,
     only: [:index, :show, :status, :raw, :trace]
   before_action :authorize_update_build!,
-    except: [:index, :show, :status, :raw, :trace, :cancel_all]
+    except: [:index, :show, :status, :raw, :trace, :cancel_all, :erase]
   before_action :authorize_erase_build!, only: [:erase]
 
   layout 'project'
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 0d992c4c01f..1b2b0d17910 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -192,7 +192,7 @@ module Ci
       project.build_timeout
     end
 
-    def owned_by?(current_user)
+    def triggered_by?(current_user)
       user == current_user
     end
 
diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb
index f158cda2f0e..1ab391a5a9d 100644
--- a/app/policies/ci/build_policy.rb
+++ b/app/policies/ci/build_policy.rb
@@ -10,11 +10,15 @@ module Ci
       end
     end
 
-    condition(:owner_of_build) do
-      can?(:developer_access) && @subject.owned_by?(@user)
+    condition(:owner_of_job) do
+      can?(:developer_access) && @subject.triggered_by?(@user)
     end
 
-    rule { protected_ref }.prevent :update_build
-    rule { can?(:master_access) | owner_of_build }.enable :erase_build
+    rule { protected_ref }.policy do
+      prevent :update_build
+      prevent :erase_build
+    end
+
+    rule { can?(:master_access) | owner_of_job }.enable :erase_build
   end
 end
diff --git a/doc/user/permissions.md b/doc/user/permissions.md
index c03700a3501..b9532bf897f 100644
--- a/doc/user/permissions.md
+++ b/doc/user/permissions.md
@@ -197,6 +197,7 @@ instance and project. In addition, all admins can use the admin interface under
 |---------------------------------------|-----------------|-------------|----------|--------|
 | See commits and jobs                  | ✓               | ✓           | ✓        | ✓      |
 | Retry or cancel job                   |                 | ✓           | ✓        | ✓      |
+| Erase job artifacts and trace         |                 | ✓ [^7]      | ✓        | ✓      |
 | Remove project                        |                 |             | ✓        | ✓      |
 | Create project                        |                 |             | ✓        | ✓      |
 | Change project configuration          |                 |             | ✓        | ✓      |
@@ -261,5 +262,6 @@ only.
 [^4]: Not allowed for Guest, Reporter, Developer, Master, or Owner
 [^5]: Only if user is not external one.
 [^6]: Only if user is a member of the project.
+[^7]: Only if the build was triggered by the user
 [ce-18994]: https://gitlab.com/gitlab-org/gitlab-ce/issues/18994
 [new-mod]: project/new_ci_build_permissions_model.md
diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb
index 6dcbe2ff936..a116ab3c9bd 100644
--- a/lib/api/jobs.rb
+++ b/lib/api/jobs.rb
@@ -136,7 +136,6 @@ module API
         authorize_update_builds!
 
         build = find_build!(params[:job_id])
-        authorize!(:update_build, build)
         authorize!(:erase_build, build)
         return forbidden!('Job is not erasable!') unless build.erasable?
 
diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb
index 1c0f9f73c78..fa0bef39602 100644
--- a/lib/api/v3/builds.rb
+++ b/lib/api/v3/builds.rb
@@ -169,7 +169,6 @@ module API
           authorize_update_builds!
 
           build = get_build!(params[:build_id])
-          authorize!(:update_build, build)
           authorize!(:erase_build, build)
           return forbidden!('Build is not erasable!') unless build.erasable?
 
diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb
index f9688949a19..804075782c1 100644
--- a/spec/controllers/projects/jobs_controller_spec.rb
+++ b/spec/controllers/projects/jobs_controller_spec.rb
@@ -372,12 +372,14 @@ describe Projects::JobsController do
 
   describe 'POST erase' do
     before do
-      project.add_developer(user)
+      project.team << [user, role]
       sign_in(user)
 
       post_erase
     end
 
+    let(:role) { :master }
+
     context 'when job is erasable' do
       let(:job) { create(:ci_build, :erasable, :trace, pipeline: pipeline) }
 
@@ -404,6 +406,27 @@ describe Projects::JobsController do
       end
     end
 
+    context 'when user is developer' do
+      let(:role) { :developer }
+      let(:job) { create(:ci_build, :erasable, :trace, pipeline: pipeline, user: triggered_by) }
+
+      context 'when triggered by same user' do
+        let(:triggered_by) { user }
+
+        it 'has successful status' do
+          expect(response).to have_gitlab_http_status(:found)
+        end
+      end
+
+      context 'when triggered by different user' do
+        let(:triggered_by) { create(:user) }
+
+        it 'does not have successful status' do
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
     def post_erase
       post :erase, namespace_id: project.namespace,
                    project_id: project,
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 88f7b1775a0..1795ee8e9a4 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -270,8 +270,8 @@ describe Ci::Build do
     end
   end
 
-  describe '#owned_by?' do
-    subject { build.owned_by?(user) }
+  describe '#triggered_by?' do
+    subject { build.triggered_by?(user) }
 
     context 'when user is owner' do
       let(:build) { create(:ci_build, pipeline: pipeline, user: user) }
diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb
index d8e73e4a890..edf8d63a4c6 100644
--- a/spec/policies/ci/build_policy_spec.rb
+++ b/spec/policies/ci/build_policy_spec.rb
@@ -152,39 +152,57 @@ describe Ci::BuildPolicy do
     end
 
     describe 'rules for erase build' do
-      let(:project) { create(:project, :repository) }
-      let(:build) { create(:ci_build, pipeline: pipeline, user: owner) }
+      let(:project) { create(:project) }
+      let(:build) { create(:ci_build, pipeline: pipeline, ref: 'some-ref', user: owner) }
 
-      context 'when developer created a build' do
+      context 'when a developer erases a build' do
         before do
           project.add_developer(user)
         end
 
-        context 'when the build was created by the user' do
-          let(:owner) { user }
+        context 'when developers can push to the branch' do
+          before do
+            create(:protected_branch, :developers_can_merge,
+                   name: build.ref, project: project)
+          end
 
-          it { expect(policy).to be_allowed :erase_build }
+          context 'when the build was created by the developer' do
+            let(:owner) { user }
+
+            it { expect(policy).to be_allowed :erase_build }
+          end
+
+          context 'when the build was created by the other' do
+            let(:owner) { create(:user) }
+
+            it { expect(policy).to be_disallowed :erase_build }
+          end
         end
 
-        context 'when the build was created by others' do
-          let(:owner) { create(:user) }
+        context 'when no one can push or merge to the branch' do
+          let(:owner) { user }
+
+          before do
+            create(:protected_branch, :no_one_can_push,
+                   name: build.ref, project: project)
+          end
 
           it { expect(policy).to be_disallowed :erase_build }
         end
       end
 
-      context 'when master erases a build' do
+      context 'when a master erases a build' do
         before do
           project.add_master(user)
         end
 
-        context 'when the build was created by the user' do
+        context 'when the build was created by the master' do
           let(:owner) { user }
 
           it { expect(policy).to be_allowed :erase_build }
         end
 
-        context 'when the build was created by others' do
+        context 'when the build was created by the other' do
           let(:owner) { create(:user) }
 
           it { expect(policy).to be_allowed :erase_build }
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 3b7b9c889e7..8196046f6ab 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -491,6 +491,8 @@ describe API::Jobs do
 
   describe 'POST /projects/:id/jobs/:job_id/erase' do
     before do
+      project.add_master(user)
+
       post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
     end
 
diff --git a/spec/requests/api/v3/builds_spec.rb b/spec/requests/api/v3/builds_spec.rb
index 3f58b7ef384..a73bb456b52 100644
--- a/spec/requests/api/v3/builds_spec.rb
+++ b/spec/requests/api/v3/builds_spec.rb
@@ -408,6 +408,8 @@ describe API::V3::Builds do
 
   describe 'POST /projects/:id/builds/:build_id/erase' do
     before do
+      project.add_master(user)
+
       post v3_api("/projects/#{project.id}/builds/#{build.id}/erase", user)
     end
 
-- 
cgit v1.2.1


From d8fb903ef7db26566cfacf76ce10e9e7917b8f38 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Tue, 7 Nov 2017 22:45:55 +0900
Subject: Improve spec

---
 spec/controllers/projects/jobs_controller_spec.rb |  2 +-
 spec/policies/ci/build_policy_spec.rb             | 36 +++++++++----
 spec/requests/api/jobs_spec.rb                    | 62 ++++++++++++++++-------
 3 files changed, 71 insertions(+), 29 deletions(-)

diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb
index 804075782c1..e50962df553 100644
--- a/spec/controllers/projects/jobs_controller_spec.rb
+++ b/spec/controllers/projects/jobs_controller_spec.rb
@@ -422,7 +422,7 @@ describe Projects::JobsController do
         let(:triggered_by) { create(:user) }
 
         it 'does not have successful status' do
-          expect(response).to have_gitlab_http_status(:not_found)
+          expect(response).not_to have_gitlab_http_status(:found)
         end
       end
     end
diff --git a/spec/policies/ci/build_policy_spec.rb b/spec/policies/ci/build_policy_spec.rb
index edf8d63a4c6..298a9d16425 100644
--- a/spec/policies/ci/build_policy_spec.rb
+++ b/spec/policies/ci/build_policy_spec.rb
@@ -152,7 +152,7 @@ describe Ci::BuildPolicy do
     end
 
     describe 'rules for erase build' do
-      let(:project) { create(:project) }
+      let(:project) { create(:project, :repository) }
       let(:build) { create(:ci_build, pipeline: pipeline, ref: 'some-ref', user: owner) }
 
       context 'when a developer erases a build' do
@@ -162,7 +162,7 @@ describe Ci::BuildPolicy do
 
         context 'when developers can push to the branch' do
           before do
-            create(:protected_branch, :developers_can_merge,
+            create(:protected_branch, :developers_can_push,
                    name: build.ref, project: project)
           end
 
@@ -183,7 +183,7 @@ describe Ci::BuildPolicy do
           let(:owner) { user }
 
           before do
-            create(:protected_branch, :no_one_can_push,
+            create(:protected_branch, :no_one_can_push, :no_one_can_merge,
                    name: build.ref, project: project)
           end
 
@@ -196,16 +196,34 @@ describe Ci::BuildPolicy do
           project.add_master(user)
         end
 
-        context 'when the build was created by the master' do
-          let(:owner) { user }
+        context 'when masters can push to the branch' do
+          before do
+            create(:protected_branch, :masters_can_push,
+                   name: build.ref, project: project)
+          end
+
+          context 'when the build was created by the master' do
+            let(:owner) { user }
+
+            it { expect(policy).to be_allowed :erase_build }
+          end
+
+          context 'when the build was created by the other' do
+            let(:owner) { create(:user) }
 
-          it { expect(policy).to be_allowed :erase_build }
+            it { expect(policy).to be_allowed :erase_build }
+          end
         end
 
-        context 'when the build was created by the other' do
-          let(:owner) { create(:user) }
+        context 'when no one can push or merge to the branch' do
+          let(:owner) { user }
 
-          it { expect(policy).to be_allowed :erase_build }
+          before do
+            create(:protected_branch, :no_one_can_push, :no_one_can_merge,
+                   name: build.ref, project: project)
+          end
+
+          it { expect(policy).to be_disallowed :erase_build }
         end
       end
     end
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 3e0682055fe..327b1176088 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -500,35 +500,59 @@ describe API::Jobs do
   end
 
   describe 'POST /projects/:id/jobs/:job_id/erase' do
-    before do
-      project.add_master(user)
+    context 'when a master erases a build' do
+      before do
+        project.add_master(user)
 
-      post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
-    end
+        post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
+      end
+
+      context 'job is erasable' do
+        let(:job) { create(:ci_build, :trace, :artifacts, :success, project: project, pipeline: pipeline) }
 
-    context 'job is erasable' do
-      let(:job) { create(:ci_build, :trace, :artifacts, :success, project: project, pipeline: pipeline) }
+        it 'erases job content' do
+          expect(response).to have_gitlab_http_status(201)
+          expect(job).not_to have_trace
+          expect(job.artifacts_file.exists?).to be_falsy
+          expect(job.artifacts_metadata.exists?).to be_falsy
+        end
+
+        it 'updates job' do
+          job.reload
 
-      it 'erases job content' do
-        expect(response).to have_gitlab_http_status(201)
-        expect(job).not_to have_trace
-        expect(job.artifacts_file.exists?).to be_falsy
-        expect(job.artifacts_metadata.exists?).to be_falsy
+          expect(job.erased_at).to be_truthy
+          expect(job.erased_by).to eq(user)
+        end
       end
 
-      it 'updates job' do
-        job.reload
+      context 'job is not erasable' do
+        let(:job) { create(:ci_build, :trace, project: project, pipeline: pipeline) }
 
-        expect(job.erased_at).to be_truthy
-        expect(job.erased_by).to eq(user)
+        it 'responds with forbidden' do
+          expect(response).to have_gitlab_http_status(403)
+        end
       end
     end
 
-    context 'job is not erasable' do
-      let(:job) { create(:ci_build, :trace, project: project, pipeline: pipeline) }
+    context 'when a developer erases a build' do
+      before do
+        project.add_developer(user)
 
-      it 'responds with forbidden' do
-        expect(response).to have_gitlab_http_status(403)
+        post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
+      end
+
+      let(:job) { create(:ci_build, :trace, :artifacts, :success, project: project, pipeline: pipeline, user: owner) }
+
+      context 'when the build was created by the developer' do
+        let(:owner) { user }
+
+        it { expect(response).to have_gitlab_http_status(201) }
+      end
+
+      context 'when the build was created by the other' do
+        let(:owner) { create(:user) }
+
+        it { expect(response).to have_gitlab_http_status(403) }
       end
     end
   end
-- 
cgit v1.2.1


From 8029c92e1c81e4c9ab55704bff82cca5ff893a03 Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Tue, 7 Nov 2017 22:57:58 +0900
Subject: Reduce changes

---
 spec/controllers/projects/jobs_controller_spec.rb |  4 +-
 spec/requests/api/jobs_spec.rb                    | 51 ++++++++++-------------
 2 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb
index e50962df553..7490f8fefce 100644
--- a/spec/controllers/projects/jobs_controller_spec.rb
+++ b/spec/controllers/projects/jobs_controller_spec.rb
@@ -371,6 +371,8 @@ describe Projects::JobsController do
   end
 
   describe 'POST erase' do
+    let(:role) { :master }
+
     before do
       project.team << [user, role]
       sign_in(user)
@@ -378,8 +380,6 @@ describe Projects::JobsController do
       post_erase
     end
 
-    let(:role) { :master }
-
     context 'when job is erasable' do
       let(:job) { create(:ci_build, :erasable, :trace, pipeline: pipeline) }
 
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 327b1176088..2a83213e87a 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -500,47 +500,42 @@ describe API::Jobs do
   end
 
   describe 'POST /projects/:id/jobs/:job_id/erase' do
-    context 'when a master erases a build' do
-      before do
-        project.add_master(user)
-
-        post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
-      end
+    let(:role) { :master }
 
-      context 'job is erasable' do
-        let(:job) { create(:ci_build, :trace, :artifacts, :success, project: project, pipeline: pipeline) }
+    before do
+      project.team << [user, role]
 
-        it 'erases job content' do
-          expect(response).to have_gitlab_http_status(201)
-          expect(job).not_to have_trace
-          expect(job.artifacts_file.exists?).to be_falsy
-          expect(job.artifacts_metadata.exists?).to be_falsy
-        end
+      post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
+    end
 
-        it 'updates job' do
-          job.reload
+    context 'job is erasable' do
+      let(:job) { create(:ci_build, :trace, :artifacts, :success, project: project, pipeline: pipeline) }
 
-          expect(job.erased_at).to be_truthy
-          expect(job.erased_by).to eq(user)
-        end
+      it 'erases job content' do
+        expect(response).to have_gitlab_http_status(201)
+        expect(job).not_to have_trace
+        expect(job.artifacts_file.exists?).to be_falsy
+        expect(job.artifacts_metadata.exists?).to be_falsy
       end
 
-      context 'job is not erasable' do
-        let(:job) { create(:ci_build, :trace, project: project, pipeline: pipeline) }
+      it 'updates job' do
+        job.reload
 
-        it 'responds with forbidden' do
-          expect(response).to have_gitlab_http_status(403)
-        end
+        expect(job.erased_at).to be_truthy
+        expect(job.erased_by).to eq(user)
       end
     end
 
-    context 'when a developer erases a build' do
-      before do
-        project.add_developer(user)
+    context 'job is not erasable' do
+      let(:job) { create(:ci_build, :trace, project: project, pipeline: pipeline) }
 
-        post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
+      it 'responds with forbidden' do
+        expect(response).to have_gitlab_http_status(403)
       end
+    end
 
+    context 'when a developer erases a build' do
+      let(:role) { :developer }
       let(:job) { create(:ci_build, :trace, :artifacts, :success, project: project, pipeline: pipeline, user: owner) }
 
       context 'when the build was created by the developer' do
-- 
cgit v1.2.1