Merge branch 'dblessing-google-oauth2-timeout' into 'master'

Set default timeout for Google OAuth to prevent 503s

Closes #198056

See merge request gitlab-org/gitlab!30653

app/controllers/google_api/authorizations_controller.rb

@@ -15,6 +15,9 @@ module GoogleApi
       session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at] =
         expires_at.to_s
 
+    rescue ::Faraday::TimeoutError, ::Faraday::ConnectionFailed
+      flash[:alert] = _('Timeout connecting to the Google API. Please try again.')
+    ensure
       redirect_to redirect_uri_from_session
     end
 

changelogs/unreleased/dblessing-google-oauth2-timeout.yml

+---
+title: Set timeout for Google OAuth to prevent 503 error
+merge_request: 30653
+author:
+type: fixed

config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb

+# frozen_string_literal: true
+
+module OmniAuth
+  module Strategies
+    class OAuth2
+      alias_method :original_callback_phase, :callback_phase
+
+      # Monkey patch until PR is merged and released upstream
+      # https://github.com/omniauth/omniauth-oauth2/pull/129
+      def callback_phase
+        original_callback_phase
+      rescue ::Faraday::TimeoutError, ::Faraday::ConnectionFailed => e
+        fail!(:timeout, e)
+      end
+    end
+  end
+end

lib/gitlab/auth/o_auth/provider.rb

@@ -66,7 +66,10 @@ module Gitlab
               nil
             end
           else
-            Gitlab.config.omniauth.providers.find { |provider| provider.name == name }
+            provider = Gitlab.config.omniauth.providers.find { |provider| provider.name == name }
+            merge_provider_args_with_defaults!(provider)
+
+            provider
           end
         end
 
@@ -81,6 +84,15 @@ module Gitlab
           config = config_for(name)
           config && config['icon']
         end
+
+        def self.merge_provider_args_with_defaults!(provider)
+          return unless provider
+
+          provider['args'] ||= {}
+
+          defaults = Gitlab::OmniauthInitializer.default_arguments_for(provider['name'])
+          provider['args'].deep_merge!(defaults.deep_stringify_keys)
+        end
       end
     end
   end

lib/gitlab/omniauth_initializer.rb

@@ -2,6 +2,8 @@
 
 module Gitlab
   class OmniauthInitializer
+    OAUTH2_TIMEOUT_SECONDS = 10
+
     def initialize(devise_config)
       @devise_config = devise_config
     end
@@ -15,6 +17,47 @@ module Gitlab
       end
     end
 
+    class << self
+      def default_arguments_for(provider_name)
+        case provider_name
+        when 'cas3'
+          { on_single_sign_out: cas3_signout_handler }
+        when 'authentiq'
+          { remote_sign_out_handler: authentiq_signout_handler }
+        when 'shibboleth'
+          { fail_with_empty_uid: true }
+        when 'google_oauth2'
+          { client_options: { connection_opts: { request: { timeout: OAUTH2_TIMEOUT_SECONDS } } } }
+        else
+          {}
+        end
+      end
+
+      private
+
+      def cas3_signout_handler
+        lambda do |request|
+          ticket = request.params[:session_index]
+          raise "Service Ticket not found." unless Gitlab::Auth::OAuth::Session.valid?(:cas3, ticket)
+
+          Gitlab::Auth::OAuth::Session.destroy(:cas3, ticket)
+          true
+        end
+      end
+
+      def authentiq_signout_handler
+        lambda do |request|
+          authentiq_session = request.params['sid']
+          if Gitlab::Auth::OAuth::Session.valid?(:authentiq, authentiq_session)
+            Gitlab::Auth::OAuth::Session.destroy(:authentiq, authentiq_session)
+            true
+          else
+            false
+          end
+        end
+      end
+    end
+
     private
 
     def add_provider_to_devise(*args)
@@ -33,7 +76,8 @@ module Gitlab
         # An Array from the configuration will be expanded.
         provider_arguments.concat provider['args']
       when Hash
-        hash_arguments = provider['args'].merge(provider_defaults(provider))
+        defaults = provider_defaults(provider)
+        hash_arguments = provider['args'].deep_symbolize_keys.deep_merge(defaults)
 
         # A Hash from the configuration will be passed as is.
         provider_arguments << normalize_hash_arguments(hash_arguments)
@@ -43,7 +87,7 @@ module Gitlab
     end
 
     def normalize_hash_arguments(args)
-      args.symbolize_keys!
+      args.deep_symbolize_keys!
 
       # Rails 5.1 deprecated the use of string names in the middleware
       # (https://github.com/rails/rails/commit/83b767ce), so we need to
@@ -66,38 +110,7 @@ module Gitlab
     end
 
     def provider_defaults(provider)
-      case provider['name']
-      when 'cas3'
-        { on_single_sign_out: cas3_signout_handler }
-      when 'authentiq'
-        { remote_sign_out_handler: authentiq_signout_handler }
-      when 'shibboleth'
-        { fail_with_empty_uid: true }
-      else
-        {}
-      end
-    end
-
-    def cas3_signout_handler
-      lambda do |request|
-        ticket = request.params[:session_index]
-        raise "Service Ticket not found." unless Gitlab::Auth::OAuth::Session.valid?(:cas3, ticket)
-
-        Gitlab::Auth::OAuth::Session.destroy(:cas3, ticket)
-        true
-      end
-    end
-
-    def authentiq_signout_handler
-      lambda do |request|
-        authentiq_session = request.params['sid']
-        if Gitlab::Auth::OAuth::Session.valid?(:authentiq, authentiq_session)
-          Gitlab::Auth::OAuth::Session.destroy(:authentiq, authentiq_session)
-          true
-        else
-          false
-        end
-      end
+      self.class.default_arguments_for(provider['name'])
     end
 
     def omniauth_customized_providers

lib/google_api/auth.rb

@@ -37,6 +37,10 @@ module GoogleApi
       Gitlab::Auth::OAuth::Provider.config_for('google_oauth2')
     end
 
+    def client_options
+      config.args.client_options.deep_symbolize_keys
+    end
+
     def client
       return @client if defined?(@client)
 
@@ -49,7 +53,8 @@ module GoogleApi
         config.app_secret,
         site: 'https://accounts.google.com',
         token_url: '/o/oauth2/token',
-        authorize_url: '/o/oauth2/auth'
+        authorize_url: '/o/oauth2/auth',
+        **client_options
       )
     end
   end

locale/gitlab.pot

@@ -22439,6 +22439,9 @@ msgstr ""
 msgid "Timeout"
 msgstr ""
 
+msgid "Timeout connecting to the Google API. Please try again."
+msgstr ""
+
 msgid "Time|hr"
 msgid_plural "Time|hrs"
 msgstr[0] ""

spec/controllers/google_api/authorizations_controller_spec.rb

@@ -12,10 +12,6 @@ describe GoogleApi::AuthorizationsController do
 
     before do
       sign_in(user)
-
-      allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance|
-        allow(instance).to receive(:get_token).and_return([token, expires_at])
-      end
     end
 
     shared_examples_for 'access denied' do
@@ -38,6 +34,12 @@ describe GoogleApi::AuthorizationsController do
       context 'session key matches state param' do
         let(:state) { session_key }
 
+        before do
+          allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance|
+            allow(instance).to receive(:get_token).and_return([token, expires_at])
+          end
+        end
+
         it 'sets token and expires_at in session' do
           subject
 
@@ -63,6 +65,22 @@ describe GoogleApi::AuthorizationsController do
 
         it_behaves_like 'access denied'
       end
+
+      context 'when a Faraday exception occurs' do
+        let(:state) { session_key }
+
+        [::Faraday::TimeoutError, ::Faraday::ConnectionFailed].each do |error|
+          it "sets a flash alert on #{error}" do
+            allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance|
+              allow(instance).to receive(:get_token).and_raise(error.new(nil))
+            end
+
+            subject
+
+            expect(flash[:alert]).to eq('Timeout connecting to the Google API. Please try again.')
+          end
+        end
+      end
     end
 
     context 'state param is present, but session key is blank' do

spec/lib/gitlab/auth/o_auth/provider_spec.rb

@@ -63,7 +63,7 @@ describe Gitlab::Auth::OAuth::Provider do
     context 'for an OmniAuth provider' do
       before do
         provider = OpenStruct.new(
-          name: 'google',
+          name: 'google_oauth2',
           app_id: 'asd123',
           app_secret: 'asd123'
         )
@@ -71,8 +71,16 @@ describe Gitlab::Auth::OAuth::Provider do
       end
 
       context 'when the provider exists' do
+        subject { described_class.config_for('google_oauth2') }
+
         it 'returns the config' do
-          expect(described_class.config_for('google')).to be_a(OpenStruct)
+          expect(subject).to be_a(OpenStruct)
+        end
+
+        it 'merges defaults with the given configuration' do
+          defaults = Gitlab::OmniauthInitializer.default_arguments_for('google_oauth2').deep_stringify_keys
+
+          expect(subject['args']).to include(defaults)
         end
       end
 

spec/lib/gitlab/omniauth_initializer_spec.rb

@@ -86,6 +86,22 @@ describe Gitlab::OmniauthInitializer do
       subject.execute([cas3_config])
     end
 
+    it 'configures defaults for google_oauth2' do
+      google_config = {
+        'name' => 'google_oauth2',
+        "args" => { "access_type" => "offline", "approval_prompt" => '' }
+      }
+
+      expect(devise_config).to receive(:omniauth).with(
+        :google_oauth2,
+        access_type: "offline",
+        approval_prompt: "",
+        client_options: { connection_opts: { request: { timeout: Gitlab::OmniauthInitializer::OAUTH2_TIMEOUT_SECONDS } } }
+      )
+
+      subject.execute([google_config])
+    end
+
     it 'converts client_auth_method to a Symbol for openid_connect' do
       openid_connect_config = {
         'name' => 'openid_connect',

spec/lib/google_api/auth_spec.rb

@@ -40,5 +40,19 @@ describe GoogleApi::Auth do
       expect(token).to eq('token')
       expect(expires_at).to eq('expires_at')
     end
+
+    it 'expects the client to receive default options' do
+      config = Gitlab::Auth::OAuth::Provider.config_for('google_oauth2')
+
+      expect(OAuth2::Client).to receive(:new).with(
+        config.app_id,
+        config.app_secret,
+        hash_including(
+          **config.args.client_options.deep_symbolize_keys
+        )
+      ).and_call_original
+
+      client.get_token('xxx')
+    end
   end
 end