Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use sub:username identity claim by default when persisting behind a feature flag #10196

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

danielleadams
Copy link
Contributor

@danielleadams danielleadams commented Apr 12, 2022

Description of changes

This pull requests does the following:

  • persists "sub::username" format on Mutation requests
  • adds a feature flag that is default for existing apps to use "username"
  • adds a warning for current owner based auth on compile either explicitly use "username" if they want to keep using it

Description of how you validated changes

  • yarn test
  • e2e tests in a later PR
  • manual testing matrix

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

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

@danielleadams danielleadams changed the title use sub:username identity claim by default when persisting behind a feature flag feat: use sub:username identity claim by default when persisting behind a feature flag Apr 12, 2022
Copy link
Contributor

@akshbhu akshbhu left a comment

Choose a reason for hiding this comment

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

Some minor comments but overall LGTM

"iam",
"indicies",
"integtestFn",
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was reordered, not added, so I'll leave it and we can remove non words in another PR.

Copy link
Contributor

@marcvberg marcvberg left a comment

Choose a reason for hiding this comment

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

Looks like some snapshots need to be updated, otherwise seems good

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2022

Codecov Report

Merging #10196 (47bbb83) into master (846048c) will increase coverage by 0.18%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master   #10196      +/-   ##
==========================================
+ Coverage   54.25%   54.43%   +0.18%     
==========================================
  Files         849      854       +5     
  Lines       46939    47026      +87     
  Branches    10004    10027      +23     
==========================================
+ Hits        25467    25599     +132     
+ Misses      19456    19413      -43     
+ Partials     2016     2014       -2     
Impacted Files Coverage Δ
...mation/service-walkthroughs/appSync-walkthrough.ts 15.30% <0.00%> (ø)
...r-utils/awscloudformation/utils/layerCloudState.ts 15.49% <0.00%> (-0.23%) ⬇️
...amplify-category-geo/src/service-utils/mapUtils.ts 57.69% <ø> (ø)
...amplify-cli-core/src/feature-flags/featureFlags.ts 80.85% <ø> (ø)
.../extensions/amplify-helpers/update-amplify-meta.ts 14.01% <0.00%> (ø)
...auth-transformer/src/__tests__/acm-test-library.ts 100.00% <ø> (ø)
...-auth-transformer/src/resolvers/mutation.create.ts 95.45% <ø> (+6.81%) ⬆️
...-auth-transformer/src/resolvers/mutation.update.ts 95.65% <ø> (+6.52%) ⬆️
...y-graphql-auth-transformer/src/resolvers/search.ts 68.31% <ø> (ø)
...ql-auth-transformer/src/resolvers/subscriptions.ts 100.00% <ø> (+4.16%) ⬆️
... and 58 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@danielleadams danielleadams changed the base branch from master to revert-10213-revert-10189-ic-changes-query April 15, 2022 15:15
@danielleadams
Copy link
Contributor Author

@marcvberg this has been rebased

Copy link
Contributor

@marcvberg marcvberg left a comment

Choose a reason for hiding this comment

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

LGTM

@danielleadams danielleadams changed the base branch from revert-10213-revert-10189-ic-changes-query to master April 15, 2022 18:13
@danielleadams danielleadams changed the base branch from master to revert-10213-revert-10189-ic-changes-query April 15, 2022 18:37
@danielleadams danielleadams force-pushed the ic-changes-mutation branch 4 times, most recently from d724f00 to 5c16224 Compare April 23, 2022 02:48
@danielleadams danielleadams force-pushed the revert-10213-revert-10189-ic-changes-query branch from e6aca26 to 9f3ef5f Compare April 26, 2022 00:16
@danielleadams danielleadams changed the base branch from revert-10213-revert-10189-ic-changes-query to master April 26, 2022 02:10
@danielleadams danielleadams force-pushed the ic-changes-mutation branch 2 times, most recently from 32b5209 to 4c5dd5f Compare April 26, 2022 17:30
@danielleadams
Copy link
Contributor Author

When #10222 is merged, these tests will pass (they are due to mismatched snapshots)

@danielleadams danielleadams force-pushed the ic-changes-mutation branch 2 times, most recently from 2de3c91 to d46056b Compare April 28, 2022 00:08
@danielleadams danielleadams merged commit 947aae6 into aws-amplify:master Apr 28, 2022
@danielleadams danielleadams deleted the ic-changes-mutation branch April 28, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants