From 596e77bc6ff547992f6ceb19b0c63d3d130e6724 Mon Sep 17 00:00:00 2001 From: Jared Erwin Date: Fri, 10 Jul 2026 09:18:27 -0700 Subject: [PATCH 1/4] Fix SMART _include/_revinclude compartment leak (SQL provider) MSRC: SMART patient-scoped tokens could retrieve resources outside their patient compartment via _include/_revinclude expansion (PHI disclosure). Reuse the existing SmartCompartmentSearchExpression -> UNION-of-CTEs and intersect it with the produced _include/_revinclude rows in the SQL query generator, instead of the deprecated CompartmentAssignment table. Covers SMART v1 and v2 granular scopes. - SqlQueryGenerator: capture the compartment union, regenerate before the include table expansion, and EXISTS-intersect produced include/revinclude rows against the union. - SqlCompartmentSearchRewriter (SMART-gated): additively enumerate every materialized type-specific reference param targeting the compartment root, because the compartment definition maps Encounter/Condition/Procedure/ ImagingStudy to the non-materialized clinical-patient (resolve()-based) param. Every union member stays constrained to ReferenceResourceId = compartmentId, matching the SMART "any resource that refers to them" model. - SearchOptionsFactory: map the reversed wildcard _revinclude=*:* produced "*" sentinel to DomainResource ("all compartment types") so every revincluded type is covered by the union. - Add two integration repro tests (_include and _revinclude) plus the SmartCompartmentLeak.json fixture. - ADR-2607 documents the issue, repro, and chosen union-intersection design. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...-2607-smart-include-compartment-scoping.md | 57 ++++++ .../Features/Search/Expressions/Expression.cs | 9 + .../SmartCompartmentSearchRewriter.cs | 13 +- .../SqlCompartmentSearchRewriter.cs | 60 ++++-- .../Features/Search/SearchOptionsFactory.cs | 27 ++- .../SearchParamTableExpressionExtensions.cs | 35 ++++ .../QueryGenerators/SqlQueryGenerator.cs | 96 ++++++++- .../Microsoft.Health.Fhir.Tests.Common.csproj | 1 + .../TestFiles/R4/SmartCompartmentLeak.json | 193 ++++++++++++++++++ .../Smart/SmartSearchSharedFixture.cs | 3 + .../Features/Smart/SmartSearchTests.cs | 75 ++++++- 11 files changed, 545 insertions(+), 24 deletions(-) create mode 100644 docs/arch/adr-2607-smart-include-compartment-scoping.md create mode 100644 src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json diff --git a/docs/arch/adr-2607-smart-include-compartment-scoping.md b/docs/arch/adr-2607-smart-include-compartment-scoping.md new file mode 100644 index 0000000000..3538c133ee --- /dev/null +++ b/docs/arch/adr-2607-smart-include-compartment-scoping.md @@ -0,0 +1,57 @@ +# ADR-2607: Enforce SMART Patient Compartment Scoping on `_include` / `_revinclude` (SQL) + +**Status**: Proposed +**Date**: 2026-07-10 +**Feature**: smart-include-compartment-leak + +Labels: [Security](https://github.com/microsoft/fhir-server/labels/Security) | [Area-SMART](https://github.com/microsoft/fhir-server/labels/Area-SMART) | [Area-Search](https://github.com/microsoft/fhir-server/labels/Area-Search) + +## Context + +An MSRC report showed that a SMART-on-FHIR patient-scoped caller (a token such as `patient/*.read` confined to a single compartment, e.g. `Patient/CHILD`) can read resources **outside** its patient compartment by abusing search inclusions. The primary search is correctly compartment-restricted, but the `_include` / `_revinclude` expansion is not: the included/revincluded resources are resolved by plain reference joins with no compartment predicate. A caller can therefore start from an in-compartment resource and pull in resources belonging to *other* patients — a PHI disclosure. + +The compartment restriction is applied to the *matched* rows of a search, but the SQL that materializes includes (built in `SqlQueryGenerator.HandleTableKindInclude`) joins `ReferenceSearchParam` to `Resource` and returns whatever is referenced, unfiltered. Any reference that crosses a compartment boundary leaks. + +Constraints: +- **SQL data provider only.** Cosmos DB is being deprecated and is explicitly out of scope for this fix. +- Must not regress existing SMART include/revinclude behavior, including wildcard (`_revinclude=*`) and iterate cases, nor the SMART V2 fine-grained (`ApplyFineGrainedAccessControlWithSearchParameters`) path, which already constrains includes by scope through its own union rewrite. +- Must not collide with the query-plan / custom-query hash cache. + +### Reproduction + +Two integration tests were added to `SmartSearchTests` (Shared integration suite, run against a real SQL Server) before any fix, and both **failed** (confirming the leak): + +1. **`_include`** — a patient-scoped caller searches a resource in its compartment with an `_include` whose target resource belongs to a different patient, and asserts the out-of-compartment target is **not** returned. +2. **`_revinclude`** — the symmetric case: a `_revinclude` that pulls in resources referencing an out-of-compartment resource, asserting they are **not** returned. + +These two tests are the RED baseline; the fix turns them GREEN while the pre-existing SMART include/revinclude tests stay GREEN. + +## Options Considered + +1. **Post-filter in Core/API after retrieval** — drop out-of-compartment includes after the SQL result is read back. *(rejected: the PHI has already crossed the storage boundary into server memory; also breaks `_total`, paging, and the `$includes` continuation contract.)* +2. **Core expression rewriter that rewrites each include into a compartment-scoped sub-search** — express the constraint provider-agnostically in the Core expression tree. *(viable, but heavy: include expansion is emitted as a single generated statement in the SQL layer, SMART include handling already lives there, and a Core rewrite would have to reproduce the include CTE structure.)* +3. **Hand-rolled SQL predicate via the combined compartment search parameter (`clinical-patient`)** — require the produced resource to carry a `ReferenceSearchParam` row for the combined compartment parameter pointing at the compartment id. *(rejected after investigation: the common/combined search parameters that `CompartmentDefinition` maps to — `clinical-patient` and friends — are **never materialized as index rows**. Patient references are indexed only under type-specific parameters such as `Observation-subject`. A predicate keyed on the combined parameter matches nothing and drops legitimate in-compartment resources.)* +4. **Reuse the `dbo.CompartmentAssignment` membership table** — join the produced rows against the compartment-assignment table that older compartment search used. *(rejected: `CompartmentAssignment` is **deprecated** and must not be used by new code.)* +5. **Reuse the SMART compartment UNION the primary query already builds, and intersect it with the produced include/revinclude rows** — the same `SmartCompartmentSearchExpression` → UNION-of-CTEs mechanism that scopes the primary search is extended to the included rows. *(chosen: no new membership store, no deprecated table, and it composes with SMART V1 and V2 — including granular scopes.)* + +## Decision + +Reuse the compartment restriction the server **already** builds for the primary search rather than inventing a second membership check. `SearchOptionsFactory` emits a `SmartCompartmentSearchExpression`; the SQL rewrite (`SmartCompartmentSearchRewriter` / `SqlCompartmentSearchRewriter`) turns it into a set of per-resource-type CTEs — each keyed on a **materialized, type-specific reference parameter** that points at the compartment id — `UNION`ed into a single "compartment membership" CTE that is then intersected with the primary query. This is the mechanism the user asked us to follow; the leak exists only because that intersection was never applied to includes. + +The fix extends that same union to the produced rows of `_include` / `_revinclude`: + +- **Broaden the union's covered types.** The compartment union is built over the primary search types *and* the types produced by include/revinclude expansion (`primary ∪ produced`), so the membership CTE enumerates the included resource types. With no includes this set equals the primary types, so non-include queries are byte-for-byte unchanged. A reversed wildcard `_revinclude=*:*` under an unrestricted scope produces an open-ended type set (there is no bound list of types that may reference the compartment root); this is represented as *all* compartment resource types so every revincluded type is covered. +- **Enumerate membership through materialized reference parameters.** For a SMART compartment the union additively includes, for each compartment resource type, every materialized reference parameter that targets the compartment root type — not only the parameter named in the `CompartmentDefinition`. This is required because the definition maps several clinical types (e.g. `Encounter`, `Condition`, `Procedure`, `ImagingStudy`) to the combined `clinical-patient` parameter, which is never materialized (Option 3); the resources are indexed only under type-specific parameters such as `Encounter-subject`. The addition is safe: every union member is still constrained to `ReferenceResourceId = compartmentId`, so it can only admit resources that reference the compartment root itself — precisely the SMART model of "any resource that refers to the patient." +- **Re-generate and intersect at the include.** Includes are emitted in a separate statement (`INSERT INTO @FilteredData`, then a new `;WITH`). The compartment union is re-generated inside that statement — mirroring the existing SMART V2 scope-union handling — and applied as an `EXISTS` intersection: the include **target** (for `_include`) or the include **source** (for `_revinclude`) must appear in the compartment membership CTE. + +This composes across SMART versions. A V1 request, or a V2 request with granular scopes but no per-scope search parameters, carries only the compartment union, which is re-applied to includes. When a V2 fine-grained scope union is *also* present, both unions are re-generated and intersected independently, so a produced row must satisfy the compartment **and** the scope. The compartment intersection is applied even when the scope grants all resource types (`patient/*.read`), because the compartment — not the scope — is the confidentiality boundary. Because the predicate adds real SQL text (and the compartment id is a hashed parameter), the generated query gets a distinct query hash and cannot collide with non-compartment custom queries in the plan/hash cache. + +## Consequences + +- Closes the MSRC disclosure on the SQL provider for SMART V1 and V2 (including granular scopes): `_include` / `_revinclude` can no longer return resources outside the caller's patient compartment. +- Reuses the authoritative, already-tested compartment union — a single notion of "in compartment," consistent with primary compartment search — instead of a second divergent membership check, and avoids both the deprecated `CompartmentAssignment` table and the never-materialized combined-parameter predicate. +- **Correctness depends on the union enumerating membership through the materialized type-specific reference parameters** — the same index rows the primary compartment search relies on. Compartment resource types whose definition resolves only to a non-materialized combined parameter are not captured by reference alone; keeping the union's parameter resolution aligned with what the indexer actually writes is a security-sensitive invariant, guarded by the reproduction and regression suites. +- **Side effect: the base SMART compartment result is now more complete.** Because the union enumerates materialized type-specific reference parameters, a patient's own resources that were previously dropped by the non-materialized `clinical-patient` mapping (e.g. an `Encounter`, `ImagingStudy` or `Procedure` linked only via `subject`) are now correctly returned. This is a safe widening (still bounded to resources referencing the compartment root) but it does change result counts; one integration assertion that hard-coded the old undercount was updated accordingly. +- Adds an `EXISTS` sub-predicate and a re-generated union CTE to include/revinclude statements. Cost is bounded: the produced-row set is small and the membership CTE reuses the `ReferenceSearchParam` index. +- Include/revinclude of genuinely shared, non-patient-specific reference targets (e.g. `Practitioner`, `Organization`, `Location`, `Medication`) must remain available; these are represented in the compartment union's universal-type members so they are not dropped. +- **SQL-only.** Cosmos is deliberately untouched (deprecated). If Cosmos SMART includes need the same guarantee later, it is a separate follow-up. diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs index 77383749d2..bd23cce7e3 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs @@ -20,6 +20,15 @@ public abstract class Expression /// public bool IsSmartV2UnionExpressionForScopesSearchParameters { get; set; } + /// + /// Determines whether the given expression is the SMART patient compartment union expression + /// (compartment members + the compartment resource itself + universal resource types) produced by + /// . This flag is used by the SQL layer to re-apply the + /// compartment union as an intersection on _include / _revinclude produced resources so that + /// out-of-compartment resources are not disclosed. Applies to both SMART v1 and v2 patient-launch requests. + /// + public bool IsSmartCompartmentUnionExpression { get; set; } + /// /// Creates a that represents a set of ANDed expressions over a search parameter. /// diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs index 8e6c5e5f5f..99365d0b30 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs @@ -83,7 +83,18 @@ public override Expression VisitSmartCompartment(SmartCompartmentSearchExpressio } // union all those results together - return Expression.Union(UnionOperator.All, expressionList); + // Flag the union and each of its members so the SQL layer recognizes this as the SMART compartment + // union and re-applies it as an intersection on _include / _revinclude produced resources. The flag is + // set on multiple levels because downstream rewriters may reconstruct the outer UnionExpression (which + // would drop a flag set only on the outermost node); a recursive check then still finds it on a member. + foreach (Expression memberExpression in expressionList) + { + memberExpression.IsSmartCompartmentUnionExpression = true; + } + + var compartmentUnion = Expression.Union(UnionOperator.All, expressionList); + compartmentUnion.IsSmartCompartmentUnionExpression = true; + return compartmentUnion; } } } diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs index 6e75795382..b3888d5d16 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs @@ -13,6 +13,7 @@ using Microsoft.Health.Fhir.Core.Features.Search; using Microsoft.Health.Fhir.Core.Features.Search.Expressions; using Microsoft.Health.Fhir.Core.Models; +using Microsoft.Health.Fhir.ValueSets; namespace Microsoft.Health.Fhir.Core.Features.Search.Expressions { @@ -45,6 +46,31 @@ public override List BuildCompartmentSearchExpressionsGroup(Compartm var compartmentResourceTypesToSearch = new HashSet(); var searchParameterInfoList = new Dictionary ResourceTypes)>(); + void AddCompartmentSearchParameter(SearchParameterInfo searchParameter, string resourceType) + { + // Use the URL string as the key. + string searchParamUrl = searchParameter.Url.ToString(); + + if (searchParameterInfoList.TryGetValue(searchParamUrl, out var existing)) + { + // Add the compartment resource type if the key exists. + existing.ResourceTypes.Add(resourceType); + } + else + { + // Otherwise, add a new dictionary entry. + searchParameterInfoList[searchParamUrl] = (searchParameter, new HashSet { resourceType }); + } + } + + // A SMART compartment is broader than a regular compartment search: the smart user has access to any + // resource that references them (see SmartCompartmentSearchRewriter). The FHIR compartment definition + // maps several resource types (e.g. Encounter, Condition, Procedure, CareTeam) to the common `patient` + // search parameter (clinical-patient). That parameter's FHIRPath uses resolve() and is therefore never + // materialized as a ReferenceSearchParam index row, so a membership predicate keyed on it matches + // nothing and would silently drop legitimately in-compartment resources. + bool isSmartCompartment = expression is SmartCompartmentSearchExpression; + if (CompartmentDefinitionManager.Value.TryGetResourceTypes(parsedCompartmentType, out HashSet resourceTypes)) { if (expression.FilteredResourceTypes.Any(resourceType => !string.Equals(resourceType, KnownResourceTypes.DomainResource, StringComparison.Ordinal))) @@ -66,19 +92,27 @@ public override List BuildCompartmentSearchExpressionsGroup(Compartm { if (SearchParameterDefinitionManager.Value.TryGetSearchParameter(compartmentResourceType, compartmentSearchParameter, out SearchParameterInfo sp)) { - // Use the URL string as the key. - string searchParamUrl = sp.Url.ToString(); - - if (searchParameterInfoList.TryGetValue(searchParamUrl, out var info)) - { - // Add the compartment resource type if the key exists. - info.ResourceTypes.Add(compartmentResourceType); - } - else - { - // Otherwise, add a new dictionary entry. - searchParameterInfoList[searchParamUrl] = (sp, new HashSet { compartmentResourceType }); - } + AddCompartmentSearchParameter(sp, compartmentResourceType); + } + } + } + + // For SMART compartments, also include every materialized reference parameter of the resource type + // that can target the compartment root type. This keeps membership consistent with what is actually + // indexed (e.g. an Encounter is indexed under Encounter-subject, not the unmaterialized + // clinical-patient) and with the SMART model. It is additive and still constrained (below) to the + // compartment root id, so it can only admit resources that reference the compartment root itself and + // never widens the set beyond the caller's compartment. + if (isSmartCompartment) + { + foreach (SearchParameterInfo referenceParameter in SearchParameterDefinitionManager.Value.GetSearchParameters(compartmentResourceType)) + { + if (referenceParameter.Type == SearchParamType.Reference + && referenceParameter.IsSupported + && referenceParameter.TargetResourceTypes != null + && referenceParameter.TargetResourceTypes.Contains(compartmentType, StringComparer.Ordinal)) + { + AddCompartmentSearchParameter(referenceParameter, compartmentResourceType); } } } diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs index d35ca615fe..97db2fee39 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs @@ -33,6 +33,11 @@ public class SearchOptionsFactory : ISearchOptionsFactory { private static readonly string SupportedTotalTypes = $"'{TotalType.Accurate}', '{TotalType.None}'".ToLower(CultureInfo.CurrentCulture); + // Sentinel emitted by IncludeExpression.Produces for a reversed wildcard revinclude (e.g. _revinclude=*:*) + // whose SMART scope is not restricted to specific resource types. It stands in for the open-ended set of + // resource types that may reference the compartment root. See the ExpressionParser reversed wildcard handling. + private const string WildcardReferenceType = "*"; + private readonly IExpressionParser _expressionParser; private readonly RequestContextAccessor _contextAccessor; private readonly ISortingValidator _sortingValidator; @@ -403,6 +408,24 @@ public SearchOptions Create( // including those from the search path, _type parameter, and resource types returned via include/revinclude expressions requiredResourceTypes.AddRange(parsedResourceTypes); + // Resource types the SMART patient compartment union must cover. This includes the primary search + // resource types AND the resource types produced by _include / _revinclude expansion, so the compartment + // union CTE contains the included resources and the SQL layer can re-apply the compartment membership + // predicate to them (preventing disclosure of out-of-compartment resources via include/revinclude). + // When there are no include/revinclude expressions this equals the primary types, so behavior is unchanged. + var smartCompartmentFilteredResourceTypes = requiredResourceTypes.Distinct().ToArray(); + + // A reversed wildcard revinclude (e.g. _revinclude=*:*) whose SMART scope is not restricted to specific + // resource types produces the "*" sentinel instead of a concrete type list, because the set of resource + // types that may reference the compartment root is open-ended. Map that to DomainResource, the existing + // "all compartment resource types" sentinel understood by the compartment rewriter, so every resource type + // that references the compartment root is represented in the SMART compartment union and the membership + // predicate is correctly re-applied to the revincluded resources. + if (smartCompartmentFilteredResourceTypes.Contains(WildcardReferenceType)) + { + smartCompartmentFilteredResourceTypes = new[] { KnownResourceTypes.DomainResource }; + } + CheckFineGrainedAccessControl(searchExpressions, searchParams, requiredResourceTypes); var validSearchParameters = new List(); @@ -462,7 +485,7 @@ public SearchOptions Create( if (useSmartCompartmentDefinition) { - searchExpressions.Add(Expression.SmartCompartmentSearch(compartmentType, compartmentId, resourceTypesString)); + searchExpressions.Add(Expression.SmartCompartmentSearch(compartmentType, compartmentId, smartCompartmentFilteredResourceTypes)); } else { @@ -491,7 +514,7 @@ public SearchOptions Create( // Don't add the smart compartment twice. this is a patch for bug number AB#152447. if (!searchExpressions.Any(e => e.ValueInsensitiveEquals(Expression.SmartCompartmentSearch(smartCompartmentType, smartCompartmentId, null)))) { - searchExpressions.Add(Expression.SmartCompartmentSearch(smartCompartmentType, smartCompartmentId, resourceTypesString)); + searchExpressions.Add(Expression.SmartCompartmentSearch(smartCompartmentType, smartCompartmentId, smartCompartmentFilteredResourceTypes)); } } else diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs index a735c01488..023b2294f1 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs @@ -73,6 +73,15 @@ public static bool HasSmartV2UnionExpression(this SearchParamTableExpression exp return ContainsSmartV2UnionFlag(expression.Predicate); } + /// + /// Identifies if a contains the SMART patient compartment union expression. + /// + /// Instance of under evaluation. + public static bool HasSmartCompartmentUnionExpression(this SearchParamTableExpression expression) + { + return ContainsSmartCompartmentUnionFlag(expression.Predicate); + } + /// /// Sort expression by query composition logic. always is the first expression to be processed /// with SmartV2 union expressions appearing at the end of all union expressions, followed by other expressions. @@ -148,5 +157,31 @@ private static bool ContainsSmartV2UnionFlag(Expression expression) return false; } + + /// + /// Recursively checks whether the given expression or any of its descendant expressions + /// has the flag set to true. + /// + /// The root expression to search. + /// True if any expression in the tree has the flag; otherwise, false. + private static bool ContainsSmartCompartmentUnionFlag(Expression expression) + { + if (expression == null) + { + return false; + } + + if (expression.IsSmartCompartmentUnionExpression) + { + return true; + } + + if (expression is IExpressionsContainer container) + { + return container.Expressions.Any(ContainsSmartCompartmentUnionFlag); + } + + return false; + } } } diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index e2ad628365..6e69d10f25 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -48,11 +48,13 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor _cteToLimit = new HashSet(); @@ -146,6 +148,9 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions int smartV2TableCounter = 0; UnionExpression smartV2UnionExpression = null; SearchParamTableExpressionQueryGenerator smartV2QueryGenerator = null; + int smartCompartmentTableCounter = 0; + UnionExpression smartCompartmentUnionExpression = null; + SearchParamTableExpressionQueryGenerator smartCompartmentQueryGenerator = null; StringBuilder.AppendLine(";WITH"); StringBuilder.AppendDelimited($"{Environment.NewLine},", expression.SearchParamTableExpressions.SortExpressionsByQueryLogic(), (sb, tableExpression) => { @@ -169,6 +174,19 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions MarkNewParametersAsSmartScopeParameter(parametersBeforeSmartScopesAreApplied.ToHashSet()); } } + else if (tableExpression.HasSmartCompartmentUnionExpression()) + { + // The SMART patient compartment union. It participates in the primary query intersection like + // any regular union, but we also capture it here so it can be re-generated and re-applied as an + // intersection to the resources produced by _include / _revinclude. Without this, included + // resources are authorized only by type and can leak outside the caller's compartment. + smartCompartmentTableCounter = _tableExpressionCounter; + smartCompartmentUnionExpression = unionExpression; + smartCompartmentQueryGenerator = tableExpression.QueryGenerator; + _smartCompartmentUnionVisited = true; + + AppendNewSetOfUnionAllTableExpressions(context, unionExpression, tableExpression.QueryGenerator); + } else { AppendNewSetOfUnionAllTableExpressions(context, unionExpression, tableExpression.QueryGenerator); @@ -192,15 +210,39 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions sb.AppendLine($"INSERT INTO @FilteredData SELECT T1, Sid1, IsMatch, IsPartial, Row{(isSortValueNeeded ? ", SortValue " : " ")}FROM cte{_tableExpressionCounter}"); AddOptionClause(); - if (_smartV2UnionVisited) + if (_smartV2UnionVisited || _smartCompartmentUnionVisited) { - // If we have smart v2 scopes with search parameters we need to re-generate the scoped restricted data for the include + // If we have smart v2 scopes with search parameters and/or a SMART patient compartment we + // need to re-generate the scope/compartment restricted data set for the include, because the + // include CTEs are emitted in a new ;WITH statement that cannot reference the CTEs above. sb.AppendLine("OPTION (RECOMPILE)"); sb.AppendLine($";WITH"); int saveTableExpressionCounter = _tableExpressionCounter; - _tableExpressionCounter = smartV2TableCounter; - AppendSmartNewSetOfUnionAllTableExpressions(context, smartV2UnionExpression, smartV2QueryGenerator, true); + int saveUnionAggregateCTEIndex = _unionAggregateCTEIndex; + bool regeneratedUnion = false; + + if (_smartCompartmentUnionVisited) + { + _tableExpressionCounter = smartCompartmentTableCounter; + AppendNewSetOfUnionAllTableExpressions(context, smartCompartmentUnionExpression, smartCompartmentQueryGenerator, skipJoinFromPreviousUnions: true, isSmartCompartmentUnion: true); + regeneratedUnion = true; + } + + if (_smartV2UnionVisited) + { + if (regeneratedUnion) + { + // Separate the compartment union CTEs from the smart v2 union CTEs. + sb.AppendLine(); + sb.Append(","); + } + + _tableExpressionCounter = smartV2TableCounter; + AppendSmartNewSetOfUnionAllTableExpressions(context, smartV2UnionExpression, smartV2QueryGenerator, true); + } + _tableExpressionCounter = saveTableExpressionCounter; + _unionAggregateCTEIndex = saveUnionAggregateCTEIndex; sb.AppendLine(); sb.AppendLine($",cte{_tableExpressionCounter} AS (SELECT * FROM @FilteredData)"); sb.Append(","); // add comma back @@ -1199,6 +1241,33 @@ private void HandleTableKindInclude( .Append(VLatest.ReferenceSearchParam.ResourceSurrogateId, referenceSourceTableAlias).Append(" = Sid1)"); } } + + // Restrict included / revincluded resources to the caller's SMART patient compartment. This applies to + // SMART v1 and v2 (including granular scopes) whenever a compartment is enforced, even when the scope + // grants access to all resource types, so out-of-compartment resources are never disclosed via includes. + if (_smartCompartmentUnionVisited) + { + if (!includeExpression.Reversed) + { + // For _include the produced (target) resource must be in the compartment. + var scopeForCompartment = delimited.BeginDelimitedElement(); + scopeForCompartment.Append("EXISTS ("); + scopeForCompartment.Append("SELECT * FROM "); + scopeForCompartment.Append(TableExpressionName(_smartCompartmentUnionCTE)) + .Append(" WHERE ").Append(VLatest.ReferenceSearchParam.ReferenceResourceTypeId, referenceSourceTableAlias).Append(" = T1 AND ") + .Append(VLatest.Resource.ResourceSurrogateId, referenceTargetResourceTableAlias).Append(" = Sid1)"); + } + else + { + // For _revinclude the produced (source) resource must be in the compartment. + var scopeForCompartment = delimited.BeginDelimitedElement(); + scopeForCompartment.Append("EXISTS ("); + scopeForCompartment.Append("SELECT * FROM "); + scopeForCompartment.Append(TableExpressionName(_smartCompartmentUnionCTE)) + .Append(" WHERE ").Append(VLatest.ReferenceSearchParam.ResourceTypeId, referenceSourceTableAlias).Append(" = T1 AND ") + .Append(VLatest.ReferenceSearchParam.ResourceSurrogateId, referenceSourceTableAlias).Append(" = Sid1)"); + } + } } if (context.IsIncludesOperation) @@ -1422,7 +1491,7 @@ private SearchParameterQueryGeneratorContext GetContext(string tableAlias = null return new SearchParameterQueryGeneratorContext(StringBuilder, Parameters, Model, _schemaInfo, isAsyncOperation: _isAsyncOperation, tableAlias); } - private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, UnionExpression unionExpression, SearchParamTableExpressionQueryGenerator defaultQueryGenerator) + private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, UnionExpression unionExpression, SearchParamTableExpressionQueryGenerator defaultQueryGenerator, bool skipJoinFromPreviousUnions = false, bool isSmartCompartmentUnion = false) { if (unionExpression.Operator != UnionOperator.All) { @@ -1448,6 +1517,14 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union // Create a final CTE aggregating results from all previous CTEs. StringBuilder.Append(TableExpressionName(++_tableExpressionCounter)).AppendLine(" AS").AppendLine("("); + + if (isSmartCompartmentUnion) + { + // Record the aggregate CTE so included / revincluded resources can be intersected with the SMART + // patient compartment (see HandleTableKindInclude). + _smartCompartmentUnionCTE = _tableExpressionCounter; + } + for (int tableExpressionId = firstInclusiveTableExpressionId; tableExpressionId <= lastInclusiveTableExpressionId; tableExpressionId++) { using (StringBuilder.Indent()) @@ -1466,7 +1543,7 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union StringBuilder.Append(")"); // check for a previous union all, and if so, join the new union all with the previous one - if (_unionAggregateCTEIndex > -1) + if (!skipJoinFromPreviousUnions && _unionAggregateCTEIndex > -1) { var prevUnionAggregateTableName = TableExpressionName(_unionAggregateCTEIndex); var currentUnionAggregateTableName = TableExpressionName(_tableExpressionCounter); @@ -1490,7 +1567,12 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union StringBuilder.Append(")"); } - _unionAggregateCTEIndex = _tableExpressionCounter; + // During re-generation for an include, the compartment union is a standalone CTE that is applied via an + // EXISTS clause rather than chained into the primary query, so it must not update the shared aggregate index. + if (!isSmartCompartmentUnion) + { + _unionAggregateCTEIndex = _tableExpressionCounter; + } _unionVisited = true; _firstChainAfterUnionVisited = false; diff --git a/src/Microsoft.Health.Fhir.Tests.Common/Microsoft.Health.Fhir.Tests.Common.csproj b/src/Microsoft.Health.Fhir.Tests.Common/Microsoft.Health.Fhir.Tests.Common.csproj index debbb9cfd7..97395818cc 100644 --- a/src/Microsoft.Health.Fhir.Tests.Common/Microsoft.Health.Fhir.Tests.Common.csproj +++ b/src/Microsoft.Health.Fhir.Tests.Common/Microsoft.Health.Fhir.Tests.Common.csproj @@ -442,6 +442,7 @@ + diff --git a/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json b/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json new file mode 100644 index 0000000000..8e6b845594 --- /dev/null +++ b/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json @@ -0,0 +1,193 @@ +{ + "resourceType": "Bundle", + "id": "smart-compartment-leak-bundle", + "type": "batch", + "entry": [ + { + "resource": { + "resourceType": "Patient", + "id": "smart-leak-child", + "meta": { + "versionId": "1", + "lastUpdated": "2022-09-29T19:10:27.888+00:00", + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "name": [ + { + "family": "SMARTLeakChild", + "given": [ + "SMARTLeakChildGiven" + ] + } + ], + "birthDate": "2015-03-14", + "gender": "female" + }, + "request": { + "method": "PUT", + "url": "Patient/smart-leak-child" + } + }, + { + "resource": { + "resourceType": "Patient", + "id": "smart-leak-parent", + "meta": { + "versionId": "1", + "lastUpdated": "2022-09-29T19:10:27.888+00:00", + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "identifier": [ + { + "system": "http://example.org/mrn", + "value": "PARENT-SSN-000-00-1234" + } + ], + "name": [ + { + "family": "SMARTLeakParent", + "given": [ + "SMARTLeakParentGiven" + ] + } + ], + "birthDate": "1985-11-02", + "gender": "female", + "address": [ + { + "line": [ + "742 Evergreen Terrace" + ], + "city": "Springfield", + "state": "IL", + "postalCode": "62704" + } + ] + }, + "request": { + "method": "PUT", + "url": "Patient/smart-leak-parent" + } + }, + { + "resource": { + "resourceType": "Coverage", + "id": "smart-leak-coverage", + "meta": { + "versionId": "1", + "lastUpdated": "2022-09-29T15:47:55.014+00:00", + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "active", + "type": { + "coding": [ + { + "system": "http://terminology.hl7.org/CodeSystem/v3-ActCode", + "code": "EHCPOL" + } + ] + }, + "subscriber": { + "reference": "Patient/smart-leak-parent" + }, + "beneficiary": { + "reference": "Patient/smart-leak-child" + }, + "payor": [ + { + "reference": "Patient/smart-leak-child" + } + ] + }, + "request": { + "method": "PUT", + "url": "Coverage/smart-leak-coverage" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-leak-child-obs", + "meta": { + "versionId": "1", + "lastUpdated": "2022-09-29T19:10:27.911+00:00", + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-child" + }, + "effectiveDateTime": "2021-06-10" + }, + "request": { + "method": "PUT", + "url": "Observation/smart-leak-child-obs" + } + }, + { + "resource": { + "resourceType": "DiagnosticReport", + "id": "smart-leak-parent-report", + "meta": { + "versionId": "1", + "lastUpdated": "2022-09-29T19:10:27.967+00:00", + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-parent" + }, + "result": [ + { + "reference": "Observation/smart-leak-child-obs" + } + ] + }, + "request": { + "method": "PUT", + "url": "DiagnosticReport/smart-leak-parent-report" + } + } + ] +} diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchSharedFixture.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchSharedFixture.cs index 47e49d3b2e..ddc56b8b82 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchSharedFixture.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchSharedFixture.cs @@ -74,6 +74,9 @@ public async Task InitializeAsync() await LoadBundleAsync("SmartPatientD"); await LoadBundleAsync("SmartCommon"); + // Cross-compartment data used to reproduce the MSRC _include/_revinclude compartment leak. + await LoadBundleAsync("SmartCompartmentLeak"); + await UpsertResource(Samples.GetJsonSample("Medication")); await UpsertResource(Samples.GetJsonSample("Organization")); await UpsertResource(Samples.GetJsonSample("Location-example-hq")); diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs index 10bd3484cb..83a9778444 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs @@ -371,6 +371,72 @@ public async Task GivenScopesForPatientAndObservation_WhenIncludeWithWildCardReq Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Practitioner"); } + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenIncludeReferencesResourceOutsideCompartment_ThenOutOfCompartmentResourceIsNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // MSRC _include compartment leak reproduction. + // The caller is a SMART patient-scoped user (patient/*.read) confined to Patient/smart-leak-child. + // Coverage/smart-leak-coverage is inside the child's compartment (beneficiary = smart-leak-child), + // but its subscriber references Patient/smart-leak-parent, a DIFFERENT patient that is outside the + // caller's compartment. Expanding _include=Coverage:subscriber must NOT disclose the parent patient, + // because include authorization currently only checks the produced resource type, not the compartment. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-coverage")); + query.Add(new Tuple("_include", "Coverage:subscriber")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + var results = await _searchService.Value.SearchAsync("Coverage", query, CancellationToken.None); + + // The in-compartment primary match should still be returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Coverage" && x.Resource.ResourceId == "smart-leak-coverage"); + + // The out-of-compartment patient pulled in via _include must NOT be disclosed. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenRevIncludeReturnsResourceOutsideCompartment_ThenOutOfCompartmentResourceIsNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // MSRC _revinclude compartment leak reproduction. + // The caller is a SMART patient-scoped user (patient/*.read) confined to Patient/smart-leak-child. + // Observation/smart-leak-child-obs is inside the child's compartment (subject = smart-leak-child). + // DiagnosticReport/smart-leak-parent-report belongs to Patient/smart-leak-parent (subject = parent), + // so it is OUTSIDE the caller's compartment, yet its result references the child's observation. + // Expanding _revinclude=DiagnosticReport:result must NOT disclose the parent's DiagnosticReport. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child-obs")); + query.Add(new Tuple("_revinclude", "DiagnosticReport:result")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // The in-compartment primary match should still be returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs"); + + // The out-of-compartment DiagnosticReport pulled in via _revinclude must NOT be disclosed. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "DiagnosticReport" && x.Resource.ResourceId == "smart-leak-parent-report"); + } + [SkippableFact] public async Task GivenScopesWithReadForObservation_WhenChainedSearchWithPatientName_ThrowsInvalidSearchException() { @@ -725,7 +791,14 @@ public async Task GivenFhirUserClaimPatient_WhenAllResourcesRequested_UniversalR Assert.Contains(results.Results, r => r.Resource.ResourceTypeName == KnownResourceTypes.Location); Assert.Contains(results.Results, r => r.Resource.ResourceTypeName == KnownResourceTypes.Practitioner); Assert.Contains(results.Results, r => r.Resource.ResourceTypeName == KnownResourceTypes.Device); - Assert.Equal(39, results.Results.Count()); + + // Encounter, ImagingStudy and Procedure reference smart-patient-A via their `subject` element. + // The Patient compartment definition maps those types to the common `patient` search parameter + // (clinical-patient), whose FHIRPath uses resolve() and is therefore not materialized as an index + // row, so before the SMART compartment fix these in-compartment resources were silently dropped. + // The SMART compartment now also matches on the materialized type-specific reference parameters, + // so the patient's own Encounter, ImagingStudy and Procedure are correctly included (39 -> 42). + Assert.Equal(42, results.Results.Count()); } [SkippableFact] From 03195f70c54849667cfd437ea2a14abb753a6d73 Mon Sep 17 00:00:00 2001 From: Jared Erwin Date: Mon, 13 Jul 2026 13:22:59 -0700 Subject: [PATCH 2/4] Replace SMART include union with candidate authorization Authorize include and revinclude candidates with immutable compartment membership rules while preserving includes paging and formal FHIR compartment semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/SmartIncludeCandidateAuthorization.md | 628 ++++++++ ...-2607-smart-include-compartment-scoping.md | 7 +- ...didate-authorization-comparison.excalidraw | 1337 +++++++++++++++++ ...SmartIncludeRevIncludeCompartmentLeak.http | 419 ++++++ .../Features/Search/Expressions/Expression.cs | 9 - .../SmartCompartmentSearchRewriter.cs | 38 +- .../SqlCompartmentSearchRewriter.cs | 163 +- .../Features/Search/SearchOptionsFactory.cs | 24 +- .../Features/Search/SqlQueryGeneratorTests.cs | 235 ++- .../SearchParamTableExpressionExtensions.cs | 35 - .../SmartCompartmentMembershipContext.cs | 18 + ...martCompartmentMembershipContextFactory.cs | 80 + .../SmartCompartmentMembershipRule.cs | 17 + .../Search/Expressions/SqlRootExpression.cs | 16 +- .../QueryGenerators/SqlQueryGenerator.cs | 226 ++- .../Features/Search/SqlServerSearchService.cs | 11 + .../TestFiles/R4/SmartCompartmentLeak.json | 174 +++ .../Features/Smart/SmartSearchTests.cs | 260 ++++ 18 files changed, 3462 insertions(+), 235 deletions(-) create mode 100644 docs/SmartIncludeCandidateAuthorization.md create mode 100644 docs/flow diagrams/smart-include-candidate-authorization-comparison.excalidraw create mode 100644 docs/rest/SmartIncludeRevIncludeCompartmentLeak.http create mode 100644 src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContext.cs create mode 100644 src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContextFactory.cs create mode 100644 src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipRule.cs diff --git a/docs/SmartIncludeCandidateAuthorization.md b/docs/SmartIncludeCandidateAuthorization.md new file mode 100644 index 0000000000..9d3329c7e8 --- /dev/null +++ b/docs/SmartIncludeCandidateAuthorization.md @@ -0,0 +1,628 @@ +# Alternate Design: Candidate-Driven SMART Compartment Authorization for Includes + +**Status:** Proposed +**Date:** 2026-07-10 +**Related ADR:** [ADR-2607: Enforce SMART Patient Compartment Scoping on `_include` / `_revinclude` (SQL)](arch/adr-2607-smart-include-compartment-scoping.md) +**Scope:** SQL provider + +**Generated SQL comparison:** [Repeated compartment union versus candidate authorization](flow%20diagrams/smart-include-candidate-authorization-comparison.excalidraw) + +## Summary + +Instead of regenerating the complete SMART patient-compartment union for an +`_include`, `_revinclude`, or `$includes` query, authorize only the resources +produced by each include branch. + +The proposed query is driven by candidate resource keys. For each candidate, +SQL performs a semi-join against `ReferenceSearchParam` to determine whether +that specific resource belongs to the caller's patient compartment. + +This avoids: + +- Materializing all authorized resource keys. A patient compartment can contain + millions of resources, so a temporary table containing the complete + compartment is not practical. +- Regenerating the full compartment union in the second include statement. +- Carrying mutable SMART-compartment marker state through the general Core + expression hierarchy. +- Synchronizing duplicated local and field state in `SqlQueryGenerator`. + +The primary matched-resource search can continue using the existing compartment +union initially. This proposal changes only authorization of resources produced +by include expansion. + +## Problem With the Current Approach + +The primary search and include expansion are emitted as separate SQL statements: + +1. The primary query creates the SMART compartment union and stores the matched + page in `@FilteredData`. +2. The include query starts a new `;WITH` statement. +3. Because a CTE cannot cross the statement boundary, the SMART compartment + union is generated again. +4. Every `_include` and `_revinclude` branch checks its produced resource + against the regenerated union. + +This has several costs: + +- The generated SQL duplicates the compartment restrictions. +- SQL Server may evaluate the underlying union branches more than once because + CTEs are not guaranteed to be materialized. +- Wildcard includes can expand the union across many resource and search + parameter combinations. +- Large generated unions and predicates increase compilation time and memory. +- The implementation requires `_smartCompartmentUnionCTE`, + `_smartCompartmentUnionVisited`, saved counters, and + `Expression.IsSmartCompartmentUnionExpression`. + +Materializing the complete authorized resource-key set in a temporary table was +considered and tested. It is not viable because the result can contain millions +of keys. + +## Goals + +- Prevent `_include`, `_revinclude`, and `$includes` from returning resources + outside the current SMART patient compartment. +- Preserve existing include paging and continuation-token semantics. +- Avoid enumerating or materializing the full patient compartment. +- Keep authorization in SQL rather than filtering after resource retrieval. +- Support SMART V1 and V2, including fine-grained scope intersections. +- Support explicit and wildcard include expressions. +- Use formal FHIR compartment membership rules rather than treating every + reference targeting Patient as compartment membership. +- Remove the need to regenerate the SMART compartment union for includes. + +## Non-Goals + +- Changing Cosmos DB behavior. +- Adding paging support for iterative includes. The current implementation + returns a truncation warning instead of a `$includes` link for iterative + includes. +- Reintroducing the deprecated `CompartmentAssignment` table. +- Materializing a per-request table containing all authorized resource keys. +- Changing the existing `$includes` continuation-token format. + +## Core Design + +### Materialize Rules, Not Resource Keys + +The implementation needs a compact description of patient-compartment +membership: + +- Compartment root resource type, such as Patient. +- Compartment root resource ID. +- Resource types that are always allowed as shared include targets. +- For each patient-compartment resource type, the materialized search parameter + IDs that establish membership. + +Conceptually: + +```text +SmartCompartmentMembership + RootResourceTypeId + RootResourceId + SharedResourceTypeIds + MembershipParameters + ResourceTypeId -> SearchParamIds +``` + +`MembershipParameters` should contain only the formal FHIR compartment +parameters and explicitly validated materialized equivalents. It must not +include every search parameter whose target list contains Patient. + +For combined parameters such as `clinical-patient` that are not materialized, +the mapping should list the specific materialized parameters that implement the +same compartment relationship, such as the applicable `subject` or `patient` +parameter. + +The mapping is small. It can be represented as: + +- An inline `VALUES` relation. +- A small table-valued parameter. +- A small table variable with a primary key. +- A persistent metadata table populated from the model. + +Only the membership rules are stored. Resource keys are never precomputed. + +### Immutable `SmartCompartmentMembershipContext` + +The implementation represents those rules with an internal SQL-layer record: + +```text +SmartCompartmentMembershipContext + CompartmentResourceType + CompartmentResourceId + ImmutableArray + ImmutableArray + +SmartCompartmentMembershipRule + ResourceType + ImmutableArray +``` + +The context contains query metadata, not query results: + +| Field | Purpose | +|---|---| +| `CompartmentResourceType` | The compartment root type, currently normally `Patient`. | +| `CompartmentResourceId` | The authorized root resource ID from the SMART request context. | +| `SharedResourceTypes` | Resource types that retain the existing universal/shared behavior, such as Practitioner and Organization. | +| `MembershipRules` | Formal compartment membership parameters grouped by candidate resource type. | + +Search parameter URLs are stored instead of SQL `SearchParamId` values. The SQL +model resolves each URL to the database-specific numeric ID when generating the +query. This keeps the context independent of schema/model ID assignment and +allows the same structure to work across supported FHIR versions. + +Both the context and its rules are sealed records backed by `ImmutableArray`. +They are constructed once for a search and cannot be mutated while expression +visitors or the SQL generator process the query. The context is query-scoped; it +is not a global cache and it does not survive between requests. + +#### Construction + +`SmartCompartmentMembershipContextFactory` searches the pre-SQL Core expression +tree for the request's `SmartCompartmentSearchExpression`. It then: + +1. Reads the compartment type and authorized resource ID. +2. Gets formal resource-type and parameter relationships from the FHIR + `CompartmentDefinition`. +3. Resolves each relationship to supported, materialized reference search + parameters. +4. Applies only explicitly configured materialized equivalents for combined + parameters that are not directly indexed. +5. Copies the shared resource-type list from + `SmartCompartmentSearchRewriter.UniversalResourceTypes`. +6. Sorts resource types and parameter URLs before creating immutable arrays, so + generated SQL remains deterministic. + +The current explicit equivalent mapping is: + +```text +clinical-patient -> patient, subject +``` + +Each candidate equivalent must exist for that resource type, be a supported +reference parameter, and be capable of targeting the compartment root type. If +no equivalent exists and the formal parameter itself is materialized, the +formal parameter is retained. + +This is deliberately narrower than looking for every parameter that can target +Patient. For example, `Observation.focus` targets Patient but is not nominated +by the Patient `CompartmentDefinition`, so it is not added to the context. + +#### Attachment and Lifetime + +The SQL service creates the context from the original Core expression after the +normal SQL expression rewriting has completed: + +```text +Core search expression containing SmartCompartmentSearchExpression + -> SQL expression rewrites + -> SqlRootExpression + -> attach immutable SmartCompartmentMembershipContext + -> SQL generation +``` + +It is attached for both: + +- The initial search path after `IncludeRewriter`. +- The follow-up `$includes` path after `IncludesOperationRewriter`. + +Attaching it to `SqlRootExpression` keeps SQL-specific authorization metadata +out of the base Core `Expression` hierarchy. It also removes the need for +generic visitors to copy a mutable boolean marker while rebuilding expression +nodes. + +#### SQL Consumption + +`SqlQueryGenerator` reads the context only while generating an include branch. +For each produced candidate it emits three alternatives: + +1. The candidate is the compartment root resource and its ID matches. +2. The candidate type is in `SharedResourceTypes`. +3. A `ReferenceSearchParam` row exists for the candidate key and matches one of + the formal `MembershipRules`, the compartment root type, and the compartment + root ID. + +The third alternative is keyed by the candidate's +`(ResourceTypeId, ResourceSurrogateId)` and uses `EXISTS`, so membership rows do +not multiply the candidate. The predicate is emitted inside the include +branch's `WHERE` clause before its `TOP`, which preserves `$includes` paging. + +### Candidate Definition + +The resource requiring authorization differs by include direction: + +- `_include`: authorize the target resource reached through the reference. +- `_revinclude`: authorize the source resource containing the reference. + +Both are already available in `HandleTableKindInclude` as a resource type ID and +resource surrogate ID. + +### Candidate Membership Predicate + +The general predicate is: + +```sql +AND +( + -- The compartment root itself. + ( + Candidate.ResourceTypeId = @PatientResourceTypeId + AND Candidate.ResourceId = @PatientId + ) + + -- Explicitly shared resource types. + OR Candidate.ResourceTypeId IN (@SharedResourceTypeIds) + + -- Patient-compartment resource membership. + OR EXISTS + ( + SELECT 1 + FROM dbo.ReferenceSearchParam AS Membership + JOIN @AllowedCompartmentParams AS Allowed + ON Allowed.ResourceTypeId = Membership.ResourceTypeId + AND Allowed.SearchParamId = Membership.SearchParamId + WHERE Membership.ResourceTypeId = Candidate.ResourceTypeId + AND Membership.ResourceSurrogateId = Candidate.ResourceSurrogateId + AND Membership.ReferenceResourceTypeId = @PatientResourceTypeId + AND Membership.ReferenceResourceId = @PatientId + AND Membership.BaseUri IS NULL + ) +) +``` + +The exact shared-resource policy must match the existing SMART behavior. It +should be explicit rather than represented by synthetic universal members in a +large union. + +`EXISTS` is required instead of a normal join so multiple qualifying reference +rows cannot multiply the included resource. + +## Index Usage + +`ReferenceSearchParam` currently has this clustered index: + +```text +(ResourceTypeId, ResourceSurrogateId, SearchParamId) +``` + +The candidate-driven predicate supplies the first two values from the candidate +resource. SQL Server therefore performs a narrow seek over the reference rows +belonging to that resource and checks the small allowed search-parameter set. + +This is fundamentally different from driving the query from +`ReferenceResourceId = @PatientId`, which can identify millions of resources in +the compartment. + +Expected access patterns: + +- `_include`: find outgoing references from the bounded matched page, resolve + the target resource, then seek its membership rows by target resource key. +- `_revinclude`: use the target-reference index to find resources referring to + the matched page, then seek each source resource's membership rows by source + resource key. + +The existing filtered-statistics support for `ReferenceSearchParam` can remain +as a supplementary optimization, but correctness must not depend on it. + +## Integration With `SqlQueryGenerator` + +The candidate membership predicate should replace the regenerated-compartment +CTE checks currently emitted in `HandleTableKindInclude`. + +The implementation point is important: authorization must be added to the +include branch's `WHERE` clause before the branch-level `TOP`. + +The primary query may continue using the existing SMART compartment union. The +include statement no longer needs to: + +- Save and restore a compartment union table counter. +- Save a compartment union expression and query generator. +- Regenerate the compartment CTE set. +- Record `_smartCompartmentUnionCTE`. +- Discover the union through + `Expression.IsSmartCompartmentUnionExpression`. + +An immutable membership descriptor should instead be attached to +`SqlRootExpression` or another SQL-layer query context by the SQL rewriter. This +keeps SQL-specific state out of the base Core `Expression` class and ensures +that expression reconstruction cannot silently discard the marker. + +## Initial Search and `$includes` Flow + +The current paging model can be preserved. + +### Initial Search + +1. Execute the matched-resource search under the SMART compartment restriction. +2. Store the matched page in `@FilteredData`. +3. Execute each include branch. +4. Apply candidate authorization within each branch. +5. If the include result exceeds `_includesCount`, return an includes + continuation token. +6. `BundleFactory` emits a link with relation `related` targeting + `/{resourceType}/$includes`. + +The related link retains the original query and adds `includesCt`. + +### Following the Related Link + +1. `IncludesController` executes the request as an includes operation. +2. `SearchOptionsFactory` decodes `includesCt` and recreates the original + search, include expressions, SMART scopes, and compartment context. +3. `SearchIncludeImpl` restricts the matched resources to the surrogate-ID + range stored in the token. +4. Every include branch applies the include-resource cursor. +5. Every include branch applies candidate authorization. +6. Authorized branch results are combined and globally limited. +7. The extra authorized resource becomes the boundary for the next link. +8. `BundleFactory` emits that link as the bundle's `next` link. + +Authorization is re-evaluated on every page. A continuation token is not treated +as proof that a resource is still authorized. + +## Paging Requirements + +### Required Processing Order + +Each include branch must use this order: + +```text +Matched-resource range + -> include continuation boundary + -> candidate authorization + -> DISTINCT candidate resource key + -> ORDER BY resource type ID and surrogate ID + -> TOP (_includesCount + 1) +``` + +After all branches: + +```text +Authorized branch pages + -> UNION ALL + -> global DISTINCT + -> global ORDER BY resource type ID and surrogate ID + -> global TOP (_includesCount + 1) +``` + +The first `_includesCount` authorized resources are returned. The extra +authorized resource indicates that another page exists and supplies the +continuation boundary. + +### Why Authorization Cannot Be Deferred + +The current generator applies `TOP (_includesCount + 1)` inside every include +branch before the final include union. + +This shape is incorrect: + +```text +Branch TOP + -> union branches + -> authorize candidates + -> final page +``` + +Unauthorized candidates could consume a branch's `TOP` allowance. Valid +resources later in the branch would never reach the authorization stage, +causing: + +- Under-filled pages. +- Missing authorized resources. +- Incorrect partial-result detection. +- Continuation links that skip authorized resources. + +Authorization must remain inside each branch before its existing `TOP`, unless +the query generator is redesigned to remove all branch limits and apply one +authorized global limit. + +### Continuation Token + +The current include cursor can remain: + +```text +(IncludeResourceTypeId, IncludeResourceSurrogateId) +``` + +The next page applies a lexicographic boundary: + +```sql +Candidate.ResourceTypeId > @LastResourceTypeId +OR +( + Candidate.ResourceTypeId = @LastResourceTypeId + AND Candidate.ResourceSurrogateId > @LastResourceSurrogateId +) +``` + +Because the boundary is selected from the authorized `N+1` result, the cursor +advances through the authorized resource set rather than the raw candidate set. + +Unauthorized resources between two authorized resources are skipped by SQL and +do not consume page slots. + +### Multiple Include Branches + +The same global cursor is applied to every branch. Each branch returns at most +`N+1` authorized keys after that boundary. The final union deduplicates and +selects the next global page. + +Authorization should remain an `EXISTS` predicate so the same resource matching +multiple compartment references does not appear multiple times. + +### Authorization Changes Between Pages + +The query rechecks membership on every `$includes` request: + +- A resource that loses authorization is not returned on a later page. +- A resource newly authorized below the existing cursor might not appear in the + current traversal. + +This is consistent with the existing keyset-paging behavior when resources are +created, updated, or deleted between requests. Security revocation takes +priority over snapshot consistency. + +### Iterative Includes + +The current service does not generate `$includes` continuation links when +`ContainsIterativeInclude` is true. It returns a truncation warning instead. + +Candidate-driven authorization should still be applied to iterative include +branches, but adding pageable iterative includes is a separate design. + +## SMART V2 Fine-Grained Scopes + +Candidate compartment authorization and SMART V2 scope authorization are +independent intersections. + +An included resource must satisfy: + +```text +Include relationship +AND SMART patient-compartment membership +AND SMART V2 resource/search-parameter scope +``` + +The initial implementation can keep the existing SMART V2 scope-union handling +and replace only the patient-compartment union used by includes. + +A later follow-up could apply the same candidate-driven strategy to SMART V2 +scope unions if measurements show similar benefits. + +## Security Requirements + +- Membership rules must be based on `CompartmentDefinition`. +- A reference parameter is not a membership parameter merely because it can + target Patient. +- `Observation.focus` must not establish Patient compartment membership. +- Custom Patient-targeting search parameters must not automatically change + compartment membership. +- Shared-resource behavior must be explicitly defined. +- The compartment root resource must match the authorized compartment ID. +- The `$includes` token must never bypass authorization. Following a related + link with another caller's token must reapply the second caller's compartment + and scopes. + +## Performance Characteristics + +### Expected Benefits + +- Work scales primarily with include candidates rather than total compartment + size. +- No per-request table containing millions of authorized keys. +- No second copy of the complete compartment union. +- Smaller generated SQL and reduced optimizer compilation work. +- Candidate membership uses the clustered `ReferenceSearchParam` access path. +- Explicit include types allow static partition elimination. + +### Remaining Worst Case + +For `_revinclude`, a matched resource can have a very large number of incoming +references. If most candidates are unauthorized, SQL may examine many +candidates before finding `N+1` authorized results. + +This does not require materializing the patient compartment, but it must be +measured. SQL Server should be allowed to choose between: + +- Candidate-driven nested-loop semi-joins. +- Joining the inbound-reference and membership relations before the page limit. + +Hard-coded join hints should not be introduced without production-scale plan +evidence. + +## Implementation Outline + +1. Define an immutable SQL-layer SMART compartment membership descriptor. +2. Build the descriptor from the FHIR `CompartmentDefinition` and the verified + materialized search-parameter mappings. +3. Preserve the descriptor through SQL root rewrites. +4. Add a query-generator helper that emits candidate membership predicates. +5. Use that helper in every `_include` and `_revinclude` branch before `TOP`. +6. Stop regenerating the SMART compartment union in the include statement. +7. Remove the include-specific compartment union fields, local snapshots, and + expression marker when no longer required. +8. Keep the primary-query compartment union unchanged for the first iteration. +9. Capture actual execution plans and compare CPU, reads, duration, compilation + time, memory grants, and spills. + +## Required Tests + +### Authorization + +- `_include` cannot return a resource belonging to another patient. +- `_revinclude` cannot return a resource belonging to another patient. +- `Observation.subject = Patient/B` and `Observation.focus = Patient/A` is not + visible to Patient A. +- The same Observation is not returned by + `_revinclude=Observation:focus`. +- A supported custom Patient-targeting parameter does not establish membership. +- Valid materialized mappings for Encounter, Condition, Procedure, and + ImagingStudy remain accessible. +- Shared Practitioner, Organization, Location, Medication, and Device behavior + remains unchanged. + +### Paging + +- Unauthorized candidates occur before the first authorized candidate. +- Unauthorized candidates are interleaved between authorized candidates. +- The first `N+1` raw candidates in a branch are unauthorized. +- Multiple include and revinclude branches use `_includesCount=1`. +- A page boundary crosses from one resource type to another. +- Wildcard `_include=*:*` and `_revinclude=*:*` return every authorized resource + exactly once across all pages. +- Following a related link with a different Patient token does not disclose the + original patient's resources. +- Sorted searches preserve the existing two-phase includes continuation token. +- No related link is introduced for iterative includes. + +### Performance + +- Patient compartments containing millions of resources. +- `_include` from a normal matched page. +- `_revinclude` with high inbound-reference fan-out. +- Multiple explicit include branches. +- Wildcard include and revinclude. +- Dense authorized candidates and sparse authorized candidates. +- Current implementation versus candidate-driven implementation using actual + execution plans. + +## Acceptance Criteria + +- No out-of-compartment resource is returned on an initial include page or any + `$includes` continuation page. +- Authorized resources are neither skipped nor duplicated across pages. +- The existing includes continuation-token format remains compatible. +- Unauthorized candidates do not consume returned page slots. +- No complete patient-compartment key set is materialized. +- Candidate membership uses an index seek by resource type and surrogate ID in + representative plans. +- Query performance remains acceptable for compartments containing millions of + resources and for high-fan-out reverse includes. + +## Tradeoffs + +- The SQL generator gains a dedicated candidate-membership predicate. +- A compact membership-rule mapping must be maintained accurately across FHIR + versions. +- Shared-resource policy becomes explicit and testable. +- The design performs one membership probe per candidate unless SQL Server + chooses a set-based semi-join. +- The primary query and include query temporarily use different physical + implementations of the same logical compartment rule. Both must be generated + from the same membership descriptor to prevent semantic drift. + +## Open Questions + +- Should the compact `(ResourceTypeId, SearchParamId)` mapping be emitted as + inline `VALUES`, passed as a table-valued parameter, or stored as model + metadata in SQL? +- Which resource types should be treated as universally shared include targets? +- Should the primary SMART compartment query eventually adopt the same + candidate-driven membership descriptor? +- Is the existing `OPTION (RECOMPILE)` still beneficial after the large union is + removed? +- Do wildcard reverse includes need additional plan-shaping for extremely sparse + authorized candidates? diff --git a/docs/arch/adr-2607-smart-include-compartment-scoping.md b/docs/arch/adr-2607-smart-include-compartment-scoping.md index 3538c133ee..25ac3a3b05 100644 --- a/docs/arch/adr-2607-smart-include-compartment-scoping.md +++ b/docs/arch/adr-2607-smart-include-compartment-scoping.md @@ -1,8 +1,9 @@ # ADR-2607: Enforce SMART Patient Compartment Scoping on `_include` / `_revinclude` (SQL) -**Status**: Proposed +**Status**: Superseded **Date**: 2026-07-10 **Feature**: smart-include-compartment-leak +**Superseded by**: [Candidate-Driven SMART Compartment Authorization for Includes](../SmartIncludeCandidateAuthorization.md) Labels: [Security](https://github.com/microsoft/fhir-server/labels/Security) | [Area-SMART](https://github.com/microsoft/fhir-server/labels/Area-SMART) | [Area-Search](https://github.com/microsoft/fhir-server/labels/Area-Search) @@ -36,6 +37,10 @@ These two tests are the RED baseline; the fix turns them GREEN while the pre-exi ## Decision +> This historical decision was superseded before release. The implementation now retains the +> primary compartment union but authorizes each include candidate through a direct +> `ReferenceSearchParam` membership seek, as described in the replacement design. + Reuse the compartment restriction the server **already** builds for the primary search rather than inventing a second membership check. `SearchOptionsFactory` emits a `SmartCompartmentSearchExpression`; the SQL rewrite (`SmartCompartmentSearchRewriter` / `SqlCompartmentSearchRewriter`) turns it into a set of per-resource-type CTEs — each keyed on a **materialized, type-specific reference parameter** that points at the compartment id — `UNION`ed into a single "compartment membership" CTE that is then intersected with the primary query. This is the mechanism the user asked us to follow; the leak exists only because that intersection was never applied to includes. The fix extends that same union to the produced rows of `_include` / `_revinclude`: diff --git a/docs/flow diagrams/smart-include-candidate-authorization-comparison.excalidraw b/docs/flow diagrams/smart-include-candidate-authorization-comparison.excalidraw new file mode 100644 index 0000000000..2a19a52c0a --- /dev/null +++ b/docs/flow diagrams/smart-include-candidate-authorization-comparison.excalidraw @@ -0,0 +1,1337 @@ +{ + "type": "excalidraw", + "version": 2, + "source": "github-copilot-cli", + "elements": [ + { + "type": "text", + "id": "title", + "x": 40, + "y": 20, + "width": 2080, + "height": 70, + "text": "Generated SQL Comparison: SMART Compartment Authorization for Includes", + "fontSize": 28, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "insight_box", + "x": 170, + "y": 95, + "width": 1830, + "height": 70, + "strokeColor": "#605e5c", + "backgroundColor": "#f3f2f1", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "insight_text", + "x": 205, + "y": 105, + "width": 1760, + "height": 50, + "text": "Both designs keep the primary SMART compartment UNION ALL. The difference is statement 2: repeat the full membership union, or authorize only each produced include candidate.", + "fontSize": 17, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_panel", + "x": 40, + "y": 190, + "width": 1030, + "height": 1260, + "strokeColor": "#d13438", + "backgroundColor": "transparent", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "rectangle", + "id": "new_panel", + "x": 1120, + "y": 190, + "width": 1040, + "height": 1260, + "strokeColor": "#107c10", + "backgroundColor": "transparent", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_panel_title", + "x": 70, + "y": 205, + "width": 970, + "height": 65, + "text": "First implementation: compartment UNION ALL generated twice", + "fontSize": 22, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "text", + "id": "new_panel_title", + "x": 1150, + "y": 205, + "width": 980, + "height": 65, + "text": "IncludeCandidateAuthorization: candidate membership seek", + "fontSize": 22, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_stmt1", + "x": 70, + "y": 285, + "width": 970, + "height": 385, + "strokeColor": "#0078d4", + "backgroundColor": "transparent", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "rectangle", + "id": "old_stmt2", + "x": 70, + "y": 720, + "width": 970, + "height": 480, + "strokeColor": "#d13438", + "backgroundColor": "transparent", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "rectangle", + "id": "new_stmt1", + "x": 1150, + "y": 285, + "width": 980, + "height": 385, + "strokeColor": "#0078d4", + "backgroundColor": "transparent", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "rectangle", + "id": "new_stmt2", + "x": 1150, + "y": 720, + "width": 980, + "height": 480, + "strokeColor": "#107c10", + "backgroundColor": "transparent", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_stmt1_label", + "x": 90, + "y": 295, + "width": 920, + "height": 45, + "text": "SQL statement 1 - primary matched-resource page", + "fontSize": 18, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "text", + "id": "old_stmt2_label", + "x": 90, + "y": 730, + "width": 920, + "height": 45, + "text": "SQL statement 2 - include / revinclude expansion", + "fontSize": 18, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "text", + "id": "new_stmt1_label", + "x": 1170, + "y": 295, + "width": 940, + "height": 45, + "text": "SQL statement 1 - primary matched-resource page", + "fontSize": 18, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "text", + "id": "new_stmt2_label", + "x": 1170, + "y": 730, + "width": 940, + "height": 45, + "text": "SQL statement 2 - include / revinclude expansion", + "fontSize": 18, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_definition", + "x": 95, + "y": 365, + "width": 200, + "height": 115, + "strokeColor": "#0078d4", + "backgroundColor": "#cfe4fa", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_definition_text", + "x": 110, + "y": 377, + "width": 170, + "height": 95, + "text": "SMART compartment\nPatient ID + scopes\nCompartmentDefinition", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_membership1", + "x": 335, + "y": 355, + "width": 215, + "height": 135, + "strokeColor": "#f7630c", + "backgroundColor": "#fff4ce", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_membership1_text", + "x": 348, + "y": 367, + "width": 190, + "height": 112, + "text": "Membership CTEs\nPatient root + per-type\nReferenceSearchParam\nfilters", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_union1", + "x": 590, + "y": 365, + "width": 160, + "height": 115, + "strokeColor": "#5c2d91", + "backgroundColor": "#e8daef", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_union1_text", + "x": 605, + "y": 382, + "width": 130, + "height": 80, + "text": "UNION ALL\ncompartment keys\n(T1, Sid1)", + "fontSize": 16, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_fhir_query", + "x": 790, + "y": 355, + "width": 220, + "height": 135, + "strokeColor": "#0078d4", + "backgroundColor": "#cfe4fa", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_fhir_query_text", + "x": 805, + "y": 367, + "width": 190, + "height": 112, + "text": "Other FHIR query CTEs\nsearch filters + sort\nintersection with union\nTOP matched page", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_filtered", + "x": 390, + "y": 555, + "width": 330, + "height": 75, + "strokeColor": "#0c8599", + "backgroundColor": "#99e9f2", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_filtered_text", + "x": 410, + "y": 567, + "width": 290, + "height": 52, + "text": "INSERT matched page into @FilteredData", + "fontSize": 16, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "arrow", + "id": "old_a1", + "x": 295, + "y": 422, + "width": 40, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 40, + 0 + ] + ], + "strokeColor": "#0078d4", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a2", + "x": 550, + "y": 422, + "width": 40, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 40, + 0 + ] + ], + "strokeColor": "#5c2d91", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a3", + "x": 750, + "y": 422, + "width": 40, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 40, + 0 + ] + ], + "strokeColor": "#0078d4", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a4", + "x": 900, + "y": 490, + "width": 345, + "height": 65, + "points": [ + [ + 0, + 0 + ], + [ + -345, + 65 + ] + ], + "strokeColor": "#0c8599", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "text", + "id": "old_boundary", + "x": 105, + "y": 677, + "width": 900, + "height": 40, + "text": "Statement boundary: the first WITH scope is no longer available", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_membership2", + "x": 100, + "y": 805, + "width": 235, + "height": 145, + "strokeColor": "#d13438", + "backgroundColor": "#fde7e9", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_membership2_text", + "x": 112, + "y": 818, + "width": 210, + "height": 120, + "text": "REPEATED membership CTEs\nPatient root + every\nper-type compartment\nReferenceSearchParam filter", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_union2", + "x": 375, + "y": 815, + "width": 170, + "height": 125, + "strokeColor": "#d13438", + "backgroundColor": "#fde7e9", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_union2_text", + "x": 390, + "y": 835, + "width": 140, + "height": 85, + "text": "UNION ALL\nagain\n(T1, Sid1)", + "fontSize": 17, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_candidates", + "x": 590, + "y": 785, + "width": 405, + "height": 105, + "strokeColor": "#0078d4", + "backgroundColor": "#cfe4fa", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_candidates_text", + "x": 608, + "y": 797, + "width": 370, + "height": 82, + "text": "Include / revinclude candidate CTEs\n@FilteredData -> ReferenceSearchParam -> Resource", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_auth", + "x": 590, + "y": 935, + "width": 405, + "height": 105, + "strokeColor": "#f7630c", + "backgroundColor": "#fff4ce", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_auth_text", + "x": 608, + "y": 947, + "width": 370, + "height": 82, + "text": "EXISTS: candidate key must appear in\nthe regenerated compartment UNION", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "old_final", + "x": 345, + "y": 1090, + "width": 420, + "height": 75, + "strokeColor": "#0c8599", + "backgroundColor": "#99e9f2", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_final_text", + "x": 365, + "y": 1102, + "width": 380, + "height": 52, + "text": "IncludeLimit -> IncludeUnionAll -> final Resource SELECT", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "arrow", + "id": "old_a5", + "x": 335, + "y": 877, + "width": 40, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 40, + 0 + ] + ], + "strokeColor": "#d13438", + "strokeWidth": 3, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a6", + "x": 545, + "y": 877, + "width": 120, + "height": 58, + "points": [ + [ + 0, + 0 + ], + [ + 120, + 58 + ] + ], + "strokeColor": "#d13438", + "strokeWidth": 3, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a7", + "x": 790, + "y": 890, + "width": 1, + "height": 45, + "points": [ + [ + 0, + 0 + ], + [ + 0, + 45 + ] + ], + "strokeColor": "#f7630c", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a8", + "x": 790, + "y": 1040, + "width": 235, + "height": 50, + "points": [ + [ + 0, + 0 + ], + [ + -235, + 50 + ] + ], + "strokeColor": "#0c8599", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "old_a9", + "x": 555, + "y": 630, + "width": 235, + "height": 155, + "points": [ + [ + 0, + 0 + ], + [ + 235, + 155 + ] + ], + "strokeColor": "#0078d4", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "rectangle", + "id": "old_callout", + "x": 100, + "y": 1225, + "width": 910, + "height": 175, + "strokeColor": "#d13438", + "backgroundColor": "#fde7e9", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "old_callout_text", + "x": 125, + "y": 1238, + "width": 860, + "height": 145, + "text": "Repeated work: the full membership SQL is generated in both statements.\nThe include phase may revisit a compartment containing millions of keys.\nIt also requires marker, counter, and CTE state in SqlQueryGenerator.", + "fontSize": 16, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_definition", + "x": 1175, + "y": 365, + "width": 190, + "height": 115, + "strokeColor": "#0078d4", + "backgroundColor": "#cfe4fa", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_definition_text", + "x": 1190, + "y": 377, + "width": 160, + "height": 95, + "text": "SMART compartment\nPatient ID + scopes\nCompartmentDefinition", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_membership1", + "x": 1400, + "y": 355, + "width": 190, + "height": 135, + "strokeColor": "#f7630c", + "backgroundColor": "#fff4ce", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_membership1_text", + "x": 1413, + "y": 367, + "width": 165, + "height": 112, + "text": "Membership CTEs\nPatient root + per-type\nReferenceSearchParam\nfilters", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_union1", + "x": 1625, + "y": 365, + "width": 150, + "height": 115, + "strokeColor": "#5c2d91", + "backgroundColor": "#e8daef", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_union1_text", + "x": 1640, + "y": 382, + "width": 120, + "height": 80, + "text": "UNION ALL\ncompartment keys\n(T1, Sid1)", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_fhir_query", + "x": 1810, + "y": 355, + "width": 285, + "height": 135, + "strokeColor": "#0078d4", + "backgroundColor": "#cfe4fa", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_fhir_query_text", + "x": 1825, + "y": 367, + "width": 255, + "height": 112, + "text": "Other FHIR query CTEs\nsearch filters + sort\nintersection with union\nTOP matched page", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_context", + "x": 1180, + "y": 535, + "width": 365, + "height": 100, + "strokeColor": "#107c10", + "backgroundColor": "#dff6dd", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_context_text", + "x": 1195, + "y": 547, + "width": 335, + "height": 77, + "text": "Immutable SmartCompartmentMembershipContext\nroot ID | shared types | resource type -> search param IDs", + "fontSize": 14, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_filtered", + "x": 1715, + "y": 555, + "width": 330, + "height": 75, + "strokeColor": "#0c8599", + "backgroundColor": "#99e9f2", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_filtered_text", + "x": 1735, + "y": 567, + "width": 290, + "height": 52, + "text": "INSERT matched page into @FilteredData", + "fontSize": 16, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "arrow", + "id": "new_a1", + "x": 1365, + "y": 422, + "width": 35, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 35, + 0 + ] + ], + "strokeColor": "#0078d4", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a2", + "x": 1590, + "y": 422, + "width": 35, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 35, + 0 + ] + ], + "strokeColor": "#5c2d91", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a3", + "x": 1775, + "y": 422, + "width": 35, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 35, + 0 + ] + ], + "strokeColor": "#0078d4", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a4", + "x": 1950, + "y": 490, + "width": 70, + "height": 65, + "points": [ + [ + 0, + 0 + ], + [ + -70, + 65 + ] + ], + "strokeColor": "#0c8599", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a5", + "x": 1270, + "y": 480, + "width": 90, + "height": 55, + "points": [ + [ + 0, + 0 + ], + [ + 90, + 55 + ] + ], + "strokeColor": "#107c10", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "text", + "id": "new_boundary", + "x": 1185, + "y": 677, + "width": 910, + "height": 40, + "text": "Statement boundary: only @FilteredData and immutable membership rules continue", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_candidates", + "x": 1185, + "y": 805, + "width": 300, + "height": 130, + "strokeColor": "#0078d4", + "backgroundColor": "#cfe4fa", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_candidates_text", + "x": 1200, + "y": 817, + "width": 270, + "height": 105, + "text": "Include / revinclude candidate CTE\n@FilteredData -> ReferenceSearchParam -> Resource\napply continuation boundary", + "fontSize": 14, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_auth", + "x": 1530, + "y": 785, + "width": 330, + "height": 170, + "strokeColor": "#107c10", + "backgroundColor": "#dff6dd", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_auth_text", + "x": 1548, + "y": 800, + "width": 294, + "height": 140, + "text": "Candidate authorization EXISTS\n\nroot patient ID\nOR shared resource type\nOR seek ReferenceSearchParam by\ncandidate (T1, Sid1) + formal rule", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_branch_page", + "x": 1900, + "y": 805, + "width": 190, + "height": 130, + "strokeColor": "#5c2d91", + "backgroundColor": "#e8daef", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_branch_page_text", + "x": 1913, + "y": 818, + "width": 164, + "height": 105, + "text": "Authorized branch page\nDISTINCT\nORDER BY T1, Sid1\nTOP N+1", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "new_final", + "x": 1475, + "y": 1045, + "width": 500, + "height": 90, + "strokeColor": "#0c8599", + "backgroundColor": "#99e9f2", + "fillStyle": "solid", + "strokeWidth": 2, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_final_text", + "x": 1495, + "y": 1057, + "width": 460, + "height": 65, + "text": "IncludeUnionAll -> global DISTINCT / TOP N+1\n-> final Resource SELECT and next includesCt", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "rectangle", + "id": "no_repeat", + "x": 1185, + "y": 1000, + "width": 250, + "height": 85, + "strokeColor": "#107c10", + "backgroundColor": "#dff6dd", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "no_repeat_text", + "x": 1200, + "y": 1012, + "width": 220, + "height": 60, + "text": "NO second compartment\nUNION ALL", + "fontSize": 18, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "arrow", + "id": "new_a6", + "x": 1485, + "y": 870, + "width": 45, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 45, + 0 + ] + ], + "strokeColor": "#107c10", + "strokeWidth": 3, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a7", + "x": 1860, + "y": 870, + "width": 40, + "height": 1, + "points": [ + [ + 0, + 0 + ], + [ + 40, + 0 + ] + ], + "strokeColor": "#107c10", + "strokeWidth": 3, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a8", + "x": 1995, + "y": 935, + "width": 270, + "height": 110, + "points": [ + [ + 0, + 0 + ], + [ + -270, + 110 + ] + ], + "strokeColor": "#0c8599", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a9", + "x": 1880, + "y": 630, + "width": 545, + "height": 175, + "points": [ + [ + 0, + 0 + ], + [ + -545, + 175 + ] + ], + "strokeColor": "#0078d4", + "strokeWidth": 2, + "endArrowhead": "arrow" + }, + { + "type": "arrow", + "id": "new_a10", + "x": 1360, + "y": 635, + "width": 335, + "height": 150, + "points": [ + [ + 0, + 0 + ], + [ + 335, + 150 + ] + ], + "strokeColor": "#107c10", + "strokeWidth": 3, + "endArrowhead": "arrow" + }, + { + "type": "rectangle", + "id": "new_callout", + "x": 1185, + "y": 1225, + "width": 900, + "height": 175, + "strokeColor": "#107c10", + "backgroundColor": "#dff6dd", + "fillStyle": "solid", + "strokeWidth": 3, + "roundness": { + "type": 3 + } + }, + { + "type": "text", + "id": "new_callout_text", + "x": 1210, + "y": 1238, + "width": 850, + "height": 145, + "text": "Candidate-driven work: statement 2 probes only produced resource keys.\nIt uses the clustered (ResourceTypeId, ResourceSurrogateId, SearchParamId) path.\nAuthorization happens before each branch TOP, so $includes paging is unchanged.", + "fontSize": 16, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + }, + { + "type": "text", + "id": "legend", + "x": 200, + "y": 1470, + "width": 1760, + "height": 55, + "text": "Red = repeated full-compartment work | Green = candidate-only authorization | Purple = paging / UNION aggregation | Teal = persisted or final result", + "fontSize": 15, + "fontFamily": 2, + "strokeColor": "#000000", + "textAlign": "center", + "verticalAlign": "middle" + } + ], + "appState": { + "viewBackgroundColor": "#ffffff", + "gridSize": 20 + }, + "files": {} +} diff --git a/docs/rest/SmartIncludeRevIncludeCompartmentLeak.http b/docs/rest/SmartIncludeRevIncludeCompartmentLeak.http new file mode 100644 index 0000000000..b2c5b35e8c --- /dev/null +++ b/docs/rest/SmartIncludeRevIncludeCompartmentLeak.http @@ -0,0 +1,419 @@ +# SMART patient-compartment _include/_revinclude leak reproduction +# +# Prerequisites: +# - Run the R4 or R4B FHIR server locally with the SQL provider. +# - Enable authentication and the local identity provider. +# - Set FhirServer:Security:Authorization:ScopesClaim to "scope". +# - Run the requests in order. +# +# Patient/smart-patient-A is the authorized SMART compartment. +# Patient/smart-compartment-leak-patient-b is outside that compartment. +# +# Sections 5 and 6 reproduce the original ADR-2607 leak on the commit before +# the fix. Sections 2 through 4 reproduce the Observation.focus regression +# identified during review of the fix. + +@hostname = localhost:44348 +@baseUrl = https://{{hostname}} + +############################################################################### +# 1. Acquire tokens and create the reproduction data +############################################################################### + +### Get an administrator token for setup and control requests +# @name adminBearer +POST {{baseUrl}}/connect/token +content-type: application/x-www-form-urlencoded + +grant_type=client_credentials +&client_id=globalAdminServicePrincipal +&client_secret=globalAdminServicePrincipal +&scope=fhir-api + +### Create the reproduction resources +# @name setup +POST {{baseUrl}} +content-type: application/fhir+json +Authorization: Bearer {{adminBearer.response.body.access_token}} + +{ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "resource": { + "resourceType": "Patient", + "id": "smart-patient-A", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "active": true, + "name": [ + { + "family": "AuthorizedPatient", + "given": [ + "SMART" + ] + } + ] + }, + "request": { + "method": "PUT", + "url": "Patient/smart-patient-A" + } + }, + { + "resource": { + "resourceType": "Patient", + "id": "smart-compartment-leak-patient-b", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "active": true, + "name": [ + { + "family": "OutsideCompartment", + "given": [ + "PatientB" + ] + } + ] + }, + "request": { + "method": "PUT", + "url": "Patient/smart-compartment-leak-patient-b" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-compartment-focus-observation-b", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "29463-7", + "display": "Body weight" + } + ] + }, + "subject": { + "reference": "Patient/smart-compartment-leak-patient-b" + }, + "focus": [ + { + "reference": "Patient/smart-patient-A" + } + ], + "valueQuantity": { + "value": 70, + "unit": "kg", + "system": "http://unitsofmeasure.org", + "code": "kg" + } + }, + "request": { + "method": "PUT", + "url": "Observation/smart-compartment-focus-observation-b" + } + }, + { + "resource": { + "resourceType": "DiagnosticReport", + "id": "smart-compartment-focus-report-a", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "11502-2", + "display": "Laboratory report" + } + ] + }, + "subject": { + "reference": "Patient/smart-patient-A" + }, + "result": [ + { + "reference": "Observation/smart-compartment-focus-observation-b" + } + ] + }, + "request": { + "method": "PUT", + "url": "DiagnosticReport/smart-compartment-focus-report-a" + } + }, + { + "resource": { + "resourceType": "Coverage", + "id": "smart-compartment-leak-coverage", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "status": "active", + "subscriber": { + "reference": "Patient/smart-compartment-leak-patient-b" + }, + "beneficiary": { + "reference": "Patient/smart-patient-A" + }, + "payor": [ + { + "reference": "Patient/smart-patient-A" + } + ] + }, + "request": { + "method": "PUT", + "url": "Coverage/smart-compartment-leak-coverage" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-compartment-leak-observation-a", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4", + "display": "Hemoglobin A1c" + } + ] + }, + "subject": { + "reference": "Patient/smart-patient-A" + } + }, + "request": { + "method": "PUT", + "url": "Observation/smart-compartment-leak-observation-a" + } + }, + { + "resource": { + "resourceType": "DiagnosticReport", + "id": "smart-compartment-leak-report-b", + "meta": { + "tag": [ + { + "system": "https://github.com/microsoft/fhir-server/repro", + "code": "smart-compartment-include-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "11502-2", + "display": "Laboratory report" + } + ] + }, + "subject": { + "reference": "Patient/smart-compartment-leak-patient-b" + }, + "result": [ + { + "reference": "Observation/smart-compartment-leak-observation-a" + } + ] + }, + "request": { + "method": "PUT", + "url": "DiagnosticReport/smart-compartment-leak-report-b" + } + } + ] +} + +### Get a Patient A SMART token +# @name patientBearer +POST {{baseUrl}}/connect/token +content-type: application/x-www-form-urlencoded + +grant_type=client_credentials +&client_id=smart-patient-A +&client_secret=smart-patient-A +&scope=patient/*.read fhir-api + +### Control: Patient A is visible to the Patient A token +GET {{baseUrl}}/Patient?_id=smart-patient-A +Authorization: Bearer {{patientBearer.response.body.access_token}} + +### Control: Patient B is not visible to the Patient A token +# Expected: an empty search Bundle. +GET {{baseUrl}}/Patient?_id=smart-compartment-leak-patient-b +Authorization: Bearer {{patientBearer.response.body.access_token}} + +### Admin control: inspect the Observation used by the focus tests +# Its subject is Patient B, so it is outside Patient A's formal compartment. +# Its focus references Patient A, which does not establish Patient-compartment membership. +GET {{baseUrl}}/Observation/smart-compartment-focus-observation-b +Authorization: Bearer {{adminBearer.response.body.access_token}} + +############################################################################### +# 2. Direct-search control for the Observation.focus membership regression +############################################################################### + +### Patient A must not retrieve Patient B's Observation directly +# Vulnerable result: Bundle contains smart-compartment-focus-observation-b. +# Correct result: empty Bundle. +GET {{baseUrl}}/Observation?_id=smart-compartment-focus-observation-b +Authorization: Bearer {{patientBearer.response.body.access_token}} + +############################################################################### +# 3. _include reproduction for the Observation.focus membership regression +############################################################################### + +### Include an out-of-compartment Observation from an in-compartment report +# The DiagnosticReport belongs to Patient A through subject. +# The included Observation belongs to Patient B through subject; focus=Patient A +# must not make it part of Patient A's compartment. +# +# Vulnerable result: +# - DiagnosticReport/smart-compartment-focus-report-a, search.mode=match +# - Observation/smart-compartment-focus-observation-b, search.mode=include +# +# Correct result: +# - DiagnosticReport/smart-compartment-focus-report-a only +GET {{baseUrl}}/DiagnosticReport?_id=smart-compartment-focus-report-a&_include=DiagnosticReport:result +Authorization: Bearer {{patientBearer.response.body.access_token}} + +############################################################################### +# 4. _revinclude reproduction for the Observation.focus membership regression +############################################################################### + +### Reverse-include Patient B's Observation through its focus reference +# Observation.focus can target Patient but is not nominated as Observation +# membership in the R4 Patient CompartmentDefinition. +# +# Vulnerable result: +# - Patient/smart-patient-A, search.mode=match +# - Observation/smart-compartment-focus-observation-b, search.mode=include +# +# Correct result: +# - Patient/smart-patient-A only +GET {{baseUrl}}/Patient?_id=smart-patient-A&_revinclude=Observation:focus +Authorization: Bearer {{patientBearer.response.body.access_token}} + +############################################################################### +# 5. Original ADR-2607 _include leak +############################################################################### + +### Include the out-of-compartment subscriber from Patient A's Coverage +# On the commit before the ADR fix, this also returns +# Patient/smart-compartment-leak-patient-b. +# With the ADR fix, only Coverage/smart-compartment-leak-coverage is returned. +GET {{baseUrl}}/Coverage?_id=smart-compartment-leak-coverage&_include=Coverage:subscriber +Authorization: Bearer {{patientBearer.response.body.access_token}} + +############################################################################### +# 6. Original ADR-2607 _revinclude leak +############################################################################### + +### Reverse-include Patient B's report from Patient A's Observation +# On the commit before the ADR fix, this also returns +# DiagnosticReport/smart-compartment-leak-report-b. +# With the ADR fix, only Observation/smart-compartment-leak-observation-a is returned. +GET {{baseUrl}}/Observation?_id=smart-compartment-leak-observation-a&_revinclude=DiagnosticReport:result +Authorization: Bearer {{patientBearer.response.body.access_token}} + +############################################################################### +# 7. Optional cleanup +############################################################################### + +### Remove the uniquely named reproduction resources +# Patient/smart-patient-A is intentionally retained because it is shared by +# the local SMART identity-provider examples. +POST {{baseUrl}} +content-type: application/fhir+json +Authorization: Bearer {{adminBearer.response.body.access_token}} + +{ + "resourceType": "Bundle", + "type": "batch", + "entry": [ + { + "request": { + "method": "DELETE", + "url": "DiagnosticReport/smart-compartment-focus-report-a" + } + }, + { + "request": { + "method": "DELETE", + "url": "Observation/smart-compartment-focus-observation-b" + } + }, + { + "request": { + "method": "DELETE", + "url": "Coverage/smart-compartment-leak-coverage" + } + }, + { + "request": { + "method": "DELETE", + "url": "DiagnosticReport/smart-compartment-leak-report-b" + } + }, + { + "request": { + "method": "DELETE", + "url": "Observation/smart-compartment-leak-observation-a" + } + }, + { + "request": { + "method": "DELETE", + "url": "Patient/smart-compartment-leak-patient-b" + } + } + ] +} diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs index bd23cce7e3..77383749d2 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Expression.cs @@ -20,15 +20,6 @@ public abstract class Expression /// public bool IsSmartV2UnionExpressionForScopesSearchParameters { get; set; } - /// - /// Determines whether the given expression is the SMART patient compartment union expression - /// (compartment members + the compartment resource itself + universal resource types) produced by - /// . This flag is used by the SQL layer to re-apply the - /// compartment union as an intersection on _include / _revinclude produced resources so that - /// out-of-compartment resources are not disclosed. Applies to both SMART v1 and v2 patient-launch requests. - /// - public bool IsSmartCompartmentUnionExpression { get; set; } - /// /// Creates a that represents a set of ANDed expressions over a search parameter. /// diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs index 99365d0b30..d02c80395f 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SmartCompartmentSearchRewriter.cs @@ -26,6 +26,18 @@ public SmartCompartmentSearchRewriter(CompartmentSearchRewriter compartmentSearc _searchParameterDefinitionManager = EnsureArg.IsNotNull(searchParameterDefinitionManager, nameof(searchParameterDefinitionManager)); } + /// + /// Gets resource types that are shared across SMART patient compartments. + /// + public static IReadOnlyCollection UniversalResourceTypes { get; } = Array.AsReadOnly( + [ + KnownResourceTypes.Location, + KnownResourceTypes.Organization, + KnownResourceTypes.Practitioner, + KnownResourceTypes.Medication, + KnownCompartmentTypes.Device, + ]); + public override Expression VisitSmartCompartment(SmartCompartmentSearchExpression expression, object context) { SearchParameterInfo resourceTypeSearchParameter = _searchParameterDefinitionManager.Value.GetSearchParameter(KnownResourceTypes.Resource, SearchParameterNames.ResourceType); @@ -34,9 +46,9 @@ public override Expression VisitSmartCompartment(SmartCompartmentSearchExpressio var compartmentType = expression.CompartmentType; var compartmentId = expression.CompartmentId; - // A smart user compartment is used to filter all search results by the resources available to the smart user + // A smart user compartment is used to filter all search results by the resources available to the smart user. // The smart user has access to 3 things: - // 1 - any resource which refers to them + // 1 - resources that are formal members of the FHIR compartment // 2 - their own resource // 3 - any "universal" resources, such as Locations and Medications @@ -54,14 +66,7 @@ public override Expression VisitSmartCompartment(SmartCompartmentSearchExpressio expressionList.Add(Expression.And(expressionForResourceItself.ToArray())); // Finally we add in the "universal" resources, which are resources that are not compartment specific - var universalResourceTypes = new List() - { - KnownResourceTypes.Location, - KnownResourceTypes.Organization, - KnownResourceTypes.Practitioner, - KnownResourceTypes.Medication, - KnownCompartmentTypes.Device, - }; + var universalResourceTypes = UniversalResourceTypes.ToList(); // In case FilteredResourceTypes is specified and not the default, we need to filter down the universalResourceTypes to only those specified if (expression.FilteredResourceTypes.Any(resourceType => !string.Equals(resourceType, KnownResourceTypes.DomainResource, StringComparison.Ordinal))) @@ -83,18 +88,7 @@ public override Expression VisitSmartCompartment(SmartCompartmentSearchExpressio } // union all those results together - // Flag the union and each of its members so the SQL layer recognizes this as the SMART compartment - // union and re-applies it as an intersection on _include / _revinclude produced resources. The flag is - // set on multiple levels because downstream rewriters may reconstruct the outer UnionExpression (which - // would drop a flag set only on the outermost node); a recursive check then still finds it on a member. - foreach (Expression memberExpression in expressionList) - { - memberExpression.IsSmartCompartmentUnionExpression = true; - } - - var compartmentUnion = Expression.Union(UnionOperator.All, expressionList); - compartmentUnion.IsSmartCompartmentUnionExpression = true; - return compartmentUnion; + return Expression.Union(UnionOperator.All, expressionList); } } } diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs index b3888d5d16..995c2ca5e6 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs @@ -22,6 +22,14 @@ namespace Microsoft.Health.Fhir.Core.Features.Search.Expressions /// public class SqlCompartmentSearchRewriter : CompartmentSearchRewriter { + private const string ClinicalPatientSearchParameterUrl = "http://hl7.org/fhir/SearchParameter/clinical-patient"; + + private static readonly Dictionary> MaterializedEquivalentSearchParameterCodes = + new Dictionary>(StringComparer.Ordinal) + { + [ClinicalPatientSearchParameterUrl] = new[] { "patient", "subject" }, + }; + public SqlCompartmentSearchRewriter( Lazy compartmentDefinitionManager, Lazy searchParameterDefinitionManager) @@ -43,77 +51,26 @@ public override List BuildCompartmentSearchExpressionsGroup(Compartm throw new InvalidSearchOperationException(Core.Resources.CompartmentIdIsInvalid); } - var compartmentResourceTypesToSearch = new HashSet(); var searchParameterInfoList = new Dictionary ResourceTypes)>(); - void AddCompartmentSearchParameter(SearchParameterInfo searchParameter, string resourceType) - { - // Use the URL string as the key. - string searchParamUrl = searchParameter.Url.ToString(); - - if (searchParameterInfoList.TryGetValue(searchParamUrl, out var existing)) - { - // Add the compartment resource type if the key exists. - existing.ResourceTypes.Add(resourceType); - } - else - { - // Otherwise, add a new dictionary entry. - searchParameterInfoList[searchParamUrl] = (searchParameter, new HashSet { resourceType }); - } - } - - // A SMART compartment is broader than a regular compartment search: the smart user has access to any - // resource that references them (see SmartCompartmentSearchRewriter). The FHIR compartment definition - // maps several resource types (e.g. Encounter, Condition, Procedure, CareTeam) to the common `patient` - // search parameter (clinical-patient). That parameter's FHIRPath uses resolve() and is therefore never - // materialized as a ReferenceSearchParam index row, so a membership predicate keyed on it matches - // nothing and would silently drop legitimately in-compartment resources. - bool isSmartCompartment = expression is SmartCompartmentSearchExpression; + IReadOnlyDictionary> materializedParameters = + GetMaterializedCompartmentSearchParameters( + compartmentType, + expression.FilteredResourceTypes, + includeMaterializedEquivalents: expression is SmartCompartmentSearchExpression); - if (CompartmentDefinitionManager.Value.TryGetResourceTypes(parsedCompartmentType, out HashSet resourceTypes)) + foreach ((string compartmentResourceType, IReadOnlyCollection searchParameters) in materializedParameters) { - if (expression.FilteredResourceTypes.Any(resourceType => !string.Equals(resourceType, KnownResourceTypes.DomainResource, StringComparison.Ordinal))) - { - resourceTypes = resourceTypes.Where(x => expression.FilteredResourceTypes.Contains(x)).ToHashSet(); - } - - foreach (var resourceFilter in resourceTypes) + foreach (SearchParameterInfo searchParameter in searchParameters) { - compartmentResourceTypesToSearch.Add(resourceFilter); - } - } - - foreach (var compartmentResourceType in compartmentResourceTypesToSearch) - { - if (CompartmentDefinitionManager.Value.TryGetSearchParams(compartmentResourceType, parsedCompartmentType, out HashSet compartmentSearchParameters)) - { - foreach (var compartmentSearchParameter in compartmentSearchParameters) + string searchParamUrl = searchParameter.Url.ToString(); + if (searchParameterInfoList.TryGetValue(searchParamUrl, out var existing)) { - if (SearchParameterDefinitionManager.Value.TryGetSearchParameter(compartmentResourceType, compartmentSearchParameter, out SearchParameterInfo sp)) - { - AddCompartmentSearchParameter(sp, compartmentResourceType); - } + existing.ResourceTypes.Add(compartmentResourceType); } - } - - // For SMART compartments, also include every materialized reference parameter of the resource type - // that can target the compartment root type. This keeps membership consistent with what is actually - // indexed (e.g. an Encounter is indexed under Encounter-subject, not the unmaterialized - // clinical-patient) and with the SMART model. It is additive and still constrained (below) to the - // compartment root id, so it can only admit resources that reference the compartment root itself and - // never widens the set beyond the caller's compartment. - if (isSmartCompartment) - { - foreach (SearchParameterInfo referenceParameter in SearchParameterDefinitionManager.Value.GetSearchParameters(compartmentResourceType)) + else { - if (referenceParameter.Type == SearchParamType.Reference - && referenceParameter.IsSupported - && referenceParameter.TargetResourceTypes != null - && referenceParameter.TargetResourceTypes.Contains(compartmentType, StringComparer.Ordinal)) - { - AddCompartmentSearchParameter(referenceParameter, compartmentResourceType); - } + searchParameterInfoList[searchParamUrl] = (searchParameter, new HashSet { compartmentResourceType }); } } } @@ -157,5 +114,85 @@ void AddCompartmentSearchParameter(SearchParameterInfo searchParameter, string r throw new InvalidSearchOperationException(string.Format(Core.Resources.CompartmentTypeIsInvalid, compartmentType)); } } + + /// + /// Gets the supported, materialized reference parameters that formally establish compartment membership. + /// + /// The compartment resource type. + /// Optional resource types to include. + /// Whether unmaterialized combined parameters should resolve to validated materialized equivalents. + /// Materialized membership parameters grouped by resource type. + public IReadOnlyDictionary> GetMaterializedCompartmentSearchParameters( + string compartmentType, + IEnumerable filteredResourceTypes, + bool includeMaterializedEquivalents) + { + if (!Enum.TryParse(compartmentType, out ValueSets.CompartmentType parsedCompartmentType)) + { + throw new InvalidSearchOperationException(string.Format(Core.Resources.CompartmentTypeIsInvalid, compartmentType)); + } + + if (!CompartmentDefinitionManager.Value.TryGetResourceTypes(parsedCompartmentType, out HashSet resourceTypes)) + { + return new Dictionary>(); + } + + HashSet filters = filteredResourceTypes?.ToHashSet(StringComparer.Ordinal); + bool filterByResourceType = filters?.Any(resourceType => !string.Equals(resourceType, KnownResourceTypes.DomainResource, StringComparison.Ordinal)) == true; + var result = new Dictionary>(StringComparer.Ordinal); + + foreach (string resourceType in resourceTypes.Where(resourceType => !filterByResourceType || filters.Contains(resourceType))) + { + if (!CompartmentDefinitionManager.Value.TryGetSearchParams(resourceType, parsedCompartmentType, out HashSet parameterCodes)) + { + continue; + } + + var parameters = new Dictionary(StringComparer.Ordinal); + foreach (string parameterCode in parameterCodes) + { + if (!SearchParameterDefinitionManager.Value.TryGetSearchParameter(resourceType, parameterCode, out SearchParameterInfo parameter)) + { + continue; + } + + if (includeMaterializedEquivalents + && MaterializedEquivalentSearchParameterCodes.TryGetValue(parameter.Url.AbsoluteUri, out IReadOnlyCollection equivalentCodes)) + { + bool equivalentFound = false; + foreach (string equivalentCode in equivalentCodes) + { + if (SearchParameterDefinitionManager.Value.TryGetSearchParameter(resourceType, equivalentCode, out SearchParameterInfo equivalent) + && !string.Equals(equivalent.Url.AbsoluteUri, parameter.Url.AbsoluteUri, StringComparison.Ordinal) + && equivalent.Type == SearchParamType.Reference + && equivalent.IsSupported + && equivalent.TargetResourceTypes?.Contains(compartmentType, StringComparer.Ordinal) == true) + { + parameters[equivalent.Url.AbsoluteUri] = equivalent; + equivalentFound = true; + } + } + + // Some resources use a directly indexable branch of the combined parameter and have no + // resource-specific equivalent. Retain the formal parameter for those resources. + if (!equivalentFound && parameter.Type == SearchParamType.Reference && parameter.IsSupported) + { + parameters[parameter.Url.AbsoluteUri] = parameter; + } + } + else if (parameter.Type == SearchParamType.Reference && parameter.IsSupported) + { + parameters[parameter.Url.AbsoluteUri] = parameter; + } + } + + if (parameters.Count > 0) + { + result[resourceType] = parameters.Values.ToArray(); + } + } + + return result; + } } } diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs index 97db2fee39..a3a36bec92 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs @@ -36,8 +36,6 @@ public class SearchOptionsFactory : ISearchOptionsFactory // Sentinel emitted by IncludeExpression.Produces for a reversed wildcard revinclude (e.g. _revinclude=*:*) // whose SMART scope is not restricted to specific resource types. It stands in for the open-ended set of // resource types that may reference the compartment root. See the ExpressionParser reversed wildcard handling. - private const string WildcardReferenceType = "*"; - private readonly IExpressionParser _expressionParser; private readonly RequestContextAccessor _contextAccessor; private readonly ISortingValidator _sortingValidator; @@ -408,24 +406,6 @@ public SearchOptions Create( // including those from the search path, _type parameter, and resource types returned via include/revinclude expressions requiredResourceTypes.AddRange(parsedResourceTypes); - // Resource types the SMART patient compartment union must cover. This includes the primary search - // resource types AND the resource types produced by _include / _revinclude expansion, so the compartment - // union CTE contains the included resources and the SQL layer can re-apply the compartment membership - // predicate to them (preventing disclosure of out-of-compartment resources via include/revinclude). - // When there are no include/revinclude expressions this equals the primary types, so behavior is unchanged. - var smartCompartmentFilteredResourceTypes = requiredResourceTypes.Distinct().ToArray(); - - // A reversed wildcard revinclude (e.g. _revinclude=*:*) whose SMART scope is not restricted to specific - // resource types produces the "*" sentinel instead of a concrete type list, because the set of resource - // types that may reference the compartment root is open-ended. Map that to DomainResource, the existing - // "all compartment resource types" sentinel understood by the compartment rewriter, so every resource type - // that references the compartment root is represented in the SMART compartment union and the membership - // predicate is correctly re-applied to the revincluded resources. - if (smartCompartmentFilteredResourceTypes.Contains(WildcardReferenceType)) - { - smartCompartmentFilteredResourceTypes = new[] { KnownResourceTypes.DomainResource }; - } - CheckFineGrainedAccessControl(searchExpressions, searchParams, requiredResourceTypes); var validSearchParameters = new List(); @@ -485,7 +465,7 @@ public SearchOptions Create( if (useSmartCompartmentDefinition) { - searchExpressions.Add(Expression.SmartCompartmentSearch(compartmentType, compartmentId, smartCompartmentFilteredResourceTypes)); + searchExpressions.Add(Expression.SmartCompartmentSearch(compartmentType, compartmentId, resourceTypesString)); } else { @@ -514,7 +494,7 @@ public SearchOptions Create( // Don't add the smart compartment twice. this is a patch for bug number AB#152447. if (!searchExpressions.Any(e => e.ValueInsensitiveEquals(Expression.SmartCompartmentSearch(smartCompartmentType, smartCompartmentId, null)))) { - searchExpressions.Add(Expression.SmartCompartmentSearch(smartCompartmentType, smartCompartmentId, smartCompartmentFilteredResourceTypes)); + searchExpressions.Add(Expression.SmartCompartmentSearch(smartCompartmentType, smartCompartmentId, resourceTypesString)); } } else diff --git a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs index d6ef87d017..bb016d7fad 100644 --- a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs +++ b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs @@ -9,6 +9,7 @@ using System.Reflection; using System.Text; using System.Threading.Tasks; +using Microsoft.Health.Fhir.Core.Features.Definition; using Microsoft.Health.Fhir.Core.Features.Search; using Microsoft.Health.Fhir.Core.Features.Search.Expressions; using Microsoft.Health.Fhir.Core.Models; @@ -19,6 +20,7 @@ using Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions.Visitors; using Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions.Visitors.QueryGenerators; using Microsoft.Health.Fhir.SqlServer.Features.Storage; +using Microsoft.Health.Fhir.SqlServer.UnitTests.Features.Search.Expressions.Visitors.QueryGenerators; using Microsoft.Health.Fhir.Tests.Common; using Microsoft.Health.Fhir.ValueSets; using Microsoft.Health.SqlServer; @@ -33,7 +35,7 @@ namespace Microsoft.Health.Fhir.SqlServer.UnitTests.Features.Search; [Trait(Traits.OwningTeam, OwningTeam.Fhir)] [Trait(Traits.Category, Categories.Search)] -public class SqlQueryGeneratorTests +public class SqlQueryGeneratorTests : IClassFixture { private readonly ISqlServerFhirModel _fhirModel; private readonly SearchParamTableExpressionQueryGeneratorFactory _queryGeneratorFactory; @@ -41,8 +43,9 @@ public class SqlQueryGeneratorTests private readonly IndentedStringBuilder _strBuilder = new(new StringBuilder()); private readonly SqlQueryGenerator _queryGenerator; - public SqlQueryGeneratorTests() + public SqlQueryGeneratorTests(ModelInfoProviderFixture modelInfoProviderFixture) { + _ = modelInfoProviderFixture; _fhirModel = Substitute.For(); // Create real instances instead of mocking since factory is internal @@ -213,4 +216,232 @@ public void GivenReferenceSearchParameterWithMultipleTargetTypes_WhenSqlGenerate _fhirModel.Received(1).TryGetResourceTypeId("Patient", out Arg.Any()); _fhirModel.Received(1).TryGetResourceTypeId("Practitioner", out Arg.Any()); } + + [Theory] + [InlineData(false, "refTarget")] + [InlineData(true, "refSource")] + public void GivenSmartCompartmentInclude_WhenSqlGenerated_ThenCandidateMembershipIsCheckedBeforeBranchLimit( + bool reversed, + string candidateAlias) + { + // Arrange + var includeParameterUrl = new Uri("http://hl7.org/fhir/SearchParameter/Observation-subject"); + var membershipParameterUrl = new Uri("http://hl7.org/fhir/SearchParameter/DiagnosticReport-subject"); + var includeParameter = new SearchParameterInfo( + "subject", + "subject", + SearchParamType.Reference, + includeParameterUrl, + null, + "Observation.subject", + ["Patient"]); + var includeExpression = new IncludeExpression( + ["Observation"], + includeParameter, + "Observation", + "Patient", + null, + false, + reversed, + false); + var membership = new SmartCompartmentMembershipContext( + "Patient", + "patient-a", + ["Practitioner"], + [new SmartCompartmentMembershipRule("DiagnosticReport", [membershipParameterUrl])]); + SqlRootExpression sqlExpression = new( + [ + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.All), + new SearchParamTableExpression(IncludeQueryGenerator.Instance, includeExpression, SearchParamTableExpressionKind.Include), + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.IncludeLimit), + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.IncludeUnionAll), + ], + [], + membership); + SearchOptions searchOptions = new() + { + IncludeCount = 1, + MaxItemCount = 10, + Sort = [], + ResourceVersionTypes = ResourceVersionType.Latest, + }; + + ConfigureResourceTypeIds(); + _fhirModel.GetSearchParamId(includeParameterUrl).Returns((short)40); + _fhirModel.GetSearchParamId(membershipParameterUrl).Returns((short)41); + + // Act + _queryGenerator.VisitSqlRoot(sqlExpression, searchOptions); + + // Assert + string generatedSql = _strBuilder.ToString(); + Assert.Contains("smartCompartmentMembership", generatedSql); + Assert.Contains($"smartCompartmentMembership.ResourceTypeId = {candidateAlias}.ResourceTypeId", generatedSql); + Assert.Contains($"smartCompartmentMembership.ResourceSurrogateId = {candidateAlias}.ResourceSurrogateId", generatedSql); + Assert.Contains("smartCompartmentMembership.BaseUri IS NULL", generatedSql); + Assert.DoesNotContain("OPTION (RECOMPILE)", generatedSql); + _fhirModel.Received(1).GetSearchParamId(membershipParameterUrl); + } + + [Fact] + public void GivenObservationCompartmentDefinition_WhenMembershipCreated_ThenFocusIsNotAMembershipParameter() + { + // Arrange + var subject = new SearchParameterInfo( + "subject", + "subject", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/Observation-subject"), + null, + "Observation.subject", + ["Patient"]); + var focus = new SearchParameterInfo( + "focus", + "focus", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/Observation-focus"), + null, + "Observation.focus", + ["Patient"]); + SqlCompartmentSearchRewriter rewriter = CreateCompartmentRewriter( + "Observation", + ["subject"], + new Dictionary + { + ["subject"] = subject, + ["focus"] = focus, + }); + + // Act + SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create( + Expression.SmartCompartmentSearch("Patient", "patient-a", "Observation"), + rewriter); + + // Assert + SmartCompartmentMembershipRule rule = Assert.Single(membership.MembershipRules); + Assert.Equal("Observation", rule.ResourceType); + Assert.Equal(subject.Url, Assert.Single(rule.SearchParameterUrls)); + Assert.DoesNotContain(focus.Url, rule.SearchParameterUrls); + } + + [Fact] + public void GivenUnmaterializedClinicalPatientParameter_WhenMembershipCreated_ThenSubjectEquivalentIsUsed() + { + // Arrange + var clinicalPatient = new SearchParameterInfo( + "patient", + "patient", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/clinical-patient"), + null, + "Condition.subject.where(resolve() is Patient)", + ["Patient"]); + var subject = new SearchParameterInfo( + "subject", + "subject", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/Condition-subject"), + null, + "Condition.subject", + ["Patient"]); + SqlCompartmentSearchRewriter rewriter = CreateCompartmentRewriter( + "Condition", + ["patient"], + new Dictionary + { + ["patient"] = clinicalPatient, + ["subject"] = subject, + }); + + // Act + SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create( + Expression.SmartCompartmentSearch("Patient", "patient-a", "Condition"), + rewriter); + + // Assert + SmartCompartmentMembershipRule rule = Assert.Single(membership.MembershipRules); + Assert.Equal(subject.Url, Assert.Single(rule.SearchParameterUrls)); + Assert.DoesNotContain(clinicalPatient.Url, rule.SearchParameterUrls); + } + + [Fact] + public void GivenClinicalPatientParameterWithoutEquivalent_WhenMembershipCreated_ThenFormalParameterIsRetained() + { + // Arrange + var clinicalPatient = new SearchParameterInfo( + "patient", + "patient", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/clinical-patient"), + null, + "AllergyIntolerance.patient", + ["Patient"]); + SqlCompartmentSearchRewriter rewriter = CreateCompartmentRewriter( + "AllergyIntolerance", + ["patient"], + new Dictionary + { + ["patient"] = clinicalPatient, + }); + + // Act + SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create( + Expression.SmartCompartmentSearch("Patient", "patient-a", "AllergyIntolerance"), + rewriter); + + // Assert + SmartCompartmentMembershipRule rule = Assert.Single(membership.MembershipRules); + Assert.Equal(clinicalPatient.Url, Assert.Single(rule.SearchParameterUrls)); + } + + private void ConfigureResourceTypeIds() + { + var resourceTypeIds = new Dictionary(StringComparer.Ordinal) + { + ["Patient"] = 1, + ["Practitioner"] = 2, + ["Observation"] = 3, + ["DiagnosticReport"] = 4, + }; + + _fhirModel.TryGetResourceTypeId(Arg.Any(), out Arg.Any()) + .Returns(call => + { + call[1] = resourceTypeIds[(string)call[0]]; + return true; + }); + } + + private static SqlCompartmentSearchRewriter CreateCompartmentRewriter( + string resourceType, + HashSet compartmentParameterCodes, + IReadOnlyDictionary searchParameters) + { + ICompartmentDefinitionManager compartmentDefinitionManager = Substitute.For(); + compartmentDefinitionManager.TryGetResourceTypes(CompartmentType.Patient, out Arg.Any>()) + .Returns(call => + { + call[1] = new HashSet(StringComparer.Ordinal) { resourceType }; + return true; + }); + compartmentDefinitionManager.TryGetSearchParams(resourceType, CompartmentType.Patient, out Arg.Any>()) + .Returns(call => + { + call[2] = compartmentParameterCodes; + return true; + }); + + ISearchParameterDefinitionManager searchParameterDefinitionManager = Substitute.For(); + searchParameterDefinitionManager.TryGetSearchParameter(resourceType, Arg.Any(), out Arg.Any()) + .Returns(call => + { + bool found = searchParameters.TryGetValue((string)call[1], out SearchParameterInfo parameter); + call[2] = parameter; + return found; + }); + + return new SqlCompartmentSearchRewriter( + new Lazy(() => compartmentDefinitionManager), + new Lazy(() => searchParameterDefinitionManager)); + } } diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs index 023b2294f1..a735c01488 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SearchParamTableExpressionExtensions.cs @@ -73,15 +73,6 @@ public static bool HasSmartV2UnionExpression(this SearchParamTableExpression exp return ContainsSmartV2UnionFlag(expression.Predicate); } - /// - /// Identifies if a contains the SMART patient compartment union expression. - /// - /// Instance of under evaluation. - public static bool HasSmartCompartmentUnionExpression(this SearchParamTableExpression expression) - { - return ContainsSmartCompartmentUnionFlag(expression.Predicate); - } - /// /// Sort expression by query composition logic. always is the first expression to be processed /// with SmartV2 union expressions appearing at the end of all union expressions, followed by other expressions. @@ -157,31 +148,5 @@ private static bool ContainsSmartV2UnionFlag(Expression expression) return false; } - - /// - /// Recursively checks whether the given expression or any of its descendant expressions - /// has the flag set to true. - /// - /// The root expression to search. - /// True if any expression in the tree has the flag; otherwise, false. - private static bool ContainsSmartCompartmentUnionFlag(Expression expression) - { - if (expression == null) - { - return false; - } - - if (expression.IsSmartCompartmentUnionExpression) - { - return true; - } - - if (expression is IExpressionsContainer container) - { - return container.Expressions.Any(ContainsSmartCompartmentUnionFlag); - } - - return false; - } } } diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContext.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContext.cs new file mode 100644 index 0000000000..4db599e24a --- /dev/null +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContext.cs @@ -0,0 +1,18 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using System.Collections.Immutable; + +namespace Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions +{ + /// + /// Immutable SQL-layer description of SMART compartment membership rules. + /// + internal sealed record SmartCompartmentMembershipContext( + string CompartmentResourceType, + string CompartmentResourceId, + ImmutableArray SharedResourceTypes, + ImmutableArray MembershipRules); +} diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContextFactory.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContextFactory.cs new file mode 100644 index 0000000000..f1acc2cfec --- /dev/null +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipContextFactory.cs @@ -0,0 +1,80 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.Health.Fhir.Core.Features.Search.Expressions; +using Microsoft.Health.Fhir.Core.Models; + +namespace Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions +{ + /// + /// Creates SQL SMART compartment membership descriptions from Core search expressions. + /// + internal static class SmartCompartmentMembershipContextFactory + { + private static readonly ImmutableArray SharedResourceTypes = + SmartCompartmentSearchRewriter.UniversalResourceTypes.ToImmutableArray(); + + public static SmartCompartmentMembershipContext Create( + Expression expression, + SqlCompartmentSearchRewriter compartmentSearchRewriter) + { + SmartCompartmentSearchExpression smartCompartment = FindSmartCompartment(expression); + if (smartCompartment == null) + { + return null; + } + + IReadOnlyDictionary> parametersByResourceType = + compartmentSearchRewriter.GetMaterializedCompartmentSearchParameters( + smartCompartment.CompartmentType, + filteredResourceTypes: null, + includeMaterializedEquivalents: true); + + ImmutableArray rules = parametersByResourceType + .OrderBy(pair => pair.Key, StringComparer.Ordinal) + .Select(pair => new SmartCompartmentMembershipRule( + pair.Key, + pair.Value + .Select(parameter => parameter.Url) + .Distinct() + .OrderBy(url => url.AbsoluteUri, StringComparer.Ordinal) + .ToImmutableArray())) + .Where(rule => !rule.SearchParameterUrls.IsDefaultOrEmpty) + .ToImmutableArray(); + + return new SmartCompartmentMembershipContext( + smartCompartment.CompartmentType, + smartCompartment.CompartmentId, + SharedResourceTypes, + rules); + } + + private static SmartCompartmentSearchExpression FindSmartCompartment(Expression expression) + { + if (expression is SmartCompartmentSearchExpression smartCompartment) + { + return smartCompartment; + } + + if (expression is IExpressionsContainer container) + { + foreach (Expression child in container.Expressions) + { + SmartCompartmentSearchExpression result = FindSmartCompartment(child); + if (result != null) + { + return result; + } + } + } + + return null; + } + } +} diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipRule.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipRule.cs new file mode 100644 index 0000000000..92869beeeb --- /dev/null +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SmartCompartmentMembershipRule.cs @@ -0,0 +1,17 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using System; +using System.Collections.Immutable; + +namespace Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions +{ + /// + /// Describes the materialized reference search parameters that establish compartment membership for a resource type. + /// + internal sealed record SmartCompartmentMembershipRule( + string ResourceType, + ImmutableArray SearchParameterUrls); +} diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SqlRootExpression.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SqlRootExpression.cs index da5cbe445b..dda39bb66f 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SqlRootExpression.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/SqlRootExpression.cs @@ -19,13 +19,17 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions /// internal class SqlRootExpression : Expression { - public SqlRootExpression(IReadOnlyList searchParamTableExpressions, IReadOnlyList resourceTableExpressions) + public SqlRootExpression( + IReadOnlyList searchParamTableExpressions, + IReadOnlyList resourceTableExpressions, + SmartCompartmentMembershipContext smartCompartmentMembership = null) { EnsureArg.IsNotNull(searchParamTableExpressions, nameof(searchParamTableExpressions)); EnsureArg.IsNotNull(resourceTableExpressions, nameof(resourceTableExpressions)); SearchParamTableExpressions = searchParamTableExpressions; ResourceTableExpressions = resourceTableExpressions; + SmartCompartmentMembership = smartCompartmentMembership; } /// @@ -38,6 +42,16 @@ public SqlRootExpression(IReadOnlyList searchParamTa /// public IReadOnlyList ResourceTableExpressions { get; } + /// + /// Gets candidate-driven SMART compartment membership rules for include authorization. + /// + public SmartCompartmentMembershipContext SmartCompartmentMembership { get; } + + public SqlRootExpression WithSmartCompartmentMembership(SmartCompartmentMembershipContext membership) + { + return new SqlRootExpression(SearchParamTableExpressions, ResourceTableExpressions, membership); + } + public static SqlRootExpression WithSearchParamTableExpressions(params SearchParamTableExpression[] expressions) { return new SqlRootExpression(expressions, Array.Empty()); diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 6e69d10f25..6cb1288886 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -48,13 +48,11 @@ internal class SqlQueryGenerator : DefaultSqlExpressionVisitor _cteToLimit = new HashSet(); @@ -148,9 +146,6 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions int smartV2TableCounter = 0; UnionExpression smartV2UnionExpression = null; SearchParamTableExpressionQueryGenerator smartV2QueryGenerator = null; - int smartCompartmentTableCounter = 0; - UnionExpression smartCompartmentUnionExpression = null; - SearchParamTableExpressionQueryGenerator smartCompartmentQueryGenerator = null; StringBuilder.AppendLine(";WITH"); StringBuilder.AppendDelimited($"{Environment.NewLine},", expression.SearchParamTableExpressions.SortExpressionsByQueryLogic(), (sb, tableExpression) => { @@ -174,19 +169,6 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions MarkNewParametersAsSmartScopeParameter(parametersBeforeSmartScopesAreApplied.ToHashSet()); } } - else if (tableExpression.HasSmartCompartmentUnionExpression()) - { - // The SMART patient compartment union. It participates in the primary query intersection like - // any regular union, but we also capture it here so it can be re-generated and re-applied as an - // intersection to the resources produced by _include / _revinclude. Without this, included - // resources are authorized only by type and can leak outside the caller's compartment. - smartCompartmentTableCounter = _tableExpressionCounter; - smartCompartmentUnionExpression = unionExpression; - smartCompartmentQueryGenerator = tableExpression.QueryGenerator; - _smartCompartmentUnionVisited = true; - - AppendNewSetOfUnionAllTableExpressions(context, unionExpression, tableExpression.QueryGenerator); - } else { AppendNewSetOfUnionAllTableExpressions(context, unionExpression, tableExpression.QueryGenerator); @@ -210,36 +192,17 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions sb.AppendLine($"INSERT INTO @FilteredData SELECT T1, Sid1, IsMatch, IsPartial, Row{(isSortValueNeeded ? ", SortValue " : " ")}FROM cte{_tableExpressionCounter}"); AddOptionClause(); - if (_smartV2UnionVisited || _smartCompartmentUnionVisited) + if (_smartV2UnionVisited) { - // If we have smart v2 scopes with search parameters and/or a SMART patient compartment we - // need to re-generate the scope/compartment restricted data set for the include, because the + // If we have smart v2 scopes with search parameters we need to re-generate the scope + // restricted data set for the include, because the // include CTEs are emitted in a new ;WITH statement that cannot reference the CTEs above. sb.AppendLine("OPTION (RECOMPILE)"); sb.AppendLine($";WITH"); int saveTableExpressionCounter = _tableExpressionCounter; int saveUnionAggregateCTEIndex = _unionAggregateCTEIndex; - bool regeneratedUnion = false; - - if (_smartCompartmentUnionVisited) - { - _tableExpressionCounter = smartCompartmentTableCounter; - AppendNewSetOfUnionAllTableExpressions(context, smartCompartmentUnionExpression, smartCompartmentQueryGenerator, skipJoinFromPreviousUnions: true, isSmartCompartmentUnion: true); - regeneratedUnion = true; - } - - if (_smartV2UnionVisited) - { - if (regeneratedUnion) - { - // Separate the compartment union CTEs from the smart v2 union CTEs. - sb.AppendLine(); - sb.Append(","); - } - - _tableExpressionCounter = smartV2TableCounter; - AppendSmartNewSetOfUnionAllTableExpressions(context, smartV2UnionExpression, smartV2QueryGenerator, true); - } + _tableExpressionCounter = smartV2TableCounter; + AppendSmartNewSetOfUnionAllTableExpressions(context, smartV2UnionExpression, smartV2QueryGenerator, true); _tableExpressionCounter = saveTableExpressionCounter; _unionAggregateCTEIndex = saveUnionAggregateCTEIndex; @@ -1242,31 +1205,12 @@ private void HandleTableKindInclude( } } - // Restrict included / revincluded resources to the caller's SMART patient compartment. This applies to - // SMART v1 and v2 (including granular scopes) whenever a compartment is enforced, even when the scope - // grants access to all resource types, so out-of-compartment resources are never disclosed via includes. - if (_smartCompartmentUnionVisited) + if (_rootExpression.SmartCompartmentMembership != null) { - if (!includeExpression.Reversed) - { - // For _include the produced (target) resource must be in the compartment. - var scopeForCompartment = delimited.BeginDelimitedElement(); - scopeForCompartment.Append("EXISTS ("); - scopeForCompartment.Append("SELECT * FROM "); - scopeForCompartment.Append(TableExpressionName(_smartCompartmentUnionCTE)) - .Append(" WHERE ").Append(VLatest.ReferenceSearchParam.ReferenceResourceTypeId, referenceSourceTableAlias).Append(" = T1 AND ") - .Append(VLatest.Resource.ResourceSurrogateId, referenceTargetResourceTableAlias).Append(" = Sid1)"); - } - else - { - // For _revinclude the produced (source) resource must be in the compartment. - var scopeForCompartment = delimited.BeginDelimitedElement(); - scopeForCompartment.Append("EXISTS ("); - scopeForCompartment.Append("SELECT * FROM "); - scopeForCompartment.Append(TableExpressionName(_smartCompartmentUnionCTE)) - .Append(" WHERE ").Append(VLatest.ReferenceSearchParam.ResourceTypeId, referenceSourceTableAlias).Append(" = T1 AND ") - .Append(VLatest.ReferenceSearchParam.ResourceSurrogateId, referenceSourceTableAlias).Append(" = Sid1)"); - } + AppendSmartCompartmentCandidatePredicate( + delimited.BeginDelimitedElement(), + includeExpression.Reversed ? referenceSourceTableAlias : referenceTargetResourceTableAlias, + candidateIsResourceTable: !includeExpression.Reversed); } } @@ -1313,6 +1257,140 @@ private void HandleTableKindInclude( } } + private void AppendSmartCompartmentCandidatePredicate( + IndentedStringBuilder scope, + string candidateTableAlias, + bool candidateIsResourceTable) + { + const string membershipAlias = "smartCompartmentMembership"; + const string rootAlias = "smartCompartmentRoot"; + + SmartCompartmentMembershipContext membership = _rootExpression.SmartCompartmentMembership; + var candidateResourceTypeId = candidateIsResourceTable + ? VLatest.Resource.ResourceTypeId + : VLatest.ReferenceSearchParam.ResourceTypeId; + var candidateResourceSurrogateId = candidateIsResourceTable + ? VLatest.Resource.ResourceSurrogateId + : VLatest.ReferenceSearchParam.ResourceSurrogateId; + + object compartmentResourceTypeId = Parameters.AddParameter( + VLatest.Resource.ResourceTypeId, + Model.GetResourceTypeId(membership.CompartmentResourceType), + true); + object compartmentResourceId = Parameters.AddParameter( + VLatest.Resource.ResourceId, + membership.CompartmentResourceId, + true); + + scope.Append("(") + .Append("(") + .Append(candidateResourceTypeId, candidateTableAlias) + .Append(" = ") + .Append(compartmentResourceTypeId) + .Append(" AND "); + + if (candidateIsResourceTable) + { + scope.Append(VLatest.Resource.ResourceId, candidateTableAlias) + .Append(" = ") + .Append(compartmentResourceId); + } + else + { + scope.Append("EXISTS (SELECT 1 FROM ") + .Append(VLatest.Resource) + .Append(' ') + .Append(rootAlias) + .Append(" WHERE ") + .Append(VLatest.Resource.ResourceTypeId, rootAlias) + .Append(" = ") + .Append(candidateResourceTypeId, candidateTableAlias) + .Append(" AND ") + .Append(VLatest.Resource.ResourceSurrogateId, rootAlias) + .Append(" = ") + .Append(candidateResourceSurrogateId, candidateTableAlias) + .Append(" AND ") + .Append(VLatest.Resource.ResourceId, rootAlias) + .Append(" = ") + .Append(compartmentResourceId) + .Append(")"); + } + + scope.Append(")"); + + if (!membership.SharedResourceTypes.IsDefaultOrEmpty) + { + scope.Append(" OR ") + .Append(candidateResourceTypeId, candidateTableAlias) + .Append(" IN (") + .Append(string.Join( + ", ", + membership.SharedResourceTypes.Select(resourceType => Parameters.AddParameter( + VLatest.Resource.ResourceTypeId, + Model.GetResourceTypeId(resourceType), + true)))) + .Append(")"); + } + + if (!membership.MembershipRules.IsDefaultOrEmpty) + { + scope.Append(" OR EXISTS (SELECT 1 FROM ") + .Append(VLatest.ReferenceSearchParam) + .Append(' ') + .Append(membershipAlias) + .Append(" WHERE ") + .Append(VLatest.ReferenceSearchParam.ResourceTypeId, membershipAlias) + .Append(" = ") + .Append(candidateResourceTypeId, candidateTableAlias) + .Append(" AND ") + .Append(VLatest.ReferenceSearchParam.ResourceSurrogateId, membershipAlias) + .Append(" = ") + .Append(candidateResourceSurrogateId, candidateTableAlias) + .Append(" AND ") + .Append(VLatest.ReferenceSearchParam.ReferenceResourceTypeId, membershipAlias) + .Append(" = ") + .Append(compartmentResourceTypeId) + .Append(" AND ") + .Append(VLatest.ReferenceSearchParam.ReferenceResourceId, membershipAlias) + .Append(" = ") + .Append(compartmentResourceId) + .Append(" AND ") + .Append(VLatest.ReferenceSearchParam.BaseUri, membershipAlias) + .Append(" IS NULL AND ("); + + for (int ruleIndex = 0; ruleIndex < membership.MembershipRules.Length; ruleIndex++) + { + SmartCompartmentMembershipRule rule = membership.MembershipRules[ruleIndex]; + if (ruleIndex > 0) + { + scope.Append(" OR "); + } + + scope.Append("(") + .Append(VLatest.ReferenceSearchParam.ResourceTypeId, membershipAlias) + .Append(" = ") + .Append(Parameters.AddParameter( + VLatest.ReferenceSearchParam.ResourceTypeId, + Model.GetResourceTypeId(rule.ResourceType), + true)) + .Append(" AND ") + .Append(VLatest.ReferenceSearchParam.SearchParamId, membershipAlias) + .Append(" IN (") + .Append(string.Join( + ", ", + rule.SearchParameterUrls.Select(url => Parameters.AddParameter( + VLatest.ReferenceSearchParam.SearchParamId, + Model.GetSearchParamId(url), + true)))) + .Append("))"); + } + + scope.Append("))"); + } + + scope.Append(")"); + } + private void HandleTableKindIncludeLimit(SearchOptions context) { StringBuilder.Append("SELECT DISTINCT TOP (") @@ -1491,7 +1569,7 @@ private SearchParameterQueryGeneratorContext GetContext(string tableAlias = null return new SearchParameterQueryGeneratorContext(StringBuilder, Parameters, Model, _schemaInfo, isAsyncOperation: _isAsyncOperation, tableAlias); } - private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, UnionExpression unionExpression, SearchParamTableExpressionQueryGenerator defaultQueryGenerator, bool skipJoinFromPreviousUnions = false, bool isSmartCompartmentUnion = false) + private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, UnionExpression unionExpression, SearchParamTableExpressionQueryGenerator defaultQueryGenerator, bool skipJoinFromPreviousUnions = false) { if (unionExpression.Operator != UnionOperator.All) { @@ -1518,13 +1596,6 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union // Create a final CTE aggregating results from all previous CTEs. StringBuilder.Append(TableExpressionName(++_tableExpressionCounter)).AppendLine(" AS").AppendLine("("); - if (isSmartCompartmentUnion) - { - // Record the aggregate CTE so included / revincluded resources can be intersected with the SMART - // patient compartment (see HandleTableKindInclude). - _smartCompartmentUnionCTE = _tableExpressionCounter; - } - for (int tableExpressionId = firstInclusiveTableExpressionId; tableExpressionId <= lastInclusiveTableExpressionId; tableExpressionId++) { using (StringBuilder.Indent()) @@ -1567,12 +1638,7 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union StringBuilder.Append(")"); } - // During re-generation for an include, the compartment union is a standalone CTE that is applied via an - // EXISTS clause rather than chained into the primary query, so it must not update the shared aggregate index. - if (!isSmartCompartmentUnion) - { - _unionAggregateCTEIndex = _tableExpressionCounter; - } + _unionAggregateCTEIndex = _tableExpressionCounter; _unionVisited = true; _firstChainAfterUnionVisited = false; diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs index 37d42519a4..4e8480ad38 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs @@ -558,6 +558,7 @@ private async Task SearchImpl(SqlSearchOptions sqlSearchOptions, b SqlRootExpression expression = (SqlRootExpression)CreateDefaultSearchExpression(searchExpression, clonedSearchOptions) ?.AcceptVisitor(IncludeRewriter.Instance) ?? SqlRootExpression.WithResourceTableExpressions(); + expression = AttachSmartCompartmentMembership(expression, searchExpression); await CreateStats(expression, cancellationToken); @@ -2263,6 +2264,7 @@ private async Task SearchIncludeImpl(SqlSearchOptions sqlSearchOpt SqlRootExpression expression = (SqlRootExpression)CreateDefaultSearchExpression(searchExpression, clonedSearchOptions) ?.AcceptVisitor(IncludesOperationRewriter.Instance) ?? SqlRootExpression.WithResourceTableExpressions(); + expression = AttachSmartCompartmentMembership(expression, searchExpression); await CreateStats(expression, cancellationToken); @@ -2515,6 +2517,15 @@ private SqlRootExpression CreateDefaultSearchExpression(Expression rootExpressio .AcceptVisitor(TopRewriter.Instance, searchOptions); } + private SqlRootExpression AttachSmartCompartmentMembership(SqlRootExpression sqlExpression, Expression coreExpression) + { + var sqlCompartmentSearchRewriter = (SqlCompartmentSearchRewriter)_compartmentSearchRewriter; + SmartCompartmentMembershipContext membership = + SmartCompartmentMembershipContextFactory.Create(coreExpression, sqlCompartmentSearchRewriter); + + return membership == null ? sqlExpression : sqlExpression.WithSmartCompartmentMembership(membership); + } + /// /// Selects exactly one of three mutually-exclusive date/time equality strategies — legacy overlap /// (), the birthdate End-only optimization diff --git a/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json b/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json index 8e6b845594..acdb0545c2 100644 --- a/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json +++ b/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json @@ -152,6 +152,180 @@ "url": "Observation/smart-leak-child-obs" } }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-leak-focus-obs", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-parent" + }, + "focus": [ + { + "reference": "Patient/smart-leak-child" + } + ], + "effectiveDateTime": "2021-07-01" + }, + "request": { + "method": "PUT", + "url": "Observation/smart-leak-focus-obs" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-leak-root-focus-obs", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-parent" + }, + "focus": [ + { + "reference": "Patient/smart-leak-parent" + } + ] + }, + "request": { + "method": "PUT", + "url": "Observation/smart-leak-root-focus-obs" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-leak-shared-focus-obs", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-parent" + }, + "focus": [ + { + "reference": "Practitioner/smart-practitioner-A" + } + ] + }, + "request": { + "method": "PUT", + "url": "Observation/smart-leak-shared-focus-obs" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-leak-child-obs-with-outside-focus", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-child" + }, + "focus": [ + { + "reference": "Patient/smart-leak-parent" + } + ], + "effectiveDateTime": "2021-07-02" + }, + "request": { + "method": "PUT", + "url": "Observation/smart-leak-child-obs-with-outside-focus" + } + }, + { + "resource": { + "resourceType": "Procedure", + "id": "smart-leak-child-procedure", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "completed", + "code": { + "coding": [ + { + "system": "http://snomed.info/sct", + "code": "80146002", + "display": "Appendectomy" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-child" + }, + "performedDateTime": "2021-06-15" + }, + "request": { + "method": "PUT", + "url": "Procedure/smart-leak-child-procedure" + } + }, { "resource": { "resourceType": "DiagnosticReport", diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs index 83a9778444..b2ea2c45bd 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs @@ -18,6 +18,7 @@ using Microsoft.Health.Extensions.DependencyInjection; using Microsoft.Health.Fhir.Core.Exceptions; using Microsoft.Health.Fhir.Core.Extensions; +using Microsoft.Health.Fhir.Core.Features; using Microsoft.Health.Fhir.Core.Features.Context; using Microsoft.Health.Fhir.Core.Features.Definition; using Microsoft.Health.Fhir.Core.Features.Operations; @@ -437,6 +438,265 @@ public async Task GivenPatientScopeReadAll_WhenRevIncludeReturnsResourceOutsideC Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "DiagnosticReport" && x.Resource.ResourceId == "smart-leak-parent-report"); } + // ----------------------------------------------------------------------- + // Observation.focus cross-compartment leak regression tests + // + // Data layout (see SmartCompartmentLeak.json): + // smart-leak-parent = Patient B (the subscriber / subject owner) + // smart-leak-child = Patient A (the beneficiary / focus target) + // smart-leak-focus-obs subject=Parent, focus=[Child] + // smart-leak-child-obs-with-outside-focus subject=Child, focus=[Parent] + // smart-leak-child-procedure subject=Child + // ----------------------------------------------------------------------- + + [SkippableFact] + public async Task GivenPatientBScopeReadAll_WhenObservationSubjectPatientBFocusPatientA_ThenPatientANotExposedViaFocusInclude() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ caller is patient-scoped to Patient B (smart-leak-parent). + // smart-leak-focus-obs lives in Patient B's compartment (subject = Parent) and + // carries a focus reference pointing to Patient A (smart-leak-child), who is + // outside Patient B's compartment. + // Expanding _include=Observation:focus must NOT disclose Patient A. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-focus-obs")); + query.Add(new Tuple("_include", "Observation:focus")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-parent"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert ─ the in-compartment primary Observation must still be returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-focus-obs"); + + // The out-of-compartment Patient A reached via Observation.focus must NOT be disclosed. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + } + + [SkippableFact] + public async Task GivenPatientAScopeReadAll_WhenObservationOnlyReferencesPatientAThroughFocus_ThenObservationIsNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange + var query = new List> + { + new Tuple("_id", "smart-leak-focus-obs"), + }; + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert + Assert.DoesNotContain(results.Results, result => result.Resource.ResourceId == "smart-leak-focus-obs"); + } + + [SkippableFact] + public async Task GivenPatientAScopeReadAll_WhenRevIncludeObservationFocusForPatientA_ThenOutOfCompartmentObservationNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). + // smart-leak-focus-obs has focus = Patient A, but its subject = Patient B, so it + // lives OUTSIDE Patient A's compartment. Expanding _revinclude=Observation:focus + // on Patient A must NOT surface smart-leak-focus-obs. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child")); + query.Add(new Tuple("_revinclude", "Observation:focus")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Patient", query, CancellationToken.None); + + // Assert ─ Patient A itself is the primary match and must be returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + + // The Observation (focus = Patient A but subject = Patient B) is OUTSIDE Patient A's + // compartment and must NOT be disclosed via _revinclude=Observation:focus. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-focus-obs"); + } + + [SkippableFact] + public async Task GivenPatientAScopeReadAll_WhenIncludeObservationFocusOnInCompartmentObs_ThenOutOfCompartmentPatientNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). + // smart-leak-child-obs-with-outside-focus is in Patient A's compartment (subject = Child) + // but its focus points to Patient B (smart-leak-parent), who is OUTSIDE the compartment. + // Following _include=Observation:focus must NOT disclose Patient B. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child-obs-with-outside-focus")); + query.Add(new Tuple("_include", "Observation:focus")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert ─ the in-compartment Observation is the primary match and must be returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + + // Patient B (the focus target) must NOT be disclosed because it is outside Patient A's compartment. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPatientAScopeReadAll_WhenRevIncludeSubjectMappings_ThenEncounterConditionImagingStudyAllReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ caller is patient-scoped to smart-patient-A (Patient A). + // Verify that legitimate subject-based compartment members (Encounter, Condition, + // ImagingStudy) remain fully visible after the focus-leak fixture data is loaded. + // Existing fixture resources used: smart-encounter-A1, smart-condition-A1, + // smart-imagingstudy-A1 (all in SmartPatientA.json, all subject = smart-patient-A). + var query = new List>(); + query.Add(new Tuple("_id", "smart-patient-A")); + query.Add(new Tuple("_revinclude", "Encounter:subject")); + query.Add(new Tuple("_revinclude", "Condition:subject")); + query.Add(new Tuple("_revinclude", "ImagingStudy:subject")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-patient-A"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Patient", query, CancellationToken.None); + + // Assert ─ the patient plus each legitimate subject-mapped type must be present. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-patient-A"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Encounter" && x.Resource.ResourceId == "smart-encounter-A1"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Condition" && x.Resource.ResourceId == "smart-condition-A1"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "ImagingStudy" && x.Resource.ResourceId == "smart-imagingstudy-A1"); + + // The focus-based leak Observations must NOT appear: their subjects are NOT smart-patient-A. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceId == "smart-leak-focus-obs"); + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + } + + [SkippableFact] + public async Task GivenPatientAScopeReadAll_WhenRevIncludeProcedureSubject_ThenProcedureIsVisibleAndFocusObsIsNot() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ caller is patient-scoped to smart-leak-child (Patient A). + // smart-leak-child-procedure has subject = Patient A → in-compartment. + // smart-leak-focus-obs has subject = Patient B → NOT in Patient A's compartment, + // so it must not surface even when doing a broad _revinclude. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child")); + query.Add(new Tuple("_revinclude", "Procedure:subject")); + query.Add(new Tuple("_revinclude", "Observation:subject")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Patient", query, CancellationToken.None); + + // Assert ─ the patient and in-compartment resources must be present. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Procedure" && x.Resource.ResourceId == "smart-leak-child-procedure"); + + // Observations with subject = Patient A (child) are in-compartment. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + + // The Observation whose subject is Patient B must NOT appear (out-of-compartment). + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-focus-obs"); + } + + [SkippableFact] + public async Task GivenPatientBScopeReadAll_WhenIncludesPagingCrossesUnauthorizedFocusTarget_ThenAllAuthorizedTargetsAreReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange. Target surrogate IDs put the unauthorized child Patient between the + // authorized shared Practitioner and authorized parent Patient. + var query = new List>(); + query.Add(new Tuple("_tag", "testTag|smart-leak")); + query.Add(new Tuple("_include", "Observation:focus")); + query.Add(new Tuple("_includesCount", "1")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-parent"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var firstPage = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + Assert.NotNull(firstPage.IncludesContinuationToken); + + var nextQuery = new List>(query) + { + new Tuple( + KnownQueryParameterNames.IncludesContinuationToken, + ContinuationTokenEncoder.Encode(firstPage.IncludesContinuationToken)), + }; + var secondPage = await _searchService.Value.SearchAsync( + "Observation", + nextQuery, + CancellationToken.None, + isIncludesOperation: true); + + // Assert + var includedResources = firstPage.Results + .Concat(secondPage.Results) + .Where(result => result.SearchEntryMode == Microsoft.Health.Fhir.ValueSets.SearchEntryMode.Include) + .ToList(); + Assert.Contains(includedResources, result => result.Resource.ResourceId == "smart-practitioner-A"); + Assert.Contains(includedResources, result => result.Resource.ResourceId == "smart-leak-parent"); + Assert.DoesNotContain(includedResources, result => result.Resource.ResourceId == "smart-leak-child"); + } + [SkippableFact] public async Task GivenScopesWithReadForObservation_WhenChainedSearchWithPatientName_ThrowsInvalidSearchException() { From 54ef54d7b699f154e04f473d1d9faaf67850aae7 Mon Sep 17 00:00:00 2001 From: Jared Erwin Date: Mon, 13 Jul 2026 17:31:40 -0700 Subject: [PATCH 3/4] Expand SMART include compartment tests and Practitioner compartment equivalents Tests: iterate and wildcard include/revinclude leaks, SMART v2 granular scopes with includes, positive root/universal inclusion, multi-parameter dedup, sort (_lastUpdated) with includes, and compartment-definition guard tests for Patient and Practitioner. New leak-enforcement tests skip on Cosmos DB (fix is SQL-only; Cosmos is deprecated) and the compartment count assertion is now datastore-dependent. Equivalence map: cover every resolve()-based (unmaterialized) compartment parameter found in the R4 definitions - Patient adds AuditEvent-patient (agent, entity), Basic/Invoice/MeasureReport-patient (subject), Person-patient (link), Provenance-patient (target); Practitioner adds Encounter-practitioner (participant) and Person-practitioner (link). EpisodeOfCare-care-manager has no materialized equivalent and is documented as a known gap. Fixture: enable CoreFeatureConfiguration.SupportsIncludes in the SQL integration fixture (production SQL configuration); without it the includes continuation token is never created and the includes-paging test could never pass. The sort-by-search-parameter variant is blocked by a pre-existing defect (#5672) and noted in the test. Co-Authored-By: Claude Fable 5 --- docs/SmartIncludeCandidateAuthorization.md | 25 +- .../SqlCompartmentSearchRewriter.cs | 22 + .../Features/Search/SqlQueryGeneratorTests.cs | 136 ++++- .../TestFiles/R4/SmartCompartmentLeak.json | 71 +++ .../Features/Smart/SmartSearchTests.cs | 524 +++++++++++++++++- .../SqlServerFhirStorageTestsFixture.cs | 4 +- 6 files changed, 773 insertions(+), 9 deletions(-) diff --git a/docs/SmartIncludeCandidateAuthorization.md b/docs/SmartIncludeCandidateAuthorization.md index 9d3329c7e8..c56a94f8f9 100644 --- a/docs/SmartIncludeCandidateAuthorization.md +++ b/docs/SmartIncludeCandidateAuthorization.md @@ -176,10 +176,24 @@ tree for the request's `SmartCompartmentSearchExpression`. It then: 6. Sorts resource types and parameter URLs before creating immutable arrays, so generated SQL remains deterministic. -The current explicit equivalent mapping is: +The current explicit equivalent mapping covers every compartment-definition +parameter whose FHIRPath expression filters a polymorphic reference with +`resolve()` (such parameters are never materialized as `ReferenceSearchParam` +rows, because `resolve()` cannot be evaluated during indexing): ```text -clinical-patient -> patient, subject +# Patient compartment +clinical-patient -> patient, subject +AuditEvent-patient -> agent, entity +Basic-patient -> subject +Invoice-patient -> subject +MeasureReport-patient -> subject +Person-patient -> link +Provenance-patient -> target + +# Practitioner compartment (SMART user launch) +Encounter-practitioner -> participant +Person-practitioner -> link ``` Each candidate equivalent must exist for that resource type, be a supported @@ -187,6 +201,13 @@ reference parameter, and be capable of targeting the compartment root type. If no equivalent exists and the formal parameter itself is materialized, the formal parameter is retained. +Known gap: `EpisodeOfCare-care-manager` (Practitioner compartment) is +`resolve()`-based and has **no** materialized equivalent — `EpisodeOfCare` has +no other reference parameter over `careManager`. The formal parameter is +retained so the resource type still yields a rule, but it matches nothing until +the parameter becomes indexable. This mirrors the pre-existing behavior of the +primary compartment search and is not a regression. + This is deliberately narrower than looking for every parameter that can target Patient. For example, `Observation.focus` targets Patient but is not nominated by the Patient `CompartmentDefinition`, so it is not added to the context. diff --git a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs index 995c2ca5e6..e7ff754701 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/SqlCompartmentSearchRewriter.cs @@ -24,10 +24,32 @@ public class SqlCompartmentSearchRewriter : CompartmentSearchRewriter { private const string ClinicalPatientSearchParameterUrl = "http://hl7.org/fhir/SearchParameter/clinical-patient"; + // Compartment-definition search parameters whose FHIRPath expression filters a polymorphic reference with + // resolve() (e.g. "Encounter.participant.individual.where(resolve() is Practitioner)"). resolve() cannot be + // evaluated during indexing, so these parameters are never materialized as ReferenceSearchParam rows and a + // membership predicate keyed on them matches nothing. Each entry maps the unmaterialized parameter URL to + // the codes of materialized parameters that index the SAME elements; the equivalent is validated at + // resolution time (reference type, supported, targets the compartment type) and the membership predicate + // additionally constrains the reference to the compartment root's type and id, so an equivalent that + // indexes a broader element set cannot widen membership beyond the formal definition. + // Known gap: EpisodeOfCare-care-manager (Practitioner compartment) is resolve()-based and has NO + // materialized equivalent, so EpisodeOfCare membership in the Practitioner compartment cannot be + // enumerated until the parameter is indexable; the formal parameter is retained (matches nothing). private static readonly Dictionary> MaterializedEquivalentSearchParameterCodes = new Dictionary>(StringComparer.Ordinal) { + // Patient compartment. [ClinicalPatientSearchParameterUrl] = new[] { "patient", "subject" }, + ["http://hl7.org/fhir/SearchParameter/AuditEvent-patient"] = new[] { "agent", "entity" }, + ["http://hl7.org/fhir/SearchParameter/Basic-patient"] = new[] { "subject" }, + ["http://hl7.org/fhir/SearchParameter/Invoice-patient"] = new[] { "subject" }, + ["http://hl7.org/fhir/SearchParameter/MeasureReport-patient"] = new[] { "subject" }, + ["http://hl7.org/fhir/SearchParameter/Person-patient"] = new[] { "link" }, + ["http://hl7.org/fhir/SearchParameter/Provenance-patient"] = new[] { "target" }, + + // Practitioner compartment. + ["http://hl7.org/fhir/SearchParameter/Encounter-practitioner"] = new[] { "participant" }, + ["http://hl7.org/fhir/SearchParameter/Person-practitioner"] = new[] { "link" }, }; public SqlCompartmentSearchRewriter( diff --git a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs index bb016d7fad..7e1ae08ba7 100644 --- a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs +++ b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs @@ -283,6 +283,59 @@ public void GivenSmartCompartmentInclude_WhenSqlGenerated_ThenCandidateMembershi _fhirModel.Received(1).GetSearchParamId(membershipParameterUrl); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void GivenIncludeWithoutSmartCompartmentMembership_WhenSqlGenerated_ThenNoCandidatePredicateIsEmitted(bool reversed) + { + // Arrange - same include shape as the SMART compartment test, but no membership attached + // (non-SMART request). The candidate authorization predicate must not appear. + var includeParameterUrl = new Uri("http://hl7.org/fhir/SearchParameter/Observation-subject"); + var includeParameter = new SearchParameterInfo( + "subject", + "subject", + SearchParamType.Reference, + includeParameterUrl, + null, + "Observation.subject", + ["Patient"]); + var includeExpression = new IncludeExpression( + ["Observation"], + includeParameter, + "Observation", + "Patient", + null, + false, + reversed, + false); + SqlRootExpression sqlExpression = new( + [ + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.All), + new SearchParamTableExpression(IncludeQueryGenerator.Instance, includeExpression, SearchParamTableExpressionKind.Include), + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.IncludeLimit), + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.IncludeUnionAll), + ], + []); + SearchOptions searchOptions = new() + { + IncludeCount = 1, + MaxItemCount = 10, + Sort = [], + ResourceVersionTypes = ResourceVersionType.Latest, + }; + + ConfigureResourceTypeIds(); + _fhirModel.GetSearchParamId(includeParameterUrl).Returns((short)40); + + // Act + _queryGenerator.VisitSqlRoot(sqlExpression, searchOptions); + + // Assert + string generatedSql = _strBuilder.ToString(); + Assert.DoesNotContain("smartCompartmentMembership", generatedSql); + Assert.DoesNotContain("smartCompartmentRoot", generatedSql); + } + [Fact] public void GivenObservationCompartmentDefinition_WhenMembershipCreated_ThenFocusIsNotAMembershipParameter() { @@ -394,6 +447,82 @@ public void GivenClinicalPatientParameterWithoutEquivalent_WhenMembershipCreated Assert.Equal(clinicalPatient.Url, Assert.Single(rule.SearchParameterUrls)); } + [Fact] + public void GivenUnmaterializedEncounterPractitionerParameter_WhenPractitionerMembershipCreated_ThenParticipantEquivalentIsUsed() + { + // Arrange - Encounter-practitioner is resolve()-based (never materialized); Encounter-participant + // indexes the same element (Encounter.participant.individual) and must be used instead. + var practitioner = new SearchParameterInfo( + "practitioner", + "practitioner", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/Encounter-practitioner"), + null, + "Encounter.participant.individual.where(resolve() is Practitioner)", + ["Practitioner"]); + var participant = new SearchParameterInfo( + "participant", + "participant", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/Encounter-participant"), + null, + "Encounter.participant.individual", + ["Practitioner", "RelatedPerson"]); + SqlCompartmentSearchRewriter rewriter = CreateCompartmentRewriter( + "Encounter", + ["practitioner"], + new Dictionary + { + ["practitioner"] = practitioner, + ["participant"] = participant, + }, + CompartmentType.Practitioner); + + // Act + SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create( + Expression.SmartCompartmentSearch("Practitioner", "practitioner-a", "Encounter"), + rewriter); + + // Assert + SmartCompartmentMembershipRule rule = Assert.Single(membership.MembershipRules); + Assert.Equal("Encounter", rule.ResourceType); + Assert.Equal(participant.Url, Assert.Single(rule.SearchParameterUrls)); + Assert.DoesNotContain(practitioner.Url, rule.SearchParameterUrls); + } + + [Fact] + public void GivenEpisodeOfCareCareManagerWithoutEquivalent_WhenPractitionerMembershipCreated_ThenFormalParameterIsRetained() + { + // Arrange - EpisodeOfCare-care-manager is resolve()-based and has NO materialized equivalent + // (a known gap; see SqlCompartmentSearchRewriter). The formal parameter must be retained so the + // resource type still yields a rule instead of silently disappearing from the compartment. + var careManager = new SearchParameterInfo( + "care-manager", + "care-manager", + SearchParamType.Reference, + new Uri("http://hl7.org/fhir/SearchParameter/EpisodeOfCare-care-manager"), + null, + "EpisodeOfCare.careManager.where(resolve() is Practitioner)", + ["Practitioner"]); + SqlCompartmentSearchRewriter rewriter = CreateCompartmentRewriter( + "EpisodeOfCare", + ["care-manager"], + new Dictionary + { + ["care-manager"] = careManager, + }, + CompartmentType.Practitioner); + + // Act + SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create( + Expression.SmartCompartmentSearch("Practitioner", "practitioner-a", "EpisodeOfCare"), + rewriter); + + // Assert + SmartCompartmentMembershipRule rule = Assert.Single(membership.MembershipRules); + Assert.Equal(careManager.Url, Assert.Single(rule.SearchParameterUrls)); + } + private void ConfigureResourceTypeIds() { var resourceTypeIds = new Dictionary(StringComparer.Ordinal) @@ -415,16 +544,17 @@ private void ConfigureResourceTypeIds() private static SqlCompartmentSearchRewriter CreateCompartmentRewriter( string resourceType, HashSet compartmentParameterCodes, - IReadOnlyDictionary searchParameters) + IReadOnlyDictionary searchParameters, + CompartmentType compartmentType = CompartmentType.Patient) { ICompartmentDefinitionManager compartmentDefinitionManager = Substitute.For(); - compartmentDefinitionManager.TryGetResourceTypes(CompartmentType.Patient, out Arg.Any>()) + compartmentDefinitionManager.TryGetResourceTypes(compartmentType, out Arg.Any>()) .Returns(call => { call[1] = new HashSet(StringComparer.Ordinal) { resourceType }; return true; }); - compartmentDefinitionManager.TryGetSearchParams(resourceType, CompartmentType.Patient, out Arg.Any>()) + compartmentDefinitionManager.TryGetSearchParams(resourceType, compartmentType, out Arg.Any>()) .Returns(call => { call[2] = compartmentParameterCodes; diff --git a/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json b/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json index acdb0545c2..fff9db24c4 100644 --- a/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json +++ b/src/Microsoft.Health.Fhir.Tests.Common/TestFiles/R4/SmartCompartmentLeak.json @@ -362,6 +362,77 @@ "method": "PUT", "url": "DiagnosticReport/smart-leak-parent-report" } + }, + { + "resource": { + "resourceType": "DiagnosticReport", + "id": "smart-leak-child-report", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-child" + }, + "result": [ + { + "reference": "Observation/smart-leak-child-obs-with-outside-focus" + } + ] + }, + "request": { + "method": "PUT", + "url": "DiagnosticReport/smart-leak-child-report" + } + }, + { + "resource": { + "resourceType": "Observation", + "id": "smart-leak-pract-obs", + "meta": { + "tag": [ + { + "system": "testTag", + "code": "smart-leak" + } + ] + }, + "status": "final", + "code": { + "coding": [ + { + "system": "http://loinc.org", + "code": "4548-4" + } + ] + }, + "subject": { + "reference": "Patient/smart-leak-parent" + }, + "performer": [ + { + "reference": "Practitioner/smart-practitioner-A" + } + ], + "effectiveDateTime": "2021-08-01" + }, + "request": { + "method": "PUT", + "url": "Observation/smart-leak-pract-obs" + } } ] } diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs index b2ea2c45bd..2cfd996508 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs @@ -26,6 +26,7 @@ using Microsoft.Health.Fhir.Core.Features.Resources.Patch; using Microsoft.Health.Fhir.Core.Features.Search; using Microsoft.Health.Fhir.Core.Features.Search.Converters; +using Microsoft.Health.Fhir.Core.Features.Search.Expressions; using Microsoft.Health.Fhir.Core.Features.Search.Parameters; using Microsoft.Health.Fhir.Core.Features.Search.Registry; using Microsoft.Health.Fhir.Core.Features.Search.SearchValues; @@ -379,6 +380,7 @@ public async Task GivenPatientScopeReadAll_WhenIncludeReferencesResourceOutsideC ModelInfoProvider.Instance.Version != FhirSpecification.R4 && ModelInfoProvider.Instance.Version != FhirSpecification.R4B, "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); // MSRC _include compartment leak reproduction. // The caller is a SMART patient-scoped user (patient/*.read) confined to Patient/smart-leak-child. @@ -412,6 +414,7 @@ public async Task GivenPatientScopeReadAll_WhenRevIncludeReturnsResourceOutsideC ModelInfoProvider.Instance.Version != FhirSpecification.R4 && ModelInfoProvider.Instance.Version != FhirSpecification.R4B, "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); // MSRC _revinclude compartment leak reproduction. // The caller is a SMART patient-scoped user (patient/*.read) confined to Patient/smart-leak-child. @@ -456,6 +459,7 @@ public async Task GivenPatientBScopeReadAll_WhenObservationSubjectPatientBFocusP ModelInfoProvider.Instance.Version != FhirSpecification.R4 && ModelInfoProvider.Instance.Version != FhirSpecification.R4B, "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); // Arrange ─ caller is patient-scoped to Patient B (smart-leak-parent). // smart-leak-focus-obs lives in Patient B's compartment (subject = Parent) and @@ -515,6 +519,7 @@ public async Task GivenPatientAScopeReadAll_WhenRevIncludeObservationFocusForPat ModelInfoProvider.Instance.Version != FhirSpecification.R4 && ModelInfoProvider.Instance.Version != FhirSpecification.R4B, "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). // smart-leak-focus-obs has focus = Patient A, but its subject = Patient B, so it @@ -548,6 +553,7 @@ public async Task GivenPatientAScopeReadAll_WhenIncludeObservationFocusOnInCompa ModelInfoProvider.Instance.Version != FhirSpecification.R4 && ModelInfoProvider.Instance.Version != FhirSpecification.R4B, "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). // smart-leak-child-obs-with-outside-focus is in Patient A's compartment (subject = Child) @@ -657,6 +663,7 @@ public async Task GivenPatientBScopeReadAll_WhenIncludesPagingCrossesUnauthorize ModelInfoProvider.Instance.Version != FhirSpecification.R4 && ModelInfoProvider.Instance.Version != FhirSpecification.R4B, "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); // Arrange. Target surrogate IDs put the unauthorized child Patient between the // authorized shared Practitioner and authorized parent Patient. @@ -697,6 +704,501 @@ public async Task GivenPatientBScopeReadAll_WhenIncludesPagingCrossesUnauthorize Assert.DoesNotContain(includedResources, result => result.Resource.ResourceId == "smart-leak-child"); } + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenIterativeIncludeCrossesCompartment_ThenOutOfCompartmentResourceIsNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). Two-hop chain: + // DiagnosticReport/smart-leak-child-report (subject = Child, in-compartment) + // --result--> Observation/smart-leak-child-obs-with-outside-focus (subject = Child, in-compartment) + // --focus--> Patient/smart-leak-parent (OUTSIDE the compartment). + // The iterative include hop must be compartment-checked just like the first hop. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child-report")); + query.Add(new Tuple("_include", "DiagnosticReport:result")); + query.Add(new Tuple("_include:iterate", "Observation:focus")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("DiagnosticReport", query, CancellationToken.None); + + // Assert ─ the in-compartment report and its in-compartment result Observation are returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "DiagnosticReport" && x.Resource.ResourceId == "smart-leak-child-report"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + + // The out-of-compartment Patient reached on the second (iterate) hop must NOT be disclosed. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenIterativeRevIncludeCrossesCompartment_ThenOutOfCompartmentResourceIsNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). Two-hop reverse chain: + // _revinclude=Observation:subject pulls the child's Observations (in-compartment), then + // _revinclude:iterate=DiagnosticReport:result pulls reports referencing those Observations: + // DiagnosticReport/smart-leak-child-report (subject = Child) → in-compartment, returned + // DiagnosticReport/smart-leak-parent-report (subject = Parent) → OUTSIDE, must not be disclosed. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child")); + query.Add(new Tuple("_revinclude", "Observation:subject")); + query.Add(new Tuple("_revinclude:iterate", "DiagnosticReport:result")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Patient", query, CancellationToken.None); + + // Assert ─ the patient, their Observations, and the in-compartment report are returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "DiagnosticReport" && x.Resource.ResourceId == "smart-leak-child-report"); + + // The parent's report references the child's Observation but is outside the compartment. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "DiagnosticReport" && x.Resource.ResourceId == "smart-leak-parent-report"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenWildcardInclude_ThenOnlyCompartmentResourcesReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). _include=* expands every + // reference of the in-compartment Coverage: beneficiary/payor = Child (compartment root, allowed), + // subscriber = Parent (outside the compartment, must not be disclosed). + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-coverage")); + query.Add(new Tuple("_include", "*")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Coverage", query, CancellationToken.None); + + // Assert + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Coverage" && x.Resource.ResourceId == "smart-leak-coverage"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenWildcardRevInclude_ThenOnlyCompartmentResourcesReturnedWithoutDuplicates() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). _revinclude=* expands every + // resource referencing the child. Observation/smart-leak-focus-obs references the child only via + // `focus` (its subject is the Parent), so it is NOT a compartment member and must not be disclosed. + // Coverage/smart-leak-coverage references the child through BOTH beneficiary and payor and must be + // returned exactly once. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child")); + query.Add(new Tuple("_revinclude", "*")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Patient", query, CancellationToken.None); + + // Assert ─ in-compartment referencing resources are returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Procedure" && x.Resource.ResourceId == "smart-leak-child-procedure"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "DiagnosticReport" && x.Resource.ResourceId == "smart-leak-child-report"); + + // Multi-parameter membership (beneficiary + payor) must not produce duplicates. + Assert.Single(results.Results, x => x.Resource.ResourceTypeName == "Coverage" && x.Resource.ResourceId == "smart-leak-coverage"); + + // The Observation referencing the child only via `focus` is outside the compartment. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-focus-obs"); + } + + [SkippableFact] + public async Task GivenSmartV2GranularScopesWithFilters_WhenIncludeReferencesResourceOutsideCompartment_ThenCompartmentIsStillEnforced() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ SMART v2 fine-grained scopes WITH search parameters + // (patient/Coverage.s?status=active + patient/Patient.s), confined to Patient A (smart-leak-child). + // This exercises the coexistence of the re-generated v2 scope union and the compartment candidate + // predicate in the same include statement. The Patient scope grants the Patient TYPE, but the + // compartment must still exclude the out-of-compartment subscriber (smart-leak-parent). + var scopeRestrictionCoverage = new ScopeRestriction("Coverage", Core.Features.Security.DataActions.Search, "patient", CreateSearchParams(("status", "active"))); + var scopeRestrictionPatient = new ScopeRestriction("Patient", Core.Features.Security.DataActions.Search, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestrictionCoverage, scopeRestrictionPatient }, true); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-coverage")); + query.Add(new Tuple("_include", "Coverage:subscriber")); + query.Add(new Tuple("_include", "Coverage:beneficiary")); + + // Act + var results = await _searchService.Value.SearchAsync("Coverage", query, CancellationToken.None); + + // Assert ─ the active in-compartment Coverage and the in-compartment beneficiary are returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Coverage" && x.Resource.ResourceId == "smart-leak-coverage"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + + // The subscriber is allowed by TYPE under the Patient scope but is outside the compartment. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenIncludeResolvesToCompartmentRootPatient_ThenPatientIsReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ positive path for the compartment-root branch of the candidate predicate: + // including the subject of the child's own Observation must return the child Patient itself. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-child-obs")); + query.Add(new Tuple("_include", "Observation:subject")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenIncludeResolvesToUniversalResourceType_ThenUniversalResourceIsReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Arrange ─ positive path for the shared/universal-type branch of the candidate predicate: + // caller is patient-scoped to Patient B (smart-leak-parent); smart-leak-shared-focus-obs is in + // B's compartment (subject = Parent) and its focus references a Practitioner, a universal type + // that every SMART compartment may read. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-shared-focus-obs")); + query.Add(new Tuple("_include", "Observation:focus")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-parent"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-shared-focus-obs"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Practitioner" && x.Resource.ResourceId == "smart-practitioner-A"); + } + + [SkippableFact] + public async Task GivenPatientScopeReadAll_WhenSortedSearchWithInclude_ThenCompartmentIsEnforcedOnIncludes() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is patient-scoped to Patient A (smart-leak-child). A sorted search emits the + // includes after the sorted filtered-data split, so the include compartment predicate must hold + // there too: the subject include resolves to the compartment root (allowed) while the focus include + // of smart-leak-child-obs-with-outside-focus points at the Parent (denied). + // NOTE: this test sorts by _lastUpdated. Sorting by a search parameter value (e.g. _sort=date) + // combined with a SMART compartment returns an empty result set even WITHOUT includes — a + // pre-existing defect on main in the sort-value/union interplay, unrelated to include + // authorization. Extend this test to _sort= once that defect is fixed. + // Tracked by https://github.com/microsoft/fhir-server/issues/5672. + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "patient"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-leak-child"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Patient"; + + var query = new List>(); + query.Add(new Tuple("_tag", "testTag|smart-leak")); + query.Add(new Tuple("_sort", "-_lastUpdated")); + query.Add(new Tuple("_include", "Observation:subject")); + query.Add(new Tuple("_include", "Observation:focus")); + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert ─ both in-compartment Observations are returned and the subject include resolves to the child. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-child-obs-with-outside-focus"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-child"); + + // The Observation whose subject is the Parent must not be a match, and the Parent must not be included. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-focus-obs"); + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPatientCompartmentDefinition_WhenMaterializedMembershipResolved_ThenEveryResolvableCompartmentTypeIsCovered() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Guard for the materialized-equivalence map in SqlCompartmentSearchRewriter: every Patient + // compartment resource type whose definition resolves to at least one supported reference search + // parameter must yield a membership rule. A compartment definition or search parameter change that + // silently drops a resource type from SMART compartment membership fails here instead of shipping. + (CompartmentDefinitionManager compartmentDefinitionManager, IReadOnlyDictionary> membership) = + await ResolveMaterializedMembershipAsync("Patient"); + + AssertMaterializedMembershipCoversResolvableTypes(compartmentDefinitionManager, membership, Microsoft.Health.Fhir.ValueSets.CompartmentType.Patient); + + // Types mapped through the unmaterialized clinical-patient parameter must resolve to their + // materialized type-specific equivalents. + foreach (string subjectMappedType in new[] { "Encounter", "Procedure", "ImagingStudy" }) + { + Assert.Contains( + membership[subjectMappedType], + parameter => parameter.Url.AbsoluteUri.EndsWith($"/{subjectMappedType}-subject", StringComparison.Ordinal) || parameter.Code == "subject"); + } + + // Resource-specific resolve()-based parameters must resolve to the equivalents that index the + // same elements (see MaterializedEquivalentSearchParameterCodes). + Assert.Contains(membership["Provenance"], parameter => parameter.Code == "target"); + Assert.Contains(membership["AuditEvent"], parameter => parameter.Code == "agent"); + Assert.Contains(membership["AuditEvent"], parameter => parameter.Code == "entity"); + Assert.Contains(membership["Person"], parameter => parameter.Code == "link"); + + // Membership must remain the FORMAL compartment definition: Observation is a member via + // subject/performer only — `focus` merely references the patient and must never be membership. + Assert.DoesNotContain(membership["Observation"], parameter => parameter.Code == "focus"); + } + + [SkippableFact] + public async Task GivenPractitionerCompartmentDefinition_WhenMaterializedMembershipResolved_ThenEveryResolvableCompartmentTypeIsCovered() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + + // Same guard as the Patient compartment, for Practitioner (SMART user launch context). + (CompartmentDefinitionManager compartmentDefinitionManager, IReadOnlyDictionary> membership) = + await ResolveMaterializedMembershipAsync("Practitioner"); + + AssertMaterializedMembershipCoversResolvableTypes(compartmentDefinitionManager, membership, Microsoft.Health.Fhir.ValueSets.CompartmentType.Practitioner); + + // Encounter-practitioner and Person-practitioner are resolve()-based (never materialized) and must + // resolve to the equivalents indexing the same elements. + Assert.Contains(membership["Encounter"], parameter => parameter.Code == "participant"); + Assert.DoesNotContain(membership["Encounter"], parameter => parameter.Code == "practitioner"); + Assert.Contains(membership["Person"], parameter => parameter.Code == "link"); + + // Known gap: EpisodeOfCare-care-manager is resolve()-based with no materialized equivalent; the + // formal parameter is retained so the rule exists but matches nothing until the parameter is indexable. + Assert.Contains(membership["EpisodeOfCare"], parameter => parameter.Code == "care-manager"); + + // Observation is a member of the Practitioner compartment via `performer` only. + Assert.Contains(membership["Observation"], parameter => parameter.Code == "performer"); + Assert.DoesNotContain(membership["Observation"], parameter => parameter.Code == "focus"); + } + + [SkippableFact] + public async Task GivenPractitionerScopeReadAll_WhenIncludeReferencesPatientOutsideCompartment_ThenPatientIsNotReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is user-scoped to Practitioner/smart-practitioner-A (SMART user launch). + // Observation/smart-leak-pract-obs is in the practitioner's compartment (performer = Practitioner A) + // but its subject is Patient/smart-leak-parent, who has no general-practitioner and is therefore + // OUTSIDE the practitioner's compartment. Expanding _include=Observation:subject must not disclose them. + var query = new List>(); + query.Add(new Tuple("_id", "smart-leak-pract-obs")); + query.Add(new Tuple("_include", "Observation:subject")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "user"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-practitioner-A"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Practitioner"; + + // Act + var results = await _searchService.Value.SearchAsync("Observation", query, CancellationToken.None); + + // Assert ─ the in-compartment Observation (member via performer) is returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-pract-obs"); + + // The out-of-compartment Patient reached via Observation.subject must NOT be disclosed. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-leak-parent"); + } + + [SkippableFact] + public async Task GivenPractitionerScopeReadAll_WhenRevIncludeObservations_ThenOnlyPerformerMembersAreReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is user-scoped to Practitioner/smart-practitioner-A. + // smart-leak-pract-obs references the practitioner via `performer` (a Practitioner-compartment + // parameter for Observation) and must be returned. smart-leak-shared-focus-obs references the + // practitioner only via `focus`, which is NOT a compartment parameter, and must NOT be returned. + var query = new List>(); + query.Add(new Tuple("_id", "smart-practitioner-A")); + query.Add(new Tuple("_revinclude", "Observation:performer")); + query.Add(new Tuple("_revinclude", "Observation:focus")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "user"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-practitioner-A"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Practitioner"; + + // Act + var results = await _searchService.Value.SearchAsync("Practitioner", query, CancellationToken.None); + + // Assert ─ the practitioner (compartment root) and the performer-member Observation are returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Practitioner" && x.Resource.ResourceId == "smart-practitioner-A"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-pract-obs"); + + // The Observation referencing the practitioner only via `focus` is outside the compartment. + Assert.DoesNotContain(results.Results, x => x.Resource.ResourceTypeName == "Observation" && x.Resource.ResourceId == "smart-leak-shared-focus-obs"); + } + + [SkippableFact] + public async Task GivenPractitionerScopeReadAll_WhenEncounterMemberViaParticipantEquivalent_ThenEncounterAndInCompartmentSubjectAreReturned() + { + Skip.If( + ModelInfoProvider.Instance.Version != FhirSpecification.R4 && + ModelInfoProvider.Instance.Version != FhirSpecification.R4B, + "This test is only valid for R4 and R4B"); + SkipIfIncludeCompartmentEnforcementNotSupported(); + + // Arrange ─ caller is user-scoped to Practitioner/smart-practitioner-A. + // The Practitioner compartment maps Encounter via `practitioner` (Encounter-practitioner), which is + // resolve()-based and never materialized; membership must resolve through the Encounter-participant + // equivalent. smart-encounter-A1 has participant.individual = Practitioner A, and its subject + // (smart-patient-A) is also in the practitioner's compartment via general-practitioner. + var query = new List>(); + query.Add(new Tuple("_id", "smart-encounter-A1")); + query.Add(new Tuple("_include", "Encounter:subject")); + + var scopeRestriction = new ScopeRestriction(KnownResourceTypes.All, Core.Features.Security.DataActions.Read, "user"); + + ConfigureFhirRequestContext(_contextAccessor, new List() { scopeRestriction }); + _contextAccessor.RequestContext.AccessControlContext.CompartmentId = "smart-practitioner-A"; + _contextAccessor.RequestContext.AccessControlContext.CompartmentResourceType = "Practitioner"; + + // Act + var results = await _searchService.Value.SearchAsync("Encounter", query, CancellationToken.None); + + // Assert ─ the Encounter is discoverable through the participant equivalent, and the include of its + // in-compartment subject is returned. + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Encounter" && x.Resource.ResourceId == "smart-encounter-A1"); + Assert.Contains(results.Results, x => x.Resource.ResourceTypeName == "Patient" && x.Resource.ResourceId == "smart-patient-A"); + } + + private async Task<(CompartmentDefinitionManager CompartmentDefinitionManager, IReadOnlyDictionary> Membership)> ResolveMaterializedMembershipAsync(string compartmentType) + { + var compartmentDefinitionManager = new CompartmentDefinitionManager(ModelInfoProvider.Instance); + await compartmentDefinitionManager.StartAsync(CancellationToken.None); + var rewriter = new SqlCompartmentSearchRewriter( + new Lazy(() => compartmentDefinitionManager), + new Lazy(() => _fixture.SearchParameterDefinitionManager)); + + IReadOnlyDictionary> membership = + rewriter.GetMaterializedCompartmentSearchParameters(compartmentType, filteredResourceTypes: null, includeMaterializedEquivalents: true); + + return (compartmentDefinitionManager, membership); + } + + private void AssertMaterializedMembershipCoversResolvableTypes( + CompartmentDefinitionManager compartmentDefinitionManager, + IReadOnlyDictionary> membership, + Microsoft.Health.Fhir.ValueSets.CompartmentType compartmentType) + { + Assert.True(compartmentDefinitionManager.TryGetResourceTypes(compartmentType, out HashSet compartmentResourceTypes)); + + var uncoveredResourceTypes = new List(); + foreach (string resourceType in compartmentResourceTypes) + { + if (!compartmentDefinitionManager.TryGetSearchParams(resourceType, compartmentType, out HashSet parameterCodes)) + { + continue; + } + + bool hasResolvableReferenceParameter = parameterCodes.Any(code => + _fixture.SearchParameterDefinitionManager.TryGetSearchParameter(resourceType, code, out SearchParameterInfo parameter) + && parameter.Type == Microsoft.Health.Fhir.ValueSets.SearchParamType.Reference + && parameter.IsSupported); + + if (hasResolvableReferenceParameter && !membership.ContainsKey(resourceType)) + { + uncoveredResourceTypes.Add(resourceType); + } + } + + Assert.True(uncoveredResourceTypes.Count == 0, $"{compartmentType} compartment resource types silently dropped from SMART membership: {string.Join(", ", uncoveredResourceTypes)}"); + } + [SkippableFact] public async Task GivenScopesWithReadForObservation_WhenChainedSearchWithPatientName_ThrowsInvalidSearchException() { @@ -1056,9 +1558,12 @@ public async Task GivenFhirUserClaimPatient_WhenAllResourcesRequested_UniversalR // The Patient compartment definition maps those types to the common `patient` search parameter // (clinical-patient), whose FHIRPath uses resolve() and is therefore not materialized as an index // row, so before the SMART compartment fix these in-compartment resources were silently dropped. - // The SMART compartment now also matches on the materialized type-specific reference parameters, - // so the patient's own Encounter, ImagingStudy and Procedure are correctly included (39 -> 42). - Assert.Equal(42, results.Results.Count()); + // On SQL the SMART compartment now also matches on the materialized type-specific reference + // parameters, so the patient's own Encounter, ImagingStudy and Procedure are correctly included + // (39 -> 42). Cosmos DB uses its own compartment rewriter, which was not changed (deprecated), + // so it still returns the pre-fix undercount of 39. + int expectedCount = string.Equals(_fixture.FhirRuntimeConfiguration.DataStore, KnownDataStores.SqlServer, StringComparison.OrdinalIgnoreCase) ? 42 : 39; + Assert.Equal(expectedCount, results.Results.Count()); } [SkippableFact] @@ -1368,6 +1873,19 @@ private static string CreateSmartV2TestResourceId(string scenario) return $"smart-v2-{scenario}-{Guid.NewGuid():N}"; } + // The SMART _include/_revinclude compartment enforcement (candidate authorization) is implemented in the + // SQL data provider only. Cosmos DB resolves includes by direct id lookups with no compartment predicate + // (and does not support iterative includes or the $includes operation), so the leak is NOT fixed there. + // Cosmos DB is deprecated and out of scope for this fix — see + // docs/arch/adr-2607-smart-include-compartment-scoping.md. A follow-up is required if Cosmos SMART + // includes ever need the same guarantee. + private void SkipIfIncludeCompartmentEnforcementNotSupported() + { + Skip.If( + !string.Equals(_fixture.FhirRuntimeConfiguration.DataStore, KnownDataStores.SqlServer, StringComparison.OrdinalIgnoreCase), + "SMART include/revinclude compartment enforcement is implemented in the SQL data provider only (Cosmos DB is deprecated; see ADR-2607)."); + } + private void ConfigureFhirRequestContext( RequestContextAccessor contextAccessor, ICollection scopes, diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs index f6865c655d..b732142ff6 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/SqlServerFhirStorageTestsFixture.cs @@ -103,7 +103,9 @@ internal SqlServerFhirStorageTestsFixture(int maximumSupportedSchemaVersion, str SchemaInformation = new SchemaInformation(SchemaVersionConstants.Min, maximumSupportedSchemaVersion); - _options = coreFeatures ?? Options.Create(new CoreFeatureConfiguration()); + // SupportsIncludes matches the production SQL configuration and enables the $includes continuation + // path (IncludesContinuationToken); without it, include-paging tests can never receive a token. + _options = coreFeatures ?? Options.Create(new CoreFeatureConfiguration { SupportsIncludes = true }); } public string TestConnectionString { get; private set; } From af8f0bc8f442a172f64c65e2799dd4961542ce1a Mon Sep 17 00:00:00 2001 From: Jared Erwin Date: Thu, 16 Jul 2026 11:42:08 -0700 Subject: [PATCH 4/4] Review cleanups and fail-open detection for SMART include authorization - Remove orphaned sentinel comment in SearchOptionsFactory left from an earlier iteration. - Remove unused skipJoinFromPreviousUnions parameter from AppendNewSetOfUnionAllTableExpressions and the no-op save/restore of _unionAggregateCTEIndex around the SMART v2 include regeneration. - Add a fail-open detector in AttachSmartCompartmentMembership: if the request context is compartment-bound (fhirUser-derived CompartmentResourceType is set) and the expression contains include CTEs but no membership context could be constructed, log critical. System scopes never set CompartmentResourceType, so they cannot trip the detector. - Add a canary unit test exercising the factory -> attach -> generate chain with the SearchOptionsFactory-shaped expression tree, so any regression that silently drops include authorization fails CI. Co-Authored-By: Claude Fable 5 --- .../Features/Search/SearchOptionsFactory.cs | 3 - .../Features/Search/SqlQueryGeneratorTests.cs | 88 +++++++++++++++++++ .../QueryGenerators/SqlQueryGenerator.cs | 8 +- .../Features/Search/SqlServerSearchService.cs | 21 ++++- 4 files changed, 110 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs index a3a36bec92..d35ca615fe 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs @@ -33,9 +33,6 @@ public class SearchOptionsFactory : ISearchOptionsFactory { private static readonly string SupportedTotalTypes = $"'{TotalType.Accurate}', '{TotalType.None}'".ToLower(CultureInfo.CurrentCulture); - // Sentinel emitted by IncludeExpression.Produces for a reversed wildcard revinclude (e.g. _revinclude=*:*) - // whose SMART scope is not restricted to specific resource types. It stands in for the open-ended set of - // resource types that may reference the compartment root. See the ExpressionParser reversed wildcard handling. private readonly IExpressionParser _expressionParser; private readonly RequestContextAccessor _contextAccessor; private readonly ISortingValidator _sortingValidator; diff --git a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs index 7e1ae08ba7..843a479849 100644 --- a/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs +++ b/src/Microsoft.Health.Fhir.SqlServer.UnitTests/Features/Search/SqlQueryGeneratorTests.cs @@ -523,6 +523,94 @@ public void GivenEpisodeOfCareCareManagerWithoutEquivalent_WhenPractitionerMembe Assert.Equal(careManager.Url, Assert.Single(rule.SearchParameterUrls)); } + [Fact] + public void GivenSmartCompartmentExpressionInsideAndTree_WhenMembershipCreatedAndSqlGenerated_ThenIncludeCandidatePredicateIsEmitted() + { + // Canary for the SMART include enforcement chain. SqlServerSearchService derives the membership + // context by locating the SmartCompartmentSearchExpression inside the core expression tree and + // attaching the result to the SqlRootExpression; the query generator emits the candidate + // authorization predicate only when that context is present. If any link breaks — the compartment + // expression gets wrapped in a node the factory cannot traverse, a rewriter drops the attached + // context, or the generator stops honoring it — include CTEs silently revert to the pre-fix + // cross-compartment leak. This test exercises the factory -> attach -> generate chain end to end + // with the same tree shape SearchOptionsFactory produces (smart node as a child of the top-level And). + var includeParameterUrl = new Uri("http://hl7.org/fhir/SearchParameter/Observation-subject"); + var membershipParameterUrl = new Uri("http://hl7.org/fhir/SearchParameter/DiagnosticReport-subject"); + var membershipParameter = new SearchParameterInfo( + "subject", + "subject", + SearchParamType.Reference, + membershipParameterUrl, + null, + "DiagnosticReport.subject", + ["Patient"]); + SqlCompartmentSearchRewriter rewriter = CreateCompartmentRewriter( + "DiagnosticReport", + ["subject"], + new Dictionary + { + ["subject"] = membershipParameter, + }); + + Expression coreExpression = Expression.And( + new SearchParameterExpression(new SearchParameterInfo("_type", "_type"), new StringExpression(StringOperator.Equals, FieldName.String, null, "Observation", false)), + Expression.SmartCompartmentSearch("Patient", "patient-a", "Observation")); + + SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create(coreExpression, rewriter); + + Assert.NotNull(membership); + Assert.Equal("Patient", membership.CompartmentResourceType); + Assert.Equal("patient-a", membership.CompartmentResourceId); + SmartCompartmentMembershipRule rule = Assert.Single(membership.MembershipRules); + Assert.Equal("DiagnosticReport", rule.ResourceType); + Assert.Equal(membershipParameterUrl, Assert.Single(rule.SearchParameterUrls)); + + var includeParameter = new SearchParameterInfo( + "subject", + "subject", + SearchParamType.Reference, + includeParameterUrl, + null, + "Observation.subject", + ["Patient"]); + var includeExpression = new IncludeExpression( + ["Observation"], + includeParameter, + "Observation", + "Patient", + null, + false, + false, + false); + SqlRootExpression sqlExpression = new SqlRootExpression( + [ + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.All), + new SearchParamTableExpression(IncludeQueryGenerator.Instance, includeExpression, SearchParamTableExpressionKind.Include), + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.IncludeLimit), + new SearchParamTableExpression(null, null, SearchParamTableExpressionKind.IncludeUnionAll), + ], + []).WithSmartCompartmentMembership(membership); + SearchOptions searchOptions = new() + { + IncludeCount = 1, + MaxItemCount = 10, + Sort = [], + ResourceVersionTypes = ResourceVersionType.Latest, + }; + + ConfigureResourceTypeIds(); + _fhirModel.GetSearchParamId(includeParameterUrl).Returns((short)40); + _fhirModel.GetSearchParamId(membershipParameterUrl).Returns((short)41); + + _queryGenerator.VisitSqlRoot(sqlExpression, searchOptions); + + string generatedSql = _strBuilder.ToString(); + Assert.Contains("smartCompartmentMembership.ResourceTypeId = refTarget.ResourceTypeId", generatedSql); + Assert.Contains("smartCompartmentMembership.ResourceSurrogateId = refTarget.ResourceSurrogateId", generatedSql); + Assert.Contains("smartCompartmentMembership.BaseUri IS NULL", generatedSql); + _fhirModel.Received(1).GetSearchParamId(membershipParameterUrl); + } + private void ConfigureResourceTypeIds() { var resourceTypeIds = new Dictionary(StringComparer.Ordinal) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs index 6cb1288886..c99fed4164 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs @@ -200,12 +200,9 @@ public override object VisitSqlRoot(SqlRootExpression expression, SearchOptions sb.AppendLine("OPTION (RECOMPILE)"); sb.AppendLine($";WITH"); int saveTableExpressionCounter = _tableExpressionCounter; - int saveUnionAggregateCTEIndex = _unionAggregateCTEIndex; _tableExpressionCounter = smartV2TableCounter; AppendSmartNewSetOfUnionAllTableExpressions(context, smartV2UnionExpression, smartV2QueryGenerator, true); - _tableExpressionCounter = saveTableExpressionCounter; - _unionAggregateCTEIndex = saveUnionAggregateCTEIndex; sb.AppendLine(); sb.AppendLine($",cte{_tableExpressionCounter} AS (SELECT * FROM @FilteredData)"); sb.Append(","); // add comma back @@ -1569,7 +1566,7 @@ private SearchParameterQueryGeneratorContext GetContext(string tableAlias = null return new SearchParameterQueryGeneratorContext(StringBuilder, Parameters, Model, _schemaInfo, isAsyncOperation: _isAsyncOperation, tableAlias); } - private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, UnionExpression unionExpression, SearchParamTableExpressionQueryGenerator defaultQueryGenerator, bool skipJoinFromPreviousUnions = false) + private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, UnionExpression unionExpression, SearchParamTableExpressionQueryGenerator defaultQueryGenerator) { if (unionExpression.Operator != UnionOperator.All) { @@ -1595,7 +1592,6 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union // Create a final CTE aggregating results from all previous CTEs. StringBuilder.Append(TableExpressionName(++_tableExpressionCounter)).AppendLine(" AS").AppendLine("("); - for (int tableExpressionId = firstInclusiveTableExpressionId; tableExpressionId <= lastInclusiveTableExpressionId; tableExpressionId++) { using (StringBuilder.Indent()) @@ -1614,7 +1610,7 @@ private void AppendNewSetOfUnionAllTableExpressions(SearchOptions context, Union StringBuilder.Append(")"); // check for a previous union all, and if so, join the new union all with the previous one - if (!skipJoinFromPreviousUnions && _unionAggregateCTEIndex > -1) + if (_unionAggregateCTEIndex > -1) { var prevUnionAggregateTableName = TableExpressionName(_unionAggregateCTEIndex); var currentUnionAggregateTableName = TableExpressionName(_tableExpressionCounter); diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs index 4e8480ad38..4492bfd127 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs @@ -2523,7 +2523,26 @@ private SqlRootExpression AttachSmartCompartmentMembership(SqlRootExpression sql SmartCompartmentMembershipContext membership = SmartCompartmentMembershipContextFactory.Create(coreExpression, sqlCompartmentSearchRewriter); - return membership == null ? sqlExpression : sqlExpression.WithSmartCompartmentMembership(membership); + if (membership == null) + { + // Fail-open detector. AccessControlContext.CompartmentResourceType is populated only from a + // parsed fhirUser claim (system scopes never set it), and SearchOptionsFactory adds a + // SmartCompartmentSearchExpression whenever it is set — so a compartment-bound request whose + // expression yields no membership context means the include CTEs are about to be generated + // WITHOUT compartment authorization (the pre-fix _include/_revinclude leak). This should be + // unreachable; if it ever fires, a rewrite step is hiding or dropping the compartment expression. + if (!string.IsNullOrWhiteSpace(_requestContextAccessor.RequestContext?.AccessControlContext?.CompartmentResourceType) + && sqlExpression.SearchParamTableExpressions.Any(t => t.Kind == SearchParamTableExpressionKind.Include)) + { + _logger.LogCritical( + "SMART {CompartmentResourceType} compartment restriction is active but no include authorization context was constructed; _include/_revinclude results are not compartment-filtered for this request.", + _requestContextAccessor.RequestContext.AccessControlContext.CompartmentResourceType); + } + + return sqlExpression; + } + + return sqlExpression.WithSmartCompartmentMembership(membership); } ///