External auth adheres to local requests setting

The External Authorization client used Excon to make HTTP calls.
This changes the client to use the safe Gitlab::HTTP client which
will block local requests, unless they're explicitly enabled.
The local requests for system services is enabled by default so
existing default behavior of external authorizations is preserved.

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