diff options
| -rw-r--r-- | config/feature_flags/development/invert_omniauth_args_merging.yml | 8 | ||||
| -rw-r--r-- | lib/gitlab/auth/saml/config.rb | 15 | ||||
| -rw-r--r-- | lib/gitlab/omniauth_initializer.rb | 11 | ||||
| -rw-r--r-- | spec/lib/gitlab/auth/saml/config_spec.rb | 35 | ||||
| -rw-r--r-- | spec/lib/gitlab/omniauth_initializer_spec.rb | 117 |
5 files changed, 2 insertions, 184 deletions
diff --git a/config/feature_flags/development/invert_omniauth_args_merging.yml b/config/feature_flags/development/invert_omniauth_args_merging.yml deleted file mode 100644 index 1a5d0a08541..00000000000 --- a/config/feature_flags/development/invert_omniauth_args_merging.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: invert_omniauth_args_merging -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/135770 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/430348 -milestone: '16.6' -type: development -group: group::authentication -default_enabled: false diff --git a/lib/gitlab/auth/saml/config.rb b/lib/gitlab/auth/saml/config.rb index e6c9f04eff5..7524d8b9f85 100644 --- a/lib/gitlab/auth/saml/config.rb +++ b/lib/gitlab/auth/saml/config.rb @@ -8,21 +8,6 @@ module Gitlab def enabled? ::AuthHelper.saml_providers.any? end - - def default_attribute_statements - defaults = OmniAuth::Strategies::SAML.default_options[:attribute_statements].to_hash.deep_symbolize_keys - defaults[:nickname] = %w[username nickname] - defaults[:name] << 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name' - defaults[:name] << 'http://schemas.microsoft.com/ws/2008/06/identity/claims/name' - defaults[:email] << 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress' - defaults[:email] << 'http://schemas.microsoft.com/ws/2008/06/identity/claims/emailaddress' - defaults[:first_name] << 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname' - defaults[:first_name] << 'http://schemas.microsoft.com/ws/2008/06/identity/claims/givenname' - defaults[:last_name] << 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname' - defaults[:last_name] << 'http://schemas.microsoft.com/ws/2008/06/identity/claims/surname' - - defaults - end end DEFAULT_PROVIDER_NAME = 'saml' diff --git a/lib/gitlab/omniauth_initializer.rb b/lib/gitlab/omniauth_initializer.rb index 0bcd5b1196a..81ad7a7f9e1 100644 --- a/lib/gitlab/omniauth_initializer.rb +++ b/lib/gitlab/omniauth_initializer.rb @@ -29,8 +29,6 @@ module Gitlab { authorize_params: { gl_auth_type: 'login' } } - when ->(provider_name) { AuthHelper.saml_providers.include?(provider_name.to_sym) } - { attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements } else {} end @@ -63,7 +61,7 @@ module Gitlab provider_arguments.concat arguments provider_arguments << defaults unless defaults.empty? when Hash, GitlabSettings::Options - hash_arguments = merge_hash_defaults_and_args(defaults, arguments) + 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. @@ -82,13 +80,6 @@ module Gitlab provider_arguments end - def merge_hash_defaults_and_args(defaults, arguments) - return arguments.to_hash if defaults.empty? - return defaults.deep_merge(arguments.deep_symbolize_keys) if Feature.enabled?(:invert_omniauth_args_merging) - - arguments.to_hash.deep_symbolize_keys.deep_merge(defaults) - end - def normalize_hash_arguments(args) args.deep_symbolize_keys! diff --git a/spec/lib/gitlab/auth/saml/config_spec.rb b/spec/lib/gitlab/auth/saml/config_spec.rb index c19171bb6f8..2ecc26f9b96 100644 --- a/spec/lib/gitlab/auth/saml/config_spec.rb +++ b/spec/lib/gitlab/auth/saml/config_spec.rb @@ -19,41 +19,6 @@ RSpec.describe Gitlab::Auth::Saml::Config do end end - describe '.default_attribute_statements' do - it 'includes upstream defaults, nickname and Microsoft values' do - expect(described_class.default_attribute_statements).to match_array( - { - nickname: %w[username nickname], - name: [ - 'name', - 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name', - 'http://schemas.microsoft.com/ws/2008/06/identity/claims/name' - ], - email: [ - 'email', - 'mail', - 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress', - 'http://schemas.microsoft.com/ws/2008/06/identity/claims/emailaddress' - ], - first_name: [ - 'first_name', - 'firstname', - 'firstName', - 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname', - 'http://schemas.microsoft.com/ws/2008/06/identity/claims/givenname' - ], - last_name: [ - 'last_name', - 'lastname', - 'lastName', - 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname', - 'http://schemas.microsoft.com/ws/2008/06/identity/claims/surname' - ] - } - ) - end - end - describe '#external_groups' do let(:config_1) { described_class.new('saml1') } diff --git a/spec/lib/gitlab/omniauth_initializer_spec.rb b/spec/lib/gitlab/omniauth_initializer_spec.rb index e12c0f4e78b..9b46b8eccc8 100644 --- a/spec/lib/gitlab/omniauth_initializer_spec.rb +++ b/spec/lib/gitlab/omniauth_initializer_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::OmniauthInitializer, feature_category: :system_access do - include LoginHelpers - +RSpec.describe Gitlab::OmniauthInitializer do let(:devise_config) { class_double(Devise) } subject(:initializer) { described_class.new(devise_config) } @@ -226,119 +224,6 @@ RSpec.describe Gitlab::OmniauthInitializer, feature_category: :system_access do subject.execute([shibboleth_config]) end - context 'when SAML providers are configured' do - it 'configures default args for a single SAML provider' do - stub_omniauth_config(providers: [{ name: 'saml', args: { idp_sso_service_url: 'https://saml.example.com' } }]) - - expect(devise_config).to receive(:omniauth).with( - :saml, - { - idp_sso_service_url: 'https://saml.example.com', - attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements - } - ) - - initializer.execute(Gitlab.config.omniauth.providers) - end - - context 'when configuration provides matching keys' do - before do - stub_omniauth_config( - providers: [ - { - name: 'saml', - args: { idp_sso_service_url: 'https://saml.example.com', attribute_statements: { email: ['custom_attr'] } } - } - ] - ) - end - - it 'merges arguments with user configuration preference' do - expect(devise_config).to receive(:omniauth).with( - :saml, - { - idp_sso_service_url: 'https://saml.example.com', - attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements - .merge({ email: ['custom_attr'] }) - } - ) - - initializer.execute(Gitlab.config.omniauth.providers) - end - - it 'merges arguments with defaults preference when invert_omniauth_args_merging is not enabled' do - stub_feature_flags(invert_omniauth_args_merging: false) - - expect(devise_config).to receive(:omniauth).with( - :saml, - { - idp_sso_service_url: 'https://saml.example.com', - attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements - } - ) - - initializer.execute(Gitlab.config.omniauth.providers) - end - end - - it 'configures defaults args for multiple SAML providers' do - stub_omniauth_config( - providers: [ - { name: 'saml', args: { idp_sso_service_url: 'https://saml.example.com' } }, - { - name: 'saml2', - args: { strategy_class: 'OmniAuth::Strategies::SAML', idp_sso_service_url: 'https://saml2.example.com' } - } - ] - ) - - expect(devise_config).to receive(:omniauth).with( - :saml, - { - idp_sso_service_url: 'https://saml.example.com', - attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements - } - ) - expect(devise_config).to receive(:omniauth).with( - :saml2, - { - idp_sso_service_url: 'https://saml2.example.com', - strategy_class: OmniAuth::Strategies::SAML, - attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements - } - ) - - initializer.execute(Gitlab.config.omniauth.providers) - end - - it 'merges arguments with user configuration preference for custom SAML provider' do - stub_omniauth_config( - providers: [ - { - name: 'custom_saml', - args: { - strategy_class: 'OmniAuth::Strategies::SAML', - idp_sso_service_url: 'https://saml2.example.com', - attribute_statements: { email: ['custom_attr'] } - } - } - ] - ) - - expect(devise_config).to receive(:omniauth).with( - :custom_saml, - { - idp_sso_service_url: 'https://saml2.example.com', - strategy_class: OmniAuth::Strategies::SAML, - attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements - .merge({ email: ['custom_attr'] }) - } - ) - - initializer.execute(Gitlab.config.omniauth.providers) - end - end - it 'configures defaults for google_oauth2' do google_config = { 'name' => 'google_oauth2', |