Commit 443c19f8 authored by Drew Blessing's avatar Drew Blessing Committed by Drew Blessing

Full SCIM Identities support for the SCIM API

The SCIM API now supports SCIM Identities in all endpoints
when the `scim_identities` feature flag is enabled for a group.
For now the feature flag remains disabled by default.
parent 232c6d22
...@@ -28,9 +28,7 @@ module Users ...@@ -28,9 +28,7 @@ module Users
end end
end end
unless identity_params.empty? build_identity(user)
user.identities.build(identity_params)
end
user user
end end
...@@ -41,6 +39,12 @@ module Users ...@@ -41,6 +39,12 @@ module Users
[:extern_uid, :provider] [:extern_uid, :provider]
end end
def build_identity(user)
return if identity_params.empty?
user.identities.build(identity_params)
end
def can_create_user? def can_create_user?
(current_user.nil? && Gitlab::CurrentSettings.allow_signup?) || current_user&.admin? (current_user.nil? && Gitlab::CurrentSettings.allow_signup?) || current_user&.admin?
end end
......
...@@ -23,7 +23,7 @@ class ScimFinder ...@@ -23,7 +23,7 @@ class ScimFinder
def scim_identities_enabled? def scim_identities_enabled?
strong_memoize(:scim_identities_enabled) do strong_memoize(:scim_identities_enabled) do
Feature.enabled?(:scim_identities, group) ::EE::Gitlab::Scim::Feature.scim_identities_enabled?(group)
end end
end end
...@@ -47,9 +47,9 @@ class ScimFinder ...@@ -47,9 +47,9 @@ class ScimFinder
parser = EE::Gitlab::Scim::ParamsParser.new(params) parser = EE::Gitlab::Scim::ParamsParser.new(params)
if eq_filter_on_extern_uid?(parser) if eq_filter_on_extern_uid?(parser)
by_extern_uid(parser) by_extern_uid(parser.filter_params[:extern_uid])
elsif eq_filter_on_username?(parser) elsif eq_filter_on_username?(parser)
by_username(parser) by_username(parser.filter_params[:username])
else else
raise UnsupportedFilter raise UnsupportedFilter
end end
...@@ -59,18 +59,18 @@ class ScimFinder ...@@ -59,18 +59,18 @@ class ScimFinder
parser.filter_operator == :eq && parser.filter_params[:extern_uid].present? parser.filter_operator == :eq && parser.filter_params[:extern_uid].present?
end end
def by_extern_uid(parser) def by_extern_uid(extern_uid)
return group.scim_identities.with_extern_uid(parser.filter_params[:extern_uid]) if scim_identities_enabled? return group.scim_identities.with_extern_uid(extern_uid) if scim_identities_enabled?
Identity.where_group_saml_uid(saml_provider, parser.filter_params[:extern_uid]) Identity.where_group_saml_uid(saml_provider, extern_uid)
end end
def eq_filter_on_username?(parser) def eq_filter_on_username?(parser)
parser.filter_operator == :eq && parser.filter_params[:username].present? parser.filter_operator == :eq && parser.filter_params[:username].present?
end end
def by_username(parser) def by_username(username)
user = User.find_by_username(parser.filter_params[:username]) user = User.find_by_username(username) || User.find_by_any_email(username)
return group.scim_identities.for_user(user) if scim_identities_enabled? return group.scim_identities.for_user(user) if scim_identities_enabled?
......
...@@ -51,6 +51,13 @@ module EE ...@@ -51,6 +51,13 @@ module EE
super.push(:saml_provider_id) super.push(:saml_provider_id)
end end
override :build_identity
def build_identity(user)
return build_scim_identity(user) if params[:provider] == 'group_scim'
super
end
override :identity_params override :identity_params
def identity_params def identity_params
if group_id_for_saml.present? if group_id_for_saml.present?
...@@ -60,6 +67,10 @@ module EE ...@@ -60,6 +67,10 @@ module EE
end end
end end
def scim_identity_attributes
[:group_id, :extern_uid]
end
def saml_provider_id def saml_provider_id
strong_memoize(:saml_provider_id) do strong_memoize(:saml_provider_id) do
group = GroupFinder.new(current_user).execute(id: group_id_for_saml) group = GroupFinder.new(current_user).execute(id: group_id_for_saml)
...@@ -75,6 +86,12 @@ module EE ...@@ -75,6 +86,12 @@ module EE
issuer: params[:certificate_issuer]) issuer: params[:certificate_issuer])
end end
end end
def build_scim_identity(user)
scim_identity_params = params.slice(*scim_identity_attributes)
user.scim_identities.build(scim_identity_params.merge(active: true))
end
end end
end end
end end
...@@ -2,6 +2,8 @@ ...@@ -2,6 +2,8 @@
module API module API
class Scim < Grape::API class Scim < Grape::API
include ::Gitlab::Utils::StrongMemoize
prefix 'api/scim' prefix 'api/scim'
version 'v2' version 'v2'
content_type :json, 'application/scim+json' content_type :json, 'application/scim+json'
...@@ -19,16 +21,6 @@ module API ...@@ -19,16 +21,6 @@ module API
API.logger API.logger
end end
def destroy_identity(identity)
GroupSaml::Identity::DestroyService.new(identity).execute(transactional: true)
true
rescue => e
logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
false
end
def render_scim_error(error_class, message) def render_scim_error(error_class, message)
error!({ with: error_class }.merge(detail: message), error_class::STATUS) error!({ with: error_class }.merge(detail: message), error_class::STATUS)
end end
...@@ -69,25 +61,70 @@ module API ...@@ -69,25 +61,70 @@ module API
parsed_hash = parser.update_params parsed_hash = parser.update_params
if parser.deprovision_user? if parser.deprovision_user?
destroy_identity(identity) deprovision(identity)
elsif reprovisionable?(identity) && parser.reprovision_user?
reprovision(identity)
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) scim_conflict!(message: 'Email has already been taken') if email_taken?(parsed_hash[:email], identity)
result = ::Users::UpdateService.new(identity.user, result = ::Users::UpdateService.new(identity.user,
parsed_hash.except(:extern_uid) parsed_hash.except(:extern_uid, :active)
.merge(user: identity.user)).execute .merge(user: identity.user)).execute
result[:status] == :success result[:status] == :success
end end
end end
def reprovisionable?(identity)
return true if identity.respond_to?(:active) && !identity.active?
false
end
def email_taken?(email, identity) def email_taken?(email, identity)
return unless email return unless email
User.by_any_email(email.downcase).where.not(id: identity.user.id).exists? User.by_any_email(email.downcase).where.not(id: identity.user.id).exists?
end end
def find_user_identity(group, extern_uid)
return unless group.saml_provider
return group.scim_identities.with_extern_uid(extern_uid).first if scim_identities_enabled?
GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: extern_uid)
end
def scim_identities_enabled?
strong_memoize(:scim_identities_enabled) do
::EE::Gitlab::Scim::Feature.scim_identities_enabled?(@group)
end
end
def deprovision(identity)
if scim_identities_enabled?
::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
else
GroupSaml::Identity::DestroyService.new(identity).execute(transactional: true)
end
true
rescue => e
logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
false
end
def reprovision(identity)
::EE::Gitlab::Scim::ReprovisionService.new(identity).execute
true
rescue => e
logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
false
end
end end
resource :Users do resource :Users do
...@@ -118,7 +155,7 @@ module API ...@@ -118,7 +155,7 @@ module API
get ':id', requirements: USER_ID_REQUIREMENTS do get ':id', requirements: USER_ID_REQUIREMENTS do
group = find_and_authenticate_group!(params[:group]) group = find_and_authenticate_group!(params[:group])
identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id]) identity = find_user_identity(group, params[:id])
scim_not_found!(message: "Resource #{params[:id]} not found") unless identity scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
...@@ -154,7 +191,7 @@ module API ...@@ -154,7 +191,7 @@ module API
scim_error!(message: 'Missing ID') unless params[:id] scim_error!(message: 'Missing ID') unless params[:id]
group = find_and_authenticate_group!(params[:group]) group = find_and_authenticate_group!(params[:group])
identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id]) identity = find_user_identity(group, params[:id])
scim_not_found!(message: "Resource #{params[:id]} not found") unless identity scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
...@@ -174,11 +211,11 @@ module API ...@@ -174,11 +211,11 @@ module API
scim_error!(message: 'Missing ID') unless params[:id] scim_error!(message: 'Missing ID') unless params[:id]
group = find_and_authenticate_group!(params[:group]) group = find_and_authenticate_group!(params[:group])
identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id]) identity = find_user_identity(group, params[:id])
scim_not_found!(message: "Resource #{params[:id]} not found") unless identity scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
destroyed = destroy_identity(identity) destroyed = deprovision(identity)
scim_not_found!(message: "Resource #{params[:id]} not found") unless destroyed scim_not_found!(message: "Resource #{params[:id]} not found") unless destroyed
......
...@@ -29,9 +29,11 @@ module EE ...@@ -29,9 +29,11 @@ module EE
end end
def active def active
# We don't block the user yet when deprovisioning object_active = object.try(:active)
# So the user is always active, until the identity link is removed.
true return true if object_active.nil?
object_active
end end
def email_type def email_type
......
# frozen_string_literal: true
module EE
module Gitlab
module Scim
class DeprovisionService
attr_reader :identity
delegate :user, :group, to: :identity
def initialize(identity)
@identity = identity
end
def execute
ScimIdentity.transaction do
identity.update!(active: false)
remove_group_access
end
end
private
def remove_group_access
return unless group_membership
return if group.last_owner?(user)
::Members::DestroyService.new(user).execute(group_membership)
end
def group_membership
@group_membership ||= group.group_member(user)
end
end
end
end
end
# frozen_string_literal: true
module EE
module Gitlab
module Scim
class Feature
def self.scim_identities_enabled?(group)
::Feature.enabled?(:scim_identities, group)
end
end
end
end
end
...@@ -15,6 +15,10 @@ module EE ...@@ -15,6 +15,10 @@ module EE
update_params[:active] == false update_params[:active] == false
end end
def reprovision_user?
!deprovision_user?
end
def post_params def post_params
@post_params ||= process_post_params @post_params ||= process_post_params
end end
......
...@@ -6,7 +6,6 @@ module EE ...@@ -6,7 +6,6 @@ module EE
class ProvisioningService class ProvisioningService
include ::Gitlab::Utils::StrongMemoize include ::Gitlab::Utils::StrongMemoize
IDENTITY_PROVIDER = 'group_saml'
PASSWORD_AUTOMATICALLY_SET = true PASSWORD_AUTOMATICALLY_SET = true
SKIP_EMAIL_CONFIRMATION = false SKIP_EMAIL_CONFIRMATION = false
DEFAULT_ACCESS = :guest DEFAULT_ACCESS = :guest
...@@ -39,9 +38,27 @@ module EE ...@@ -39,9 +38,27 @@ module EE
ProvisioningResponse.new(status: :success, identity: identity) ProvisioningResponse.new(status: :success, identity: identity)
end end
def scim_identities_enabled?
strong_memoize(:scim_identities_enabled) do
::EE::Gitlab::Scim::Feature.scim_identities_enabled?(@group)
end
end
def identity_provider
strong_memoize(:identity_provider) do
next 'group_scim' if scim_identities_enabled?
'group_saml'
end
end
def identity def identity
strong_memoize(:identity) do strong_memoize(:identity) do
::Identity.with_extern_uid(IDENTITY_PROVIDER, @parsed_hash[:extern_uid]).first 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
end end
end end
...@@ -67,8 +84,9 @@ module EE ...@@ -67,8 +84,9 @@ module EE
def user_params def user_params
@parsed_hash.tap do |hash| @parsed_hash.tap do |hash|
hash[:skip_confirmation] = SKIP_EMAIL_CONFIRMATION hash[:skip_confirmation] = SKIP_EMAIL_CONFIRMATION
hash[:saml_provider_id] = @group.saml_provider.id hash[:saml_provider_id] = @group.saml_provider&.id
hash[:provider] = IDENTITY_PROVIDER hash[:group_id] = @group.id
hash[:provider] = identity_provider
hash[:email_confirmation] = hash[:email] hash[:email_confirmation] = hash[:email]
hash[:username] = valid_username hash[:username] = valid_username
hash[:password] = hash[:password_confirmation] = random_password hash[:password] = hash[:password_confirmation] = random_password
......
# frozen_string_literal: true
module EE
module Gitlab
module Scim
class ReprovisionService
attr_reader :identity
delegate :user, :group, to: :identity
DEFAULT_ACCESS = :guest
def initialize(identity)
@identity = identity
end
def execute
ScimIdentity.transaction do
identity.update!(active: true)
add_member unless existing_member?
end
end
private
def add_member
group.add_user(user, DEFAULT_ACCESS)
end
def existing_member?
::GroupMember.member_of_group?(group, user)
end
end
end
end
end
...@@ -55,6 +55,10 @@ describe ScimFinder do ...@@ -55,6 +55,10 @@ describe ScimFinder do
it 'allows lookup by userName' do it 'allows lookup by userName' do
expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id
end end
it 'allows lookup by userName when userName is an email address' do
expect(finder.search(filter: "userName eq #{id.user.email}").first).to eq id
end
end end
context 'when scim_identities is disabled' do context 'when scim_identities is disabled' do
......
...@@ -39,4 +39,14 @@ describe ::EE::API::Entities::Scim::User do ...@@ -39,4 +39,14 @@ describe ::EE::API::Entities::Scim::User do
it 'contains the resource type' do it 'contains the resource type' do
expect(subject[:meta][:resourceType]).to eq('User') expect(subject[:meta][:resourceType]).to eq('User')
end end
context 'with a SCIM identity' do
let(:identity) { build(:scim_identity, user: user) }
it 'contains active false when the identity is not active' do
identity.active = false
expect(subject[:active]).to be false
end
end
end end
# frozen_string_literal: true
require 'spec_helper'
describe ::EE::Gitlab::Scim::DeprovisionService do
describe '#execute' do
let_it_be(:identity) { create(:scim_identity, active: true) }
let_it_be(:group) { identity.group }
let_it_be(:user) { identity.user }
let(:service) { described_class.new(identity) }
it 'deactivates scim identity' do
service.execute
expect(identity.active).to be false
end
it 'removes group access' do
create(:group_member, group: group, user: user, access_level: GroupMember::REPORTER)
service.execute
expect(group.members.pluck(:user_id)).not_to include(user.id)
end
it 'does not remove the last owner' do
create(:group_member, group: group, user: user, access_level: GroupMember::OWNER)
service.execute
expect(identity.group.members.pluck(:user_id)).to include(user.id)
end
it 'does not change group membership when the user is not a member' do
expect { service.execute }.not_to change { group.members.count }
end
end
end
...@@ -98,4 +98,18 @@ describe EE::Gitlab::Scim::ParamsParser do ...@@ -98,4 +98,18 @@ describe EE::Gitlab::Scim::ParamsParser do
expect(described_class.new(Operations: operations).deprovision_user?).to be false expect(described_class.new(Operations: operations).deprovision_user?).to be false
end end
end end
describe '#reprovision_user?' do
it 'returns true when reprovisioning' do
operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'True' }]
expect(described_class.new(Operations: operations).reprovision_user?).to be true
end
it 'returns false when not reprovisioning' do
operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }]
expect(described_class.new(Operations: operations).reprovision_user?).to be false
end
end
end end
...@@ -9,86 +9,138 @@ describe ::EE::Gitlab::Scim::ProvisioningService do ...@@ -9,86 +9,138 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
before do before do
stub_licensed_features(group_saml: true) stub_licensed_features(group_saml: true)
create(:saml_provider, group: group)
end end
context 'valid params' do shared_examples 'scim provisioning' do
let(:service_params) do context 'valid params' do
{ let_it_be(:service_params) do
email: 'work@example.com', {
name: 'Test Name', email: 'work@example.com',
extern_uid: 'test_uid', name: 'Test Name',
username: 'username' extern_uid: 'test_uid',
}.freeze username: 'username'
end }
end
it 'succeeds' do def user
expect(service.execute.status).to eq(:success) User.find_by(email: service_params[:email])
end end
it 'creates the identity' do it 'succeeds' do
expect { service.execute }.to change { Identity.count }.by(1) expect(service.execute.status).to eq(:success)
end end
it 'creates the user' do it 'creates the user' do
expect { service.execute }.to change { User.count }.by(1) expect { service.execute }.to change { User.count }.by(1)
end end
it 'creates the group member' do it 'creates the group member' do
expect { service.execute }.to change { GroupMember.count }.by(1) expect { service.execute }.to change { GroupMember.count }.by(1)
end end
it 'creates the correct user attributes' do it 'creates the correct user attributes' do
service.execute service.execute
expect(User.find_by(service_params.except(:extern_uid))).to be_a(User) expect(user).to be_a(User)
end end
it 'user record requires confirmation' do it 'creates the member with guest access level' do
service.execute service.execute
user = User.find_by(email: service_params[:email]) access_level = group.group_member(user).access_level
expect(user).to be_present expect(access_level).to eq(Gitlab::Access::GUEST)
expect(user).not_to be_confirmed end
end
context 'when the current minimum password length is different from the default minimum password length' do it 'user record requires confirmation' do
before do service.execute
stub_application_setting minimum_password_length: 21
expect(user).to be_present
expect(user).not_to be_confirmed
end end
it 'creates the user' do context 'when the current minimum password length is different from the default minimum password length' do
expect { service.execute }.to change { User.count }.by(1) before do
stub_application_setting minimum_password_length: 21
end
it 'creates the user' do
expect { service.execute }.to change { User.count }.by(1)
end
end end
end
context 'existing user' do context 'existing user' do
before do before do
create(:user, email: 'work@example.com') 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
end
it 'does not create a new user' do context 'invalid params' do
expect { service.execute }.not_to change { User.count } let_it_be(:service_params) do
{
email: 'work@example.com',
name: 'Test Name',
extern_uid: 'test_uid'
}
end end
it 'fails with conflict' do it 'fails with error' do
expect(service.execute.status).to eq(:conflict) expect(service.execute.status).to eq(:error)
end end
end end
end end
context 'invalid params' do context 'when scim_identities is disabled' do
let(:service_params) do before do
stub_feature_flags(scim_identities: false)
create(:saml_provider, group: group)
end
let_it_be(:service_params) do
{ {
email: 'work@example.com', email: 'work@example.com',
name: 'Test Name', name: 'Test Name',
extern_uid: 'test_uid' extern_uid: 'test_uid',
}.freeze username: 'username'
}
end end
it 'fails with error' do it_behaves_like 'scim provisioning'
expect(service.execute.status).to eq(:error)
it 'creates the identity' do
expect { service.execute }.to change { Identity.count }.by(1)
expect { service.execute }.not_to change { ScimIdentity.count }
end
end
context 'when scim_identities is enabled' do
before do
stub_feature_flags(scim_identities: true)
end
let_it_be(:service_params) do
{
email: 'work@example.com',
name: 'Test Name',
extern_uid: 'test_uid',
username: 'username'
}
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 end
end end
end end
......
# frozen_string_literal: true
require 'spec_helper'
describe ::EE::Gitlab::Scim::ReprovisionService do
describe '#execute' do
let_it_be(:identity) { create(:scim_identity, active: false) }
let_it_be(:group) { identity.group }
let_it_be(:user) { identity.user }
let(:service) { described_class.new(identity) }
it 'activates scim identity' do
service.execute
expect(identity.active).to be true
end
it 'creates the member' do
service.execute
expect(group.members.pluck(:user_id)).to include(user.id)
end
it 'creates the member with guest access level' do
service.execute
access_level = group.group_member(user).access_level
expect(access_level).to eq(Gitlab::Access::GUEST)
end
it 'does not change group membership when the user is already a member' do
create(:group_member, group: group, user: user)
expect { service.execute }.not_to change { group.members.count }
end
end
end
This diff is collapsed.
...@@ -23,6 +23,23 @@ describe Users::BuildService do ...@@ -23,6 +23,23 @@ describe Users::BuildService do
it 'sets all allowed attributes' do it 'sets all allowed attributes' do
expect(Identity).to receive(:new).with(hash_including(identity_params)).and_call_original expect(Identity).to receive(:new).with(hash_including(identity_params)).and_call_original
expect(ScimIdentity).not_to receive(:new)
service.execute
end
end
context 'with scim identity' do
before do
params.merge!(scim_identity_params)
end
let_it_be(:scim_identity_params) { { extern_uid: 'uid', provider: 'group_scim', group_id: 1 } }
it 'passes allowed attributes to scim identity' do
scim_identity_params.delete(:provider)
expect(ScimIdentity).to receive(:new).with(hash_including(scim_identity_params)).and_call_original
expect(Identity).not_to receive(:new)
service.execute service.execute
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