Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

fix: handle include/revInclude correctly #92

Merged
merged 2 commits into from
Sep 12, 2022
Merged

fix: handle include/revInclude correctly #92

merged 2 commits into from
Sep 12, 2022

Conversation

rsmayda
Copy link
Contributor

@rsmayda rsmayda commented Sep 12, 2022

Issue #, if available: #83

Description of changes:

  • Moving the logic of dealing with non-root resourceTypes returned from a search-type operation from filterOutUnusableScope to authorizeAndFilterReadResponse
  • Make hasSystemAccess more explicit
  • Improve test coverage

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rsmayda rsmayda added the bug Something isn't working label Sep 12, 2022
@rsmayda rsmayda requested a review from a team as a code owner September 12, 2022 10:39
@rsmayda rsmayda self-assigned this Sep 12, 2022
@kharbutli-meta
Copy link

Other than the unit tests, were there any other tests run to verify functionality?

src/smartAuthorizationHelper.test.ts Show resolved Hide resolved
@@ -378,6 +392,7 @@ export class SMARTHandler implements Authorization {
this.adminAccessTypes,
fhirServiceBaseUrl,
this.fhirVersion,
'write',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to do the same validation as for read requests above here for write requests? I know the bug was mainly introduced with the search feature, but is there any chance it could be replicated for write through a Bundle or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more test to address this comment, let me know your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good!

src/smartScopeHelper.test.ts Outdated Show resolved Hide resolved
src/smartScopeHelper.test.ts Show resolved Hide resolved
src/smartAuthorizationHelper.ts Outdated Show resolved Hide resolved
src/smartAuthorizationHelper.ts Outdated Show resolved Hide resolved
src/smartAuthorizationHelper.ts Outdated Show resolved Hide resolved
src/smartAuthorizationHelper.ts Show resolved Hide resolved
@@ -172,16 +172,5 @@ export function filterOutUnusableScope(
),
);

// We should only return the scopes iff there is at least 1 valid scope for the given resourceType

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing this logic instead of updating it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the removal of search-type from this line

if (resourceType === '*' || resourceType === reqResourceType) {

We will have the same behavior described in this comment

@rsmayda
Copy link
Contributor Author

rsmayda commented Sep 12, 2022

Other than the unit tests, were there any other tests run to verify functionality?

I was able to deploy and test locally

Copy link
Contributor

@ssvegaraju ssvegaraju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@rsmayda rsmayda merged commit 203bbc0 into mainline Sep 12, 2022
@rsmayda rsmayda deleted the abc branch September 12, 2022 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants