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