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

feat: remove unneeded scope checks in authorizer #347

Merged
merged 2 commits into from
Jun 14, 2021

Conversation

carvantes
Copy link
Contributor

Description of changes:

Checking for the openid, profile, aws.cognito.signin.user.admin scopes on the APIGW authorizer is not really doing anything for us. Our AuthZ is entirely based on the cognito:groups

This feature is mostly used with custom scopes, and when the API methods are all defined in APIGW(unlike our big {proxy+} method) so that different methods are authorized by different scopes. e.g. creating a custom scope weather.read to be used on a weather API

btw, this change simplifies the multi-tenancy setup.

Checklist:

  • Have you successfully deployed to an AWS account with your changes?
  • [n/a] Have you written new tests for your core changes, as applicable?

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

@carvantes carvantes requested review from nguyen102 and rsmayda June 11, 2021 09:14
@carvantes carvantes requested a review from Bingjiling June 11, 2021 09:14
README.md Outdated
@@ -117,15 +117,9 @@ To find what FHIR Server supports, use the `GET Metadata` Postman request to ret

**Authorizing a user**

FHIR Works on AWS uses Role-Based Access Control (RBAC) to determine what operations and what resource types a user can access. The default rule set can be found in [RBACRules.ts](https://github.com/awslabs/fhir-works-on-aws-deployment/blob/mainline/src/RBACRules.ts). To access the API, you must use the OAuth access token. This access token must include scopes of either `openid`, `profile` or `aws.cognito.signin.user.admin`.
Copy link
Contributor

Choose a reason for hiding this comment

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

To get groups to be put into the JWT the requester must use these scopes during the request (this was required about a year ago perhaps it changed).

So the idea behind these is the fail early principal (fail at authorized level and not even invoke proxy lambda). That said if removing simplifies the flow I am okay removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the RBAC package if groups is null in JWT token it will be an empty array and will result in "Unauthorized" exception

Copy link
Contributor

@rsmayda rsmayda left a comment

Choose a reason for hiding this comment

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

Nothing blocking; leaving the explicit scopes needed in the README maybe helpful, but you are linking to the cognito docs

Copy link
Contributor

@rsmayda rsmayda left a comment

Choose a reason for hiding this comment

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

I double checked the docs AND I couldn't find where it explicitly states how to get group claim in the access token. Please find a better link or leave in the required scopes in README

@github-actions github-actions bot added size/xs and removed size/s labels Jun 11, 2021
@carvantes
Copy link
Contributor Author

I double checked the docs AND I couldn't find where it explicitly states how to get group claim in the access token. Please find a better link or leave in the required scopes in README

ok, I reverted the docs changes. I didn't realize that cognito:groups may not be present depending of scopes.

I couldn't find concise docs either, but from my tests I found that openId scope is sufficient for cognito:groups to show up, and that doing initiate_auth with the SDK always returns cognito:groups regardless of scopes. Still, I think it's ok to leave all the scopes in the docs since we are not changing the client AllowedOAuthScopes

{
  "sub": "xxxxxxxxxx",
  "cognito:groups": [
    "practitioner"
  ],
  "token_use": "access",
  "scope": "openid",
  "auth_time": 1623428796,
  "iss": "https://cognito-idp.us-east-2.amazonaws.com/us-east-2_xxxxxxxx",
  "exp": 1623432396,
  "iat": 1623428796,
  "version": 2,
  "jti": "8cbfa15e-a64b-44c1-bac0-a03214e56e0f",
  "client_id": "xxxxxxxxxx",
  "username": "tenant33345643"
}

@carvantes carvantes requested a review from rsmayda June 11, 2021 17:14
@carvantes carvantes merged commit e0e9364 into develop Jun 14, 2021
@carvantes carvantes deleted the dev-no-scope-checks branch June 14, 2021 15:37
carvantes added a commit that referenced this pull request Jun 14, 2021
awsbakha pushed a commit to awsbakha/fhir-works-on-aws-deployment that referenced this pull request Jun 16, 2021
awsbakha pushed a commit to awsbakha/fhir-works-on-aws-deployment that referenced this pull request Jun 16, 2021
carvantes added a commit that referenced this pull request Jun 17, 2021
* feat: add tenantId attribute to Cognito user pool
* feat: remove unneeded scope checks in authorizer (#347)
rsmayda added a commit that referenced this pull request Jun 25, 2021
BREAKING CHANGE: Interact with ES via aliases. Aliases need to be added to existing index prior to deployment (feat: Use alias for all ES operations #349)
- Run the addAlias script prior to deploying. This script will create aliases for all existing indices


To highlight some of the features being merged in:
- Add logging framework (feat: deployment change for adding logging framework  #310)
- Add $docref implementation (feat: add $docref implementation #332)
- Add better search integration tests (test: add search tests for exact token matching #306, feat: Add post search and integ tests #296)
- Add DLQ for ddbToEs sync failures (feat: add DLQ for ddbToEs sync failures #295)

Full list of changes merged in: 

* feat: Update dependencies to support SearchFilters better (#197)

* fix: Update persistence and interface dependencies (#201)

* fix(search): bump search version to pick up bug fixes (#202)

* chore: add PR size labeler (#198)

* chore: add PR size labeler

* chore: update labeler yml syntax

* empty commit fix mainline protection

* feat: bump routing and interface version to use validators (#208)

* docs: Update docs, Third party file & diagram (#207)

* Fixes a link in DEVELOPMENT.md (#215)

* docs: add a troubleshooting case for windows (#220)

* chore: change log retention and add DDB tables as output (#222)

* test: Add tests for RBAC Auth (#229)

* test: update cognito usernames (#230)

* test: Default fhirClient should use the role of practitioner (#231)

* chore: dependency update (#232)

* chore: Update serverless dependencies and use serverless-bundle to optimize package size (#240)

* fix: http-errors from routing dependency (#243)

* chore: update deploy script for working with local packages (#245)

* chore: update log settings (#252)

* chore: add encryption, https-only, access logging to all buckets (#253)

* chore: Update routing dependency. Pass in user agent value (#263)

* chore: fix gh merge workflow to do ff-only (#264)

* chore: use our own script instead of merge action (#265)

* chore: fix yaml syntax

* chore: fix merge script

* chore: use merge token in merge script

* ci: add workaround for merge with branch protection

* feat: support Implementation Guides (#266)

* feat: add Implementation Guides compiler script (#192)
* feat: load compiled implementation guides (#199)
* feat: add Implementation Guides integ tests (#205)
* chore: add compile-igs yarn script
* feat: add HAPI validator lambda fn (#221)
* feat: Compile IG StructureDefinitions (#235)
* feat: add useHapiValidator CFN parameter (#236)
* test: IGs: Validation during create/update and supportedProfile in CapStatement (#237)
* feat: specify paths to IG resources (#241)
* fix: copy IG files to webpack deployment package (#247)
* chore: add third party attributions file (#251)
* feat: add support for DSTU3 on Java Hapi validator (#254)
* docs: add IGs documentation (#256)
* chore: update gh actions tu build and deploy hapi validator (#257)
* feat: support Implementation Guides
* docs: fix changelog headers
* chore: update version in package.json

* ci: use HAPI validator on integ tests (#269)

* chore: Update routing package dependency. Update custom agent env variable with correct package version (#271)

* docs: removing team email from feedback (#275)

* fix: fix unable to locate compiledImplementationGuides/* glob (#277)

* fix: exit from installation script if serverless deployment fails (#280)

* docs: Clarify IG documentation (#279)

* docs: Updated FHIR Works readme (#283)

* feat: enhance date and token search. Add integ tests (#284)

* test: add token search integ tests (#285)

* chore(release): 2.6.0 (#286)

* docs: Update CUSTOM_USER_AGENT (#287)

* fix: update persistence dependency; to fix meta bug (#288)

* feat: enhance numeric and quantity search (#291)

* fix: Suppress deprecation warning when writing to Info_Output.yml during installation (#294)

* feat: add DLQ for ddbToEs sync failures (#295)

* chore: lock down versions of external deployment dependencies (#298)

* fix: increment persistence package (#300)

* feat: Add post search and integ tests (#296)

* feat: Add post search and integ tests

* chore: add security scanning (#302)

* feat(search): support Period type fields for date params (#299)

* test: add search tests for exact token matching (#306)

* chore(release): 2.7.0  (#307)

* chore: add cfn-nag to all yaml changes (#308)

* chore: dependency updates (#321)

* chore: dependency updates

* fix: resolved registry url updated

* chore: upgrade dependencies (#322)

* chore: bump dependency version (#328)

* chore: bump dependency version

* chore: adding GitHub issue templates (#325)

* chore: update README for another Auth url

* feat: add logging framework  (#310)

* chore: workflow only run if yaml/yml is updated (#333)

* feat: add $docref implementation (#332)

* chore(deps): bump browserslist from 4.16.3 to 4.16.6 (#335)

* chore(deps): bump browserslist from 4.16.3 to 4.16.6

* trigger mainline protection check

* chore(deps): bump browserslist from 4.16.3 to 4.16.6 in /auditLogMover (#334)

* chore(deps): bump browserslist from 4.16.3 to 4.16.6 in /auditLogMover

* trigger mainline protection check

* chore: dependency fix (#339)

* chore: release 2.8.0 (#338)

* fix: Allow running sls offline with Hapi Validator (#343)

* fix: typo for passing in custom log level (#345)

* chore: script for add alias to existing index (#346)

* feat: remove unneeded scope checks in authorizer (#347)

* Revert "feat: remove unneeded scope checks in authorizer (#347)"

* feat!: Use alias for all ES operations (#349)

* BREAKING CHANGE: Aliases need to be added to existing index
* Run the addAlias [script](https://github.com/awslabs/fhir-works-on-aws-deployment/blob/94a3187a6fb7a673946a215869c154048603389b/scripts/elasticsearch-operations.js) created in this [PR](#346) will create aliases for all existing indices 
* Update or create resource in a specific type will automatically create alias for the corresponding index

* chore(release): 3.0.0 (#353)

* chore: remove routing package from bundle.externals (#354)

* chore(deps): bump ssri from 6.0.1 to 6.0.2 in /auditLogMover (#351)

* chore(deps): bump ssri from 6.0.1 to 6.0.2 in /auditLogMover

* chore(deps): bump lodash from 4.17.20 to 4.17.21 in /auditLogMover (#350)

* chore(deps): bump lodash from 4.17.20 to 4.17.21 in /auditLogMover

* chore(deps): bump ssri from 6.0.1 to 6.0.2 (#352)

* chore(deps): bump ssri from 6.0.1 to 6.0.2

* chore(deps-dev): bump lodash from 4.17.20 to 4.17.21 (#355)

* chore(deps-dev): bump lodash from 4.17.20 to 4.17.21

* chore: update dependencies (#356)

* fix: type definition issues

Co-authored-by: Tim Nguyen <nguyen102@users.noreply.github.com>
Co-authored-by: Nestor Carvantes <carvantes@gmail.com>
Co-authored-by: Yanyu Zheng <yz2690@columbia.edu>
Co-authored-by: zheyanyu <zheyanyu@amazon.com>
Co-authored-by: J Kendal <13680617+joekendal@users.noreply.github.com>
Co-authored-by: Tim Nguyen <thingut@amazon.com>
Co-authored-by: justinusmenzel <69522360+justinusmenzel@users.noreply.github.com>
Co-authored-by: Justinus Menzel <justinus.menzel@abacusinsights.com>
Co-authored-by: Emil Diaz <emilhdiaz@users.noreply.github.com>
Co-authored-by: shyogesh-sw <79225266+shyogesh-sw@users.noreply.github.com>
Co-authored-by: Sanket Dharwadkar <sdharwad@amazon.com>
Co-authored-by: Bakha Nurzhanov <74073037+awsbakha@users.noreply.github.com>
Co-authored-by: Ranjan Bhandari <ranjanbhandari@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Zambonilli added a commit to Zambonilli/fhir-works-on-aws-deployment that referenced this pull request Jul 27, 2021
…3.0.0

* commit 'e0e93646ab367b4d18868ab55fb21aee1780fd61':
  feat: remove unneeded scope checks in authorizer (awslabs#347)
Zambonilli added a commit to Zambonilli/fhir-works-on-aws-deployment that referenced this pull request Jul 27, 2021
…3.0.0

* commit 'd103ad03b3321039e9a8fb06be84819adedb32b7':
  Revert "feat: remove unneeded scope checks in authorizer (awslabs#347)"
carvantes added a commit that referenced this pull request Aug 18, 2021
* feat: add tenantId attribute to Cognito user pool (#348)

* feat: remove unneeded scope checks in authorizer (#347)

* feat: update lambda state machine to accommodate tenantId (#367)

* feat: add "enableMultiTenancy" CFN parameter  (#381)

* test: add multi-tenancy integ tests (#387)

* fix: remove _id, _tenantId from bulk export results (#384)

* feat: Group export scripts (#389)

* fix: add multi-tenant metadata route (#392)

* fix: allow more concurrent export jobs for multi-tenant deployments (#397)

* test: integ tests for Group export (#393)

* feat: add ES hard delete config value (#398)

* docs: update postman collection and docs to use Id token  (#399)

* docs: add multi-tenancy docs (#400)


Co-authored-by: Yanyu Zheng <yz2690@columbia.edu>

BREAKING CHANGE: The Cognito IdToken is now used instead of the accessToken to authorize requests.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants