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

fix(amplify-category-auth): update front end config on pull #8093

Merged
merged 1 commit into from
Sep 10, 2021
Merged

fix(amplify-category-auth): update front end config on pull #8093

merged 1 commit into from
Sep 10, 2021

Conversation

lazpavel
Copy link
Contributor

@lazpavel lazpavel commented Sep 3, 2021

Description of changes

  • ensure that for projects created before 5.2.0, on pull we populate the auth frontend config

Description of how you validated changes

  • yarn test passes,
  • manual testing by creating a project with an older Amplify CLI and pulling into an empty folder populates the front end configuration

Checklist

  • PR description included
  • yarn test passes

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

@ericclemmons
Copy link
Contributor

When running $(yarn global bin)/amplify-dev pull against my backend created via Amplify CLI in July (https://github.com/aws-amplify/amplify-ui/tree/main/environments/auth-with-username-no-attributes), I'm getting the following aws-exports.js with an empty aws_cognito_login_mechanisms:

/* eslint-disable */
// WARNING: DO NOT EDIT. This file is automatically generated by AWS Amplify. It will be overwritten.

const awsmobile = {
    "aws_project_region": "us-east-1",
    "aws_cognito_identity_pool_id": "us-east-1:******",
    "aws_cognito_region": "us-east-1",
    "aws_user_pools_id": "us-east-1_******",
    "aws_user_pools_web_client_id": "******",
    "oauth": {},
    "aws_cognito_login_mechanisms": [],
    "aws_cognito_signup_attributes": [
        "EMAIL"
    ],
    "aws_cognito_mfa_configuration": "OFF",
    "aws_cognito_mfa_types": [
        "SMS"
    ],
    "aws_cognito_password_protection_settings": {
        "passwordPolicyMinLength": 8,
        "passwordPolicyCharacters": []
    }
};


export default awsmobile;

Is username a default value somewhere? It shows up correctly in https://us-east-1.admin.amplifyapp.com/admin/d2zuq5rjkps8u4/dev/auth:

Screen Shot 2021-09-03 at 10 36 46 AM

This what's in my parameters.json:

{
    "identityPoolName": "authwithusernamenoat3aaa0954_identitypool_3aaa0954",
    "allowUnauthenticatedIdentities": false,
    "resourceNameTruncated": "authwi3aaa0954",
    "userPoolName": "authwithusernamenoat3aaa0954_userpool_3aaa0954",
    "autoVerifiedAttributes": [
        "email"
    ],
    "mfaConfiguration": "OFF",
    "mfaTypes": [
        "SMS Text Message"
    ],
    "smsAuthenticationMessage": "Your authentication code is {####}",
    "smsVerificationMessage": "Your verification code is {####}",
    "emailVerificationSubject": "Your verification code",
    "emailVerificationMessage": "Your verification code is {####}",
    "defaultPasswordPolicy": false,
    "passwordPolicyMinLength": 8,
    "passwordPolicyCharacters": [],
    "requiredAttributes": [
        "email"
    ],
    "userpoolClientGenerateSecret": false,
    "userpoolClientRefreshTokenValidity": 30,
    "userpoolClientWriteAttributes": [
        "email"
    ],
    "userpoolClientReadAttributes": [
        "email"
    ],
    "userpoolClientLambdaRole": "authwi3aaa0954_userpoolclient_lambda_role",
    "userpoolClientSetAttributes": false,
    "sharedId": "3aaa0954",
    "resourceName": "authwithusernamenoat3aaa0954",
    "authSelections": "identityPoolAndUserPool",
    "authRoleArn": {
        "Fn::GetAtt": [
            "AuthRole",
            "Arn"
        ]
    },
    "unauthRoleArn": {
        "Fn::GetAtt": [
            "UnauthRole",
            "Arn"
        ]
    },
    "useDefault": "default",
    "userPoolGroupList": [],
    "serviceName": "Cognito",
    "usernameCaseSensitive": false,
    "dependsOn": []
}

I don't see a username specified anywhere in paramters.json nor my cloudformation template.

Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

@ericclemmons asked me to take a look at the PR so even if it is a draft I reviewed it to give some directions for the future changes.

@lazpavel lazpavel marked this pull request as ready for review September 7, 2021 15:09
@lazpavel lazpavel requested a review from a team as a code owner September 7, 2021 15:09
@lazpavel lazpavel marked this pull request as draft September 7, 2021 15:58
@lazpavel lazpavel requested a review from attilah September 7, 2021 18:31
@lazpavel lazpavel marked this pull request as ready for review September 7, 2021 18:31
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test for this?

Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

I agree with @cjihrig's comments, please fix those before merging.

Regarding the awsexports (and others) generation make sure we have a test that preserves the custom values if they were present and also have a test for removal, when someone removes auth and such, then the configured values MUST be removed from awsexports (and others)

@lazpavel lazpavel changed the base branch from master to 2493-clear-require-cache-between-lambda-invocations September 10, 2021 19:22
@lazpavel lazpavel changed the base branch from 2493-clear-require-cache-between-lambda-invocations to master September 10, 2021 19:22
@jhockett jhockett merged commit b8949b2 into aws-amplify:master Sep 10, 2021
jhockett added a commit to jhockett/amplify-cli that referenced this pull request Sep 10, 2021
jhockett added a commit that referenced this pull request Sep 10, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v5.6.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v5.6.0.

@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Sep 14, 2021
marcvberg pushed a commit to marcvberg/amplify-cli that referenced this pull request Oct 13, 2021
marcvberg pushed a commit to marcvberg/amplify-cli that referenced this pull request Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants