Merge branch '353331-apply-omniauth-defaults-when-args-are-absent' into 'master'

Apply omniauth defaults when no arguments are given

See merge request gitlab-org/gitlab!81752

app/controllers/oauth/authorizations_controller.rb

@@ -12,10 +12,7 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController
   # Overridden from Doorkeeper::AuthorizationsController to
   # include the call to session.delete
   def new
-    logger.info("#{self.class.name}#new: pre_auth_params['scope'] = #{pre_auth_params['scope'].inspect}")
-
     if pre_auth.authorizable?
-      logger.info("#{self.class.name}#new: pre_auth.scopes = #{pre_auth.scopes.to_a.inspect}")
       if skip_authorization? || matching_token?
         auth = authorization.authorize
         parsed_redirect_uri = URI.parse(auth.redirect_uri)
@@ -46,15 +43,9 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController
     auth_type = params.delete('gl_auth_type')
     return unless auth_type == 'login'
 
-    logger.info("#{self.class.name}: BEFORE application has read_user: #{application_has_read_user_scope?}")
-    logger.info("#{self.class.name}: BEFORE scope = #{params['scope'].inspect}")
-
     ensure_read_user_scope!
 
     params['scope'] = Gitlab::Auth::READ_USER_SCOPE.to_s if application_has_read_user_scope?
-
-    logger.info("#{self.class.name}: AFTER application has read_user: #{application_has_read_user_scope?}")
-    logger.info("#{self.class.name}: AFTER scope = #{params['scope'].inspect}")
   end
 
   # Configure the application to support read_user scope, if it already

lib/gitlab/omniauth_initializer.rb

@@ -3,6 +3,7 @@
 module Gitlab
   class OmniauthInitializer
     OAUTH2_TIMEOUT_SECONDS = 10
+    ConfigurationError = Class.new(StandardError)
 
     def initialize(devise_config)
       @devise_config = devise_config
@@ -75,16 +76,29 @@ module Gitlab
         provider_arguments << provider[argument] if provider[argument]
       end
 
-      case provider['args']
+      arguments = provider.fetch('args', {})
+      defaults = provider_defaults(provider)
+
+      case arguments
       when Array
-        # An Array from the configuration will be expanded.
-        provider_arguments.concat provider['args']
+        # An Array from the configuration will be expanded
+        provider_arguments.concat arguments
+        provider_arguments << defaults unless defaults.empty?
       when Hash
-        defaults = provider_defaults(provider)
-        hash_arguments = provider['args'].deep_symbolize_keys.deep_merge(defaults)
+        hash_arguments = arguments.deep_symbolize_keys.deep_merge(defaults)
+        normalized = normalize_hash_arguments(hash_arguments)
 
         # A Hash from the configuration will be passed as is.
-        provider_arguments << normalize_hash_arguments(hash_arguments)
+        provider_arguments << normalized unless normalized.empty?
+      else
+        # this will prevent the application from starting in development mode.
+        # we still set defaults, and let the application start in prod.
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(
+          ConfigurationError.new("Arguments were provided for #{provider['name']}, but not as an array or a hash"),
+          provider_name: provider['name'],
+          arguments_type: arguments.class.name
+        )
+        provider_arguments << defaults unless defaults.empty?
       end
 
       provider_arguments

spec/lib/gitlab/omniauth_initializer_spec.rb

@@ -5,7 +5,161 @@ require 'spec_helper'
 RSpec.describe Gitlab::OmniauthInitializer do
   let(:devise_config) { class_double(Devise) }
 
-  subject { described_class.new(devise_config) }
+  subject(:initializer) { described_class.new(devise_config) }
+
+  describe '.arguments_for' do
+    let(:devise_config) { nil }
+
+    let(:arguments) { initializer.send(:arguments_for, provider) }
+
+    context 'when there are no args at all' do
+      let(:provider) { { 'name' => 'unknown' } }
+
+      it 'returns an empty array' do
+        expect(arguments).to eq []
+      end
+    end
+
+    context 'when there is an app_id and an app_secret' do
+      let(:provider) { { 'name' => 'unknown', 'app_id' => 1, 'app_secret' => 2 } }
+
+      it 'includes both of them, in positional order' do
+        expect(arguments).to eq [1, 2]
+      end
+    end
+
+    context 'when there is an app_id and an app_secret, and an array of args' do
+      let(:provider) do
+        {
+          'name' =>  'unknown',
+          'app_id' => 1,
+          'app_secret' => 2,
+          'args' => %w[one two three]
+        }
+      end
+
+      it 'concatenates the args on the end' do
+        expect(arguments).to eq [1, 2, 'one', 'two', 'three']
+      end
+    end
+
+    context 'when there is an app_id and an app_secret, and an array of args, and default values' do
+      let(:provider) do
+        {
+          'name' =>  'unknown',
+          'app_id' => 1,
+          'app_secret' => 2,
+          'args' => %w[one two three]
+        }
+      end
+
+      before do
+        expect(described_class)
+          .to receive(:default_arguments_for).with('unknown')
+          .and_return({ default_arg: :some_value })
+      end
+
+      it 'concatenates the args on the end' do
+        expect(arguments)
+          .to eq [1, 2, 'one', 'two', 'three', { default_arg: :some_value }]
+      end
+    end
+
+    context 'when there is an app_id and an app_secret, and a hash of args' do
+      let(:provider) do
+        {
+          'name' =>  'unknown',
+          'app_id' => 1,
+          'app_secret' => 2,
+          'args' => { 'foo' => 100, 'bar' => 200, 'nested' => { 'value' => 300 } }
+        }
+      end
+
+      it 'concatenates the args on the end' do
+        expect(arguments)
+          .to eq [1, 2, { foo: 100, bar: 200, nested: { value: 300 } }]
+      end
+    end
+
+    context 'when there is an app_id and an app_secret, and a hash of args, and default arguments' do
+      let(:provider) do
+        {
+          'name' =>  'unknown',
+          'app_id' => 1,
+          'app_secret' => 2,
+          'args' => { 'foo' => 100, 'bar' => 200, 'nested' => { 'value' => 300 } }
+        }
+      end
+
+      before do
+        expect(described_class)
+          .to receive(:default_arguments_for).with('unknown')
+          .and_return({ default_arg: :some_value })
+      end
+
+      it 'concatenates the args on the end' do
+        expect(arguments)
+          .to eq [1, 2, { default_arg: :some_value, foo: 100, bar: 200, nested: { value: 300 } }]
+      end
+    end
+
+    context 'when there is an app_id and an app_secret, no args, and default values' do
+      let(:provider) do
+        {
+          'name' =>  'unknown',
+          'app_id' => 1,
+          'app_secret' => 2
+        }
+      end
+
+      before do
+        expect(described_class)
+          .to receive(:default_arguments_for).with('unknown')
+          .and_return({ default_arg: :some_value })
+      end
+
+      it 'concatenates the args on the end' do
+        expect(arguments)
+          .to eq [1, 2, { default_arg: :some_value }]
+      end
+    end
+
+    context 'when there are args, of an unsupported type' do
+      let(:provider) do
+        {
+          'name' =>  'unknown',
+          'args' => 1
+        }
+      end
+
+      context 'when there are default arguments' do
+        before do
+          expect(described_class)
+            .to receive(:default_arguments_for).with('unknown')
+            .and_return({ default_arg: :some_value })
+        end
+
+        it 'tracks a configuration error' do
+          expect(::Gitlab::ErrorTracking)
+            .to receive(:track_and_raise_for_dev_exception)
+            .with(described_class::ConfigurationError, provider_name: 'unknown', arguments_type: 'Integer')
+
+          expect(arguments)
+            .to eq [{ default_arg: :some_value }]
+        end
+      end
+
+      context 'when there are no default arguments' do
+        it 'tracks a configuration error' do
+          expect(::Gitlab::ErrorTracking)
+            .to receive(:track_and_raise_for_dev_exception)
+            .with(described_class::ConfigurationError, provider_name: 'unknown', arguments_type: 'Integer')
+
+          expect(arguments).to be_empty
+        end
+      end
+    end
+  end
 
   describe '#execute' do
     it 'configures providers from array' do
@@ -105,9 +259,48 @@ RSpec.describe Gitlab::OmniauthInitializer do
     it 'configures defaults for gitlab' do
       conf = {
         'name' => 'gitlab',
-        "args" => {}
+        "args" => { 'client_options' => { 'site' => generate(:url) } }
       }
 
+      expect(devise_config).to receive(:omniauth).with(
+        :gitlab,
+        client_options: { site: conf.dig('args', 'client_options', 'site') },
+        authorize_params: { gl_auth_type: 'login' }
+      )
+
+      subject.execute([conf])
+    end
+
+    it 'configures defaults for gitlab, when arguments are not provided' do
+      conf = { 'name' => 'gitlab' }
+
+      expect(devise_config).to receive(:omniauth).with(
+        :gitlab,
+        authorize_params: { gl_auth_type: 'login' }
+      )
+
+      subject.execute([conf])
+    end
+
+    it 'configures defaults for gitlab, when array arguments are provided' do
+      conf = { 'name' => 'gitlab', 'args' => ['a'] }
+
+      expect(devise_config).to receive(:omniauth).with(
+        :gitlab,
+        'a',
+        authorize_params: { gl_auth_type: 'login' }
+      )
+
+      subject.execute([conf])
+    end
+
+    it 'tracks a configuration error if the arguments are neither a hash nor an array' do
+      conf = { 'name' => 'gitlab', 'args' => 17 }
+
+      expect(::Gitlab::ErrorTracking)
+        .to receive(:track_and_raise_for_dev_exception)
+        .with(described_class::ConfigurationError, provider_name: 'gitlab', arguments_type: 'Integer')
+
       expect(devise_config).to receive(:omniauth).with(
         :gitlab,
         authorize_params: { gl_auth_type: 'login' }