From 33a83f7a7e979a2fd0ceb9a4ff948332c0a723f0 Mon Sep 17 00:00:00 2001 From: Madhu Reddy Date: Thu, 26 Mar 2026 12:32:32 +0530 Subject: [PATCH 1/5] [#73431] SCIM User API return duplicate records https://community.openproject.org/work_packages/73431 From a87bb81179afca26b602050e62e52eb02bdb194b Mon Sep 17 00:00:00 2001 From: Madhu Reddy Date: Thu, 26 Mar 2026 13:48:02 +0530 Subject: [PATCH 2/5] [#73431] Fix SCIM v2 user API returning duplicate records The index action on the SCIM users endpoint was returning duplicate records when a user belonged to multiple groups or had multiple auth provider links. This happened because storage_scope uses left_joins, which produces one row per join match. Added .distinct to the index query to eliminate duplicates. Also extracted scim_index_storage_query and scim_all_attribute_names into their own private methods for better readability. --- .../scim_v2/base_controller_actions.rb | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/app/controllers/scim_v2/base_controller_actions.rb b/app/controllers/scim_v2/base_controller_actions.rb index 01b3448478c..0280aa8061c 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 @@ -73,21 +67,39 @@ module ScimV2 end end + # Builds the base query for the SCIM index action, + # applying any SCIM filter params if present. 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) + 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) From 1f7146b2fad098deea7d792915298605727d8bc7 Mon Sep 17 00:00:00 2001 From: Madhu Reddy Date: Thu, 26 Mar 2026 13:50:32 +0530 Subject: [PATCH 3/5] [#73431] Add tests for duplicate SCIM v2 user results Adds request specs for the SCIM v2 users index endpoint to make sure each user is returned only once, even with multiple group memberships or auth provider links. Covers the duplicate-record regression caused by joined relations. --- spec/requests/scim_v2/groups_spec.rb | 208 +++++++++++++++------------ spec/requests/scim_v2/users_spec.rb | 20 ++- 2 files changed, 135 insertions(+), 93 deletions(-) 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, From ea36ce0ac4636a8581133a29fafbe7d54c37e91a Mon Sep 17 00:00:00 2001 From: Madhu Reddy Date: Thu, 26 Mar 2026 18:52:31 +0530 Subject: [PATCH 4/5] [#73431] update username From 7890b631ccdb4b9b8a993fa09667221048335cdd Mon Sep 17 00:00:00 2001 From: Madhu Reddy <7227078+cholarajaa@users.noreply.github.com> Date: Thu, 26 Mar 2026 22:40:50 +0530 Subject: [PATCH 5/5] [#73431] move comment to appropriate line --- app/controllers/scim_v2/base_controller_actions.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/scim_v2/base_controller_actions.rb b/app/controllers/scim_v2/base_controller_actions.rb index 0280aa8061c..0617022889d 100644 --- a/app/controllers/scim_v2/base_controller_actions.rb +++ b/app/controllers/scim_v2/base_controller_actions.rb @@ -67,10 +67,10 @@ module ScimV2 end end - # Builds the base query for the SCIM index action, - # applying any SCIM filter params if present. private + # 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?