From 2b2c8e9e96ccbd3479095dc70d2c952520923ff5 Mon Sep 17 00:00:00 2001
From: Adil Farrukh <afarrukh@gitlab.com>
Date: Mon, 20 Jan 2025 15:43:59 +0000
Subject: [PATCH] Merge branch 'dblessing_fix_null_external_saml' into 'master'

Ensure user external attribute is preserved and not set to nil

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177671

Merged-by: Aboobacker MK <akarakath@gitlab.com>
Approved-by: Hakeem Abdul-Razak <habdul-razak@gitlab.com>
Approved-by: Jessie Young <jessieyoung@gitlab.com>
Approved-by: Smriti Garg <sgarg@gitlab.com>
Reviewed-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Jessie Young <jessieyoung@gitlab.com>
Co-authored-by: Drew Blessing <drew@gitlab.com>

(cherry picked from commit 22aa0f823998cc61b3f1087b63a1d69ecbdeef4c)

f030e6a9 Ensure user external attribute is preserved and not null
03249172 Apply 1 suggestion(s) to 1 file(s)
5b732c1f Early return

Co-authored-by: Aboobacker MK <akarakath@gitlab.com>
---
 lib/gitlab/auth/saml/user.rb           | 15 ++++++-----
 spec/lib/gitlab/auth/saml/user_spec.rb | 37 +++++++++++++++++++-------
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/lib/gitlab/auth/saml/user.rb b/lib/gitlab/auth/saml/user.rb
index 135dc7c7dc417ab2..2445741e545268ea 100644
--- a/lib/gitlab/auth/saml/user.rb
+++ b/lib/gitlab/auth/saml/user.rb
@@ -18,7 +18,7 @@ def find_user
           user ||= find_or_build_ldap_user if auto_link_ldap_user?
           user ||= build_new_user if signup_enabled?
 
-          user&.external = external_user?
+          user&.external = external_user? if any_external_config_present?
 
           user
         end
@@ -52,6 +52,10 @@ def external_users_enabled?
           !saml_config.external_groups.nil?
         end
 
+        def any_external_config_present?
+          external_provider? || external_users_enabled?
+        end
+
         def auth_hash=(auth_hash)
           @auth_hash = Gitlab::Auth::Saml::AuthHash.new(auth_hash)
         end
@@ -59,11 +63,10 @@ def auth_hash=(auth_hash)
         private
 
         def external_user?
-          if external_provider?
-            true
-          elsif external_users_enabled?
-            intersecting_external_groups?
-          end
+          return true if external_provider?
+          return intersecting_external_groups? if external_users_enabled?
+
+          false
         end
 
         def intersecting_external_groups?
diff --git a/spec/lib/gitlab/auth/saml/user_spec.rb b/spec/lib/gitlab/auth/saml/user_spec.rb
index 4b5cc355ad463328..797d84028f7dd740 100644
--- a/spec/lib/gitlab/auth/saml/user_spec.rb
+++ b/spec/lib/gitlab/auth/saml/user_spec.rb
@@ -40,6 +40,7 @@
           saml_user.save # rubocop:disable Rails/SaveBang
           expect(gl_user).to be_valid
           expect(gl_user).to eq existing_user
+          expect(gl_user.external).to be false
           identity = gl_user.identities.first
           expect(identity.extern_uid).to eql uid
           expect(identity.provider).to eql 'saml'
@@ -47,24 +48,40 @@
       end
 
       context 'external groups' do
-        before do
-          stub_saml_group_config(%w[Interns])
+        context 'no external groups configuration is defined' do
+          it 'does not mark the user as external' do
+            saml_user.save # rubocop:disable Rails/SaveBang -- Not ActiveRecord object
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be false
+          end
+
+          it 'does not change a user manually set as external' do
+            existing_user.update!(external: true)
+
+            saml_user.save # rubocop:disable Rails/SaveBang -- Not ActiveRecord object
+            expect(gl_user).to be_valid
+            expect(gl_user.external).to be true
+          end
         end
 
         context 'are defined' do
-          it 'marks the user as external' do
+          before do
             stub_saml_group_config(%w[Freelancers])
+          end
+
+          it 'marks the user as external' do
             saml_user.save # rubocop:disable Rails/SaveBang
             expect(gl_user).to be_valid
             expect(gl_user.external).to be_truthy
           end
-        end
 
-        context 'are defined but the user does not belong there' do
-          it 'does not mark the user as external' do
-            saml_user.save # rubocop:disable Rails/SaveBang
-            expect(gl_user).to be_valid
-            expect(gl_user.external).to be_falsey
+          context 'are defined but the user does not belong there' do
+            it 'does not mark the user as external' do
+              stub_saml_group_config(%w[Interns])
+              saml_user.save # rubocop:disable Rails/SaveBang -- Not ActiveRecord object
+              expect(gl_user).to be_valid
+              expect(gl_user.external).to be false
+            end
           end
         end
 
@@ -142,7 +159,7 @@
             stub_saml_group_config(%w[Interns])
             saml_user.save # rubocop:disable Rails/SaveBang
             expect(gl_user).to be_valid
-            expect(gl_user.external).to be_falsey
+            expect(gl_user.external).to be false
           end
         end
       end
-- 
GitLab