Address review comments with playing pipeline schedulersh-add-schedule-pipeline-run-now

4 files changed, 53 insertions, 21 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index dd6593650e7..b478e7b5e05 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -1,6 +1,7 @@
 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]
@@ -42,21 +43,13 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   end
 
   def play
-    limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule)
-
-    if limiter.throttled?(throttle_key, 1)
-      flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.'
-      return redirect_to pipeline_schedules_path(@project)
-    end
-
     job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id)
 
-    flash[:notice] =
-      if job_id
-        "Successfully scheduled a pipeline to run. Go to the <a href=\"#{project_pipelines_path(@project)}\">Pipelines page</a> for details.".html_safe
-      else
-        'Unable to schedule a pipeline to run immediately'
-      end
+    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
@@ -81,8 +74,15 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
 
   private
 
-  def throttle_key
-    "user:#{current_user.id}:schedule:#{schedule.id}"
+  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
diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb
index 53add503c60..4cd3bdefda3 100644
--- a/lib/gitlab/action_rate_limiter.rb
+++ b/lib/gitlab/action_rate_limiter.rb
@@ -12,11 +12,15 @@ module Gitlab
       @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_rate_limiter:#{action}:#{key}"
+        cache_key = action_key(key)
         value = redis.incr(cache_key)
         redis.expire(cache_key, expiry_time) if value == 1
       end
@@ -24,8 +28,20 @@ module Gitlab
       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 ffc1259eb8f..966ffdf6996 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -370,13 +370,27 @@ describe Projects::PipelineSchedulesController do
     set(:user) { create(:user) }
     let(:ref) { 'master' }
 
-    context 'when a developer makes the request' do
+    before do
+      project.add_developer(user)
+
+      sign_in(user)
+    end
+
+    context 'when an anonymous user makes the request' do
       before do
-        project.add_developer(user)
+        sign_out(user)
+      end
 
-        sign_in(user)
+      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')
 
diff --git a/spec/lib/gitlab/action_rate_limiter_spec.rb b/spec/lib/gitlab/action_rate_limiter_spec.rb
index 84661605623..542fc03e555 100644
--- a/spec/lib/gitlab/action_rate_limiter_spec.rb
+++ b/spec/lib/gitlab/action_rate_limiter_spec.rb
@@ -2,8 +2,10 @@ require 'spec_helper'
 
 describe Gitlab::ActionRateLimiter do
   let(:redis) { double('redis') }
-  let(:key) { 'user:1' }
-  let(:cache_key) { "action_rate_limiter:test_action:#{key}" }
+  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) }