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

Zero-config + Sign in with Apple #626

Merged
merged 34 commits into from
Nov 8, 2021
Merged

Zero-config + Sign in with Apple #626

merged 34 commits into from
Nov 8, 2021

Conversation

ericclemmons
Copy link
Contributor

@ericclemmons ericclemmons commented Nov 3, 2021

Description of changes:

  • Updates authMachine to have both loginMechanisms and socialProviders based on new Amplify CLI zero-config aws-exports.js. (Closes Update examples to use zero-config #609)

    This means that most usage will just be withAuthenticator or <Authenticator>!

  • Adds Sign in with Apple button (Closes Sign in with Apple #607)

    apple.mp4
  • Fix React to use loginMechanisms and socialProviders

  • Fix Angular to use loginMechanisms and socialProviders

  • Fix Vue to use loginMechanisms and socialProviders

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@changeset-bot
Copy link

changeset-bot bot commented Nov 3, 2021

🦋 Changeset detected

Latest commit: 122b146

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@aws-amplify/ui Minor
@aws-amplify/ui-angular Minor
@aws-amplify/ui-react Minor
@aws-amplify/ui-vue Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@lgtm-com
Copy link

lgtm-com bot commented Nov 3, 2021

This pull request introduces 1 alert when merging 54fe748 into 391e2f3 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Copy link
Contributor Author

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

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

The primary change is that login_mechanisms has been split out into loginMechanisms (username, email, phone_number) and socialProviders based on the zero-config values.

@@ -1,3 +1,4 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the environments/* updates are from yarn environments pull using the new CLI beta – 6.5.0-beta.0

const actorState: SignUpState = getActorState(state);
const actorState = getActorState(state) as SignUpState;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These assignments were getting errors only in Angular. I think there's an opportunity for types to get cleaned up to prevent as casting:

Screen Shot 2021-11-04 at 9 59 04 AM
Screen Shot 2021-11-04 at 9 59 19 AM

@ericclemmons ericclemmons marked this pull request as ready for review November 5, 2021 16:59
@ericclemmons
Copy link
Contributor Author

@ErikCH found the issue! https://github.com/aws-amplify/amplify-ui/tree/main/environments/auth-with-totp-mfa is an imported Cognito User Pool, so it isn't getting the zero-config values correctly like others:

// aws-exports.js
/* 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_region": "us-east-1",
    "aws_user_pools_id": "us-east-1_•••••",
    "aws_user_pools_web_client_id": "•••••",
    "oauth": {},
    "aws_cognito_username_attributes": [],
    "aws_cognito_social_providers": [],
    "aws_cognito_signup_attributes": [],
    "aws_cognito_mfa_configuration": "ON",
    "aws_cognito_mfa_types": [],
    "aws_cognito_password_protection_settings": {
        "passwordPolicyCharacters": []
    },
    "aws_cognito_verification_mechanisms": []
};


export default awsmobile;

Notice how aws_cognito_username_attributes is empty, so owe default to ['username'].

@lazpavel What's your take on it?

@ericclemmons
Copy link
Contributor Author

ericclemmons commented Nov 5, 2021

I introduced a new adminui-auth-with-totp-mfa (2c84a18) for our e2e test until the issue in #626 (comment) is resolved.

@ericclemmons ericclemmons temporarily deployed to ci November 5, 2021 20:48 Inactive
@ericclemmons ericclemmons temporarily deployed to ci November 5, 2021 20:48 Inactive
@ericclemmons ericclemmons temporarily deployed to ci November 5, 2021 20:48 Inactive
@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2021

This pull request introduces 4 alerts and fixes 2 when merging 9fd0972 into 6c57ef8 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

fixed alerts:

  • 2 for Unused variable, import, function or class

Comment on lines +156 to +160
// Prefer explicitly configured settings over default CLI values
return {
loginMechanisms: loginMechanisms ?? cliLoginMechanisms,
socialProviders: socialProviders ?? cliSocialProviders.sort(),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, understood

Copy link
Contributor

@ErikCH ErikCH left a comment

Choose a reason for hiding this comment

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

Looking at the Vue changes, and overall looks good. But I'll refer to @wlee221 expertise in the other areas and Angular

Copy link
Contributor

@wlee221 wlee221 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@lgtm-com
Copy link

lgtm-com bot commented Nov 8, 2021

This pull request introduces 4 alerts and fixes 2 when merging 52d7fe1 into d71e656 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

fixed alerts:

  • 2 for Unused variable, import, function or class

@ericclemmons ericclemmons temporarily deployed to ci November 8, 2021 18:01 Inactive
@ericclemmons ericclemmons temporarily deployed to ci November 8, 2021 18:01 Inactive
@ericclemmons ericclemmons temporarily deployed to ci November 8, 2021 18:01 Inactive
@lgtm-com
Copy link

lgtm-com bot commented Nov 8, 2021

This pull request introduces 4 alerts and fixes 2 when merging 122b146 into d71e656 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

fixed alerts:

  • 2 for Unused variable, import, function or class

@ericclemmons ericclemmons temporarily deployed to ci November 8, 2021 18:18 Inactive
@ericclemmons ericclemmons temporarily deployed to ci November 8, 2021 18:18 Inactive
@ericclemmons ericclemmons temporarily deployed to ci November 8, 2021 18:18 Inactive
@ericclemmons ericclemmons merged commit f84e994 into main Nov 8, 2021
@ericclemmons ericclemmons deleted the 609-zero-config branch November 8, 2021 18:39
@github-actions github-actions bot mentioned this pull request Nov 8, 2021
thaddmt added a commit that referenced this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Angular An issue or a feature-request for Angular platform Authenticator An issue or a feature-request for an Authenticator UI Component React An issue or a feature-request for React platform Vue An issue or a feature-request for Vue platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update examples to use zero-config Sign in with Apple
3 participants