Prevent maintainers from editing PipelineSchedule

Merge branch 'security-force-ci-schedule-ownership-14-8' into '14-8-stable-ee'

See merge request gitlab-org/security/gitlab!2423

Changelog: security

app/controllers/projects/pipeline_schedules_controller.rb

@@ -7,7 +7,8 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   before_action :authorize_play_pipeline_schedule!, only: [:play]
   before_action :authorize_read_pipeline_schedule!
   before_action :authorize_create_pipeline_schedule!, only: [:new, :create]
-  before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create, :play]
+  before_action :authorize_update_pipeline_schedule!, only: [:edit, :update]
+  before_action :authorize_take_ownership_pipeline_schedule!, only: [:take_ownership]
   before_action :authorize_admin_pipeline_schedule!, only: [:destroy]
   feature_category :continuous_integration
@@ -108,6 +109,10 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
     return access_denied! unless can?(current_user, :update_pipeline_schedule, schedule)
   end
+  def authorize_take_ownership_pipeline_schedule!
+    return access_denied! unless can?(current_user, :take_ownership_pipeline_schedule, schedule)
+  end
+
   def authorize_admin_pipeline_schedule!
     return access_denied! unless can?(current_user, :admin_pipeline_schedule, schedule)
   end

app/policies/ci/pipeline_schedule_policy.rb

@@ -15,11 +15,14 @@ module Ci
     rule { can?(:create_pipeline) }.enable :play_pipeline_schedule
     rule { can?(:admin_pipeline) | (can?(:update_build) & owner_of_schedule) }.policy do
-      enable :update_pipeline_schedule
       enable :admin_pipeline_schedule
       enable :read_pipeline_schedule_variables
     end
+    rule { admin | (owner_of_schedule & can?(:update_build)) }.policy do
+      enable :update_pipeline_schedule
+    end
+
     rule { can?(:admin_pipeline_schedule) & ~owner_of_schedule }.policy do
       enable :take_ownership_pipeline_schedule
     end

doc/ci/pipelines/schedules.md

@@ -53,6 +53,20 @@ is installed on.
 ![Schedules list](img/pipeline_schedules_list.png)
+## Edit a pipeline schedule
+
+> Introduced in GitLab 14.8, only a pipeline schedule owner can edit the schedule.
+
+The owner of a pipeline schedule can edit it:
+
+1. On the top bar, select **Menu > Projects** and find your project.
+1. In the left sidebar, select **CI/CD > Schedules**.
+1. Next to the schedule, select **Edit** (**{pencil}**) and fill in the form.
+
+The user must have the Developer role or above for the project. If the user is
+not the owner of the schedule, they must first take ownership.
+of the schedule.
+
 ### Using variables
 You can pass any number of arbitrary variables. They are available in

lib/api/ci/pipeline_schedules.rb

@@ -93,7 +93,7 @@ module API
           requires :pipeline_schedule_id, type: Integer, desc: 'The pipeline schedule id'
         end
         post ':id/pipeline_schedules/:pipeline_schedule_id/take_ownership' do
-          authorize! :update_pipeline_schedule, pipeline_schedule
+          authorize! :take_ownership_pipeline_schedule, pipeline_schedule
           if pipeline_schedule.own!(current_user)
             present pipeline_schedule, with: Entities::Ci::PipelineScheduleDetails

spec/controllers/projects/pipeline_schedules_controller_spec.rb

@@ -13,10 +13,43 @@ RSpec.describe Projects::PipelineSchedulesController do
     project.add_developer(user)
   end
+  shared_examples 'access update schedule' do
+    describe 'security' do
+      it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
+        expect { go }.to be_allowed_for(:admin)
+      end
+
+      it 'is denied for admin when admin mode disabled' do
+        expect { go }.to be_denied_for(:admin)
+      end
+
+      it { expect { go }.to be_denied_for(:owner).of(project) }
+      it { expect { go }.to be_denied_for(:maintainer).of(project) }
+      it { expect { go }.to be_denied_for(:developer).of(project) }
+      it { expect { go }.to be_denied_for(:reporter).of(project) }
+      it { expect { go }.to be_denied_for(:guest).of(project) }
+      it { expect { go }.to be_denied_for(:user) }
+      it { expect { go }.to be_denied_for(:external) }
+      it { expect { go }.to be_denied_for(:visitor) }
+
+      context 'when user is schedule owner' do
+        it { expect { go }.to be_allowed_for(:owner).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_allowed_for(:maintainer).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:reporter).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:guest).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:user).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:external).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:visitor).own(pipeline_schedule) }
+      end
+    end
+  end
+
   describe 'GET #index' do
     render_views
     let(:scope) { nil }
