Merge branch '37093-resolve-protected-path-conflict' into 'master'

Allow "Admin Area Protected Paths" to be used at the same time as "Git and container registry failed authentication ban"

See merge request gitlab-org/gitlab!21090

config/initializers/1_settings.rb

@@ -645,6 +645,7 @@ Settings.rack_attack.git_basic_auth['ip_whitelist'] ||= %w{127.0.0.1}
 Settings.rack_attack.git_basic_auth['maxretry'] ||= 10
 Settings.rack_attack.git_basic_auth['findtime'] ||= 1.minute
 Settings.rack_attack.git_basic_auth['bantime'] ||= 1.hour
+Settings.rack_attack['admin_area_protected_paths_enabled'] ||= false
 
 #
 # Gitaly

config/initializers/rack_attack_new.rb

+# Specs for this file can be found on:
+# * spec/lib/gitlab/throttle_spec.rb
+# * spec/requests/rack_attack_global_spec.rb
 module Gitlab::Throttle
   def self.settings
     Gitlab::CurrentSettings.current_application_settings
   end
 
+  # Returns true if we should use the Admin Area protected paths throttle
   def self.protected_paths_enabled?
-    !self.omnibus_protected_paths_present? &&
-      self.settings.throttle_protected_paths_enabled?
+    return false if should_use_omnibus_protected_paths?
+
+    self.settings.throttle_protected_paths_enabled?
+  end
+
+  # To be removed in 13.0: https://gitlab.com/gitlab-org/gitlab/issues/29952
+  def self.should_use_omnibus_protected_paths?
+    !Settings.rack_attack.admin_area_protected_paths_enabled &&
+      self.omnibus_protected_paths_present?
   end
 
   def self.omnibus_protected_paths_present?

doc/user/admin_area/settings/protected_paths.md

@@ -60,18 +60,14 @@ NOTE: **Note:** If Omnibus settings are present, applications settings will be a
 
 To migrate from Omnibus GitLab 12.3 and earlier settings:
 
-1. Disable the Protected Paths throttle from Omnibus, by changing `rack_attack_enabled` value to `false` on [`rack_attack.rb.erb`](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/files/gitlab-cookbooks/gitlab/templates/default/rack_attack.rb.erb#L18):
-
-   ```ruby
-   rack_attack_enabled = false
-   ```
-
 1. Customize and enable your protected paths settings by following [Configure using GitLab UI](#configure-using-gitlab-ui) section.
 
-1. Restart GitLab:
+1. SSH into your frontend nodes and add to `/etc/gitlab/gitlab.rb`:
 
-   ```bash
-   sudo gitlab-ctl restart
+   ```ruby
+   gitlab_rails['rack_attack_admin_area_protected_paths_enabled'] = true
    ```
 
+1. [Reconfigure GitLab](../../../administration/restart_gitlab.md#omnibus-gitlab-reconfigure) for the changes to take effect.
+
 That's it. Protected paths throttle are now managed by GitLab admin settings.

spec/lib/gitlab/throttle_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Throttle do
+  describe '.protected_paths_enabled?' do
+    subject { described_class.protected_paths_enabled? }
+
+    context 'when omnibus protected paths throttle should be used' do
+      before do
+        expect(described_class).to receive(:should_use_omnibus_protected_paths?).and_return(true)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when omnibus protected paths throttle should not be used' do
+      before do
+        expect(described_class).to receive(:should_use_omnibus_protected_paths?).and_return(false)
+      end
+
+      it 'returns Application Settings throttle_protected_paths_enabled?' do
+        expect(Gitlab::CurrentSettings.current_application_settings).to receive(:throttle_protected_paths_enabled?)
+
+        subject
+      end
+    end
+  end
+
+  describe '.should_use_omnibus_protected_paths?' do
+    subject { described_class.should_use_omnibus_protected_paths? }
+
+    context 'when rack_attack.admin_area_protected_paths_enabled config is unspecified' do
+      context 'when the omnibus protected paths throttle has been recently used (it has data)' do
+        before do
+          expect(described_class).to receive(:omnibus_protected_paths_present?).and_return(true)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when the omnibus protected paths throttle has not been recently used' do
+        before do
+          expect(described_class).to receive(:omnibus_protected_paths_present?).and_return(false)
+        end
+
+        it { is_expected.to be_falsey }
+      end
+    end
+
+    context 'when rack_attack.admin_area_protected_paths_enabled config is false' do
+      before do
+        stub_config(rack_attack: {
+          admin_area_protected_paths_enabled: false
+        })
+      end
+
+      context 'when the omnibus protected paths throttle has been recently used (it has data)' do
+        before do
+          expect(described_class).to receive(:omnibus_protected_paths_present?).and_return(true)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when the omnibus protected paths throttle has not been recently used' do
+        before do
+          expect(described_class).to receive(:omnibus_protected_paths_present?).and_return(false)
+        end
+
+        it { is_expected.to be_falsey }
+      end
+    end
+
+    context 'when rack_attack.admin_area_protected_paths_enabled config is true' do
+      before do
+        stub_config(rack_attack: {
+          admin_area_protected_paths_enabled: true
+        })
+
+        expect(described_class).not_to receive(:omnibus_protected_paths_present?)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+end

spec/requests/rack_attack_global_spec.rb

@@ -249,10 +249,10 @@ describe 'Rack Attack global throttles' do
           expect_rejection { post protected_path_that_does_not_require_authentication, params: post_params }
         end
 
-        context 'when Omnibus throttle is present' do
+        context 'when Omnibus throttle should be used' do
           before do
             allow(Gitlab::Throttle)
-              .to receive(:omnibus_protected_paths_present?).and_return(true)
+              .to receive(:should_use_omnibus_protected_paths?).and_return(true)
           end
 
           it 'allows requests over the rate limit' do
@@ -298,7 +298,7 @@ describe 'Rack Attack global throttles' do
         it_behaves_like 'rate-limited token-authenticated requests'
       end
 
-      context 'when Omnibus throttle is present' do
+      context 'when Omnibus throttle should be used' do
         let(:request_args) { [api(api_partial_url, personal_access_token: token)] }
         let(:other_user_request_args) { [api(api_partial_url, personal_access_token: other_user_token)] }
 
@@ -309,7 +309,7 @@ describe 'Rack Attack global throttles' do
           stub_application_setting(settings_to_set)
 
           allow(Gitlab::Throttle)
-            .to receive(:omnibus_protected_paths_present?).and_return(true)
+            .to receive(:should_use_omnibus_protected_paths?).and_return(true)
         end
 
         it 'allows requests over the rate limit' do
@@ -339,7 +339,7 @@ describe 'Rack Attack global throttles' do
 
       it_behaves_like 'rate-limited web authenticated requests'
 
-      context 'when Omnibus throttle is present' do
+      context 'when Omnibus throttle should be used' do
         before do
           settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period
           settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds
@@ -347,7 +347,7 @@ describe 'Rack Attack global throttles' do
           stub_application_setting(settings_to_set)
 
           allow(Gitlab::Throttle)
-            .to receive(:omnibus_protected_paths_present?).and_return(true)
+            .to receive(:should_use_omnibus_protected_paths?).and_return(true)
 
           login_as(user)
         end