Commit 292c99c3 authored by Drew Blessing's avatar Drew Blessing Committed by Drew Blessing

Allow SCIM to create an identity for an existing user

When the `:scim_identities` feature is enabled, allow SCIM to
associate with an existing user - create the SCIM identity, and
add the user as a member of the group.

This closes a gap compared to the prior implementation where an existing
SAML identity would be used to associate SCIM (since SAML + SCIM
identities were the same).

We also have more latitude to associate a dedicated SCIM identity
with an existing user because there's no security risk of account
takeover as with SAML identities.
parent 328060f0
---
title: Allow SCIM to create an identity for an existing user
merge_request: 28379
author:
type: fixed
......@@ -132,7 +132,7 @@ module API
check_group_saml_configured
end
desc 'Get SAML users' do
desc 'Get SCIM users' do
detail 'This feature was introduced in GitLab 11.10.'
end
get do
......@@ -149,7 +149,7 @@ module API
scim_error!(message: 'Unsupported Filter')
end
desc 'Get a SAML user' do
desc 'Get a SCIM user' do
detail 'This feature was introduced in GitLab 11.10.'
end
get ':id', requirements: USER_ID_REQUIREMENTS do
......@@ -164,7 +164,7 @@ module API
present identity, with: ::EE::API::Entities::Scim::User
end
desc 'Create a SAML user' do
desc 'Create a SCIM user' do
detail 'This feature was introduced in GitLab 11.10.'
end
post do
......@@ -184,7 +184,7 @@ module API
end
end
desc 'Updates a SAML user' do
desc 'Updates a SCIM user' do
detail 'This feature was introduced in GitLab 11.10.'
end
patch ':id', requirements: USER_ID_REQUIREMENTS do
......@@ -204,7 +204,7 @@ module API
end
end
desc 'Removes a SAML user' do
desc 'Removes a SCIM user' do
detail 'This feature was introduced in GitLab 11.10.'
end
delete ':id', requirements: USER_ID_REQUIREMENTS do
......
......@@ -16,16 +16,15 @@ module EE
end
def execute
return success_response if existing_member?
return error_response(errors: ["Missing params: #{missing_params}"]) unless missing_params.empty?
return success_response if existing_identity_and_member?
clear_memoization(:identity)
if user.save && member.errors.empty?
success_response
else
error_response
end
return create_identity if create_identity_only?
return create_identity_and_member if existing_user?
create_user_and_member
rescue => e
logger.error(error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
......@@ -34,8 +33,22 @@ module EE
private
def success_response
ProvisioningResponse.new(status: :success, identity: identity)
def create_identity
return success_response if identity.save
error_response(objects: [identity])
end
def create_identity_and_member
return success_response if identity.save && member.errors.empty?
error_response(objects: [identity, member])
end
def create_user_and_member
return success_response if user.save && member.errors.empty?
error_response(objects: [user, identity, member])
end
def scim_identities_enabled?
......@@ -54,20 +67,49 @@ module EE
def identity
strong_memoize(:identity) do
if scim_identities_enabled?
@group.scim_identities.with_extern_uid(@parsed_hash[:extern_uid]).first
else
::Identity.with_extern_uid(identity_provider, @parsed_hash[:extern_uid]).first
end
next saml_identity unless scim_identities_enabled?
identity = @group.scim_identities.with_extern_uid(@parsed_hash[:extern_uid]).first
next identity if identity
build_scim_identity
end
end
def saml_identity
::Identity.with_extern_uid(identity_provider, @parsed_hash[:extern_uid]).first
end
def user
@user ||= ::Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
strong_memoize(:user) do
next build_user unless scim_identities_enabled?
user = ::User.find_by_any_email(@parsed_hash[:email])
next user if user
build_user
end
end
def error_response(errors: nil)
errors ||= [user, identity, member].compact.flat_map { |obj| obj.errors.full_messages }
def build_user
::Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
end
def build_scim_identity
@scim_identity ||=
@group.scim_identities.new(
user: user,
extern_uid: @parsed_hash[:extern_uid],
active: true
)
end
def success_response
ProvisioningResponse.new(status: :success, identity: identity)
end
def error_response(errors: nil, objects: [])
errors ||= objects.compact.flat_map { |obj| obj.errors.full_messages }
conflict = errors.any? { |error| error.include?('has already been taken') }
ProvisioningResponse.new(status: conflict ? :conflict : :error, message: errors.to_sentence)
......@@ -104,14 +146,32 @@ module EE
Uniquify.new.string(clean_username) { |s| !NamespacePathValidator.valid_path?(s) }
end
def missing_params
@missing_params ||= ([:extern_uid, :email, :username] - @parsed_hash.keys)
end
def member
strong_memoize(:member) do
next @group.group_member(user) if existing_member?(user)
@group.add_user(user, DEFAULT_ACCESS) if user.valid?
end
end
def existing_member?
identity && ::GroupMember.member_of_group?(@group, identity.user)
def create_identity_only?
scim_identities_enabled? && existing_user? && existing_member?(user)
end
def existing_identity_and_member?
identity&.persisted? && existing_member?(identity.user)
end
def existing_member?(user)
::GroupMember.member_of_group?(@group, user)
end
def existing_user?
user&.persisted?
end
end
end
......
......@@ -11,6 +11,16 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
stub_licensed_features(group_saml: true)
end
shared_examples 'success response' do
it 'contains a success status' do
expect(service.execute.status).to eq(:success)
end
it 'contains an identity in the response' do
expect(service.execute.identity).to be_a(Identity).or be_a(ScimIdentity)
end
end
shared_examples 'scim provisioning' do
context 'valid params' do
let_it_be(:service_params) do
......@@ -26,9 +36,7 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
User.find_by(email: service_params[:email])
end
it 'succeeds' do
expect(service.execute.status).to eq(:success)
end
it_behaves_like 'success response'
it 'creates the user' do
expect { service.execute }.to change { User.count }.by(1)
......@@ -68,20 +76,6 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
expect { service.execute }.to change { User.count }.by(1)
end
end
context 'existing user' do
before do
create(:user, email: 'work@example.com')
end
it 'does not create a new user' do
expect { service.execute }.not_to change { User.count }
end
it 'fails with conflict' do
expect(service.execute.status).to eq(:conflict)
end
end
end
context 'invalid params' do
......@@ -96,6 +90,23 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
it 'fails with error' do
expect(service.execute.status).to eq(:error)
end
it 'fails with missing params' do
expect(service.execute.message).to eq("Missing params: [:username]")
end
end
end
shared_examples 'existing user when scim identities are enabled' do
it 'does not create a new user' do
expect { service.execute }.not_to change { User.count }
end
it_behaves_like 'success response'
it 'creates the identity' do
expect { service.execute }.to change { ScimIdentity.count }.by(1)
expect { service.execute }.not_to change { Identity.count }
end
end
......@@ -105,6 +116,8 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
create(:saml_provider, group: group)
end
it_behaves_like 'scim provisioning'
let_it_be(:service_params) do
{
email: 'work@example.com',
......@@ -114,12 +127,24 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
}
end
it_behaves_like 'scim provisioning'
it 'creates the identity' do
expect { service.execute }.to change { Identity.count }.by(1)
expect { service.execute }.not_to change { ScimIdentity.count }
end
context 'existing user' do
before do
create(:user, email: 'work@example.com')
end
it 'does not create a new user' do
expect { service.execute }.not_to change { User.count }
end
it 'fails with conflict' do
expect(service.execute.status).to eq(:conflict)
end
end
end
context 'when scim_identities is enabled' do
......@@ -127,6 +152,8 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
stub_feature_flags(scim_identities: true)
end
it_behaves_like 'scim provisioning'
let_it_be(:service_params) do
{
email: 'work@example.com',
......@@ -136,12 +163,37 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
}
end
it_behaves_like 'scim provisioning'
it 'creates the scim identity' do
expect { service.execute }.to change { ScimIdentity.count }.by(1)
expect { service.execute }.not_to change { Identity.count }
end
context 'existing user' do
before do
create(:email, user: user, email: 'work@example.com')
end
let(:user) { create(:user) }
context 'when user is not an existing group member' do
it_behaves_like 'existing user when scim identities are enabled'
it 'creates the group member' do
expect { service.execute }.to change { GroupMember.count }.by(1)
end
end
context 'when user is an existing group member' do
before do
group.add_guest(user)
end
it_behaves_like 'existing user when scim identities are enabled'
it 'does not create the group member' do
expect { service.execute }.not_to change { GroupMember.count }
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