-
Notifications
You must be signed in to change notification settings - Fork 820
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
fix: added Post Confirmation trigger permission from auth to Custom trigger template #7673
Conversation
This pull request introduces 2 alerts when merging 442c8cc into 00d6676 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging ff46fa7 into 155a071 - view on LGTM.com new alerts:
|
...scloudformation/triggers/PostConfirmation/cloudformation-templates/PostConfirmation.json.ejs
Outdated
Show resolved
Hide resolved
...y-category-auth/src/provider-utils/awscloudformation/utils/generate-auth-trigger-template.ts
Outdated
Show resolved
Hide resolved
...y-category-auth/src/provider-utils/awscloudformation/utils/generate-auth-trigger-template.ts
Outdated
Show resolved
Hide resolved
...y-category-auth/src/provider-utils/awscloudformation/utils/generate-auth-trigger-template.ts
Outdated
Show resolved
Hide resolved
...y-category-auth/src/provider-utils/awscloudformation/utils/generate-auth-trigger-template.ts
Outdated
Show resolved
Hide resolved
const lambdaFunction = await getLambdaFunction(functionName, meta.providers.awscloudformation.Region); | ||
const clients = await getUserPoolClients(id, clientIds, region); | ||
await addUserToUserPool(id, region); | ||
const lambdaFunction = await getLambdaFunction(functionName, region); |
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.
Are we invoking the PostConfirmation trigger lambda directly instead of testing it through Cognito to see if it is properly configured and working as expected? If that's the case, we need to test it end-to-end, not in isolation like this.
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 tried with AdminConfirmSingup
Call which calls PostConfirmation Auth Trigger , But there is no way currently as When I tried invoking though USerPool, got following errors :
AdminCreateUser -> Admin ConfirmSignUp -> error: user state is FORCE_PASSWD_CHANGE(NOT Authorized)
AdminCreateUser -> AdminSetPassword -> Admin ConfirmSignUp -> error: user state is CONFIRMED(NOT Authorized)
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.
We are creating and confirming users in the graphql e2e tests by using AmplifyJS calls, no need to admin confirm it, use the same method that client applications will use.
This pull request introduces 1 alert when merging 02289a8 into 0489c7f - view on LGTM.com new alerts:
|
@attilah This PR solves a pretty annoying problem -- I noticed it's stalled -- what's preventing it from getting merged? |
Any update on merging this fix? This is a very annoying regression that needs to be fixed ASAP. Thanks. |
a036123
to
b9afcc2
Compare
This pull request introduces 2 alerts when merging b9afcc2 into 6db6d66 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging efe3177 into 1539b24 - view on LGTM.com new alerts:
|
closing in favour of #11482 |
Description of changes
Issue #7582 , if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.