Merge branch 'dblessing_sso_update_enterprise_users' into 'master'

Update user attributes for Group SAML enterprise users

See merge request gitlab-org/gitlab!72792

doc/user/group/saml_sso/index.md

@@ -245,8 +245,9 @@ On subsequent visits, you should be able to go [sign in to GitLab.com with SAML]
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/263661) in GitLab 13.7.
 
 GitLab allows setting certain user attributes based on values from the SAML response.
-This affects users created on first sign-in via Group SAML. Existing users'
-attributes are not affected regardless of the values sent in the SAML response.
+Existing users will have these attributes updated if the user was originally
+provisioned by the group. Users are provisioned by the group when the account was
+created via [SCIM](scim_setup.md) or by first sign-in with SAML SSO for GitLab.com groups.
 
 #### Supported user attributes
 

ee/lib/gitlab/auth/group_saml/auth_hash.rb

@@ -4,15 +4,22 @@ module Gitlab
   module Auth
     module GroupSaml
       class AuthHash < Gitlab::Auth::Saml::AuthHash
+        include Gitlab::Utils::StrongMemoize
+
         ALLOWED_USER_ATTRIBUTES = %w(can_create_group projects_limit).freeze
 
         def groups
           Array.wrap(get_raw('groups') || get_raw('Groups'))
         end
 
-        ALLOWED_USER_ATTRIBUTES.each do |attribute|
-          define_method(attribute) do
-            Array(get_raw(attribute)).first
+        # Access user attributes by hash.
+        #
+        # auth_hash.user_attributes['can_create_group']
+        def user_attributes
+          strong_memoize(:user_attributes) do
+            ALLOWED_USER_ATTRIBUTES.to_h do |attr|
+              [attr, Array(get_raw(attr)).first]
+            end
           end
         end
       end

ee/lib/gitlab/auth/group_saml/user.rb

@@ -18,6 +18,7 @@ module Gitlab
         override :find_and_update!
         def find_and_update!
           add_or_update_user_identities
+          set_provisioned_user_attributes!(gl_user)
 
           save("GroupSaml Provider ##{@saml_provider.id}")
           # Do not return un-persisted user so user is prompted
@@ -61,14 +62,6 @@ module Gitlab
           super.tap do |user|
             user.provisioned_by_group_id = saml_provider.group_id
             user.skip_confirmation_notification!
-
-            # rubocop:disable GitlabSecurity/PublicSend
-            AuthHash::ALLOWED_USER_ATTRIBUTES.each do |attribute|
-              next unless value = auth_hash.public_send(attribute)
-
-              user.public_send("#{attribute}=", value)
-            end
-            # rubocop:enable GitlabSecurity/PublicSend
           end
         end
 
@@ -99,6 +92,12 @@ module Gitlab
           MembershipUpdater.new(gl_user, saml_provider, auth_hash).execute
         end
 
+        def set_provisioned_user_attributes!(user)
+          return unless user.provisioned_by_group_id == saml_provider.group_id
+
+          user.assign_attributes(auth_hash.user_attributes.compact)
+        end
+
         override :block_after_signup?
         def block_after_signup?
           false

ee/spec/lib/gitlab/auth/group_saml/auth_hash_spec.rb

@@ -41,11 +41,11 @@ RSpec.describe Gitlab::Auth::GroupSaml::AuthHash do
       let(:raw_info_attr) { { 'can_create_group' => %w(true), 'projects_limit' => %w(20) } }
 
       it 'returns the proper can_create_groups value' do
-        expect(saml_auth_hash.can_create_group).to eq "true"
+        expect(saml_auth_hash.user_attributes['can_create_group']).to eq "true"
       end
 
       it 'returns the proper projects_limit value' do
-        expect(saml_auth_hash.projects_limit).to eq "20"
+        expect(saml_auth_hash.user_attributes['projects_limit']).to eq "20"
       end
     end
 
@@ -53,11 +53,11 @@ RSpec.describe Gitlab::Auth::GroupSaml::AuthHash do
       let(:raw_info_attr) { { 'can_create_group' => 'false', 'projects_limit' => '20' } }
 
       it 'returns the proper can_create_groups value' do
-        expect(saml_auth_hash.can_create_group).to eq "false"
+        expect(saml_auth_hash.user_attributes['can_create_group']).to eq "false"
       end
 
       it 'returns the proper projects_limit value' do
-        expect(saml_auth_hash.projects_limit).to eq "20"
+        expect(saml_auth_hash.user_attributes['projects_limit']).to eq "20"
       end
     end
 
@@ -65,11 +65,11 @@ RSpec.describe Gitlab::Auth::GroupSaml::AuthHash do
       let(:raw_info_attr) { {} }
 
       it 'returns nil for can_create_group' do
-        expect(saml_auth_hash.can_create_group).to eq nil
+        expect(saml_auth_hash.user_attributes['can_create_group']).to eq nil
       end
 
       it 'returns nil for can_create_groups' do
-        expect(saml_auth_hash.projects_limit).to eq nil
+        expect(saml_auth_hash.user_attributes['projects_limit']).to eq nil
       end
     end
   end

ee/spec/lib/gitlab/auth/group_saml/user_spec.rb

@@ -58,6 +58,25 @@ RSpec.describe Gitlab::Auth::GroupSaml::User do
         expect(find_and_update.provisioned_by_group).to be_nil
       end
 
+      context 'when user attributes are present but the user is not provisioned' do
+        before do
+          identity.user.update!(can_create_group: false, projects_limit: 10)
+
+          auth_hash[:extra][:raw_info] =
+            OneLogin::RubySaml::Attributes.new(
+              'can_create_group' => %w(true), 'projects_limit' => %w(20)
+            )
+        end
+
+        it 'does not update the user can_create_group attribute' do
+          expect(find_and_update.can_create_group).to eq(false)
+        end
+
+        it 'does not update the user projects_limit attribute' do
+          expect(find_and_update.projects_limit).to eq(10)
+        end
+      end
+
       context 'when the user has multiple group saml identities' do
         let(:saml_provider2) { create(:saml_provider) }
 
@@ -140,10 +159,29 @@ RSpec.describe Gitlab::Auth::GroupSaml::User do
             expect(find_and_update).to eq user
           end
 
-          it 'updates idenitity' do
+          it 'updates identity' do
             expect { find_and_update }.to change { user.group_saml_identities.count }.by(1)
           end
 
+          context 'when user attributes are present' do
+            before do
+              user.update!(can_create_group: false, projects_limit: 10)
+
+              auth_hash[:extra][:raw_info] =
+                OneLogin::RubySaml::Attributes.new(
+                  'can_create_group' => %w(true), 'projects_limit' => %w(20)
+                )
+            end
+
+            it 'updates the user with correct can_create_group attribute' do
+              expect(find_and_update.can_create_group).to eq(true)
+            end
+
+            it 'updates the user with correct projects_limit attribute' do
+              expect(find_and_update.projects_limit).to eq(20)
+            end
+          end
+
           context 'without feature flag turned on' do
             before do
               stub_feature_flags(block_password_auth_for_saml_users: false)