diff options
3 files changed, 317 insertions, 223 deletions
diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index 2ee6229cf68..3f395bd9cea 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -33,6 +33,8 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end def update + return access_denied! unless can?(current_user, :update_pipeline_schedule, schedule) + if Ci::CreatePipelineScheduleService .new(@project, current_user, schedule_params).update(schedule) redirect_to namespace_project_pipeline_schedules_path(@project.namespace.becomes(Namespace), @project) diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index 1877e89bb23..2506c179157 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -1,4 +1,15 @@ module Ci class PipelineSchedulePolicy < PipelinePolicy + alias_method :pipeline_schedule, :subject + + def rules + super + + access = pipeline_schedule.project.team.max_member_access(user.id) + + if access == Gitlab::Access::DEVELOPER && pipeline_schedule.owner != user + cannot! :update_pipeline_schedule + end + end end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 106d4b7496b..5f6b3c0a187 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Projects::PipelineSchedulesController do + include AccessMatchersForController + set(:project) { create(:empty_project, :public) } let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } @@ -53,343 +55,422 @@ describe Projects::PipelineSchedulesController do end describe 'POST #create' do - before do - create(:user).tap do |user| - project.add_developer(user) - sign_in(user) - end - end - - let(:basic_param) do - { description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' } - end - - context 'when variables_attributes is empty' do - let(:schedule) do - basic_param - end - - it 'creates a new schedule' do - expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } - .to change { Ci::PipelineSchedule.count }.by(1) - .and change { Ci::PipelineScheduleVariable.count }.by(0) - - expect(response).to have_http_status(:found) - end - end - - context 'when variables_attributes has one variable' do - let(:schedule) do - basic_param.merge({ - variables_attributes: [ { key: 'AAA', value: 'AAA123' } ] - }) - end - - it 'creates a new schedule' do - expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } - .to change { Ci::PipelineSchedule.count }.by(1) - .and change { Ci::PipelineScheduleVariable.count }.by(1) - - expect(response).to have_http_status(:found) - expect(Ci::PipelineScheduleVariable.last.key).to eq("AAA") - expect(Ci::PipelineScheduleVariable.last.value).to eq("AAA123") - end - - context 'when the same key has already been persisted' do - it 'returns an error that the key of variable is invaild' do - post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule - - pipeline_schedule_variable = build(:ci_pipeline_schedule_variable, key: 'AAA', pipeline_schedule: assigns(:schedule)) - expect(pipeline_schedule_variable).to be_invalid + describe 'functionality' do + before do + create(:user).tap do |user| + project.add_developer(user) + sign_in(user) end end - end - context 'when variables_attributes has one variable and key is empty' do - let(:schedule) do - basic_param.merge({ - variables_attributes: [ { key: '', value: 'AAA123' } ] - }) + let(:basic_param) do + { description: 'aaaaaaaa', cron: '0 4 * * *', cron_timezone: 'UTC', ref: 'master', active: '1' } end - it 'returns an error that the key of variable is invaild' do - expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } - .to change { Ci::PipelineSchedule.count }.by(0) - .and change { Ci::PipelineScheduleVariable.count }.by(0) - - expect(assigns(:schedule).errors['variables.key']).not_to be_empty - end - end - - context 'when variables_attributes has two variables and unique' do - let(:schedule) do - basic_param.merge({ - variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ] - }) - end - - it 'creates a new schedule' do - expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } - .to change { Ci::PipelineSchedule.count }.by(1) - .and change { Ci::PipelineScheduleVariable.count }.by(2) - - expect(response).to have_http_status(:found) - expect(Ci::PipelineScheduleVariable.first.key).to eq("AAA") - expect(Ci::PipelineScheduleVariable.first.value).to eq("AAA123") - expect(Ci::PipelineScheduleVariable.last.key).to eq("BBB") - expect(Ci::PipelineScheduleVariable.last.value).to eq("BBB123") - end - end - - context 'when variables_attributes has two variables and duplicted' do - let(:schedule) do - basic_param.merge({ - variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ] - }) - end - - it 'returns an error that the keys of variable are duplicated' do - expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } - .to change { Ci::PipelineSchedule.count }.by(0) - .and change { Ci::PipelineScheduleVariable.count }.by(0) + context 'when variables_attributes is empty' do + let(:schedule) { basic_param } - expect(assigns(:schedule).errors['variables.key']).not_to be_empty - end - end - end + it 'creates a new schedule' do + expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } + .to change { Ci::PipelineSchedule.count }.by(1) + .and change { Ci::PipelineScheduleVariable.count }.by(0) - describe 'PUT #update' do - before do - create(:user).tap do |user| - project.add_developer(user) - sign_in(user) + expect(response).to have_http_status(:found) + end end - end - let(:basic_param) do - { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' } - end + context 'when variables_attributes has one variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' } ] + }) + end - context 'when a pipeline schedule has no variables' do - context 'when params do not include variables' do - let(:schedule) { basic_param } + it 'creates a new schedule' do + expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } + .to change { Ci::PipelineSchedule.count }.by(1) + .and change { Ci::PipelineScheduleVariable.count }.by(1) - it 'updates only scheduled pipeline attributes' do - put :update, namespace_id: project.namespace.to_param, - project_id: project, id: pipeline_schedule, schedule: schedule + expect(response).to have_http_status(:found) + expect(Ci::PipelineScheduleVariable.last.key).to eq("AAA") + expect(Ci::PipelineScheduleVariable.last.value).to eq("AAA123") + end - pipeline_schedule.reload + context 'when the same key has already been persisted' do + it 'returns an error that the key of variable is invaild' do + post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule - expect(response).to have_http_status(:found) - expect(pipeline_schedule.description).to eq('updated_desc') - expect(pipeline_schedule.cron).to eq('0 1 * * *') - expect(pipeline_schedule.cron_timezone).to eq('UTC') - expect(pipeline_schedule.ref).to eq('patch-x') - expect(pipeline_schedule.active).to eq(true) - expect(pipeline_schedule.variables).to be_empty + pipeline_schedule_variable = build(:ci_pipeline_schedule_variable, key: 'AAA', pipeline_schedule: assigns(:schedule)) + expect(pipeline_schedule_variable).to be_invalid + end end end - context 'when params include one variable' do + context 'when variables_attributes has one variable and key is empty' do let(:schedule) do basic_param.merge({ - variables_attributes: [ { key: 'AAA', value: 'AAA123' } ] + variables_attributes: [ { key: '', value: 'AAA123' } ] }) end - it 'inserts new variable to the pipeline schedule' do - expect do - put :update, namespace_id: project.namespace.to_param, - project_id: project, id: pipeline_schedule, schedule: schedule - end.to change { Ci::PipelineScheduleVariable.count }.by(1) - - pipeline_schedule.reload + it 'returns an error that the key of variable is invaild' do + expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } + .to change { Ci::PipelineSchedule.count }.by(0) + .and change { Ci::PipelineScheduleVariable.count }.by(0) - expect(response).to have_http_status(:found) - expect(pipeline_schedule.variables.last.key).to eq('AAA') - expect(pipeline_schedule.variables.last.value).to eq('AAA123') + expect(assigns(:schedule).errors['variables.key']).not_to be_empty end end - context 'when params include two unique variables' do + context 'when variables_attributes has two variables and unique' do let(:schedule) do basic_param.merge({ variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ] }) end - it 'inserts two new variables to the pipeline schedule' do - expect do - put :update, namespace_id: project.namespace.to_param, - project_id: project, id: pipeline_schedule, schedule: schedule - end.to change { Ci::PipelineScheduleVariable.count }.by(2) - - pipeline_schedule.reload + it 'creates a new schedule' do + expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } + .to change { Ci::PipelineSchedule.count }.by(1) + .and change { Ci::PipelineScheduleVariable.count }.by(2) expect(response).to have_http_status(:found) - expect(pipeline_schedule.variables.first.key).to eq('AAA') - expect(pipeline_schedule.variables.first.value).to eq('AAA123') - expect(pipeline_schedule.variables.last.key).to eq('BBB') - expect(pipeline_schedule.variables.last.value).to eq('BBB123') + expect(Ci::PipelineScheduleVariable.first.key).to eq("AAA") + expect(Ci::PipelineScheduleVariable.first.value).to eq("AAA123") + expect(Ci::PipelineScheduleVariable.last.key).to eq("BBB") + expect(Ci::PipelineScheduleVariable.last.value).to eq("BBB123") end end - context 'when params include two duplicated variables' do + context 'when variables_attributes has two variables and duplicted' do let(:schedule) do basic_param.merge({ variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ] }) end - it 'returns an error that variables are duplciated' do - put :update, namespace_id: project.namespace.to_param, - project_id: project, id: pipeline_schedule, schedule: schedule + it 'returns an error that the keys of variable are duplicated' do + expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule } + .to change { Ci::PipelineSchedule.count }.by(0) + .and change { Ci::PipelineScheduleVariable.count }.by(0) expect(assigns(:schedule).errors['variables.key']).not_to be_empty end end end - context 'when a pipeline schedule has one variable' do - let!(:pipeline_schedule_variable) do - create(:ci_pipeline_schedule_variable, key: 'CCC', - pipeline_schedule: pipeline_schedule) + describe 'security' do + let(:action) do + proc do |user| + post :create, namespace_id: project.namespace.to_param, + project_id: project, + schedule: { description: 'aaaaaaaa', cron: '0 4 * * *', + cron_timezone: 'UTC', ref: 'master', active: '1' } + end end - context 'when params do not include variables' do - let(:schedule) { basic_param } + specify { expect(action).to be_allowed_for(:admin) } + specify { expect(action).to be_allowed_for(:owner).of(project) } + specify { expect(action).to be_allowed_for(:master).of(project) } + specify { expect(action).to be_allowed_for(:developer).of(project) } + specify { expect(action).to be_denied_for(:reporter).of(project) } + specify { expect(action).to be_denied_for(:guest).of(project) } + specify { expect(action).to be_denied_for(:user) } + specify { expect(action).to be_denied_for(:external) } + specify { expect(action).to be_denied_for(:visitor) } + end + end - it 'updates only scheduled pipeline attributes' do - put :update, namespace_id: project.namespace.to_param, - project_id: project, id: pipeline_schedule, schedule: schedule + describe 'PUT #update' do + describe 'functionality' do + let(:user) { create(:user) } + let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: user) } + + before do + project.add_developer(user) - pipeline_schedule.reload + sign_in(user) + end - expect(response).to have_http_status(:found) - expect(pipeline_schedule.description).to eq('updated_desc') - expect(pipeline_schedule.cron).to eq('0 1 * * *') - expect(pipeline_schedule.cron_timezone).to eq('UTC') - expect(pipeline_schedule.ref).to eq('patch-x') - expect(pipeline_schedule.active).to eq(true) - expect(pipeline_schedule.variables.count).to eq(1) - expect(pipeline_schedule.variables.last.key).to eq('CCC') + context 'when a pipeline schedule has no variables' do + let(:basic_param) do + { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' } end - end - context 'when params include one variable' do - context 'when adds a new variable' do + context 'when params do not include variables' do + let(:schedule) { basic_param } + + it 'updates only scheduled pipeline attributes' do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule + + pipeline_schedule.reload + + expect(response).to have_http_status(:found) + expect(pipeline_schedule.description).to eq('updated_desc') + expect(pipeline_schedule.cron).to eq('0 1 * * *') + expect(pipeline_schedule.cron_timezone).to eq('UTC') + expect(pipeline_schedule.ref).to eq('patch-x') + expect(pipeline_schedule.active).to eq(true) + expect(pipeline_schedule.variables).to be_empty + end + end + + context 'when params include one variable' do let(:schedule) do basic_param.merge({ - variables_attributes: [ { key: 'AAA', value: 'AAA123' }] + variables_attributes: [ { key: 'AAA', value: 'AAA123' } ] }) end - it 'adds the new variable' do + it 'inserts new variable to the pipeline schedule' do expect do put :update, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule, schedule: schedule end.to change { Ci::PipelineScheduleVariable.count }.by(1) + pipeline_schedule.reload + + expect(response).to have_http_status(:found) expect(pipeline_schedule.variables.last.key).to eq('AAA') + expect(pipeline_schedule.variables.last.value).to eq('AAA123') end end - context 'when updates a variable' do + context 'when params include two unique variables' do let(:schedule) do basic_param.merge({ - variables_attributes: [ { id: pipeline_schedule_variable.id, value: 'new_value' } ] + variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ] }) end - it 'updates the variable' do + it 'inserts two new variables to the pipeline schedule' do expect do put :update, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule, schedule: schedule - end.not_to change { Ci::PipelineScheduleVariable.count } + end.to change { Ci::PipelineScheduleVariable.count }.by(2) - pipeline_schedule_variable.reload + pipeline_schedule.reload - expect(pipeline_schedule_variable.value).to eq('new_value') + expect(response).to have_http_status(:found) + expect(pipeline_schedule.variables.first.key).to eq('AAA') + expect(pipeline_schedule.variables.first.value).to eq('AAA123') + expect(pipeline_schedule.variables.last.key).to eq('BBB') + expect(pipeline_schedule.variables.last.value).to eq('BBB123') end end - context 'when deletes a variable' do + context 'when params include two duplicated variables' do let(:schedule) do basic_param.merge({ - variables_attributes: [ { id: pipeline_schedule_variable.id, _destroy: true } ] + variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ] }) end - it 'delete the existsed variable' do - expect do - put :update, namespace_id: project.namespace.to_param, - project_id: project, id: pipeline_schedule, schedule: schedule - end.to change { Ci::PipelineScheduleVariable.count }.by(-1) + it 'returns an error that variables are duplciated' do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule + + expect(assigns(:schedule).errors['variables.key']).not_to be_empty end end end - end - end - describe 'GET edit' do - context 'TODO: integrate to bottom' do - let(:user) { create(:user) } + context 'when a pipeline schedule has one variable' do + let(:basic_param) do + { description: 'updated_desc', cron: '0 1 * * *', cron_timezone: 'UTC', ref: 'patch-x', active: '1' } + end - before do - project.add_master(user) + let!(:pipeline_schedule_variable) do + create(:ci_pipeline_schedule_variable, key: 'CCC', + pipeline_schedule: pipeline_schedule) + end - sign_in(user) - end + context 'when params do not include variables' do + let(:schedule) { basic_param } - it 'loads the pipeline schedule' do - get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + it 'updates only scheduled pipeline attributes' do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule - expect(response).to have_http_status(:ok) - expect(assigns(:schedule)).to eq(pipeline_schedule) + pipeline_schedule.reload + + expect(response).to have_http_status(:found) + expect(pipeline_schedule.description).to eq('updated_desc') + expect(pipeline_schedule.cron).to eq('0 1 * * *') + expect(pipeline_schedule.cron_timezone).to eq('UTC') + expect(pipeline_schedule.ref).to eq('patch-x') + expect(pipeline_schedule.active).to eq(true) + expect(pipeline_schedule.variables.count).to eq(1) + expect(pipeline_schedule.variables.last.key).to eq('CCC') + end + end + + context 'when params include one variable' do + context 'when adds a new variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { key: 'AAA', value: 'AAA123' }] + }) + end + + it 'adds the new variable' do + expect do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule + end.to change { Ci::PipelineScheduleVariable.count }.by(1) + + expect(pipeline_schedule.variables.last.key).to eq('AAA') + end + end + + context 'when updates a variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { id: pipeline_schedule_variable.id, value: 'new_value' } ] + }) + end + + it 'updates the variable' do + expect do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule + end.not_to change { Ci::PipelineScheduleVariable.count } + + pipeline_schedule_variable.reload + + expect(pipeline_schedule_variable.value).to eq('new_value') + end + end + + context 'when deletes a variable' do + let(:schedule) do + basic_param.merge({ + variables_attributes: [ { id: pipeline_schedule_variable.id, _destroy: true } ] + }) + end + + it 'delete the existsed variable' do + expect do + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, schedule: schedule + end.to change { Ci::PipelineScheduleVariable.count }.by(-1) + end + end + end end end - context 'when a developer created a pipeline schedule' do - context 'when the developer edits' do - it 'can edit variables' do - # TODO: + describe 'security' do + context 'when a developer created a pipeline schedule' do + let(:developer_1) { create(:user) } + let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: developer_1) } + + before do + project.add_developer(developer_1) end - end - context 'when other developers edit' do - it 'can not edit variables' do - # TODO: + context 'when the developer updates' do + let(:action) do + proc do |user| + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, + schedule: { description: 'updated_desc' } + end + end + + specify { expect(action).to be_allowed_for(developer_1) } end - end - context 'when a master edits' do - it 'can edit variables' do - # TODO: + context 'when another developer updates' do + let(:action) do + proc do |user| + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, + schedule: { description: 'updated_desc' } + end + end + + specify { expect(action).to be_denied_for(:developer).of(project) } end - end - end - context 'when a master created a pipeline schedule' do - context 'when the master edits' do - it 'can edit variables' do - # TODO: + context 'when a master updates' do + let(:action) do + proc do |user| + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, + schedule: { description: 'updated_desc' } + end + end + + specify { expect(action).to be_allowed_for(:master).of(project) } end end - context 'when other masters edit' do - it 'can edit variables' do - # TODO: + context 'when a master created a pipeline schedule' do + let(:master_1) { create(:user) } + let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: master_1) } + + before do + project.add_master(master_1) end - end - context 'when developers edit' do - it 'can not edit variables' do - # TODO: + context 'when the master updates' do + let(:action) do + proc do |user| + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, + schedule: { description: 'updated_desc' } + end + end + + specify { expect(action).to be_allowed_for(master_1) } + end + + context 'when other masters updates' do + let(:action) do + proc do |user| + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, + schedule: { description: 'updated_desc' } + end + end + + specify { expect(action).to be_allowed_for(:master).of(project) } + end + + context 'when a developer updates' do + let(:action) do + proc do |user| + put :update, namespace_id: project.namespace.to_param, + project_id: project, id: pipeline_schedule, + schedule: { description: 'updated_desc' } + end + end + + specify { expect(action).to be_denied_for(:developer).of(project) } end end end end + describe 'GET edit' do + let(:user) { create(:user) } + + before do + project.add_master(user) + + sign_in(user) + end + + it 'loads the pipeline schedule' do + get :edit, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + + expect(response).to have_http_status(:ok) + expect(assigns(:schedule)).to eq(pipeline_schedule) + end + end + describe 'DELETE #destroy' do set(:user) { create(:user) } |