Fix SAML error 500 when no groups are defined for usertc-saml-fix-false-empty

When there are no groups defined in the auth hash attributes,
`Gitlab::Saml::AuthHash#groups` should return an empty array, and
`Gitlab::Saml::User#find_user` should not mark the user as external.

Closes gitlab-org/gitlab-ce#38923.

6 files changed, 74 insertions, 22 deletions

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 00000000000..987f596475b
--- /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 67a5f368bdb..33d19373098 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 00000000000..a555935aea3
--- /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 59923bfb14d..1c23fb5f285 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 079f244475c..28d39a32f02 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 3e117530151..4aed40bf22d 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