Addresses backend comments48717-rate-limit-raw-controller-show

* Makes spec description more explicit
* Refactor request call to avoid duplicated call
* Add a guard conditional on ActionRateLimiter to disable throttling
* Ensure case-sensitive files are treated as different files (removes
downcase from ActionRateLimiter)
* Changes method name to be more accurate (log_request instead of
register_request)

6 files changed, 76 insertions, 43 deletions

diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index cd715ad43f7..3254229d9cb 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -23,7 +23,7 @@ class Projects::RawController < Projects::ApplicationController
 
     return unless limiter.throttled?([@project, @commit, @path], raw_blob_request_limit)
 
-    limiter.register_request(request, :raw_blob_request_limit, current_user)
+    limiter.log_request(request, :raw_blob_request_limit, current_user)
 
     flash[:alert] = _('You cannot access the raw file. Please wait a minute.')
     redirect_to project_blob_path(@project, File.join(@ref, @path))
diff --git a/app/views/admin/application_settings/_performance.html.haml b/app/views/admin/application_settings/_performance.html.haml
index 4b27d9808e2..b52171afc69 100644
--- a/app/views/admin/application_settings/_performance.html.haml
+++ b/app/views/admin/application_settings/_performance.html.haml
@@ -19,6 +19,6 @@
       = f.label :raw_blob_request_limit, _('Raw blob request rate limit per minute'), class: 'label-bold'
       = f.number_field :raw_blob_request_limit, class: 'form-control'
       .form-text.text-muted
-        = _('Highest number of requests per minute for each raw request, default to 300.')
+        = _('Highest number of requests per minute for each raw path, default to 300. To disable throttling set to 0.')
 
   = f.submit 'Save changes', class: "btn btn-success"
diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb
index 253106811dc..fdb06d00548 100644
--- a/lib/gitlab/action_rate_limiter.rb
+++ b/lib/gitlab/action_rate_limiter.rb
@@ -34,9 +34,10 @@ module Gitlab
     # be throttled.
     #
     # key - An array of ActiveRecord instances or strings
-    # threshold_value - The maximum number of times this action should occur in the given time interval
+    # threshold_value - The maximum number of times this action should occur in the given time interval. If number is zero is considered disabled.
     def throttled?(key, threshold_value)
-      self.increment(key) > threshold_value
+      threshold_value > 0 &&
+        self.increment(key) > threshold_value
     end
 
     # Logs request into auth.log
@@ -44,7 +45,7 @@ module Gitlab
     # request - Web request to be logged
     # type - A symbol key that represents the request.
     # current_user - Current user of the request, it can be nil.