+
     let!(:inactive_pipeline_schedule) do
       create(:ci_pipeline_schedule, :inactive, project: project)
     end
@@ -130,12 +163,15 @@ RSpec.describe Projects::PipelineSchedulesController do
       it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
         expect { go }.to be_allowed_for(:admin)
       end
+
       it 'is denied for admin when admin mode disabled' do
         expect { go }.to be_denied_for(:admin)
       end
+
       it { expect { go }.to be_allowed_for(:owner).of(project) }
       it { expect { go }.to be_allowed_for(:maintainer).of(project) }
       it { expect { go }.to be_allowed_for(:developer).of(project) }
+
       it { expect { go }.to be_denied_for(:reporter).of(project) }
       it { expect { go }.to be_denied_for(:guest).of(project) }
       it { expect { go }.to be_denied_for(:user) }
@@ -284,20 +320,7 @@ RSpec.describe Projects::PipelineSchedulesController do
     describe 'security' do
       let(:schedule) { { description: 'updated_desc' } }
-      it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
-        expect { go }.to be_allowed_for(:admin)
-      end
-      it 'is denied for admin when admin mode disabled' do
-        expect { go }.to be_denied_for(:admin)
-      end
-      it { expect { go }.to be_allowed_for(:owner).of(project) }
-      it { expect { go }.to be_allowed_for(:maintainer).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
-      it { expect { go }.to be_denied_for(:reporter).of(project) }
-      it { expect { go }.to be_denied_for(:guest).of(project) }
-      it { expect { go }.to be_denied_for(:user) }
-      it { expect { go }.to be_denied_for(:external) }
-      it { expect { go }.to be_denied_for(:visitor) }
+      it_behaves_like 'access update schedule'
       context 'when a developer created a pipeline schedule' do
         let(:developer_1) { create(:user) }
@@ -308,8 +331,10 @@ RSpec.describe Projects::PipelineSchedulesController do
         end
         it { expect { go }.to be_allowed_for(developer_1) }
+
+        it { expect { go }.to be_denied_for(:owner).of(project) }
+        it { expect { go }.to be_denied_for(:maintainer).of(project) }
         it { expect { go }.to be_denied_for(:developer).of(project) }
-        it { expect { go }.to be_allowed_for(:maintainer).of(project) }
       end
       context 'when a maintainer created a pipeline schedule' do
@@ -321,17 +346,21 @@ RSpec.describe Projects::PipelineSchedulesController do
         end
         it { expect { go }.to be_allowed_for(maintainer_1) }
-        it { expect { go }.to be_allowed_for(:maintainer).of(project) }
+
+        it { expect { go }.to be_denied_for(:owner).of(project) }
+        it { expect { go }.to be_denied_for(:maintainer).of(project) }
         it { expect { go }.to be_denied_for(:developer).of(project) }
       end
     end
     def go
-      put :update, params: { namespace_id: project.namespace.to_param,
-                             project_id: project,
-                             id: pipeline_schedule,
-                             schedule: schedule },
-                   as: :html
+      put :update, params: {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: pipeline_schedule,
+          schedule: schedule
+        },
+        as: :html
     end
   end
@@ -341,6 +370,7 @@ RSpec.describe Projects::PipelineSchedulesController do
       before do
         project.add_maintainer(user)
+        pipeline_schedule.update!(owner: user)
         sign_in(user)
       end
@@ -352,22 +382,7 @@ RSpec.describe Projects::PipelineSchedulesController do
       end
     end
-    describe 'security' do
-      it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
-        expect { go }.to be_allowed_for(:admin)
-      end
-      it 'is denied for admin when admin mode disabled' do
-        expect { go }.to be_denied_for(:admin)
-      end
-      it { expect { go }.to be_allowed_for(:owner).of(project) }
-      it { expect { go }.to be_allowed_for(:maintainer).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
-      it { expect { go }.to be_denied_for(:reporter).of(project) }
-      it { expect { go }.to be_denied_for(:guest).of(project) }
-      it { expect { go }.to be_denied_for(:user) }
-      it { expect { go }.to be_denied_for(:external) }
-      it { expect { go }.to be_denied_for(:visitor) }
-    end
+    it_behaves_like 'access update schedule'
     def go
       get :edit, params: { namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id }
@@ -379,17 +394,30 @@ RSpec.describe Projects::PipelineSchedulesController do
       it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
         expect { go }.to be_allowed_for(:admin)
       end
+
       it 'is denied for admin when admin mode disabled' do
         expect { go }.to be_denied_for(:admin)
       end
+
       it { expect { go }.to be_allowed_for(:owner).of(project) }
       it { expect { go }.to be_allowed_for(:maintainer).of(project) }
-      it { expect { go }.to be_allowed_for(:developer).of(project).own(pipeline_schedule) }
+      it { expect { go }.to be_denied_for(:developer).of(project) }
       it { expect { go }.to be_denied_for(:reporter).of(project) }
       it { expect { go }.to be_denied_for(:guest).of(project) }
       it { expect { go }.to be_denied_for(:user) }
       it { expect { go }.to be_denied_for(:external) }
       it { expect { go }.to be_denied_for(:visitor) }
+
+      context 'when user is schedule owner' do
+        it { expect { go }.to be_denied_for(:owner).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:maintainer).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:developer).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:reporter).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:guest).of(project).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:user).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:external).own(pipeline_schedule) }
+        it { expect { go }.to be_denied_for(:visitor).own(pipeline_schedule) }
+      end
     end
     def go

