From 211d2963d13df49a3ca84c8780064c3bf1803a95 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Mon, 13 Nov 2023 08:51:59 -0600
Subject: [PATCH] Revert "Support Microsoft SAML attribute names by default"
This reverts commit 3c426a5654c08dd44bfc70f6bea6fcac24ce70e2, reversing
changes made to 337617595d9b80c98193e30b54f5720a30329723.
Changelog: removed
---
.../invert_omniauth_args_merging.yml | 8 --
ee/app/models/saml_provider.rb | 2 +-
ee/spec/models/saml_provider_spec.rb | 6 +-
lib/gitlab/auth/saml/config.rb | 15 ---
lib/gitlab/omniauth_initializer.rb | 11 +-
spec/lib/gitlab/auth/saml/config_spec.rb | 35 ------
spec/lib/gitlab/omniauth_initializer_spec.rb | 117 +-----------------
7 files changed, 6 insertions(+), 188 deletions(-)
delete mode 100644 config/feature_flags/development/invert_omniauth_args_merging.yml
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 1a5d0a085411f..0000000000000
--- 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/ee/app/models/saml_provider.rb b/ee/app/models/saml_provider.rb
index 8dedee8047802..d7a24ea96606c 100644
--- a/ee/app/models/saml_provider.rb
+++ b/ee/app/models/saml_provider.rb
@@ -27,7 +27,7 @@ def settings
defaults.to_h.merge(
idp_cert_fingerprint: certificate_fingerprint,
idp_sso_target_url: sso_url,
- attribute_statements: ::Gitlab::Auth::Saml::Config.default_attribute_statements
+ attribute_statements: { nickname: %w[username nickname] }
)
end
diff --git a/ee/spec/models/saml_provider_spec.rb b/ee/spec/models/saml_provider_spec.rb
index 44cd7b8944e05..8964459f2dcf7 100644
--- a/ee/spec/models/saml_provider_spec.rb
+++ b/ee/spec/models/saml_provider_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe SamlProvider, feature_category: :system_access do
+RSpec.describe SamlProvider do
let(:group) { create(:group) }
subject(:saml_provider) { create(:saml_provider, group: group) }
@@ -155,8 +155,8 @@
expect(settings[:idp_sso_target_url]).to eq saml_provider.sso_url
end
- it 'includes default attribute statements' do
- expect(settings[:attribute_statements]).to match_array(::Gitlab::Auth::Saml::Config.default_attribute_statements)
+ it 'includes nickname attribute statements' do
+ expect(settings[:attribute_statements][:nickname]).to match_array(%w[nickname username])
end
context 'when saml_message_max_byte_size present in gitlab settings ' do
diff --git a/lib/gitlab/auth/saml/config.rb b/lib/gitlab/auth/saml/config.rb
index e6c9f04eff5d6..7524d8b9f8587 100644
--- a/lib/gitlab/auth/saml/config.rb
+++ b/lib/gitlab/auth/saml/config.rb
@@ -8,21 +8,6 @@ class << self
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 0bcd5b1196a35..81ad7a7f9e1ea 100644
--- a/lib/gitlab/omniauth_initializer.rb
+++ b/lib/gitlab/omniauth_initializer.rb
@@ -29,8 +29,6 @@ def default_arguments_for(provider_name)
{
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 @@ def arguments_for(provider)
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 @@ def arguments_for(provider)
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 c19171bb6f8fd..2ecc26f9b9641 100644
--- a/spec/lib/gitlab/auth/saml/config_spec.rb
+++ b/spec/lib/gitlab/auth/saml/config_spec.rb
@@ -19,41 +19,6 @@
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 e12c0f4e78b0e..9b46b8eccc8b5 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 @@
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',
--
GitLab