Disable changing user attributes when updating SCIM provisioned user

Merge branch 'security-disable_user_updates_in_scim_patch_api-14-10' into '14-10-stable-ee'

See merge request gitlab-org/security/gitlab!2454

Changelog: security

doc/api/scim.md

@@ -170,13 +170,13 @@ Returns a `201` status code if successful.
 
 Fields that can be updated are:
 
-| SCIM/IdP field                   | GitLab field                           |
-|:---------------------------------|:---------------------------------------|
-| `id/externalId`                  | `extern_uid`                           |
-| `name.formatted`                 | `name`                                 |
-| `emails\[type eq "work"\].value` | `email`                                |
-| `active`                         | Identity removal if `active` = `false` |
-| `userName`                       | `username`                             |
+| SCIM/IdP field                   | GitLab field                                                                 |
+|:---------------------------------|:-----------------------------------------------------------------------------|
+| `id/externalId`                  | `extern_uid`                                                                 |
+| `name.formatted`                 | `name` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058))     |
+| `emails\[type eq "work"\].value` | `email` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058))    |
+| `active`                         | Identity removal if `active` = `false`                                       |
+| `userName`                       | `username` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) |
 
 ```plaintext
 PATCH /api/scim/v2/groups/:group_path/Users/:id

ee/lib/api/scim.rb

@@ -75,13 +75,10 @@ module API
           elsif parsed_hash[:extern_uid]
             identity.update(parsed_hash.slice(:extern_uid))
           else
-            scim_conflict!(message: 'Email has already been taken') if email_taken?(parsed_hash[:email], identity)
-
-            result = ::Users::UpdateService.new(identity.user,
-                                                parsed_hash.except(:extern_uid, :active)
-                                                  .merge(user: identity.user)).execute
-
-            result[:status] == :success
+            # With 15.0, we no longer allow modifying user attributes.
+            # However, we mark the operation as successful to avoid breaking
+            # existing automations
+            true
           end
         end
 
@@ -91,12 +88,6 @@ module API
           false
         end
 
-        def email_taken?(email, identity)
-          return unless email
-
-          User.by_any_email(email.downcase).where.not(id: identity.user.id).exists?
-        end
-
         def find_user_identity(group, extern_uid)
           return unless group.saml_provider
 

ee/spec/requests/api/scim_spec.rb

@@ -371,7 +371,6 @@ RSpec.describe API::Scim do
 
         it 'does not call reprovision service when identity is already active' do
           expect(::EE::Gitlab::Scim::ReprovisionService).not_to receive(:new)
-          expect(::Users::UpdateService).to receive(:new).and_call_original
 
           call_patch_api(params)
         end
@@ -394,36 +393,36 @@ RSpec.describe API::Scim do
           end
         end
 
-        context 'name' do
-          before do
-            params = { Operations: [{ 'op': 'Replace', 'path': 'name.formatted', 'value': 'new_name' }] }.to_query
+        context 'user attributes' do
+          context 'name' do
+            before do
+              params = { Operations: [{ 'op': 'Replace', 'path': 'name.formatted', 'value': 'new_name' }] }.to_query
 
-            call_patch_api(params)
-          end
+              call_patch_api(params)
+            end
 
-          it 'responds with 204' do
-            expect(response).to have_gitlab_http_status(:no_content)
-          end
+            it 'responds with 204' do
+              expect(response).to have_gitlab_http_status(:no_content)
+            end
 
-          it 'updates the name' do
-            expect(user.reload.name).to eq('new_name')
-          end
+            it 'does not update the name' do
+              expect(user.reload.name).not_to eq('new_name')
+            end
 
-          it 'responds with an empty response' do
-            expect(response.body).to eq('')
+            it 'responds with an empty response' do
+              expect(response.body).to eq('')
+            end
           end
-        end
 
-        context 'email' do
-          context 'non existent email' do
+          context 'email' do
             before do
               params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
 
               call_patch_api(params)
             end
 
-            it 'updates the email' do
-              expect(user.reload.unconfirmed_email).to eq('new@mail.com')
+            it 'does not update the email' do
+              expect(user.reload.unconfirmed_email).not_to eq('new@mail.com')
             end
 
             it 'responds with 204' do
@@ -431,21 +430,23 @@ RSpec.describe API::Scim do
             end
           end
 
-          context 'existent email' do
+          context 'userName' do
             before do
-              create(:user, email: 'new@mail.com')
-
-              params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
+              params = { Operations: [{ 'op': 'Replace', 'path': 'userName', 'value': 'new_username' }] }.to_query
 
               call_patch_api(params)
             end
 
-            it 'does not update a duplicated email' do
-              expect(user.reload.unconfirmed_email).not_to eq('new@mail.com')
+            it 'responds with 204' do
+              expect(response).to have_gitlab_http_status(:no_content)
+            end
+
+            it 'does not update the username' do
+              expect(user.reload.username).not_to eq('new_username')
             end
 
-            it 'responds with 209' do
-              expect(response).to have_gitlab_http_status(:conflict)
+            it 'responds with an empty response' do
+              expect(response.body).to eq('')
             end
           end
         end