diff --git a/changelogs/unreleased/dblessing_okta_scim.yml b/changelogs/unreleased/dblessing_okta_scim.yml new file mode 100644 index 0000000000000000000000000000000000000000..16baae420040f5c8318b3d6c66e935ab82720d22 --- /dev/null +++ b/changelogs/unreleased/dblessing_okta_scim.yml @@ -0,0 +1,5 @@ +--- +title: Add support for Okta as a SCIM provider +merge_request: 25649 +author: +type: added diff --git a/ee/app/finders/scim_finder.rb b/ee/app/finders/scim_finder.rb index 6f0601b8b5c1533b88e394e484db7d7ef24a306a..2c93e0e0f9fe6766e0bb641d90448648f061d93f 100644 --- a/ee/app/finders/scim_finder.rb +++ b/ee/app/finders/scim_finder.rb @@ -49,6 +49,9 @@ def filter_identities(params) if eq_filter_on_extern_uid?(parser) by_extern_uid(parser.filter_params[:extern_uid]) elsif eq_filter_on_username?(parser) + identity = by_extern_uid(parser.filter_params[:username]) + return identity if identity.present? + by_username(parser.filter_params[:username]) else raise UnsupportedFilter diff --git a/ee/lib/ee/api/entities/scim/user.rb b/ee/lib/ee/api/entities/scim/user.rb index e07df7a1c37a3e870e49537eebcafd48a82978c9..4c6e13c928fa4113a37041853a10dde41c3507a2 100644 --- a/ee/lib/ee/api/entities/scim/user.rb +++ b/ee/lib/ee/api/entities/scim/user.rb @@ -10,9 +10,10 @@ class User < Grape::Entity expose :active expose :email_user, as: :emails, using: ::EE::API::Entities::Scim::Emails - expose 'name.formatted' do |identity, _options| - identity.user.name + expose :name, using: ::EE::API::Entities::Scim::UserName do |identity, _options| + identity.user end + expose :meta do expose :resource_type, as: :resourceType end diff --git a/ee/lib/ee/api/entities/scim/user_name.rb b/ee/lib/ee/api/entities/scim/user_name.rb new file mode 100644 index 0000000000000000000000000000000000000000..2021124efc6360bc907dfe6849200a85263a328f --- /dev/null +++ b/ee/lib/ee/api/entities/scim/user_name.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module EE + module API + module Entities + module Scim + class UserName < Grape::Entity + expose :name, as: :formatted + expose :first_name, as: :givenName + expose :last_name, as: :familyName + end + end + end + end +end diff --git a/ee/lib/ee/gitlab/scim/params_parser.rb b/ee/lib/ee/gitlab/scim/params_parser.rb index 7b1b5d6f7d004ec4cc274b085a05b32109d5395d..0ca94d1d7157ba2a6c462dd7e8e7ac152b1f4c9d 100644 --- a/ee/lib/ee/gitlab/scim/params_parser.rb +++ b/ee/lib/ee/gitlab/scim/params_parser.rb @@ -4,7 +4,7 @@ module EE module Gitlab module Scim class ParamsParser - OPERATIONS_OPERATORS = %w[Replace Add].freeze + OPERATIONS_OPERATORS = %w[replace add].freeze def initialize(params) @params = params.with_indifferent_access @@ -43,9 +43,9 @@ def filter_parser def process_operations @params[:Operations].each_with_object({}) do |operation, hash| - next unless OPERATIONS_OPERATORS.include?(operation[:op]) + next unless OPERATIONS_OPERATORS.include?(operation[:op].downcase) - hash.merge!(AttributeTransform.new(operation[:path]).map_to(operation[:value])) + hash.merge!(transformed_operation(operation)) end end @@ -76,6 +76,19 @@ def parse_name formatted_name ||= [name[:givenName], name[:familyName]].compact.join(' ') @hash[:name] = formatted_name end + + # The `path` key is optional per the SCIM spec. + # Ex. Azure uses `path` while Okta does not. + def transformed_operation(operation) + key, value = + if operation[:path] + operation.values_at(:path, :value) + else + [operation[:value].each_key.first, operation[:value].each_value.first] + end + + AttributeTransform.new(key).map_to(value) + end end end end diff --git a/ee/spec/finders/scim_finder_spec.rb b/ee/spec/finders/scim_finder_spec.rb index a628dd7404a620b7457ced45bff92e41c8ee654b..9a444d7a78e87765660587152d6201c81b7b62f7 100644 --- a/ee/spec/finders/scim_finder_spec.rb +++ b/ee/spec/finders/scim_finder_spec.rb @@ -56,8 +56,18 @@ expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id 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 + context 'allows lookup by userName' do + it 'finds user when userName is an email address' do + expect(finder.search(filter: "userName eq #{id.user.email}").first).to eq id + end + + it 'finds user by username' do + expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id + end + + it 'finds user by extern_uid' do + expect(finder.search(filter: "userName eq \"#{id.extern_uid}\"").first).to eq id + end end end diff --git a/ee/spec/lib/ee/api/entities/scim/user_name_spec.rb b/ee/spec/lib/ee/api/entities/scim/user_name_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..c1b1bb6c86e79ff5ec03b52b54ec2fc9b680373c --- /dev/null +++ b/ee/spec/lib/ee/api/entities/scim/user_name_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::EE::API::Entities::Scim::UserName do + let(:user) { build(:user) } + + subject { described_class.new(user).as_json } + + it 'contains the name' do + expect(subject[:formatted]).to eq(user.name) + end + + it 'contains the first name' do + expect(subject[:givenName]).to eq(user.first_name) + end + + it 'contains the last name' do + expect(subject[:familyName]).to eq(user.last_name) + end +end diff --git a/ee/spec/lib/ee/api/entities/scim/user_spec.rb b/ee/spec/lib/ee/api/entities/scim/user_spec.rb index 65f4813c9a8a0c8f7938fc7d6da5c7071ef3459d..abdef633cf0caedc0b44861260d7520b146b57f6 100644 --- a/ee/spec/lib/ee/api/entities/scim/user_spec.rb +++ b/ee/spec/lib/ee/api/entities/scim/user_spec.rb @@ -25,7 +25,15 @@ end it 'contains the name' do - expect(subject[:'name.formatted']).to eq(user.name) + expect(subject[:name][:formatted]).to eq(user.name) + end + + it 'contains the first name' do + expect(subject[:name][:givenName]).to eq(user.first_name) + end + + it 'contains the last name' do + expect(subject[:name][:familyName]).to eq(user.last_name) end it 'contains the email' do diff --git a/ee/spec/lib/ee/gitlab/scim/params_parser_spec.rb b/ee/spec/lib/ee/gitlab/scim/params_parser_spec.rb index 29a8f3dacbe2c5088e02760dfdcb524af05f093c..5d0942d91fc77687f99ef8a55a1773ad71e875b8 100644 --- a/ee/spec/lib/ee/gitlab/scim/params_parser_spec.rb +++ b/ee/spec/lib/ee/gitlab/scim/params_parser_spec.rb @@ -32,22 +32,56 @@ end describe '#update_params' do - it 'returns the correct operation attributes' do - operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] + shared_examples 'scim operation active false' do + it 'returns the correct operation attributes' do + expect(described_class.new(Operations: operations).update_params).to eq(active: false) + end + end + + shared_examples 'scim operation empty' do + it 'returns an empty hash for the wrong operations' do + expect(described_class.new(Operations: operations).update_params).to eq({}) + end + end - expect(described_class.new(Operations: operations).update_params).to eq(active: false) + shared_examples 'scim operation update name' do + it 'can update name from displayName' do + expect(described_class.new(Operations: operations).update_params).to include(name: 'My Name Is') + end end - it 'returns an empty hash for the wrong operations' do - operations = [{ 'op': 'Replace', 'path': 'test', 'value': 'False' }] + context 'when path key is present' do + it_behaves_like 'scim operation active false' do + let(:operations) { [{ 'op': 'replace', 'path': 'active', 'value': 'False' }] } + end - expect(described_class.new(Operations: operations).update_params).to eq({}) + it_behaves_like 'scim operation empty' do + let(:operations) { [{ 'op': 'replace', 'path': 'test', 'value': 'False' }] } + end + + it_behaves_like 'scim operation update name' do + let(:operations) { [{ 'op': 'replace', 'path': 'displayName', 'value': 'My Name Is' }] } + end end - it 'can update name from displayName' do - operations = [{ 'op': 'Replace', 'path': 'displayName', 'value': 'My Name Is' }] + context 'when path key is not present' do + it_behaves_like 'scim operation active false' do + let(:operations) { [{ 'op': 'replace', 'value': { 'active': false } }] } + end + + it_behaves_like 'scim operation empty' do + let(:operations) { [{ 'op': 'replace', 'value': { 'test': false } }] } + end - expect(described_class.new(Operations: operations).update_params).to include(name: 'My Name Is') + it_behaves_like 'scim operation update name' do + let(:operations) { [{ 'op': 'replace', 'value': { 'displayName': 'My Name Is' } }] } + end + end + + context 'with capitalized op values for Azure' do + it_behaves_like 'scim operation active false' do + let(:operations) { [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] } + end end end @@ -87,27 +121,33 @@ describe '#deprovision_user?' do it 'returns true when deprovisioning' do - operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] + operations = [{ 'op': 'replace', 'path': 'active', 'value': 'False' }] expect(described_class.new(Operations: operations).deprovision_user?).to be true end it 'returns false when not deprovisioning' do - operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'True' }] + operations = [{ 'op': 'replace', 'path': 'active', 'value': 'True' }] expect(described_class.new(Operations: operations).deprovision_user?).to be false end + + it 'returns true when deprovisioning without a path key' do + operations = [{ 'op': 'replace', 'value': { 'active': false } }] + + expect(described_class.new(Operations: operations).deprovision_user?).to be true + end end describe '#reprovision_user?' do it 'returns true when reprovisioning' do - operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'True' }] + 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' }] + operations = [{ 'op': 'replace', 'path': 'active', 'value': 'False' }] expect(described_class.new(Operations: operations).reprovision_user?).to be false end diff --git a/ee/spec/requests/api/scim_spec.rb b/ee/spec/requests/api/scim_spec.rb index eb19162f5a1ca7d154da74e9f4cbaf6f4372bd8d..bf97ac0be07c54f7713968d5323db72eb4c668a4 100644 --- a/ee/spec/requests/api/scim_spec.rb +++ b/ee/spec/requests/api/scim_spec.rb @@ -277,18 +277,6 @@ def scim_api(url, token: true) end end end - - context 'Remove user' do - before do - params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query - - patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}") - end - - it 'responds with 204' do - expect(response).to have_gitlab_http_status(:no_content) - end - end end end @@ -375,12 +363,34 @@ def scim_api(url, token: true) describe 'PATCH api/scim/v2/groups/:group/Users/:id' do context 'Remove user' do - it 'removes the identity link' do - params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query + shared_examples 'remove user' do + it 'responds with 204' do + expect(response).to have_gitlab_http_status(:no_content) + end - patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}") + it 'removes the identity link' do + expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + end - expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound) + context 'when path key is present' do + before do + params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query + + patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}") + end + + it_behaves_like 'remove user' + end + + context 'when the path key is not present' do + before do + params = { Operations: [{ 'op': 'replace', 'value': { 'active': false } }] }.to_query + + patch scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}?#{params}") + end + + it_behaves_like 'remove user' end end end