Merge branch 'saml-ldap-link-flow' into 'master'

Adjust the SAML control flow to allow LDAP identities to be added to an existing SAML user.

It correctly lets an existing SAML user to add their LDAP identity automatically at login.

A customer had issues with the `auto_link_ldap_user` feature. The flow was not working if there was an account with a SAML identity, but no LDAP identity. GitLab would pick up the correct LDAP person, but due to the order of the flow, that LDAP person was never associated with the user.

Fixes #17346

/cc @dblessing @balameb @stanhu

See merge request !4498

CHANGELOG

@@ -5,6 +5,7 @@ v 8.8.5
   - Fix todos page throwing errors when you have a project pending deletion
   - Disable Webhooks before proceeding with the GitHub import
   - Fix importer for GitHub comments on diff
+  - Adjust the SAML control flow to allow LDAP identities to be added to an existing SAML user
 
 v 8.8.4
   - Fix LDAP-based login for users with 2FA enabled. !4493

lib/gitlab/o_auth/user.rb

@@ -69,13 +69,20 @@ module Gitlab
         return unless ldap_person
 
         # If a corresponding person exists with same uid in a LDAP server,
-        # set up a Gitlab user with dual LDAP and Omniauth identities.
-        if user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)
-          # Case when a LDAP user already exists in Gitlab. Add the Omniauth identity to existing account.
+        # check if the user already has a GitLab account.
+        user = Gitlab::LDAP::User.find_by_uid_and_provider(ldap_person.dn, ldap_person.provider)
+        if user
+          # Case when a LDAP user already exists in Gitlab. Add the OAuth identity to existing account.
+          log.info "LDAP account found for user #{user.username}. Building new #{auth_hash.provider} identity."
           user.identities.build(extern_uid: auth_hash.uid, provider: auth_hash.provider)
         else
-          # No account in Gitlab yet: create it and add the LDAP identity
-          user = build_new_user
+          log.info "No existing LDAP account was found in GitLab. Checking for #{auth_hash.provider} account."
+          user = find_by_uid_and_provider
+          if user.nil?
+            log.info "No user found using #{auth_hash.provider} provider. Creating a new one."
+            user = build_new_user
+          end
+          log.info "Correct account has been found. Adding LDAP identity to user: #{user.username}."
           user.identities.new(provider: ldap_person.provider, extern_uid: ldap_person.dn)
         end
 

lib/gitlab/saml/user.rb

@@ -12,12 +12,12 @@ module Gitlab
       end
 
       def gl_user
-        @user ||= find_by_uid_and_provider
-
         if auto_link_ldap_user?
           @user ||= find_or_create_ldap_user
         end
 
+        @user ||= find_by_uid_and_provider
+
         if auto_link_saml_user?
           @user ||= find_by_email
         end

spec/lib/gitlab/saml/user_spec.rb

@@ -145,6 +145,7 @@ describe Gitlab::Saml::User, lib: true do
               allow(ldap_user).to receive(:email) { %w(john@mail.com john2@example.com) }
               allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
               allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
+              allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
             end
 
             context 'and no account for the LDAP user' do
@@ -177,6 +178,23 @@ describe Gitlab::Saml::User, lib: true do
                                                           ])
               end
             end
+
+            context 'user has SAML user, and wants to add their LDAP identity' do
+              it 'adds the LDAP identity to the existing SAML user' do
+                create(:omniauth_user, email: 'john@mail.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'saml', username: 'john')
+                local_hash = OmniAuth::AuthHash.new(uid: 'uid=user1,ou=People,dc=example', provider: provider, info: info_hash)
+                local_saml_user = described_class.new(local_hash)
+                local_saml_user.save
+                local_gl_user = local_saml_user.gl_user
+
+                expect(local_gl_user).to be_valid
+                expect(local_gl_user.identities.length).to eql 2
+                identities_as_hash = local_gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
+                expect(identities_as_hash).to match_array([ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+                                                            { provider: 'saml', extern_uid: 'uid=user1,ou=People,dc=example' }
+                                                          ])
+              end
+            end
           end
         end
       end