diff options
| -rw-r--r-- | app/controllers/projects/pipeline_schedules_controller.rb | 31 | ||||
| -rw-r--r-- | app/helpers/gitlab_routing_helper.rb | 5 | ||||
| -rw-r--r-- | app/models/repository.rb | 6 | ||||
| -rw-r--r-- | app/policies/ci/pipeline_policy.rb | 16 | ||||
| -rw-r--r-- | app/policies/ci/pipeline_schedule_policy.rb | 10 | ||||
| -rw-r--r-- | app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml | 4 | ||||
| -rw-r--r-- | app/workers/run_pipeline_schedule_worker.rb | 22 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml | 5 | ||||
| -rw-r--r-- | config/routes/project.rb | 1 | ||||
| -rw-r--r-- | lib/gitlab/action_rate_limiter.rb | 47 | ||||
| -rw-r--r-- | spec/controllers/projects/pipeline_schedules_controller_spec.rb | 67 | ||||
| -rw-r--r-- | spec/lib/gitlab/action_rate_limiter_spec.rb | 29 | ||||
| -rw-r--r-- | spec/models/repository_spec.rb | 9 | ||||
| -rw-r--r-- | spec/policies/ci/pipeline_schedule_policy_spec.rb | 92 | ||||
| -rw-r--r-- | spec/workers/run_pipeline_schedule_worker_spec.rb | 39 |
15 files changed, 371 insertions, 12 deletions
diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index ec7c645df5a..b478e7b5e05 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,9 +1,11 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController before_action :schedule, except: [:index, :new, :create] + before_action :play_rate_limit, only: [:play] + 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] + before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create, :play] before_action :authorize_admin_pipeline_schedule!, only: [:destroy] def index @@ -40,6 +42,18 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end end + def play + job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id) + + if job_id + flash[:notice] = "Successfully scheduled a pipeline to run. Go to the <a href=\"#{project_pipelines_path(@project)}\">Pipelines page</a> for details.".html_safe + else + flash[:alert] = 'Unable to schedule a pipeline to run immediately' + end + + redirect_to pipeline_schedules_path(@project) + end + def take_ownership if schedule.update(owner: current_user) redirect_to pipeline_schedules_path(@project) @@ -60,6 +74,17 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController private + def play_rate_limit + return unless current_user + + limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule) + + return unless limiter.throttled?([current_user, schedule], 1) + + flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + redirect_to pipeline_schedules_path(@project) + end + def schedule @schedule ||= project.pipeline_schedules.find(params[:id]) end @@ -70,6 +95,10 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController variables_attributes: [:id, :key, :value, :_destroy] ) end + def authorize_play_pipeline_schedule! + return access_denied! unless can?(current_user, :play_pipeline_schedule, schedule) + end + def authorize_update_pipeline_schedule! return access_denied! unless can?(current_user, :update_pipeline_schedule, schedule) end diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index a77aa0ad2cc..7f3c118c7ab 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -182,6 +182,11 @@ module GitlabRoutingHelper edit_project_pipeline_schedule_path(project, schedule) end + def play_pipeline_schedule_path(schedule, *args) + project = schedule.project + play_project_pipeline_schedule_path(project, schedule, *args) + end + def take_ownership_pipeline_schedule_path(schedule, *args) project = schedule.project take_ownership_project_pipeline_schedule_path(project, schedule, *args) diff --git a/app/models/repository.rb b/app/models/repository.rb index 552a354d1ce..4ec8ec9c8b2 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -221,6 +221,12 @@ class Repository branch_names.include?(branch_name) end + def tag_exists?(tag_name) + return false unless raw_repository + + tag_names.include?(tag_name) + end + def ref_exists?(ref) !!raw_repository&.ref_exists?(ref) rescue ArgumentError diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index 4e689a9efd5..6363c382ff8 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/app/policies/ci/pipeline_policy.rb @@ -2,16 +2,18 @@ module Ci class PipelinePolicy < BasePolicy delegate { @subject.project } - condition(:protected_ref) do - access = ::Gitlab::UserAccess.new(@user, project: @subject.project) + condition(:protected_ref) { ref_protected?(@user, @subject.project, @subject.tag?, @subject.ref) } - if @subject.tag? - !access.can_create_tag?(@subject.ref) + rule { protected_ref }.prevent :update_pipeline + + def ref_protected?(user, project, tag, ref) + access = ::Gitlab::UserAccess.new(user, project: project) + + if tag + !access.can_create_tag?(ref) else - !access.can_update_branch?(@subject.ref) + !access.can_update_branch?(ref) end end - - rule { protected_ref }.prevent :update_pipeline end end diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index 6b7598e1821..abcf536b2f7 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -2,13 +2,23 @@ module Ci class PipelineSchedulePolicy < PipelinePolicy alias_method :pipeline_schedule, :subject + condition(:protected_ref) do + ref_protected?(@user, @subject.project, @subject.project.repository.tag_exists?(@subject.ref), @subject.ref) + end + condition(:owner_of_schedule) do can?(:developer_access) && pipeline_schedule.owned_by?(@user) end + rule { can?(:developer_access) }.policy do + enable :play_pipeline_schedule + end + rule { can?(:master_access) | owner_of_schedule }.policy do enable :update_pipeline_schedule enable :admin_pipeline_schedule end + + rule { protected_ref }.prevent :play_pipeline_schedule end end diff --git a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml index bd8c38292d6..f8c4005a9e0 100644 --- a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml +++ b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml @@ -26,10 +26,12 @@ = pipeline_schedule.owner&.name %td .pull-right.btn-group + - if can?(current_user, :play_pipeline_schedule, pipeline_schedule) + = link_to play_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('Play'), class: 'btn' do + = icon('play') - if can?(current_user, :update_pipeline_schedule, pipeline_schedule) = link_to take_ownership_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('PipelineSchedules|Take ownership'), class: 'btn' do = s_('PipelineSchedules|Take ownership') - - if can?(current_user, :update_pipeline_schedule, pipeline_schedule) = link_to edit_pipeline_schedule_path(pipeline_schedule), title: _('Edit'), class: 'btn' do = icon('pencil') - if can?(current_user, :admin_pipeline_schedule, pipeline_schedule) diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb new file mode 100644 index 00000000000..7725ad319a3 --- /dev/null +++ b/app/workers/run_pipeline_schedule_worker.rb @@ -0,0 +1,22 @@ +class RunPipelineScheduleWorker + include ApplicationWorker + include PipelineQueue + + enqueue_in group: :creation + + def perform(schedule_id, user_id) + schedule = Ci::PipelineSchedule.find_by(id: schedule_id) + user = User.find_by(id: user_id) + + return unless schedule && user + + run_pipeline_schedule(schedule, user) + end + + def run_pipeline_schedule(schedule, user) + Ci::CreatePipelineService.new(schedule.project, + user, + ref: schedule.ref) + .execute(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule) + end +end diff --git a/changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml b/changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml new file mode 100644 index 00000000000..6d06f695f10 --- /dev/null +++ b/changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml @@ -0,0 +1,5 @@ +--- +title: Add button to run scheduled pipeline immediately +merge_request: +author: +type: added diff --git a/config/routes/project.rb b/config/routes/project.rb index 093da10f57f..239b5480321 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -179,6 +179,7 @@ constraints(ProjectUrlConstrainer.new) do resources :pipeline_schedules, except: [:show] do member do + post :play post :take_ownership end end diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb new file mode 100644 index 00000000000..4cd3bdefda3 --- /dev/null +++ b/lib/gitlab/action_rate_limiter.rb @@ -0,0 +1,47 @@ +module Gitlab + # This class implements a simple rate limiter that can be used to throttle + # certain actions. Unlike Rack Attack and Rack::Throttle, which operate at + # the middleware level, this can be used at the controller level. + class ActionRateLimiter + TIME_TO_EXPIRE = 60 # 1 min + + attr_accessor :action, :expiry_time + + def initialize(action:, expiry_time: TIME_TO_EXPIRE) + @action = action + @expiry_time = expiry_time + end + + # Increments the given cache key and increments the value by 1 with the + # given expiration time. Returns the incremented value. + # + # key - An array of ActiveRecord instances + def increment(key) + value = 0 + + Gitlab::Redis::Cache.with do |redis| + cache_key = action_key(key) + value = redis.incr(cache_key) + redis.expire(cache_key, expiry_time) if value == 1 + end + + value + end + + # Increments the given key and returns true if the action should + # be throttled. + # + # key - An array of ActiveRecord instances + # threshold_value - The maximum number of times this action should occur in the given time interval + def throttled?(key, threshold_value) + self.increment(key) > threshold_value + end + + private + + def action_key(key) + serialized = key.map { |obj| "#{obj.class.model_name.to_s.underscore}:#{obj.id}" }.join(":") + "action_rate_limiter:#{action}:#{serialized}" + end + end +end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 4e52e261920..966ffdf6996 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -3,10 +3,12 @@ require 'spec_helper' describe Projects::PipelineSchedulesController do include AccessMatchersForController - set(:project) { create(:project, :public) } - let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } + set(:project) { create(:project, :public, :repository) } + set(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } describe 'GET #index' do + render_views + let(:scope) { nil } let!(:inactive_pipeline_schedule) do create(:ci_pipeline_schedule, :inactive, project: project) @@ -96,7 +98,7 @@ describe Projects::PipelineSchedulesController do end end - context 'when variables_attributes has two variables and duplicted' do + context 'when variables_attributes has two variables and duplicated' do let(:schedule) do basic_param.merge({ variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }] @@ -364,6 +366,65 @@ describe Projects::PipelineSchedulesController do end end + describe 'POST #play', :clean_gitlab_redis_cache do + set(:user) { create(:user) } + let(:ref) { 'master' } + + before do + project.add_developer(user) + + sign_in(user) + end + + context 'when an anonymous user makes the request' do + before do + sign_out(user) + end + + it 'does not allow pipeline to be executed' do + expect(RunPipelineScheduleWorker).not_to receive(:perform_async) + + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when a developer makes the request' do + it 'executes a new pipeline' do + expect(RunPipelineScheduleWorker).to receive(:perform_async).with(pipeline_schedule.id, user.id).and_return('job-123') + + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + + expect(flash[:notice]).to start_with 'Successfully scheduled a pipeline to run' + expect(response).to have_gitlab_http_status(302) + end + + it 'prevents users from scheduling the same pipeline repeatedly' do + 2.times do + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + end + + expect(flash.to_a.size).to eq(2) + expect(flash[:alert]).to eq 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + expect(response).to have_gitlab_http_status(302) + end + end + + context 'when a developer attempts to schedule a protected ref' do + it 'does not allow pipeline to be executed' do + create(:protected_branch, project: project, name: ref) + protected_schedule = create(:ci_pipeline_schedule, project: project, ref: ref) + + expect(RunPipelineScheduleWorker).not_to receive(:perform_async) + + post :play, namespace_id: project.namespace.to_param, project_id: project, id: protected_schedule.id + + expect(response).to have_gitlab_http_status(404) + end + end + end + describe 'DELETE #destroy' do set(:user) { create(:user) } diff --git a/spec/lib/gitlab/action_rate_limiter_spec.rb b/spec/lib/gitlab/action_rate_limiter_spec.rb new file mode 100644 index 00000000000..542fc03e555 --- /dev/null +++ b/spec/lib/gitlab/action_rate_limiter_spec.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe Gitlab::ActionRateLimiter do + let(:redis) { double('redis') } + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:key) { [user, project] } + let(:cache_key) { "action_rate_limiter:test_action:user:#{user.id}:project:#{project.id}" } + + subject { described_class.new(action: :test_action, expiry_time: 100) } + + before do + allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis) + end + + it 'increases the throttle count and sets the expire time' do + expect(redis).to receive(:incr).with(cache_key).and_return(1) + expect(redis).to receive(:expire).with(cache_key, 100) + + expect(subject.throttled?(key, 1)).to be false + end + + it 'returns true if the key is throttled' do + expect(redis).to receive(:incr).with(cache_key).and_return(2) + expect(redis).not_to receive(:expire) + + expect(subject.throttled?(key, 1)).to be true + end +end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 799d99c0369..bdc430c9095 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1163,6 +1163,15 @@ describe Repository do end end + describe '#tag_exists?' do + it 'uses tag_names' do + allow(repository).to receive(:tag_names).and_return(['foobar']) + + expect(repository.tag_exists?('foobar')).to eq(true) + expect(repository.tag_exists?('master')).to eq(false) + end + end + describe '#branch_names', :use_clean_rails_memory_store_caching do let(:fake_branch_names) { ['foobar'] } diff --git a/spec/policies/ci/pipeline_schedule_policy_spec.rb b/spec/policies/ci/pipeline_schedule_policy_spec.rb new file mode 100644 index 00000000000..1b0e9fac355 --- /dev/null +++ b/spec/policies/ci/pipeline_schedule_policy_spec.rb @@ -0,0 +1,92 @@ +require 'spec_helper' + +describe Ci::PipelineSchedulePolicy, :models do + set(:user) { create(:user) } + set(:project) { create(:project, :repository) } + set(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project) } + + let(:policy) do + described_class.new(user, pipeline_schedule) + end + + describe 'rules' do + describe 'rules for protected ref' do + before do + project.add_developer(user) + end + + context 'when no one can push or merge to the branch' do + before do + create(:protected_branch, :no_one_can_push, + name: pipeline_schedule.ref, project: project) + end + + it 'does not include ability to play pipeline schedule' do + expect(policy).to be_disallowed :play_pipeline_schedule + end + end + + context 'when developers can push to the branch' do + before do + create(:protected_branch, :developers_can_merge, + name: pipeline_schedule.ref, project: project) + end + + it 'includes ability to update pipeline' do + expect(policy).to be_allowed :play_pipeline_schedule + end + end + + context 'when no one can create the tag' do + let(:tag) { 'v1.0.0' } + + before do + pipeline_schedule.update(ref: tag) + + create(:protected_tag, :no_one_can_create, + name: pipeline_schedule.ref, project: project) + end + + it 'does not include ability to play pipeline schedule' do + expect(policy).to be_disallowed :play_pipeline_schedule + end + end + + context 'when no one can create the tag but it is not a tag' do + before do + create(:protected_tag, :no_one_can_create, + name: pipeline_schedule.ref, project: project) + end + + it 'includes ability to play pipeline schedule' do + expect(policy).to be_allowed :play_pipeline_schedule + end + end + end + + describe 'rules for owner of schedule' do + before do + project.add_developer(user) + pipeline_schedule.update(owner: user) + end + + it 'includes abilities to do do all operations on 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 + end + + describe 'rules for a master' do + before do + project.add_master(user) + end + + it 'includes abilities to do do all operations on 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 + end + end +end diff --git a/spec/workers/run_pipeline_schedule_worker_spec.rb b/spec/workers/run_pipeline_schedule_worker_spec.rb new file mode 100644 index 00000000000..481a84837f9 --- /dev/null +++ b/spec/workers/run_pipeline_schedule_worker_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' + +describe RunPipelineScheduleWorker do + describe '#perform' do + set(:project) { create(:project) } + set(:user) { create(:user) } + set(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project ) } + let(:worker) { described_class.new } + + context 'when a project not found' do + it 'does not call the Service' do + expect(Ci::CreatePipelineService).not_to receive(:new) + expect(worker).not_to receive(:run_pipeline_schedule) + + worker.perform(100000, user.id) + end + end + + context 'when a user not found' do + it 'does not call the Service' do + expect(Ci::CreatePipelineService).not_to receive(:new) + expect(worker).not_to receive(:run_pipeline_schedule) + + worker.perform(pipeline_schedule.id, 10000) + end + end + + context 'when everything is ok' do + let(:create_pipeline_service) { instance_double(Ci::CreatePipelineService) } + + it 'calls the Service' do + expect(Ci::CreatePipelineService).to receive(:new).with(project, user, ref: pipeline_schedule.ref).and_return(create_pipeline_service) + expect(create_pipeline_service).to receive(:execute).with(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: pipeline_schedule) + + worker.perform(pipeline_schedule.id, user.id) + end + end + end +end |