diff options
author | Shinya Maeda <shinya@gitlab.com> | 2017-11-07 02:47:05 +0900 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2017-11-07 02:47:05 +0900 |
commit | afef38533727cf32a7be324243a25b4db5eb5498 (patch) | |
tree | cfdd2c96bd0c1a7ee1fd85f0b63fd2edf0d1fe3f | |
parent | cb5e35d562a1bf0737c1ad3316c3723775fada01 (diff) | |
download | gitlab-ce-afef38533727cf32a7be324243a25b4db5eb5498.tar.gz |
Add doc. Fix spec. Add erase_build in protected_ref rule
-rw-r--r-- | app/controllers/projects/jobs_controller.rb | 2 | ||||
-rw-r--r-- | app/models/ci/build.rb | 2 | ||||
-rw-r--r-- | app/policies/ci/build_policy.rb | 12 | ||||
-rw-r--r-- | doc/user/permissions.md | 2 | ||||
-rw-r--r-- | lib/api/jobs.rb | 1 | ||||
-rw-r--r-- | lib/api/v3/builds.rb | 1 | ||||
-rw-r--r-- | spec/controllers/projects/jobs_controller_spec.rb | 25 | ||||
-rw-r--r-- | spec/models/ci/build_spec.rb | 4 | ||||
-rw-r--r-- | spec/policies/ci/build_policy_spec.rb | 40 | ||||
-rw-r--r-- | spec/requests/api/jobs_spec.rb | 2 | ||||
-rw-r--r-- | 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 |