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

For Auth, add PreventUserExistenceErrors parameter and update NodeJS runtime version #3534

Merged
merged 2 commits into from
Oct 29, 2020

Conversation

beomseoklee
Copy link
Contributor

@beomseoklee beomseoklee commented Feb 27, 2020

Issue: #4214

Description of changes:
According to the Amazon Cognito documentation, if PreventUserExistenceErrors parameter is enabled automatically after February 15th 2020, but it's somehow not through the AWS CloudFormation template.

For the security best practice, this option needs to be enabled, so I've added PreventUserExistenceErrors parameter.

In addition to the parameter, I've updated NodeJS runtime to the latest version for AWS Lambda functions.

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

…rs: ENABLED.

To use the latest AWS Lambda runtime, updated the runtime to nodejs12.x
@@ -367,7 +369,7 @@ Resources:
- ' }'
- '};'
Handler: index.handler
Runtime: nodejs10.x
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try pushing this template and testing it?
I believe Cloudformation doesn't support in-line lambda's with Node 12.x runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CloudFormation does support Lambda Node 12.x runtime. Here is the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is for the Lambda functions - not for the in-line custom resources which you define in your cloudformation file. Have you tried pushing this template successfully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean "for the Lambda functions"? If you mean I was able to create those Lambda functions with Node 12.x runtime, like I said, CloudFormation supports Lambda Node 12.x runtime, so the answer is yes.
image

Not for OpenIdLambda because I have not figured out how to create that one yet.

@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #3534 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #3534    +/-   ##
========================================
  Coverage   58.18%   58.18%            
========================================
  Files         407      407            
  Lines       18758    18758            
  Branches     3750     3555   -195     
========================================
  Hits        10914    10914            
- Misses       7162     7182    +20     
+ Partials      682      662    -20     
Impacted Files Coverage Δ
packages/amplify-util-mock/src/api/api.ts 0.00% <0.00%> (ø)
packages/graphql-mapping-template/src/print.ts 34.65% <0.00%> (ø)
packages/amplify-util-mock/src/storage/storage.ts 0.00% <0.00%> (ø)
...ges/amplify-util-mock/src/CFNParser/stack/index.ts 0.00% <0.00%> (ø)
...es/amplify-util-mock/src/api/resolver-overrides.ts 0.00% <0.00%> (ø)
...es/graphql-transformer-core/src/stripDirectives.ts 35.29% <0.00%> (ø)
.../amplify-cli-core/src/state-manager/pathManager.ts 67.08% <0.00%> (ø)
.../amplify-util-mock/src/utils/lambda/loadMinimal.ts 0.00% <0.00%> (ø)
...graphql-transformer-core/src/TransformerContext.ts 25.09% <0.00%> (ø)
...mplify-appsync-simulator/src/resolvers/function.ts 15.38% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85c9c93...131ae85. Read the comment docs.

@edwardfoyle edwardfoyle merged commit 5d474d3 into aws-amplify:master Oct 29, 2020
edwardfoyle added a commit that referenced this pull request Oct 29, 2020
edwardfoyle added a commit that referenced this pull request Oct 30, 2020
@edwardfoyle
Copy link
Contributor

@beomseoklee Had to revert this due to an odd error happening in one of our tests. We'll figure out what's going on with the test and merge this again.

This is the failed test: https://app.circleci.com/pipelines/github/aws-amplify/amplify-cli/2542/workflows/bbbb0bed-b0a4-4a0d-96c1-8cb6ba2f3739/jobs/36430

We need to change the expected error message here to "Incorrect username or password" but when I tried to update the test to that, for some reason the error we're getting is "Only radix 2, 4, 8, 16, 32 are supported" likely related to this issue just need to dig into why it's happening

r0zar pushed a commit to r0zar/amplify-cli that referenced this pull request Nov 19, 2020
r0zar pushed a commit to r0zar/amplify-cli that referenced this pull request Nov 19, 2020
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants