VCST-5239: Added Organization-scoped roles

[DmitryGrishinVirtoworks]

Description

References

QA-test:

Jira-link:

https://virtocommerce.atlassian.net/browse/VCST-5239

Artifact URL:

https://vc3prerelease.blob.core.windows.net/packages/VirtoCommerce.ProfileExperienceApiModule_3.1010.0-pr-137-ac25.zip

[alexeyshibanov]

Request changes — VCST-5239 (profile experience API).

Good improvements: role resolution now iterates all security accounts (not [0]), and the org+membership role union is exposed correctly. Two things must be resolved before merge — the alpha dependency pin and the global-role filter scalability — plus a behavior note and an N+1. Inline below.

[alexeyshibanov]

Per-row resolver / N+1: for each contact (× each security account) this does an org GetByIdAsync (cached, same org) + a membership search. Over a 20-row connection that's un-batched. Caches bound it, but consider a DataLoader keyed by orgId to batch role resolution across the page.

[DmitryGrishinVirtoworks]

Good point. I'd prefer to address this in a separate PR to keep the scope focused - does it need to be fixed here, or is a follow-up acceptable?

[alexeyshibanov]

Follow-up isn't justified here — the fix is local and the batch primitive already exists in this PR, so let's keep it in-PR.

Why it's cheap now: you already added GetRolesForUsersInOrgAsync(userIds, orgId) → IDictionary<userId, roles> on the search service — that's exactly a batch-loader fetch signature. So this is a standard graphql-dotnet DataLoader (analog: XCatalog ProductType.isConfigurable → GetOrAddBatchLoader<string,bool>). IDataLoaderContextAccessor is DI-registered by Xapi (.AddDataLoader() in Xapi.Web/Module.cs) — inject it into the ctor.

rolesInOrganization:

var loader = dataLoader.Context.GetOrAddBatchLoader<string, IReadOnlyCollection<Role>>(
    $"contact_rolesInOrg_{organizationId}",               // orgId in the key — it's a field argument
    async ids => (await svc.GetRolesForUsersInOrgAsync(ids.ToList(), organizationId))
        .ToDictionary(kv => kv.Key,
            kv => (IReadOnlyCollection<Role>)kv.Value
                .Select(r => new Role { Id = r.RoleId, Name = r.RoleName }).ToList()));

return loader.LoadAsync(userIds)
    .Then(perAccount => perAccount.SelectMany(x => x ?? []).DistinctBy(r => r.Id).ToList());

isLockedInOrganization (no new service method — the criteria already has UserIds + OnlyLocked):

var loader = dataLoader.Context.GetOrAddBatchLoader<string, bool>(
    $"contact_lockedInOrg_{organizationId}",
    async ids =>
    {
        var locked = await svc.SearchAllNoCloneAsync(new OrganizationMembershipSearchCriteria
            { OrganizationId = organizationId, UserIds = ids.ToList(), OnlyLocked = true });
        return locked.GroupBy(m => m.UserId).ToDictionary(g => g.Key, _ => true);   // only the locked ones
    });

return loader.LoadAsync(userIds).Then(flags => flags.Any(x => x));                  // missing → false

Notes:

• The fetch returns a partial dict — missing keys resolve to default (null/false), no backfill needed. (See x-cart GetCartProductDataLoader, which returns response.Products.ToDictionary(x => x.Id) with no backfill and a null default.)
• Keep the loader keyed by userId (that's what batches across the whole page); the per-contact union over the contact's own SecurityAccounts stays in .Then because the batch key (userId) is finer than the field's granularity (contact). Mapping OrganizationRole → Role lives in the loader (GraphQL layer), so .Then is just the union.
• GroupBy(...).ToDictionary(...) guards against duplicate (userId, orgId) rows.
• Add a component test asserting the fetch runs once for a page of N contacts (Times.Once with the union of userIds) — that's the batch proof, and the main reason a separate PR looked tempting; it's small.

Net: two DataLoaders + one ctor param + one test — small enough to land here rather than defer.

[alexeyshibanov]

Merge blocker. Pins VirtoCommerce.CustomerModule.Core to 3.1014.0-alpha.985-vcst-5239. The customer PR must merge and publish a stable Core package first, then re-pin this to the released version before merge. Same in module.manifest.

[alexeyshibanov]

Same alpha dependency — re-pin to the released Customer version before merge; don't ship a manifest depending on an alpha.

[alexeyshibanov]

Second-pass review — net-new findings (verified against branch head 065a26e).

In addition to my still-open threads (the contact.rolesInOrganization N+1, the alpha CustomerModule.Core pin, and the me.roles contract change), a deeper two-lens pass surfaced the following. Each is verified against the code.

---

🟠 High — rolesInOrganization / isLockedInOrganization accept an arbitrary organizationId with no authorization on that org

ContactType (Schemas/ContactType.cs) resolves both fields from the caller-supplied organizationId argument and queries membership data directly (GetRolesForUsersInOrgAsync / SearchAsync(OnlyLocked)), with no check that the requested org is the caller's current org or one they are entitled to. Top-level contact access only requires sharing an organization with the contact — so a caller who shares Org A with a contact can pass organizationId = <Org B> and read that contact's roles / lock state in Org B. This is horizontal information disclosure across organizations.

Recommendation: bind these fields to context.GetCurrentOrganizationId(), or authorize the requested organizationId against the caller before querying membership.

---

🟠 Should-fix — locked-organization filtering runs after paging, corrupting TotalCount and page size

SearchMembersQueryHandler.FilterLockedOrganizationsAsync (Commands/SearchMembersQueryHandler.cs L64-82) removes locked orgs from the already-paged result.Results and adjusts TotalCount by only the count removed from the current page:

• locked orgs on later pages → TotalCount stays inflated;
• locked orgs on the current page → the page returns short (e.g. 15 of a requested 20) instead of being backfilled.

Recommendation: apply the locked-org exclusion in the underlying search criteria/query (before Skip/Take and TotalCount), or over-fetch/repage with a bounded, tested strategy.

---

🟠 Should-fix — rolesInOrganization does not deduplicate user ids before the batch call

GetRolesInOrganizationAsync filters null/empty ids but not duplicates, then calls GetRolesForUsersInOrgAsync(userIds, …), which builds a dictionary keyed by user id and throws ArgumentException on a duplicate. A contact with two SecurityAccounts sharing a user id fails the whole field. Add .Distinct(StringComparer.OrdinalIgnoreCase) on userIds before the call. (The same guard is worth adding on the customer-module batch method itself — noted on that PR.)

---

⚪ Nits

• FilterLockedOrganizationsAsync assumes non-null result.Results; a partially-initialized MemberSearchResult would NRE. Cheap to guard given the method's whole job is post-filtering a search result.
• Test gaps: no coverage for paged locked-org filtering (total-count / short-page cases), and no schema/resolver tests for rolesInOrganization (duplicate/empty ids, the exact organizationId forwarded to the membership lookup, empty-org).

---

✅ Verified correct (for the record)

• rolesInOrganization correctly scopes the role lookup to the organizationId argument, filters null/empty ids before the call, and de-duplicates the resulting roles by RoleId.
• The UserManager / RoleManager factory instances are created and disposed correctly; awaits are present throughout.

[alexeyshibanov]

Follow-up on a9993e9 — the cross-org disclosure fix and the pre-pagination locked-org filter both look right, and moving rolesInOrganization / isLockedInOrganization onto batch loaders resolves the earlier N+1. Two small items plus one semantic confirmation.

1. Dead organizationId argument on both fields

isLockedInOrganization and rolesInOrganization still declare .Argument<StringGraphType>("organizationId", …), but both resolvers now derive the org from context.GetCurrentOrganizationId() and ignore the argument entirely. The schema advertises an input that does nothing. Either drop the .Argument(...) declarations, or, if you want to keep them for compatibility, validate the supplied value against the current org and reject a mismatch rather than silently ignoring it.

2. GetGlobalRolesByUserAsync — DB round-trips in the loop

The method is correct, but it hits the database ~3× per user, sequentially:

• roleManager.Roles.Where(r => roleNames.Contains(r.Name)) executes a query per user. The role set is small — hoist roleManager.Roles.ToList() (or a Name→Role map) once before the loop and filter in memory to remove those N queries.
• FindByIdAsync + GetRolesAsync are awaited sequentially in the foreach, so the batch does O(users) round-trips. This is acceptable given a single UserManager isn't thread-safe, and the hoist above removes the bulk of the cost anyway.

3. Global roles unioned into rolesInOrganization — intended?

rolesInOrganization now unions global ASP.NET Identity roles (orgRoles.Concat(userGlobalRoles).DistinctBy) on top of the org-membership roles. That's additive and new (it wasn't part of the original field). Worth confirming it's intended, since a field named "roles in organization" returning global roles is a semantics call — and it interacts with the "user-level override is additive" discussion on the customer PR (VirtoCommerce/vc-module-customer#304): if deny/override semantics land there, this union would need to respect them.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
54.4% Coverage on New Code
2.7% Duplication on New Code

See analysis details on SonarQube Cloud