Commit 1075f10a authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

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

Disable changing user attributes when updating SCIM provisioned user

See merge request gitlab-org/security/gitlab!2454
parents 03c9b6af ae4eb586
...@@ -171,12 +171,12 @@ Returns a `201` status code if successful. ...@@ -171,12 +171,12 @@ Returns a `201` status code if successful.
Fields that can be updated are: Fields that can be updated are:
| SCIM/IdP field | GitLab field | | SCIM/IdP field | GitLab field |
|:---------------------------------|:---------------------------------------| |:---------------------------------|:-----------------------------------------------------------------------------|
| `id/externalId` | `extern_uid` | | `id/externalId` | `extern_uid` |
| `name.formatted` | `name` | | `name.formatted` | `name` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) |
| `emails\[type eq "work"\].value` | `email` | | `emails\[type eq "work"\].value` | `email` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) |
| `active` | Identity removal if `active` = `false` | | `active` | Identity removal if `active` = `false` |
| `userName` | `username` | | `userName` | `username` ([Removed](https://gitlab.com/gitlab-org/gitlab/-/issues/363058)) |
```plaintext ```plaintext
PATCH /api/scim/v2/groups/:group_path/Users/:id PATCH /api/scim/v2/groups/:group_path/Users/:id
......
...@@ -75,13 +75,10 @@ module API ...@@ -75,13 +75,10 @@ module API
elsif parsed_hash[:extern_uid] elsif parsed_hash[:extern_uid]
identity.update(parsed_hash.slice(:extern_uid)) identity.update(parsed_hash.slice(:extern_uid))
else else
scim_conflict!(message: 'Email has already been taken') if email_taken?(parsed_hash[:email], identity) # With 15.0, we no longer allow modifying user attributes.
# However, we mark the operation as successful to avoid breaking
result = ::Users::UpdateService.new(identity.user, # existing automations
parsed_hash.except(:extern_uid, :active) true
.merge(user: identity.user)).execute
result[:status] == :success
end end
end end
...@@ -91,12 +88,6 @@ module API ...@@ -91,12 +88,6 @@ module API
false false
end 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) def find_user_identity(group, extern_uid)
return unless group.saml_provider return unless group.saml_provider
......
...@@ -371,7 +371,6 @@ RSpec.describe API::Scim do ...@@ -371,7 +371,6 @@ RSpec.describe API::Scim do
it 'does not call reprovision service when identity is already active' do it 'does not call reprovision service when identity is already active' do
expect(::EE::Gitlab::Scim::ReprovisionService).not_to receive(:new) expect(::EE::Gitlab::Scim::ReprovisionService).not_to receive(:new)
expect(::Users::UpdateService).to receive(:new).and_call_original
call_patch_api(params) call_patch_api(params)
end end
...@@ -394,6 +393,7 @@ RSpec.describe API::Scim do ...@@ -394,6 +393,7 @@ RSpec.describe API::Scim do
end end
end end
context 'user attributes' do
context 'name' do context 'name' do
before do before do
params = { Operations: [{ 'op': 'Replace', 'path': 'name.formatted', 'value': 'new_name' }] }.to_query params = { Operations: [{ 'op': 'Replace', 'path': 'name.formatted', 'value': 'new_name' }] }.to_query
...@@ -405,8 +405,8 @@ RSpec.describe API::Scim do ...@@ -405,8 +405,8 @@ RSpec.describe API::Scim do
expect(response).to have_gitlab_http_status(:no_content) expect(response).to have_gitlab_http_status(:no_content)
end end
it 'updates the name' do it 'does not update the name' do
expect(user.reload.name).to eq('new_name') expect(user.reload.name).not_to eq('new_name')
end end
it 'responds with an empty response' do it 'responds with an empty response' do
...@@ -415,15 +415,14 @@ RSpec.describe API::Scim do ...@@ -415,15 +415,14 @@ RSpec.describe API::Scim do
end end
context 'email' do context 'email' do
context 'non existent email' do
before do before do
params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
call_patch_api(params) call_patch_api(params)
end end
it 'updates the email' do it 'does not update the email' do
expect(user.reload.unconfirmed_email).to eq('new@mail.com') expect(user.reload.unconfirmed_email).not_to eq('new@mail.com')
end end
it 'responds with 204' do it 'responds with 204' do
...@@ -431,21 +430,23 @@ RSpec.describe API::Scim do ...@@ -431,21 +430,23 @@ RSpec.describe API::Scim do
end end
end end
context 'existent email' do context 'userName' do
before do before do
create(:user, email: 'new@mail.com') params = { Operations: [{ 'op': 'Replace', 'path': 'userName', 'value': 'new_username' }] }.to_query
params = { Operations: [{ 'op': 'Replace', 'path': 'emails[type eq "work"].value', 'value': 'new@mail.com' }] }.to_query
call_patch_api(params) call_patch_api(params)
end end
it 'does not update a duplicated email' do it 'responds with 204' do
expect(user.reload.unconfirmed_email).not_to eq('new@mail.com') 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 end
it 'responds with 209' do it 'responds with an empty response' do
expect(response).to have_gitlab_http_status(:conflict) expect(response.body).to eq('')
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment