godfat nice catchiesfeature/sm/31771-do-not-allow-jobs-to-be-erased

7 files changed, 13 insertions, 16 deletions

diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb
index 2a8bb05453a..3f04b0c73fd 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, :erase]
+    except: [:index, :show, :status, :raw, :trace, :cancel_all]
   before_action :authorize_admin_build!, only: :erase
 
   layout 'project'
diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
index f40679ab9db..366208e8f5e 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: -> (*) { can?(current_user, :admin_build, build) && build.erasable? } do |build|
+  expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :admin_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 77130c57f3d..f95c82e5b14 100644
--- a/app/views/projects/jobs/show.html.haml
+++ b/app/views/projects/jobs/show.html.haml
@@ -70,7 +70,7 @@
                     class: 'js-raw-link-controller has-tooltip controllers-buttons' do
               = icon('file-text-o')
 
-          - if can?(current_user, :admin_build, @build) && @build.erasable?
+          - if @build.erasable? && can?(current_user, :admin_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/changelogs/unreleased/feature-sm-31771-do-not-allow-jobs-to-be-erased.yml b/changelogs/unreleased/feature-sm-31771-do-not-allow-jobs-to-be-erased.yml
index c56dd6aae18..7eb2a4e51b3 100644
--- a/changelogs/unreleased/feature-sm-31771-do-not-allow-jobs-to-be-erased.yml
+++ b/changelogs/unreleased/feature-sm-31771-do-not-allow-jobs-to-be-erased.yml
@@ -1,4 +1,4 @@
 ---
 title: Forbid Developer to erase job
-merge_request: 11538
+merge_request: 12838
 author:
diff --git a/doc/user/permissions.md b/doc/user/permissions.md
index 719e351fb8b..f154295b42e 100644
--- a/doc/user/permissions.md
+++ b/doc/user/permissions.md
@@ -192,12 +192,12 @@ instance and project. In addition, all admins can use the admin interface under
 | Action                                | Guest, Reporter | Developer   | Master   | Admin  |
 |---------------------------------------|-----------------|-------------|----------|--------|
 | See commits and jobs                  | ✓               | ✓           | ✓        | ✓      |
-| Retry or cancel job                   |                 | ✓           | ✓        | ✓      |
+| Retry or cancel job                   |                 | ✓ [^6]      | ✓ [^6]   | ✓      |
+| Erase job artifacts and trace         |                 |             | ✓ [^6]   | ✓      |
 | Remove project                        |                 |             | ✓        | ✓      |
 | Create project                        |                 |             | ✓        | ✓      |
 | Change project configuration          |                 |             | ✓        | ✓      |
 | Add specific runners                  |                 |             | ✓        | ✓      |
-| Erase jobs                            |                 |             | ✓        | ✓      |
 | Add shared runners                    |                 |             |          | ✓      |
 | See events in the system              |                 |             |          | ✓      |
 | Admin interface                       |                 |             |          | ✓      |
@@ -251,5 +251,6 @@ only.
 [^3]: Not allowed for Guest, Reporter, Developer, Master, or Owner
 [^4]: Only if user is not external one.
 [^5]: Only if user is a member of the project.
+[^6]: Only if user has a permission to merge/push to the ref (branch/tag)
 [ce-18994]: https://gitlab.com/gitlab-org/gitlab-ce/issues/18994
 [new-mod]: project/new_ci_build_permissions_model.md
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 786c71398b9..d59a4eb10a9 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -18,11 +18,14 @@ describe API::Jobs do
   let(:reporter) { create(:project_member, :reporter, project: project).user }
   let(:guest) { create(:project_member, :guest, project: project).user }
 
+  before do
+    project.add_developer(user)
+  end
+
   describe 'GET /projects/:id/jobs' do
     let(:query) { Hash.new }
 
     before do
-      project.add_developer(user)
       get api("/projects/#{project.id}/jobs", api_user), query
     end
 
@@ -86,7 +89,6 @@ describe API::Jobs do
     let(:query) { Hash.new }
 
     before do
-      project.add_developer(user)
       get api("/projects/#{project.id}/pipelines/#{pipeline.id}/jobs", api_user), query
     end
 
@@ -157,7 +159,6 @@ describe API::Jobs do
 
   describe 'GET /projects/:id/jobs/:job_id' do
     before do
-      project.add_developer(user)
       get api("/projects/#{project.id}/jobs/#{job.id}", api_user)
     end
 
@@ -189,7 +190,6 @@ describe API::Jobs do
 
   describe 'GET /projects/:id/jobs/:job_id/artifacts' do
     before do
-      project.add_developer(user)
       get api("/projects/#{project.id}/jobs/#{job.id}/artifacts", api_user)
     end
 
@@ -228,7 +228,6 @@ describe API::Jobs do
     let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) }
 
     before do
-      project.add_developer(user)
       job.success
     end
 
@@ -326,7 +325,6 @@ describe API::Jobs do
     let(:job) { create(:ci_build, :trace, pipeline: pipeline) }
 
     before do
-      project.add_developer(user)
       get api("/projects/#{project.id}/jobs/#{job.id}/trace", api_user)
     end
 
@@ -348,7 +346,6 @@ describe API::Jobs do
 
   describe 'POST /projects/:id/jobs/:job_id/cancel' do
     before do
-      project.add_developer(user)
       post api("/projects/#{project.id}/jobs/#{job.id}/cancel", api_user)
     end
 
@@ -382,7 +379,6 @@ describe API::Jobs do
     let(:job) { create(:ci_build, :canceled, pipeline: pipeline) }
 
     before do
-      project.add_developer(user)
       post api("/projects/#{project.id}/jobs/#{job.id}/retry", api_user)
     end
 
@@ -415,6 +411,7 @@ describe API::Jobs do
 
   describe 'POST /projects/:id/jobs/:job_id/erase' do
     before do
+      project.team.truncate
       project.add_master(user)
       post api("/projects/#{project.id}/jobs/#{job.id}/erase", user)
     end
@@ -448,7 +445,6 @@ describe API::Jobs do
 
   describe 'POST /projects/:id/jobs/:job_id/artifacts/keep' do
     before do
-      project.add_developer(user)
       post api("/projects/#{project.id}/jobs/#{job.id}/artifacts/keep", user)
     end
 
@@ -475,7 +471,6 @@ describe API::Jobs do
 
   describe 'POST /projects/:id/jobs/:job_id/play' do
     before do
-      project.add_developer(user)
       post api("/projects/#{project.id}/jobs/#{job.id}/play", api_user)
     end
 
diff --git a/spec/requests/api/v3/builds_spec.rb b/spec/requests/api/v3/builds_spec.rb
index e5a718515f2..e90da9624b3 100644
--- a/spec/requests/api/v3/builds_spec.rb
+++ b/spec/requests/api/v3/builds_spec.rb
@@ -410,6 +410,7 @@ describe API::V3::Builds do
     let(:user) { create(:user) }
 
     before do
+      project.team.truncate
       project.add_master(user)
       post v3_api("/projects/#{project.id}/builds/#{build.id}/erase", user)
     end