Fix SMART system search scope filtering

src/Microsoft.Health.Fhir.Core/Features/Context/ScopeRestriction.cs

@@ -35,6 +35,16 @@ public ScopeRestriction(string resource, DataActions allowedAction, string user,
         // Finer-grained resource constraints using search parameters for SMART V2 compliance
         public SearchParams SearchParameters { get; }
 
+        /// <summary>
+        /// Determines whether this restriction permits any of the requested data actions.
+        /// </summary>
+        /// <param name="dataActions">The data actions to check.</param>
+        /// <returns><see langword="true"/> when at least one requested action is permitted.</returns>
+        public bool AllowsAny(DataActions dataActions)
+        {
+            return (AllowedDataAction & dataActions) != DataActions.None;
+        }
+
         public bool Equals(ScopeRestriction other)
         {
             if (other == null)

src/Microsoft.Health.Fhir.Core/Features/Search/Access/ExpressionAccessControl.cs

@@ -24,6 +24,15 @@ public ExpressionAccessControl(RequestContextAccessor<IFhirRequestContext> reque
     }
 
     public void CheckAndRaiseAccessExceptions(Expression expression)
+    {
+        CheckAndRaiseAccessExceptions(
+            expression,
+            _requestContextAccessor.RequestContext?.AccessControlContext?.AllowedResourceActions?.ToList());
+    }
+
+    public void CheckAndRaiseAccessExceptions(
+        Expression expression,
+        IReadOnlyCollection<ScopeRestriction> applicableScopeRestrictions)
     {
         if (expression == null)
         {
@@ -39,7 +48,7 @@ public void CheckAndRaiseAccessExceptions(Expression expression)
                     out IReadOnlyList<ChainedExpression> chainedExpressions,
                     out _))
             {
-                var validResourceTypes = _requestContextAccessor.RequestContext?.AccessControlContext.AllowedResourceActions.Select(r => r.Resource).ToHashSet();
+                var validResourceTypes = applicableScopeRestrictions?.Select(r => r.Resource).ToHashSet();
 
                 // check resource type restrictions from SMART clinical scopes
                 foreach (var type in chainedExpressions

src/Microsoft.Health.Fhir.Core/Features/Search/ISearchOptionsFactory.cs

@@ -5,6 +5,7 @@
 
 using System;
 using System.Collections.Generic;
+using Microsoft.Health.Fhir.Core.Features.Security;
 
 namespace Microsoft.Health.Fhir.Core.Features.Search
 {
@@ -18,6 +19,25 @@ SearchOptions Create(
             bool onlyIds = false,
             bool isIncludesOperation = false);
 
+        /// <summary>
+        /// Creates <see cref="SearchOptions"/> using only SMART scope restrictions that permit one of the supplied actions.
+        /// </summary>
+        /// <remarks>
+        /// The default interface implementation delegates to the overload that does not take <paramref name="scopeDataActions"/>,
+        /// so implementations should override this method to enforce action-specific scope filtering.
+        /// </remarks>
+        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);
+        }
+
         SearchOptions Create(
             string compartmentType,
             string compartmentId,

src/Microsoft.Health.Fhir.Core/Features/Search/ISearchService.cs

@@ -7,6 +7,7 @@
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Health.Fhir.Core.Features.Security;
 using Microsoft.Health.Fhir.Core.Models;
 
 namespace Microsoft.Health.Fhir.Core.Features.Search
@@ -38,6 +39,35 @@ Task<SearchResult> SearchAsync(
             bool onlyIds = false,
             bool isIncludesOperation = false);
 
+        /// <summary>
+        /// Searches resources using only SMART scope restrictions that permit one of the supplied actions.
+        /// </summary>
+        /// <remarks>
+        /// The default interface implementation delegates to the overload that does not take <paramref name="scopeDataActions"/>,
+        /// so implementations should override this method to enforce action-specific scope filtering.
+        /// </remarks>
+        /// <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(
+            string resourceType,
+            IReadOnlyList<Tuple<string, string>> queryParameters,
+            DataActions scopeDataActions,
+            CancellationToken cancellationToken,
+            bool isAsyncOperation = false,
+            ResourceVersionType resourceVersionTypes = ResourceVersionType.Latest,
+            bool onlyIds = false,
+            bool isIncludesOperation = false)
+        {
+            return SearchAsync(resourceType, queryParameters, cancellationToken, isAsyncOperation, resourceVersionTypes, onlyIds, isIncludesOperation);
+        }
+
         /// <summary>
         /// Searches the resources using the <paramref name="searchOptions"/>.
         /// </summary>

src/Microsoft.Health.Fhir.Core/Features/Search/SearchOptions.cs

@@ -6,6 +6,7 @@
 using System;
 using System.Collections.Generic;
 using Microsoft.Health.Fhir.Core.Features.Search.Expressions;
+using Microsoft.Health.Fhir.Core.Features.Security;
 using Microsoft.Health.Fhir.Core.Models;
 
 namespace Microsoft.Health.Fhir.Core.Features.Search
@@ -57,6 +58,7 @@ internal SearchOptions(SearchOptions other)
             IsAsyncOperation = other.IsAsyncOperation;
             SkipAppendIntersectionWithPredecessor = other.SkipAppendIntersectionWithPredecessor;
             ContainsIterativeInclude = other.ContainsIterativeInclude;
+            ScopeDataActions = other.ScopeDataActions;
         }
 
         /// <summary>
@@ -161,6 +163,11 @@ internal set
 
         public bool OnlyIds { get; set; }
 
+        /// <summary>
+        /// Gets the data actions that may contribute SMART scope restrictions to this search.
+        /// </summary>
+        public DataActions ScopeDataActions { get; internal set; } = DataActions.Read | DataActions.Search;
+
         /// <summary>
         /// Flag for async operations.
         /// </summary>

src/Microsoft.Health.Fhir.Core/Features/Search/SearchService.cs

@@ -16,6 +16,7 @@
 using Microsoft.Health.Fhir.Core.Exceptions;
 using Microsoft.Health.Fhir.Core.Extensions;
 using Microsoft.Health.Fhir.Core.Features.Persistence;
+using Microsoft.Health.Fhir.Core.Features.Security;
 using Microsoft.Health.Fhir.Core.Models;
 
 namespace Microsoft.Health.Fhir.Core.Features.Search
@@ -67,6 +68,22 @@ public virtual async Task<SearchResult> SearchAsync(
             return await SearchAsync(searchOptions, cancellationToken);
         }
 
+        /// <inheritdoc />
+        public virtual async Task<SearchResult> SearchAsync(
+            string resourceType,
+            IReadOnlyList<Tuple<string, string>> queryParameters,
+            DataActions scopeDataActions,
+            CancellationToken cancellationToken,
+            bool isAsyncOperation = false,
+            ResourceVersionType resourceVersionTypes = ResourceVersionType.Latest,
+            bool onlyIds = false,
+            bool isIncludesOperation = false)
+        {
+            SearchOptions searchOptions = _searchOptionsFactory.Create(resourceType, queryParameters, scopeDataActions, isAsyncOperation, resourceVersionTypes, onlyIds, isIncludesOperation);
+
+            return await SearchAsync(searchOptions, cancellationToken);
+        }
+
         /// <inheritdoc />
         public async Task<SearchResult> SearchCompartmentAsync(
             string compartmentType,

src/Microsoft.Health.Fhir.CosmosDb/Features/Search/FhirCosmosSearchService.cs

@@ -22,6 +22,7 @@
 using Microsoft.Health.Fhir.Core.Features.Persistence;
 using Microsoft.Health.Fhir.Core.Features.Search;
 using Microsoft.Health.Fhir.Core.Features.Search.Expressions;
+using Microsoft.Health.Fhir.Core.Features.Security;
 using Microsoft.Health.Fhir.Core.Models;
 using Microsoft.Health.Fhir.CosmosDb.Core.Configs;
 using Microsoft.Health.Fhir.CosmosDb.Features.Search.Queries;
@@ -177,7 +178,14 @@ public override async Task<SearchResult> SearchAsync(
                 null,
                 cancellationToken);
 
-            (IList<FhirCosmosResourceWrapper> includes, bool includesTruncated) = await PerformIncludeQueries(results, includeExpressions, revIncludeExpressions, searchOptions.IncludeCount, smartV2ScopeExpressions, cancellationToken);
+            (IList<FhirCosmosResourceWrapper> includes, bool includesTruncated) = await PerformIncludeQueries(
+                results,
+                includeExpressions,
+                revIncludeExpressions,
+                searchOptions.IncludeCount,
+                smartV2ScopeExpressions,
+                searchOptions.ScopeDataActions,
+                cancellationToken);
 
             SearchResult searchResult = CreateSearchResult(
                 searchOptions,
@@ -618,6 +626,7 @@ private SearchResult CreateSearchResult(SearchOptions searchOptions, IEnumerable
             IReadOnlyCollection<IncludeExpression> revIncludeExpressions,
             int maxIncludeCount,
             IReadOnlyList<UnionExpression> smartV2ScopeExpressions,
+            DataActions scopeDataActions,
             CancellationToken cancellationToken)
         {
             if (matches.Count == 0 || (includeExpressions.Count == 0 && revIncludeExpressions.Count == 0))
@@ -650,7 +659,7 @@ private SearchResult CreateSearchResult(SearchOptions searchOptions, IEnumerable
                     foreach (var resourceTypeGroup in referencesToInclude.GroupBy(r => r.ResourceType))
                     {
                         string resourceType = resourceTypeGroup.Key;
-                        Expression smartScopeFilter = GetSmartScopeFilterForResourceType(smartV2ScopeExpressions, resourceType, out bool hasNoAccess);
+                        Expression smartScopeFilter = GetSmartScopeFilterForResourceType(smartV2ScopeExpressions, resourceType, scopeDataActions, out bool hasNoAccess);
 
                         // Skip this resource type if no scope allows access
                         if (hasNoAccess)
@@ -778,7 +787,7 @@ private SearchResult CreateSearchResult(SearchOptions searchOptions, IEnumerable
                         // Specific resource type(s) - check scope for each type
                         foreach (string targetResourceType in targetResourceTypes)
                         {
-                            Expression smartScopeFilter = GetSmartScopeFilterForResourceType(smartV2ScopeExpressions, targetResourceType, out bool hasNoAccess);
+                            Expression smartScopeFilter = GetSmartScopeFilterForResourceType(smartV2ScopeExpressions, targetResourceType, scopeDataActions, out bool hasNoAccess);
 
                             // Skip this revinclude if no scope allows access to this resource type
                             if (hasNoAccess)
@@ -971,7 +980,11 @@ private async Task<bool> ExecuteSubQueryAsync(List<FhirCosmosResourceWrapper> in
         /// Returns a special "no access" expression if no scope allows access to this resource type (caller should skip the query).
         /// Returns the filter expression if granular scopes exist for this resource type.
         /// </summary>
-        private Expression GetSmartScopeFilterForResourceType(IReadOnlyList<UnionExpression> smartV2ScopeExpressions, string resourceType, out bool hasNoAccess)
+        private Expression GetSmartScopeFilterForResourceType(
+            IReadOnlyList<UnionExpression> smartV2ScopeExpressions,
+            string resourceType,
+            DataActions scopeDataActions,
+            out bool hasNoAccess)
         {
             hasNoAccess = false;
 
@@ -986,7 +999,9 @@ private Expression GetSmartScopeFilterForResourceType(IReadOnlyList<UnionExpress
                 return null;
             }
 
-            var scopeRestrictions = _requestContextAccessor.RequestContext?.AccessControlContext?.AllowedResourceActions;
+            var scopeRestrictions = _requestContextAccessor.RequestContext?.AccessControlContext?.AllowedResourceActions
+                ?.Where(scope => scope.AllowsAny(scopeDataActions))
+                .ToList();
             if (scopeRestrictions == null || !scopeRestrictions.Any())
             {
                 return null;

src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Search/SearchOptionsFactoryTests.cs

@@ -175,6 +175,47 @@ public static IEnumerable<object[]> GetSearchParameterTestData
                     null,
                     "(Union (All) [(And (And (Param ResourceType (StringEquals TokenCode 'Observation')) code1=doo)) OR (And (And (Param ResourceType (StringEquals TokenCode 'Encounter')) code2=goo))])",
                 };
+                yield return new object[]
+                {
+                    null,
+                    new List<ScopeRestriction>
+                    {
+                        new ScopeRestriction("Patient", DataActions.Search, "system"),
+                        new ScopeRestriction("Observation", DataActions.ReadById, "system"),
+                    },
+                    new List<Tuple<string, string>>
+                    {
+                        Tuple.Create("_type", "Observation"),
+                    },
+                    "(Param ResourceType (StringEquals TokenCode 'none'))",
+                };
+                yield return new object[]
+                {
+                    null,
+                    new List<ScopeRestriction>
+                    {
+                        new ScopeRestriction(KnownResourceTypes.All, DataActions.ReadById, "system"),
+                        new ScopeRestriction("Patient", DataActions.Search, "system"),
+                    },
+                    new List<Tuple<string, string>>
+                    {
+                        Tuple.Create("_type", "Patient,Observation"),
+                    },
+                    "(Param ResourceType (StringEquals TokenCode 'Patient'))",
+                };
+                yield return new object[]
+                {
+                    null,
+                    new List<ScopeRestriction>
+                    {
+                        new ScopeRestriction("Observation", DataActions.Read, "system"),
+                    },
+                    new List<Tuple<string, string>>
+                    {
+                        Tuple.Create("_type", "Observation"),
+                    },
+                    "(Param ResourceType (StringEquals TokenCode 'Observation'))",
+                };
             }
         }
 
@@ -800,6 +841,88 @@ public void GivenAnIncludesOperationRequest_WhenIncludesContinuationTokenIsMissi
             Assert.Throws<BadRequestException>(() => CreateSearchOptions(isIncludesOperation: true));
         }
 
+        [Fact]
+        public void GivenReadByIdScopeActions_WhenCreatingConcreteResourceSearch_ThenReadByIdRestrictionIsApplied()
+        {
+            _defaultFhirRequestContext.AccessControlContext.ApplyFineGrainedAccessControl = true;
+            _defaultFhirRequestContext.AccessControlContext.AllowedResourceActions.Add(
+                new ScopeRestriction(KnownResourceTypes.All, DataActions.ReadById, "system"));
+
+            SearchOptions options = _factory.Create(
+                KnownResourceTypes.Observation,
+                queryParameters: null,
+                scopeDataActions: DataActions.Read | DataActions.ReadById);
+
+            ValidateResourceTypeSearchParameterExpression(options.Expression, KnownResourceTypes.Observation);
+            Assert.Equal(DataActions.Read | DataActions.ReadById, options.ScopeDataActions);
+        }
+
+        [Fact]
+        public void GivenReadByIdAndSearchScopes_WhenCreatingIncludeSearch_ThenOnlySearchScopedTypesArePassedToIncludeParser()
+        {
+            _defaultFhirRequestContext.AccessControlContext.ApplyFineGrainedAccessControl = true;
+            _defaultFhirRequestContext.AccessControlContext.AllowedResourceActions.Add(
+                new ScopeRestriction(KnownResourceTypes.All, DataActions.ReadById, "system"));
+            _defaultFhirRequestContext.AccessControlContext.AllowedResourceActions.Add(
+                new ScopeRestriction(KnownResourceTypes.Patient, DataActions.Search, "system"));
+
+            const string include = "Patient:general-practitioner";
+            _expressionParser.ParseInclude(
+                    Arg.Any<string[]>(),
+                    include,
+                    false,
+                    false,
+                    Arg.Any<IReadOnlyCollection<string>>())
+                .Throws(new InvalidSearchOperationException("Expected test exception."));
+
+            Assert.Throws<InvalidSearchOperationException>(() =>
+                _factory.Create(
+                    resourceType: null,
+                    queryParameters: new[]
+                    {
+                        Tuple.Create(KnownQueryParameterNames.Type, KnownResourceTypes.Patient),
+                        Tuple.Create(SearchParameterNames.Include, include),
+                    }));
+
+            _expressionParser.Received(1).ParseInclude(
+                Arg.Any<string[]>(),
+                include,
+                false,
+                false,
+                Arg.Is<IReadOnlyCollection<string>>(resourceTypes =>
+                    resourceTypes.Count == 1 &&
+                    resourceTypes.Contains(KnownResourceTypes.Patient)));
+        }
+
+        [Fact]
+        public void GivenReadByIdScopeForRequestedIncludeType_WhenCreatingSystemSearch_ThenIncludeIsSkipped()
+        {
+            _defaultFhirRequestContext.AccessControlContext.ApplyFineGrainedAccessControl = true;
+            _defaultFhirRequestContext.AccessControlContext.AllowedResourceActions.Add(
+                new ScopeRestriction(KnownResourceTypes.All, DataActions.ReadById, "system"));
+            _defaultFhirRequestContext.AccessControlContext.AllowedResourceActions.Add(
+                new ScopeRestriction(KnownResourceTypes.Patient, DataActions.Search, "system"));
+            _expressionParser.Parse(Arg.Any<string[]>(), Arg.Any<string>(), Arg.Any<string>())
+                .Returns(new StubExpression("query"));
+
+            const string include = "Observation:subject";
+            SearchOptions options = _factory.Create(
+                resourceType: null,
+                queryParameters: new[]
+                {
+                    Tuple.Create(KnownQueryParameterNames.Type, KnownResourceTypes.Observation),
+                    Tuple.Create(SearchParameterNames.Include, include),
+                });
+
+            _expressionParser.DidNotReceive().ParseInclude(
+                Arg.Any<string[]>(),
+                include,
+                Arg.Any<bool>(),
+                Arg.Any<bool>(),
+                Arg.Any<IReadOnlyCollection<string>>());
+            Assert.Contains("'none'", options.Expression.ToString(), StringComparison.Ordinal);
+        }
+
         [Theory]
         [MemberData(nameof(GetSearchParameterTestData))]
         public void Create_AddsFineGrainedAccessControlWithSearchParametersExpressions_UsingMemberData(string resourceType, List<ScopeRestriction> scopeRestrictions, List<Tuple<string, string>> queryParameters, string expectedSubstring)