Fix SMART system search scope filtering - #5788
Open
Jared Erwin (feordin) wants to merge 1 commit into
Open
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 43c8b4b3-3e41-45af-b117-06bce9ca5517
Jared Erwin (feordin)
added
Bug-Security
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One access-control related behavioral regression was identified (include parsing can be silently skipped under FGAC in cases that previously threw) and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes SMART (v2) system-level search authorization by preserving the resource/action pairing for scope restrictions when requests don’t have a concrete route resource type, ensuring search vs read-by-id semantics are enforced consistently across _type filtering, include/revinclude validation, and Cosmos DB include queries.
Changes:
- Introduces action-aware scope filtering (
ScopeDataActions) so searches apply only.read/.s-equivalent restrictions, while direct reads by ID apply.read/.r-equivalent restrictions. - Propagates the action context through search option creation, expression access checks, and Cosmos include query scope filtering.
- Adds/extends unit + integration test coverage for mixed wildcard read-by-id + resource-scoped search scenarios.
File summaries
| File | Description |
|---|---|
| test/Microsoft.Health.Fhir.Shared.Tests.Integration/Features/Smart/SmartSearchTests.cs | Adds an integration test covering mixed wildcard read-by-id + Patient search scopes in system searches. |
| src/Microsoft.Health.Fhir.Shared.Core/Features/Search/SearchOptionsFactory.cs | Adds ScopeDataActions, filters applicable SMART scope restrictions by action, and threads filtered scopes through include parsing and access checks. |
| src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Get/GetResourceHandler.cs | Ensures GET-by-id uses `Read |
| src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Search/SearchOptionsFactoryTests.cs | Adds tests validating action-aware scope filtering and include parsing behavior under mixed scopes. |
| src/Microsoft.Health.Fhir.CosmosDb/Features/Search/FhirCosmosSearchService.cs | Applies action-aware SMART scope filtering to Cosmos include/revinclude query filtering. |
| src/Microsoft.Health.Fhir.Core/Features/Search/SearchService.cs | Adds an overload to pass scopeDataActions into SearchOptionsFactory. |
| src/Microsoft.Health.Fhir.Core/Features/Search/SearchOptions.cs | Stores ScopeDataActions on SearchOptions and copies it in the copy ctor. |
| src/Microsoft.Health.Fhir.Core/Features/Search/ISearchService.cs | Adds a new overload for action-aware SMART scope filtering. |
| src/Microsoft.Health.Fhir.Core/Features/Search/ISearchOptionsFactory.cs | Adds a new overload for action-aware SMART scope filtering at options creation time. |
| src/Microsoft.Health.Fhir.Core/Features/Search/Access/ExpressionAccessControl.cs | Adds an overload to validate include/chained expressions against the action-filtered scope set. |
| src/Microsoft.Health.Fhir.Core/Features/Context/ScopeRestriction.cs | Adds AllowsAny(DataActions) helper used to filter restrictions by requested actions. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+737
to
753
| if (allowedResourceTypesByScope != null && | ||
| !allowedResourceTypesByScope.Contains(KnownResourceTypes.All)) | ||
| { | ||
| string includeSourceResourceType = p.query?.Split(':')[0]; | ||
| if (!string.Equals(includeSourceResourceType, "*", StringComparison.Ordinal) && | ||
| !string.Equals(includeSourceResourceType, KnownResourceTypes.All, StringComparison.Ordinal) && | ||
| !allowedResourceTypesByScope.Contains(includeSourceResourceType)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| includeResourceTypeList = includeResourceTypeList.Intersect(allowedResourceTypesByScope).ToArray(); | ||
| if (includeResourceTypeList.Length == 0) | ||
| { | ||
| return null; | ||
| } | ||
| } |
Comment on lines
+22
to
+32
| SearchOptions Create( | ||
| string resourceType, | ||
| IReadOnlyList<Tuple<string, string>> queryParameters, | ||
| DataActions scopeDataActions, | ||
| bool isAsyncOperation = false, | ||
| ResourceVersionType resourceVersionTypes = ResourceVersionType.Latest, | ||
| bool onlyIds = false, | ||
| bool isIncludesOperation = false) | ||
| { | ||
| return Create(resourceType, queryParameters, isAsyncOperation, resourceVersionTypes, onlyIds, isIncludesOperation); | ||
| } |
Comment on lines
+42
to
+54
| /// <summary> | ||
| /// Searches resources using only SMART scope restrictions that permit one of the supplied actions. | ||
| /// </summary> | ||
| /// <param name="resourceType">The resource type that should be searched.</param> | ||
| /// <param name="queryParameters">The search queries.</param> | ||
| /// <param name="scopeDataActions">The data actions that may authorize the search.</param> | ||
| /// <param name="cancellationToken">The cancellation token.</param> | ||
| /// <param name="isAsyncOperation">Whether the search is part of an async operation.</param> | ||
| /// <param name="resourceVersionTypes">Which version types to include in search.</param> | ||
| /// <param name="onlyIds">Whether to return only resource IDs.</param> | ||
| /// <param name="isIncludesOperation">Whether the search is querying remaining include resources.</param> | ||
| /// <returns>A <see cref="SearchResult"/> representing the result.</returns> | ||
| Task<SearchResult> SearchAsync( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5788 +/- ##
==========================================
+ Coverage 78.28% 78.91% +0.63%
==========================================
Files 1016 1018 +2
Lines 36940 37025 +85
Branches 5619 5625 +6
==========================================
+ Hits 28920 29220 +300
+ Misses 6644 6418 -226
- Partials 1376 1387 +11 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Preserves the resource/action pairing for SMART clinical scopes when search requests do not carry a concrete route resource type.
.reador SMART v2.srestrictions..reador SMART v2.rrestrictions._type, all-resource searches, history and compartment searches, include/revinclude validation, and Cosmos DB include queries.Related issues
N/A - addresses an externally reported SMART scope authorization issue.
Testing
FHIR Team Checklist
Semver Change (docs)
Patch