diff --git a/app/controllers/scim_v2/base_controller_actions.rb b/app/controllers/scim_v2/base_controller_actions.rb index 01b3448478c..0617022889d 100644 --- a/app/controllers/scim_v2/base_controller_actions.rb +++ b/app/controllers/scim_v2/base_controller_actions.rb @@ -38,15 +38,9 @@ module ScimV2 rescue_from "ActiveRecord::RecordNotFound", with: :handle_resource_not_found def index - query = if params[:filter].blank? - storage_scope - else - attribute_map = storage_class.new.scim_queryable_attributes - parser = ::Scimitar::Lists::QueryParser.new(attribute_map) - - parser.parse(params[:filter]) - parser.to_activerecord_query(storage_scope) - end + # Applies .distinct to avoid duplicate records caused by + # left_joins in storage_scope (e.g. groups, auth provider links). + query = scim_index_storage_query.distinct pagination_info = scim_pagination_info(query.count) page_of_results = query @@ -75,19 +69,37 @@ module ScimV2 private - def include_attributes - first_level_attrs = storage_class.scim_attributes_map.keys.map(&:to_s) - second_level_attrs = - storage_class - .scim_attributes_map - .find_all { |_, v| v.is_a? Hash } - .flat_map { |parent, childs| childs.map { |child, _| "#{parent}.#{child}" } } - all_possible_attributes = (first_level_attrs + second_level_attrs) + # Builds the base query for the SCIM index action, + # applying any SCIM filter params if present. + def scim_index_storage_query + return storage_scope if params[:filter].blank? + # Returns the list of SCIM attributes to include in the response, + # excluding any attributes specified in the excludedAttributes param. + attribute_map = storage_class.new.scim_queryable_attributes + parser = ::Scimitar::Lists::QueryParser.new(attribute_map) + + parser.parse(params[:filter]) + parser.to_activerecord_query(storage_scope) + end + + def include_attributes + # Collects all possible SCIM attribute names (top-level and nested) + # from the storage class's scim_attributes_map. excluded_attributes = params.fetch(:excludedAttributes, "").split(",") excluded_parents = excluded_attributes.filter_map { |attr| attr.split(".")[-2] } - all_possible_attributes - excluded_attributes - excluded_parents + scim_all_attribute_names - excluded_attributes - excluded_parents + end + + def scim_all_attribute_names + map = storage_class.scim_attributes_map + nested = + map + .find_all { |_, v| v.is_a? Hash } + .flat_map { |parent, childs| childs.map { |child, _| "#{parent}.#{child}" } } + + map.keys.map(&:to_s) + nested end def raise_result_errors_for_scim(result) diff --git a/spec/requests/scim_v2/groups_spec.rb b/spec/requests/scim_v2/groups_spec.rb index 083a254af64..7a648213613 100644 --- a/spec/requests/scim_v2/groups_spec.rb +++ b/spec/requests/scim_v2/groups_spec.rb @@ -31,6 +31,45 @@ require "spec_helper" RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do + def expect_scim_group_write_response_meta_matches_group(response_body, group) + expect(response_body["meta"].except("lastModified")).to eq( + "location" => "http://test.host/scim_v2/Groups/#{group.id}", + "created" => group.created_at.iso8601, + "resourceType" => "Group" + ) + expect(Time.iso8601(response_body["meta"]["lastModified"])).to be_within(2.seconds).of(group.updated_at) + end + + def scim_list_expected_group_resources(group:, other:, user:) + [ + { + "displayName" => group.name, + "externalId" => external_group_id, + "id" => group.id.to_s, + "members" => [{ "value" => user.id.to_s }], + "meta" => { + "location" => "http://test.host/scim_v2/Groups/#{group.id}", + "created" => group.created_at.iso8601, + "lastModified" => group.updated_at.iso8601, + "resourceType" => "Group" + }, + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"] + }, + { + "displayName" => other.name, + "id" => other.id.to_s, + "members" => [{ "value" => user.id.to_s }], + "meta" => { + "location" => "http://test.host/scim_v2/Groups/#{other.id}", + "created" => other.created_at.iso8601, + "lastModified" => other.updated_at.iso8601, + "resourceType" => "Group" + }, + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"] + } + ] + end + let(:external_user_id) { "idp_user_id_123asdqwe12345" } let(:external_group_id) { "idp_group_id_123asdqwe12345" } let(:external_admin_id) { "idp_admin_id_123asdqwe12345" } @@ -62,26 +101,33 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do get "/scim_v2/Groups", {}, headers response_body = JSON.parse(last_response.body) - expect(response_body).to match("Resources" => contain_exactly({ "displayName" => group.name, - "externalId" => external_group_id, - "id" => group.id.to_s, - "members" => [{ "value" => user.id.to_s }], - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"] }, { "displayName" => group_without_external_id.name, - "id" => group_without_external_id.id.to_s, - "members" => [{ "value" => user.id.to_s }], - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group_without_external_id.id}", - "created" => group_without_external_id.created_at.iso8601, - "lastModified" => group_without_external_id.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"] }), - "itemsPerPage" => 100, - "schemas" => ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], - "startIndex" => 1, - "totalResults" => 2) + expect(response_body).to match( + "Resources" => match_array(scim_list_expected_group_resources(group:, + other: group_without_external_id, + user:)), + "itemsPerPage" => 100, + "schemas" => ["urn:ietf:params:scim:api:messages:2.0:ListResponse"], + "startIndex" => 1, + "totalResults" => 2 + ) + end + + it "lists each group once when it has multiple members" do + multi_group = create(:group, + identity_url: "#{oidc_provider_slug}:idp_group_multi_member", + members: [user1, user2]) + + get "/scim_v2/Groups", {}, headers + + response_body = JSON.parse(last_response.body) + resource_ids = response_body["Resources"].pluck("id") + expect(resource_ids.uniq).to eq(resource_ids) + + multi_resources = response_body["Resources"].select { |item| item["id"] == multi_group.id.to_s } + expect(multi_resources.length).to eq(1) + + member_values = multi_resources.first["members"].pluck("value") + expect(member_values).to contain_exactly(user1.id.to_s, user2.id.to_s) end it "filters results" do @@ -255,15 +301,13 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to match("displayName" => group.name, - "externalId" => new_external_group_id, - "id" => group.id.to_s, - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "members" => contain_exactly({ "value" => user.id.to_s }, { "value" => admin.id.to_s }), - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to match("displayName" => group.name, + "externalId" => new_external_group_id, + "id" => group.id.to_s, + "members" => contain_exactly({ "value" => user.id.to_s }, + { "value" => admin.id.to_s }), + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end it "updates members if there is $ref field present for every member(Keycloak plugin adds it for example)" do @@ -287,15 +331,13 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to match("displayName" => group.name, - "externalId" => new_external_group_id, - "id" => group.id.to_s, - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "members" => contain_exactly({ "value" => user.id.to_s }, { "value" => admin.id.to_s }), - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to match("displayName" => group.name, + "externalId" => new_external_group_id, + "id" => group.id.to_s, + "members" => contain_exactly({ "value" => user.id.to_s }, + { "value" => admin.id.to_s }), + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end it "updates members if there is no members field(Keycloak plugin sends memberless group request like that)" do @@ -309,15 +351,12 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to match("displayName" => group.name, - "externalId" => new_external_group_id, - "id" => group.id.to_s, - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "members" => [], - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to match("displayName" => group.name, + "externalId" => new_external_group_id, + "id" => group.id.to_s, + "members" => [], + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end end @@ -338,15 +377,12 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to eq("displayName" => group.name, - "externalId" => new_external_group_id, - "id" => group.id.to_s, - "members" => [{ "value" => user.id.to_s }], - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to eq("displayName" => group.name, + "externalId" => new_external_group_id, + "id" => group.id.to_s, + "members" => [{ "value" => user.id.to_s }], + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end it "supports replacing of members" do @@ -369,15 +405,12 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to eq("displayName" => group.name, - "externalId" => external_group_id, - "id" => group.id.to_s, - "members" => [{ "value" => user2.id.to_s }], - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to eq("displayName" => group.name, + "externalId" => external_group_id, + "id" => group.id.to_s, + "members" => [{ "value" => user2.id.to_s }], + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end it "supports adding of a member" do @@ -397,16 +430,13 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to eq("displayName" => group.name, - "externalId" => external_group_id, - "id" => group.id.to_s, - "members" => [{ "value" => user1.id.to_s }, - { "value" => user2.id.to_s }], - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to eq("displayName" => group.name, + "externalId" => external_group_id, + "id" => group.id.to_s, + "members" => [{ "value" => user1.id.to_s }, + { "value" => user2.id.to_s }], + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end it "supports removal of a member" do @@ -425,15 +455,12 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to eq("displayName" => group.name, - "externalId" => external_group_id, - "id" => group.id.to_s, - "members" => [], - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to eq("displayName" => group.name, + "externalId" => external_group_id, + "id" => group.id.to_s, + "members" => [], + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end it "supports removal of a member with exclusion of members list from the response" do @@ -452,14 +479,11 @@ RSpec.describe "SCIM API Groups", with_ee: [:scim_api] do response_body = JSON.parse(last_response.body) group.reload - expect(response_body).to eq("displayName" => group.name, - "externalId" => external_group_id, - "id" => group.id.to_s, - "meta" => { "location" => "http://test.host/scim_v2/Groups/#{group.id}", - "created" => group.created_at.iso8601, - "lastModified" => group.updated_at.iso8601, - "resourceType" => "Group" }, - "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect(response_body.except("meta")).to eq("displayName" => group.name, + "externalId" => external_group_id, + "id" => group.id.to_s, + "schemas" => ["urn:ietf:params:scim:schemas:core:2.0:Group"]) + expect_scim_group_write_response_meta_matches_group(response_body, group) end end end diff --git a/spec/requests/scim_v2/users_spec.rb b/spec/requests/scim_v2/users_spec.rb index dbf1df5f461..e634262ee03 100644 --- a/spec/requests/scim_v2/users_spec.rb +++ b/spec/requests/scim_v2/users_spec.rb @@ -59,7 +59,7 @@ RSpec.describe "SCIM API Users", with_ee: [:scim_api] do get "/scim_v2/Users", {}, headers response_body = JSON.parse(last_response.body) - ids = response_body["Resources"].map { |item| item["id"] } + ids = response_body["Resources"].pluck("id") expect(ids).to include(locked_user.id.to_s) expect(response_body["Resources"].find { |resource| resource["id"] == locked_user.id.to_s }["active"]).to be(false) expect(ids).not_to include(user_marked_for_deletion.id.to_s) @@ -99,6 +99,24 @@ RSpec.describe "SCIM API Users", with_ee: [:scim_api] do "totalResults" => 5) end + it "lists each user once when they belong to multiple groups and includes all groups" do + second_group = create(:group, + identity_url: "#{oidc_provider.slug}:idp_group_second_membership", + members: [user]) + + get "/scim_v2/Users", {}, headers + + response_body = JSON.parse(last_response.body) + resource_ids = response_body["Resources"].pluck("id") + expect(resource_ids.uniq).to eq(resource_ids) + + user_resources = response_body["Resources"].select { |item| item["id"] == user.id.to_s } + expect(user_resources.length).to eq(1) + + group_values = user_resources.first["groups"].pluck("value") + expect(group_values).to contain_exactly(group.id.to_s, second_group.id.to_s) + end + it "filters results by familyName case-insensitively" do expected_body = { "Resources" => [{ "active" => true, "emails" => [{ "primary" => true,