-
Notifications
You must be signed in to change notification settings - Fork 21
fix: scopes should not be filtered in search use cases #79
Conversation
const clonedScopeRule = emptyScopeRule(); | ||
clonedScopeRule.system.read = ['search-type']; | ||
expect( | ||
filterOutUnusableScope( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the situation mentioned in the issue, reqResourceType
should be Patient
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have misunderstood, but I think the situation was that the Patient resource was a reference in the DocumentReference, and needed to be included in the results with the _include
parameter. This test is making sure that neither scope is filtered out from a search, so that resources of both DocumentReference
and Patient
can be read by the search query. Additionally, if the reqResourceType
parameter is provided, the route is different and won't actually hit the change made to fix the bug: https://github.com/awslabs/fhir-works-on-aws-authz-smart/pull/79/files#diff-9539c459709b0437ecf87b68dce629addd51e10c3c4f7af6ab3fdfcc171a312aL57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for the test case we need to ensure: reqResourceType?: string
, is set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, after running tests searches do indeed provide the reqResourceType
parameter, whereas batch and transaction do not. So I moved the change into the correct place to reflect that. I also updated the test to set reqResourceType
and it correctly returns both scopes as valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great research!
Hi! I'm having an issue with this change. const clonedScopeRule = emptyScopeRule();
clonedScopeRule.system.read = ['search-type'];
expect(
filterOutUnusableScope(
['system/DocumentReference.read', 'system/Patient.read'],
clonedScopeRule,
'search-type',
false,
'Procedure',
),
).toEqual([]); If the user claims for this scope |
Issue #78 , if available:
Description of changes:
Updated the function checking valid operations for scope to process search use cases similar to transactions/batches. This means that searches running include will contain all expected resources and not drop scopes to limit search results since they are not directly requested.
Testing
To test that the functionality was working as intended, I deployed on SMART and created a
DocumentReference
with a reference to aPatient
resource, and searched with the example scopes (system/DocumentReference.read
,system/Patient.read
) to reproduce the issue. I then implemented the fix as well as an additional unit test, and made sure that the Patient resource was included in the search results.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.