diff --git a/changelogs/unreleased/tc-saml-fix-false-empty.yml b/changelogs/unreleased/tc-saml-fix-false-empty.yml new file mode 100644 index 0000000000000000000000000000000000000000..987f596475b5c084d50eae2b204c8f245849810d --- /dev/null +++ b/changelogs/unreleased/tc-saml-fix-false-empty.yml @@ -0,0 +1,5 @@ +--- +title: Fix SAML error 500 when no groups are defined for user +merge_request: 14913 +author: +type: fixed diff --git a/lib/gitlab/saml/auth_hash.rb b/lib/gitlab/saml/auth_hash.rb index 67a5f368bdb5ec6c2b439564bfed42441ce53853..33d19373098777c527e64c01eec278c75753937b 100644 --- a/lib/gitlab/saml/auth_hash.rb +++ b/lib/gitlab/saml/auth_hash.rb @@ -2,7 +2,7 @@ module Gitlab module Saml class AuthHash < Gitlab::OAuth::AuthHash def groups - get_raw(Gitlab::Saml::Config.groups) + Array.wrap(get_raw(Gitlab::Saml::Config.groups)) end private diff --git a/spec/lib/gitlab/saml/auth_hash_spec.rb b/spec/lib/gitlab/saml/auth_hash_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..a555935aea322a0cbb601010772efd10f66d107f --- /dev/null +++ b/spec/lib/gitlab/saml/auth_hash_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Gitlab::Saml::AuthHash do + include LoginHelpers + + let(:raw_info_attr) { { 'groups' => %w(Developers Freelancers) } } + subject(:saml_auth_hash) { described_class.new(omniauth_auth_hash) } + + let(:info_hash) do + { + name: 'John', + email: 'john@mail.com' + } + end + + let(:omniauth_auth_hash) do + OmniAuth::AuthHash.new(uid: 'my-uid', + provider: 'saml', + info: info_hash, + extra: { raw_info: OneLogin::RubySaml::Attributes.new(raw_info_attr) } ) + end + + before do + stub_saml_group_config(%w(Developers Freelancers Designers)) + end + + describe '#groups' do + it 'returns array of groups' do + expect(saml_auth_hash.groups).to eq(%w(Developers Freelancers)) + end + + context 'raw info hash attributes empty' do + let(:raw_info_attr) { {} } + + it 'returns an empty array' do + expect(saml_auth_hash.groups).to be_a(Array) + end + end + end +end diff --git a/spec/lib/gitlab/saml/user_spec.rb b/spec/lib/gitlab/saml/user_spec.rb index 59923bfb14d450810372b1d9ba80b558c898677d..1c23fb5f285f68e8f364aca7d0d3f5420984078c 100644 --- a/spec/lib/gitlab/saml/user_spec.rb +++ b/spec/lib/gitlab/saml/user_spec.rb @@ -2,13 +2,15 @@ require 'spec_helper' describe Gitlab::Saml::User do include LdapHelpers + include LoginHelpers let(:saml_user) { described_class.new(auth_hash) } let(:gl_user) { saml_user.gl_user } let(:uid) { 'my-uid' } let(:dn) { 'uid=user1,ou=People,dc=example' } let(:provider) { 'saml' } - let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new({ 'groups' => %w(Developers Freelancers Designers) }) }) } + let(:raw_info_attr) { { 'groups' => %w(Developers Freelancers Designers) } } + let(:auth_hash) { OmniAuth::AuthHash.new(uid: uid, provider: provider, info: info_hash, extra: { raw_info: OneLogin::RubySaml::Attributes.new(raw_info_attr) }) } let(:info_hash) do { name: 'John', @@ -18,22 +20,6 @@ describe Gitlab::Saml::User do let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') } describe '#save' do - def stub_omniauth_config(messages) - allow(Gitlab.config.omniauth).to receive_messages(messages) - end - - def stub_ldap_config(messages) - allow(Gitlab::LDAP::Config).to receive_messages(messages) - end - - def stub_basic_saml_config - allow(Gitlab::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } }) - end - - def stub_saml_group_config(groups) - allow(Gitlab::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', external_groups: groups, args: {} } }) - end - before do stub_basic_saml_config end @@ -402,4 +388,16 @@ describe Gitlab::Saml::User do end end end + + describe '#find_user' do + context 'raw info hash attributes empty' do + let(:raw_info_attr) { {} } + + it 'does not mark user as external' do + stub_saml_group_config(%w(Freelancers)) + + expect(saml_user.find_user.external).to be_falsy + end + end + end end diff --git a/spec/support/ldap_helpers.rb b/spec/support/ldap_helpers.rb index 079f244475cf721c37726ee74af57738c651e122..28d39a32f020f923f36d4f556135bd522b2f3d86 100644 --- a/spec/support/ldap_helpers.rb +++ b/spec/support/ldap_helpers.rb @@ -15,10 +15,7 @@ module LdapHelpers # admin_group: 'my-admin-group' # ) def stub_ldap_config(messages) - messages.each do |config, value| - allow_any_instance_of(::Gitlab::LDAP::Config) - .to receive(config.to_sym).and_return(value) - end + allow_any_instance_of(::Gitlab::LDAP::Config).to receive_messages(messages) end # Stub an LDAP person search and provide the return entry. Specify `nil` for diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 3e11753015181886c1d6150a4bb7015f3d46ae1a..4aed40bf22d39954bd98c6da556271c740540bba 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -120,4 +120,16 @@ module LoginHelpers allow_any_instance_of(Object).to receive(:user_saml_omniauth_authorize_path).and_return('/users/auth/saml') allow_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') end + + def stub_omniauth_config(messages) + allow(Gitlab.config.omniauth).to receive_messages(messages) + end + + def stub_basic_saml_config + allow(Gitlab::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } }) + end + + def stub_saml_group_config(groups) + allow(Gitlab::Saml::Config).to receive_messages({ options: { name: 'saml', groups_attribute: 'groups', external_groups: groups, args: {} } }) + end end