mirror of
https://github.com/opf/openproject.git
synced 2026-06-14 03:30:14 +00:00
Merge pull request #22514 from cholarajaa/bug/73431-scim-user-api-return-duplicate-records
[73431] SCIM user api return duplicate records
This commit is contained in:
Pavel Balashou
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user