spec/features/projects/pipeline_schedules_spec.rb

@@ -9,7 +9,77 @@ RSpec.describe 'Pipeline Schedules', :js do
   let(:scope) { nil }
   let!(:user) { create(:user) }
-  context 'logged in as maintainer' do
+  context 'logged in as the pipeline scheduler owner' do
+    before do
+      stub_feature_flags(bootstrap_confirmation_modals: false)
+      project.add_developer(user)
+      pipeline_schedule.update!(owner: user)
+      gitlab_sign_in(user)
+    end
+
+    describe 'GET /projects/pipeline_schedules' do
+      before do
+        visit_pipelines_schedules
+      end
+
+      it 'edits the pipeline' do
+        page.within('.pipeline-schedule-table-row') do
+          click_link 'Edit'
+        end
+
+        expect(page).to have_content('Edit Pipeline Schedule')
+      end
+    end
+
+    describe 'PATCH /projects/pipelines_schedules/:id/edit' do
+      before do
+        edit_pipeline_schedule
+      end
+
+      it 'displays existing properties' do
+        description = find_field('schedule_description').value
+        expect(description).to eq('pipeline schedule')
+        expect(page).to have_button('master')
+        expect(page).to have_button('UTC')
+      end
+
+      it 'edits the scheduled pipeline' do
+        fill_in 'schedule_description', with: 'my brand new description'
+
+        save_pipeline_schedule
+
+        expect(page).to have_content('my brand new description')
+      end
+
+      context 'when ref is nil' do
+        before do
+          pipeline_schedule.update_attribute(:ref, nil)
+          edit_pipeline_schedule
+        end
+
+        it 'shows the pipeline schedule with default ref' do
+          page.within('.js-target-branch-dropdown') do
+            expect(first('.dropdown-toggle-text').text).to eq('master')
+          end
+        end
+      end
+
+      context 'when ref is empty' do
+        before do
+          pipeline_schedule.update_attribute(:ref, '')
+          edit_pipeline_schedule
+        end
+
+        it 'shows the pipeline schedule with default ref' do
+          page.within('.js-target-branch-dropdown') do
+            expect(first('.dropdown-toggle-text').text).to eq('master')
+          end
+        end
+      end
+    end
+  end
+
+  context 'logged in as a project maintainer' do
     before do
       stub_feature_flags(bootstrap_confirmation_modals: false)
       project.add_maintainer(user)
@@ -46,14 +116,6 @@ RSpec.describe 'Pipeline Schedules', :js do
           end
         end
-        it 'edits the pipeline' do
-          page.within('.pipeline-schedule-table-row') do
-            click_link 'Edit'
-          end
-
-          expect(page).to have_content('Edit Pipeline Schedule')
-        end
-
         it 'deletes the pipeline' do
           accept_confirm { click_link 'Delete' }
@@ -108,53 +170,6 @@ RSpec.describe 'Pipeline Schedules', :js do
       end
     end
