Merge branch 'dblessing-external-auth-gitlab-http' into 'master'

External Authorization Client adheres to local request setting

See merge request gitlab-org/gitlab!37622

changelogs/unreleased/dblessing-external-auth-gitlab-http.yml

+---
+title: External auth adheres to local request setting
+merge_request: 37622
+author:
+type: changed

lib/gitlab/external_authorization/client.rb

@@ -17,23 +17,28 @@ module Gitlab
       end
 
       def request_access
-        response = Excon.post(
+        response = Gitlab::HTTP.post(
           service_url,
           post_params
         )
         ::Gitlab::ExternalAuthorization::Response.new(response)
-      rescue Excon::Error => e
+      rescue *Gitlab::HTTP::HTTP_ERRORS => e
         raise ::Gitlab::ExternalAuthorization::RequestFailed.new(e)
       end
 
       private
 
+      def allow_local_requests?
+        Gitlab::CurrentSettings.allow_local_requests_from_system_hooks?
+      end
+
       def post_params
         params = { headers: REQUEST_HEADERS,
                    body: body.to_json,
                    connect_timeout: timeout,
                    read_timeout: timeout,
-                   write_timeout: timeout }
+                   write_timeout: timeout,
+                   allow_local_requests: allow_local_requests? }
 
         if has_tls?
           params[:client_cert_data] = client_cert

lib/gitlab/external_authorization/response.rb

@@ -5,16 +5,16 @@ module Gitlab
     class Response
       include ::Gitlab::Utils::StrongMemoize
 
-      def initialize(excon_response)
-        @excon_response = excon_response
+      def initialize(response)
+        @response = response
       end
 
       def valid?
-        @excon_response && [200, 401, 403].include?(@excon_response.status)
+        @response && [200, 401, 403].include?(@response.code)
       end
 
       def successful?
-        valid? && @excon_response.status == 200
+        valid? && @response.code == 200
       end
 
       def reason
@@ -28,7 +28,7 @@ module Gitlab
       end
 
       def parse_response!
-        Gitlab::Json.parse(@excon_response.body)
+        Gitlab::Json.parse(@response.body)
       rescue JSON::JSONError
         # The JSON response is optional, so don't fail when it's missing
         nil

spec/lib/gitlab/external_authorization/client_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Gitlab::ExternalAuthorization::Client do
 
   describe '#request_access' do
     it 'performs requests to the configured endpoint' do
-      expect(Excon).to receive(:post).with(dummy_url, any_args)
+      expect(Gitlab::HTTP).to receive(:post).with(dummy_url, any_args)
 
       client.request_access
     end
@@ -25,7 +25,7 @@ RSpec.describe Gitlab::ExternalAuthorization::Client do
         project_classification_label: 'dummy_label',
         identities: []
       }.to_json
-      expect(Excon).to receive(:post)
+      expect(Gitlab::HTTP).to receive(:post)
                          .with(dummy_url, hash_including(body: expected_body))
 
       client.request_access
@@ -36,7 +36,7 @@ RSpec.describe Gitlab::ExternalAuthorization::Client do
         external_authorization_service_timeout: 3
       )
 
