Merge branch 'hly-skip-redis-calls-when-ip-is-trusted' into 'master'

Skip redis call when IP is whitelisted

See merge request gitlab-org/gitlab!19521

config/initializers/rack_attack_git_basic_auth.rb

 # Tell the Rack::Attack Rack middleware to maintain an IP blacklist.
 # We update the blacklist in Gitlab::Auth::IpRateLimiter.
 Rack::Attack.blocklist('Git HTTP Basic Auth') do |req|
-  next false unless Gitlab.config.rack_attack.git_basic_auth.enabled
+  rate_limiter = Gitlab::Auth::IpRateLimiter.new(req.ip)
+
+  next false if !rate_limiter.enabled? || rate_limiter.trusted_ip?
 
   Rack::Attack::Allow2Ban.filter(req.ip, Gitlab.config.rack_attack.git_basic_auth) do
     # This block only gets run if the IP was not already banned.

lib/gitlab/auth/ip_rate_limiter.rb

@@ -21,11 +21,12 @@ module Gitlab
       end
 
       def register_fail!
+        return false if trusted_ip?
+
         # Allow2Ban.filter will return false if this IP has not failed too often yet
         @banned = Rack::Attack::Allow2Ban.filter(ip, config) do
-          # If we return false here, the failure for this IP is ignored by Allow2Ban
-          # If we return true here, the count for the IP is incremented.
-          ip_can_be_banned?
+          # We return true to increment the count for this IP
+          true
         end
       end
 
@@ -33,20 +34,16 @@ module Gitlab
         @banned
       end
 
+      def trusted_ip?
+        trusted_ips.any? { |netmask| netmask.include?(ip) }
+      end
+
       private
 
       def config
         Gitlab.config.rack_attack.git_basic_auth
       end
 
-      def ip_can_be_banned?
-        !trusted_ip?
-      end
-
-      def trusted_ip?
-        trusted_ips.any? { |netmask| netmask.include?(ip) }
-      end
-
       def trusted_ips
         strong_memoize(:trusted_ips) do
           config.ip_whitelist.map do |proxy|

spec/requests/git_http_spec.rb

@@ -452,7 +452,7 @@ describe 'Git HTTP requests' do
           context "when authentication fails" do
             context "when the user is IP banned" do
               before do
-                stub_rack_attack_setting(enabled: true)
+                stub_rack_attack_setting(enabled: true, ip_whitelist: [])
               end
 
               it "responds with status 403" do

spec/requests/rack_attack_global_spec.rb

@@ -83,7 +83,7 @@ describe 'Rack Attack global throttles' do
           expect(response).to have_http_status 200
         end
 
-        expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
+        expect_any_instance_of(Rack::Attack::Request).to receive(:ip).at_least(:once).and_return('1.2.3.4')
 
         # would be over limit for the same IP
         get url_that_does_not_require_authentication

spec/support/shared_examples/requests/rack_attack_shared_examples.rb

@@ -74,7 +74,7 @@ shared_examples_for 'rate-limited token-authenticated requests' do
         expect(response).to have_http_status 200
       end
 
-      expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
+      expect_any_instance_of(Rack::Attack::Request).to receive(:ip).at_least(:once).and_return('1.2.3.4')
 
       expect_rejection { get(*get_args) }
     end
@@ -194,7 +194,7 @@ shared_examples_for 'rate-limited web authenticated requests' do
         expect(response).to have_http_status 200
       end
 
-      expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
+      expect_any_instance_of(Rack::Attack::Request).to receive(:ip).at_least(:once).and_return('1.2.3.4')
 
       expect_rejection { get url_that_requires_authentication }
     end