-    describe 'PATCH /projects/pipelines_schedules/:id/edit' do
-      before do
-        edit_pipeline_schedule
-      end
-
-      it 'displays existing properties' do
-        description = find_field('schedule_description').value
-        expect(description).to eq('pipeline schedule')
-        expect(page).to have_button('master')
-        expect(page).to have_button('UTC')
-      end
-
-      it 'edits the scheduled pipeline' do
-        fill_in 'schedule_description', with: 'my brand new description'
-
-        save_pipeline_schedule
-
-        expect(page).to have_content('my brand new description')
-      end
-
-      context 'when ref is nil' do
-        before do
-          pipeline_schedule.update_attribute(:ref, nil)
-          edit_pipeline_schedule
-        end
-
-        it 'shows the pipeline schedule with default ref' do
-          page.within('.js-target-branch-dropdown') do
-            expect(first('.dropdown-toggle-text').text).to eq('master')
-          end
-        end
-      end
-
-      context 'when ref is empty' do
-        before do
-          pipeline_schedule.update_attribute(:ref, '')
-          edit_pipeline_schedule
-        end
-
-        it 'shows the pipeline schedule with default ref' do
-          page.within('.js-target-branch-dropdown') do
-            expect(first('.dropdown-toggle-text').text).to eq('master')
-          end
-        end
-      end
-    end
-
     context 'when user creates a new pipeline schedule with variables' do
       before do
         visit_pipelines_schedules

spec/policies/ci/pipeline_schedule_policy_spec.rb

@@ -84,11 +84,14 @@ RSpec.describe Ci::PipelineSchedulePolicy, :models do
         project.add_maintainer(user)
       end
-      it 'includes abilities to do all operations on pipeline schedule' do
+      it 'allows for playing and destroying a pipeline schedule' do
         expect(policy).to be_allowed :play_pipeline_schedule
-        expect(policy).to be_allowed :update_pipeline_schedule
         expect(policy).to be_allowed :admin_pipeline_schedule
       end
+
+      it 'does not allow for updating of an existing schedule' do
+        expect(policy).not_to be_allowed :update_pipeline_schedule
+      end
     end
     describe 'rules for non-owner of schedule' do

spec/requests/api/ci/pipeline_schedules_spec.rb

@@ -291,10 +291,32 @@ RSpec.describe API::Ci::PipelineSchedules do
     end
     context 'authenticated user with invalid permissions' do
-      it 'does not update pipeline_schedule' do
-        put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
-        expect(response).to have_gitlab_http_status(:not_found)
+      context 'as a project maintainer' do
+        before do
+          project.add_maintainer(user)
+        end
+        it 'does not update pipeline_schedule' do
+          put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
+
+      context 'as a project owner' do
+        it 'does not update pipeline_schedule' do
+          put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", project.owner)
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
+
+      context 'with no special role' do
+        it 'does not update pipeline_schedule' do
+          put api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", user)
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
       end
     end
@@ -312,16 +334,21 @@ RSpec.describe API::Ci::PipelineSchedules do
       create(:ci_pipeline_schedule, project: project, owner: developer)
     end
-    context 'authenticated user with valid permissions' do
+    let(:project_maintainer) do
+      create(:user).tap { |u| project.add_maintainer(u) }
+    end
+
+    context 'as an authenticated user with valid permissions' do
       it 'updates owner' do
-        post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/take_ownership", developer)
+        expect { post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/take_ownership", project_maintainer) }
+          .to change { pipeline_schedule.reload.owner }.from(developer).to(project_maintainer)
         expect(response).to have_gitlab_http_status(:created)
         expect(response).to match_response_schema('pipeline_schedule')
       end
     end
-    context 'authenticated user with invalid permissions' do
+    context 'as an authenticated user with invalid permissions' do
       it 'does not update owner' do
         post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/take_ownership", user)
@@ -329,13 +356,23 @@ RSpec.describe API::Ci::PipelineSchedules do
       end
     end
-    context 'unauthenticated user' do
+    context 'as an unauthenticated user' do
       it 'does not update owner' do
         post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/take_ownership")
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
+
+    context 'as the existing owner of the schedule' do
+      it 'rejects the request and leaves the schedule unchanged' do
+        expect do
+          post api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}/take_ownership", developer)
+        end.not_to change { pipeline_schedule.reload.owner }
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
   describe 'DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id' do