-    def register_request(request, type, current_user)
+    def log_request(request, type, current_user)
       request_information = {
         message: 'Action_Rate_Limiter_Request',
         env: type,
@@ -68,7 +69,7 @@ module Gitlab
     def action_key(key)
       serialized = key.map do |obj|
         if obj.is_a?(String)
-          "#{obj.downcase}"
+          "#{obj}"
         else
           "#{obj.class.model_name.to_s.underscore}:#{obj.id}"
         end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 045c8f937e2..38aa2667ecd 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -5288,7 +5288,7 @@ msgstr[1] ""
 msgid "Hide values"
 msgstr ""
 
-msgid "Highest number of requests per minute for each raw request, default to 300."
+msgid "Highest number of requests per minute for each raw path, default to 300. To disable throttling set to 0."
 msgstr ""
 
 msgid "Highest role:"
diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb
index e8e9f099530..8ee3168273f 100644
--- a/spec/controllers/projects/raw_controller_spec.rb
+++ b/spec/controllers/projects/raw_controller_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Projects::RawController do
+  include RepoHelpers
+
   let(:project) { create(:project, :public, :repository) }
 
   describe 'GET #show' do
@@ -51,17 +53,11 @@ describe Projects::RawController do
       let(:file_path) { 'master/README.md' }
 
       before do
-        stub_application_setting(raw_blob_request_limit: 10)
+        stub_application_setting(raw_blob_request_limit: 5)
       end
 
       it 'prevents from accessing the raw file' do
-        11.times do
-          get :show, params: {
-            namespace_id: project.namespace,
-            project_id: project,
-            id: file_path
-          }
-        end
+        execute_raw_requests(requests: 6, project: project, file_path: file_path)
 
         expect(flash[:alert]).to eq('You cannot access the raw file. Please wait a minute.')
         expect(response).to redirect_to(project_blob_path(project, file_path))
@@ -78,45 +74,72 @@ describe Projects::RawController do
 
         expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
 
-        11.times do
-          get :show, params: {
-            namespace_id: project.namespace,
-            project_id: project,
-            id: file_path
-          }
-        end
+        execute_raw_requests(requests: 6, project: project, file_path: file_path)
       end
 
-      context 'when the request uses a different case version of a commit' do
+      context 'when the request uses a different version of a commit' do
         it 'prevents from accessing the raw file' do
-          # 5 times with the normal sha
+          # 3 times with the normal sha
           commit_sha = project.repository.commit.sha
           file_path = "#{commit_sha}/README.md"
 
-          5.times do
-            get :show, params: {
-              namespace_id: project.namespace,
-              project_id: project,
-              id: file_path
-            }
-          end
+          execute_raw_requests(requests: 3, project: project, file_path: file_path)
 
-          # 6 times with the modified version
+          # 3 times with the modified version
           modified_sha = commit_sha.gsub(commit_sha[0..5], commit_sha[0..5].upcase)
           modified_path = "#{modified_sha}/README.md"
 
-          6.times do
-            get :show, params: {
-              namespace_id: project.namespace,
-              project_id: project,
-              id: modified_path
-            }
-          end
+          execute_raw_requests(requests: 3, project: project, file_path: modified_path)
 
           expect(flash[:alert]).to eq('You cannot access the raw file. Please wait a minute.')
           expect(response).to redirect_to(project_blob_path(project, modified_path))
         end
       end
+
+      context 'when the throttling has been disabled' do
+        before do
+          stub_application_setting(raw_blob_request_limit: 0)
+        end
+
+        it 'does not prevent from accessing the raw file' do
+          execute_raw_requests(requests: 10, project: project, file_path: file_path)
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+
+      context 'with case-sensitive files' do
+        it 'prevents from accessing the specific file' do
+          create_file_in_repo(project, 'master', 'master', 'readme.md', 'Add readme.md')
+          create_file_in_repo(project, 'master', 'master', 'README.md', 'Add README.md')
+
+          commit_sha = project.repository.commit.sha
+          file_path = "#{commit_sha}/readme.md"
+
+          # Accessing downcase version of readme
+          execute_raw_requests(requests: 6, project: project, file_path: file_path)
+
+          expect(flash[:alert]).to eq('You cannot access the raw file. Please wait a minute.')
+          expect(response).to redirect_to(project_blob_path(project, file_path))
+
+          # Accessing upcase version of readme
+          file_path = "#{commit_sha}/README.md"
+
+          execute_raw_requests(requests: 1, project: project, file_path: file_path)
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+    end
+  end
+
+  def execute_raw_requests(requests:, project:, file_path:)
+    requests.times do
+      get :show, params: {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: file_path
+      }
     end
   end
 end
diff --git a/spec/lib/gitlab/action_rate_limiter_spec.rb b/spec/lib/gitlab/action_rate_limiter_spec.rb
index 792568c8c52..cf266a25819 100644
--- a/spec/lib/gitlab/action_rate_limiter_spec.rb
+++ b/spec/lib/gitlab/action_rate_limiter_spec.rb
@@ -12,7 +12,7 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
   end
 
   shared_examples 'action rate limiter' do
-    it 'increases the throttle count and sets the expire time' do
+    it 'increases the throttle count and sets the expiration time' do
       expect(redis).to receive(:incr).with(cache_key).and_return(1)
       expect(redis).to receive(:expire).with(cache_key, 100)
 
@@ -25,6 +25,15 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
 
       expect(subject.throttled?(key, 1)).to be_truthy
     end
+
+    context 'when throttling is disabled' do
+      it 'returns false and does not set expiration time' do
+        expect(redis).not_to receive(:incr)
+        expect(redis).not_to receive(:expire)
+
+        expect(subject.throttled?(key, 0)).to be_falsy
+      end
+    end
   end
 
   context 'when the key is an array of only ActiveRecord models' do
@@ -50,7 +59,7 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
     it_behaves_like 'action rate limiter'
   end
 
-  describe '#register_request' do
+  describe '#log_request' do
     let(:file_path) { 'master/README.md' }
     let(:type) { :raw_blob_request_limit }
     let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
@@ -75,7 +84,7 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
       it 'logs information to auth.log' do
         expect(Gitlab::AuthLogger).to receive(:error).with(base_attributes).once
 
-        subject.register_request(request, type, current_user)
+        subject.log_request(request, type, current_user)
       end
     end
 
@@ -92,7 +101,7 @@ describe Gitlab::ActionRateLimiter, :clean_gitlab_redis_cache do
       it 'logs information to auth.log' do
         expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
 
-        subject.register_request(request, type, current_user)
+        subject.log_request(request, type, current_user)
       end
     end
   end