Prevent maintainers from editing PipelineSchedule

Merge branch 'security-force-ci-schedule-ownership' into 'master' See merge request gitlab-org/security/gitlab!2399 Changelog: security

Prevent maintainers from editing PipelineSchedule
Merge branch 'security-force-ci-schedule-ownership' into 'master' See merge request gitlab-org/security/gitlab!2399 Changelog: security
a292f722 · drew cimino · GitLab Release Tools Bot · 3e60a454 · a292f722 · a292f722
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/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 @@ def authorize_update_pipeline_schedule!
    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

--- a/app/policies/ci/pipeline_schedule_policy.rb
+++ b/app/policies/ci/pipeline_schedule_policy.rb
@@ -15,11 +15,14 @@ class PipelineSchedulePolicy < PipelinePolicy
    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

--- a/doc/ci/pipelines/schedules.md
+++ b/doc/ci/pipelines/schedules.md
@@ -39,6 +39,20 @@ To add a pipeline schedule:
     These variables are available only when the scheduled pipeline runs,
     and not in any other pipeline run.

+## 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](#take-ownership)
+of the schedule.
+
 ## Run manually

 To trigger a pipeline schedule manually, so that it runs immediately instead of

--- a/lib/api/ci/pipeline_schedules.rb
+++ b/lib/api/ci/pipeline_schedules.rb
@@ -93,7 +93,7 @@ class PipelineSchedules < ::API::Base
          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

--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -13,10 +13,43 @@
    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 @@ def visit_pipelines_schedules
      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 @@ def go
    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 @@ def go
        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 @@ def go
        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 @@ def go

      before do
        project.add_maintainer(user)
+        pipeline_schedule.update!(owner: user)
        sign_in(user)
      end

@@ -352,22 +382,7 @@ def go
      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 @@ def go
      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

--- a/spec/features/projects/pipeline_schedules_spec.rb
+++ b/spec/features/projects/pipeline_schedules_spec.rb
@@ -9,7 +9,77 @@
  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('[data-testid="schedule-target-ref"]') do
+            expect(first('.gl-new-dropdown-button-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('[data-testid="schedule-target-ref"]') do
+            expect(first('.gl-new-dropdown-button-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 @@
          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 @@
      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('[data-testid="schedule-target-ref"]') do
-            expect(first('.gl-new-dropdown-button-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('[data-testid="schedule-target-ref"]') do
-            expect(first('.gl-new-dropdown-button-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

--- a/spec/policies/ci/pipeline_schedule_policy_spec.rb
+++ b/spec/policies/ci/pipeline_schedule_policy_spec.rb
@@ -84,11 +84,14 @@
        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

--- a/spec/requests/api/ci/pipeline_schedules_spec.rb
+++ b/spec/requests/api/ci/pipeline_schedules_spec.rb
@@ -291,10 +291,36 @@ def active?(str)
    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)
+      context 'as a project maintainer' do
+        before do
+          project.add_maintainer(user)
+        end

-        expect(response).to have_gitlab_http_status(:not_found)
+        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
+        before do
+          project.add_owner(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 '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 +338,21 @@ def active?(str)
      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 +360,23 @@ def active?(str)
      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