-      expect(Excon).to receive(:post).with(dummy_url,
+      expect(Gitlab::HTTP).to receive(:post).with(dummy_url,
                                            hash_including(
                                              connect_timeout: 3,
                                              read_timeout: 3,
@@ -58,25 +58,33 @@ RSpec.describe Gitlab::ExternalAuthorization::Client do
         client_key_pass: 'open sesame'
       }
 
-      expect(Excon).to receive(:post).with(dummy_url, hash_including(expected_params))
+      expect(Gitlab::HTTP).to receive(:post).with(dummy_url, hash_including(expected_params))
 
       client.request_access
     end
 
     it 'returns an expected response' do
-      expect(Excon).to receive(:post)
+      expect(Gitlab::HTTP).to receive(:post)
 
       expect(client.request_access)
         .to be_kind_of(::Gitlab::ExternalAuthorization::Response)
     end
 
     it 'wraps exceptions if the request fails' do
-      expect(Excon).to receive(:post) { raise Excon::Error.new('the request broke') }
+      expect(Gitlab::HTTP).to receive(:post) { raise Gitlab::HTTP::BlockedUrlError.new('the request broke') }
 
       expect { client.request_access }
         .to raise_error(::Gitlab::ExternalAuthorization::RequestFailed)
     end
 
+    it 'passes local request setting to Gitlab::HTTP' do
+      stub_application_setting(allow_local_requests_from_system_hooks: false)
+
+      expect(Gitlab::HTTP).to receive(:post).with(dummy_url, hash_including(allow_local_requests: false))
+
+      client.request_access
+    end
+
     describe 'for ldap users' do
       let(:user) do
         create(:omniauth_user,
@@ -92,7 +100,7 @@ RSpec.describe Gitlab::ExternalAuthorization::Client do
           identities: [{ provider: 'ldapprovider', extern_uid: 'external id' }],
           user_ldap_dn: 'external id'
         }.to_json
-        expect(Excon).to receive(:post)
+        expect(Gitlab::HTTP).to receive(:post)
                            .with(dummy_url, hash_including(body: expected_body))
 
         client.request_access
@@ -115,7 +123,7 @@ RSpec.describe Gitlab::ExternalAuthorization::Client do
             { provider: 'facebook', extern_uid: 'facebook_external_id' }
           ]
         }.to_json
-        expect(Excon).to receive(:post)
+        expect(Gitlab::HTTP).to receive(:post)
                            .with(dummy_url, hash_including(body: expected_body))
 
         client.request_access

spec/lib/gitlab/external_authorization/response_spec.rb

@@ -3,21 +3,21 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::ExternalAuthorization::Response do
-  let(:excon_response) { double }
+  let(:http_response) { double }
 
-  subject(:response) { described_class.new(excon_response) }
+  subject(:response) { described_class.new(http_response) }
 
   describe '#valid?' do
     it 'is valid for 200, 401, and 403 responses' do
-      [200, 401, 403].each do |status|
-        allow(excon_response).to receive(:status).and_return(status)
+      [200, 401, 403].each do |code|
+        allow(http_response).to receive(:code).and_return(code)
 
         expect(response).to be_valid
       end
     end
 
     it "is invalid for other statuses" do
-      expect(excon_response).to receive(:status).and_return(500)
+      expect(http_response).to receive(:code).and_return(500)
 
       expect(response).not_to be_valid
     end
@@ -25,13 +25,13 @@ RSpec.describe Gitlab::ExternalAuthorization::Response do
 
   describe '#reason' do
     it 'returns a reason if it was included in the response body' do
-      expect(excon_response).to receive(:body).and_return({ reason: 'Not authorized' }.to_json)
+      expect(http_response).to receive(:body).and_return({ reason: 'Not authorized' }.to_json)
 
       expect(response.reason).to eq('Not authorized')
     end
 
     it 'returns nil when there was no body' do
-      expect(excon_response).to receive(:body).and_return('')
+      expect(http_response).to receive(:body).and_return('')
 
       expect(response.reason).to eq(nil)
     end
@@ -39,14 +39,14 @@ RSpec.describe Gitlab::ExternalAuthorization::Response do
 
   describe '#successful?' do
     it 'is `true` if the status is 200' do
-      allow(excon_response).to receive(:status).and_return(200)
+      allow(http_response).to receive(:code).and_return(200)
 
       expect(response).to be_successful
     end
 
     it 'is `false` if the status is 401 or 403' do
-      [401, 403].each do |status|
-        allow(excon_response).to receive(:status).and_return(status)
+      [401, 403].each do |code|
+        allow(http_response).to receive(:code).and_return(code)
 
         expect(response).not_to be_successful
       end