-
Notifications
You must be signed in to change notification settings - Fork 249
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
chore(infra): Api migrate to Gen 2 E2E #5043
Conversation
5c1108f
to
007a014
Compare
d0499c8
to
16d1a13
Compare
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.
Left a few questions. Will review the rest later
// extract L1 CfnUserPool resources | ||
const { cfnUserPool } = backend.auth.resources.cfnResources; | ||
// modify cfnUserPool policies directly | ||
cfnUserPool.policies = { |
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.
Q: What is the default? Why do we need to change it? Can that change not be made in Gen 2?
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.
What is the default?
8 Chars, 1 lower, 1 upper, 1 symbol
Why do we need to change it?
To match the current pattern and password generator methods we have.
Can that change not be made in Gen 2?
This is the recommended way when using Gen 2.
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.
The current generatePassword method used in integ tests inlcudes a UUID, an upper case character, and a symbol. That should always meet those requirements. Am I missing something?
amplify-flutter/packages/test/amplify_test/lib/src/mock_data.dart
Lines 62 to 65 in 7cca276
String generatePassword() => | |
uuid() + | |
uppercaseLetters[random.nextInt(uppercaseLetters.length)] + | |
symbols[random.nextInt(symbols.length)]; |
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.
Ah, this function wasn't being called. Previously it was just a uuid for the password. Updated.
infra-gen2/backends/api/apiMultiAuth/amplify/auth/pre-sign-up-handler.ts
Outdated
Show resolved
Hide resolved
// extract L1 CfnUserPool resources | ||
const { cfnUserPool } = backend.auth.resources.cfnResources; | ||
// modify cfnUserPool policies directly | ||
cfnUserPool.policies = { |
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.
The current generatePassword method used in integ tests inlcudes a UUID, an upper case character, and a symbol. That should always meet those requirements. Am I missing something?
amplify-flutter/packages/test/amplify_test/lib/src/mock_data.dart
Lines 62 to 65 in 7cca276
String generatePassword() => | |
uuid() + | |
uppercaseLetters[random.nextInt(uppercaseLetters.length)] + | |
symbols[random.nextInt(symbols.length)]; |
@@ -18,6 +18,7 @@ export 'src/category/amplify_categories.dart'; | |||
|
|||
/// Config | |||
export 'src/config/amplify_config.dart'; | |||
export 'src/config/amplify_outputs/amplify_outputs.dart'; |
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.
Note: I think merge conflicts are causing changes to show in this PR that are not part of the PR. I am going to ignore files that I don't think are intended to be part of the PR and we re-review once the PR is updated.
packages/api/amplify_api/example/integration_test/graphql/api_key_test.dart
Outdated
Show resolved
Hide resolved
ff69629
to
b2977db
Compare
b9a3607
to
c3c0860
Compare
packages/api/amplify_api/example/integration_test/graphql/iam_test.dart
Outdated
Show resolved
Hide resolved
final req = ModelQueries.list<Blog>(Blog.classType); | ||
final req = ModelQueries.list<Blog>( | ||
Blog.classType, | ||
authorizationMode: APIAuthorizationType.iam, |
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.
Q: Is this change expected? Does gen 2 default to a different auth mode than gen 1?
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.
It's unclear why this is necessary now, but it appears to be more accurate since these models have multiple authentication modes and an explicit mode should have been defined from the start.
e9ad65a
to
61b6f3c
Compare
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.