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

Remaining issues when merging federated users with native Cognito accounts in PreSignUp Trigger #11565

Open
2 tasks done
hanna-becker opened this issue Jun 28, 2023 · 14 comments
Assignees
Labels
Auth Related to Auth components/category documentation Related to documentation feature requests

Comments

@hanna-becker
Copy link

How did you install the Amplify CLI?

npm

If applicable, what version of Node.js are you using?

18

Amplify CLI Version

12.1.1

What operating system are you using?

WSL2 on Windows

Did you make any manual changes to the cloud resources managed by Amplify? Please describe the changes made.

no

Describe the bug

I am using a setup very close to what the OP documented in this issue (although not in a Flutter, but an Angular application):
aws-amplify/amplify-flutter#1716

Thanks to the work being done on that issue, we do not have to throw a custom exception and reinitiate the auth flow on the client-side any longer on successfully linking a social provider!

There are still a couple of issues remaining, though, leading to an overall painful developer experience when implementing account merging, which I am documenting here. All of them are focused around Cognito.

Anyone who ran into these issues, as well, please upvote to raise visibility.

List of Issues

1. Missing ability to map (or at least hardcode to true) the “email_verified” user attribute for Facebook (and apparently some other non-Google) IDPs

For Google, we can map the email_verified attribute to Cognito user attributes. For Facebook and some other IDPs, this is not possible, atm.

By default, mapped email addresses are unverified. You can't verify a mapped email address using a one-time code. Instead, map an attribute from your IdP to get the verification status. For example, Google and most OIDC providers include the email_verified attribute.

When a user signs in through an IdP, Amazon Cognito updates the mapped attributes with the latest information from the IdP. Amazon Cognito updates each mapped attribute, even if its current value already matches the latest information.

Source

Issue: Setting “email_verified” manually on signup won’t be enough. If there is no attribute mapping present, Cognito sets the attribute to false on each login. Users with an unverified email won't be able to use the forgot password flow, even though they have a native account.

Current workaround: Call adminUpdateUserAttributes to set “email_verified” to true in the PreToken generation trigger as described here.

Why does it have to be the PreTokenGeneration trigger? Because it is the only one being called on the first and all subsequent social signings (see this doc)

Obviously, setting this attribute on each login is a sh**** workaround. And it is not straightforward to implement because of the next issue.

==================================================================================

2. Cyclic dependency for PreTokenGeneration lambda

The PreTokenGeneration lambda now needs permissions on auth to call AdminUpdateUserAttributes. When trying to add it, though, there is a cyclic dependency error, similar to what’s documented here.

We've dealt with cyclic dependency issues in Cognito triggers a lot in the past, and I’m surprised that most issues around this topic are closed, even though issues persists. My workaround, which I also got from one of these older Github issues, is to rely on the UserPoolId to be exported from somewhere else by adding this to parameters.json (cognitoUserPoolId is the value from cli-inputs.json):

  "<cognitoUserPoolId>UserPoolId": {
    "Fn::GetAtt": ["<cognitoUserPoolId>", "Outputs.UserPoolId"]
  }

Along with this in custom-policies.json

[
  {
    "Effect": "Allow",
    "Action": ["cognito-idp:AdminUpdateUserAttributes"],
    "Resource": [
      {
        "Fn::Join": [
          "",
          [
            "arn:aws:cognito-idp:",
            {
              "Ref": "AWS::Region"
            },
            ":",
            {
              "Ref": "AWS::AccountId"
            },
            ":userpool/",
            {
              "Ref": "<cognitoUserPoolId>UserPoolId"
            }
          ]
        ]
      }
    ]
  }
]

==================================================================================

3. Calling AdminSetUserPassword confirms the user, but does not trigger the PostConfirmation hook

We create user resources in the PostConfirmation hook and need it called for every user, not just the ones creating a native Cognito account. We considered calling AdminConfirmSignUp as a workaround, which brings me to the next issue.

==================================================================================

4. AdminConfirmSignUp throws “user already confirmed” error

AdminConfirmSignUp according to Cognito docs would trigger the PostConfirmation hook when running successfully, but it throws a “user already confirmed” error when called after AdminSetUserPassword.

When calling AdminConfirmSignUp before AdminSetUserPassword it throws an error with an internal stacktrace saying it’s unable to JSON.stringify a cyclic object structure. (Sorry, forgot to copy the stacktrace here.)

According to a Stackoverflow post, AdminConfirmSignUp wouldn't even trigger the PostConfirmation hook, which, if true, contradicts the docs. I cannot confirm whether this is true, because at this point, I decided to revert to calling the PostConfirmation hook manually.

==================================================================================

5. AdminLinkProviderForUser randomly throws an InvalidParameterException

AdminLinkProviderForUser randomly throws this InvalidParameterException:

"Invalid SourceUser: Cognito users with a username/password may not be passed in as a SourceUser, only as a DestinationUser"

I double-checked a million times whether I had mixed up source and destination user, but I hadn’t. This came up for others, as there is a reference to it in this issue.

Apparently, some people were able to mitigate it by assigning more memory to the lambda function. This does not work reliably in my case. My current workaround is to catch and ignore InvalidParameterExceptions for the AdminLinkProviderForUser call. From what I see in the Cognito console, it still seems to successfully link social providers with native users. I don’t know if there are any side effects I’m unaware of.

Expected behavior

Better developer experience and tested documentation on how to implement merging social provider with native accounts.

Reproduction steps

Try implementing account merging as described here by the OP: aws-amplify/amplify-flutter#1716

Project Identifier

809daeeb99aae0ff84c097e535188112

Log output

# Put your logs below this line


Additional information

No response

Before submitting, please confirm:

  • I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.
  • I have removed any sensitive information from my code snippets and submission.
@suzukidavid
Copy link

Before submitting, please confirm:
I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.
I have removed any sensitive information from my code snippets and submission.

@hanna-becker
Copy link
Author

hanna-becker commented Jun 29, 2023

I invested half my workday yesterday to compile this list of issues with suitable links to documentation. (Number 4 is a regression, btw., as we didn't have to deal with it in the past.) My intention is to raise awareness to the overall developer experience when implementing merging of social provider accounts with native Cognito accounts. A lot of people run into the same issues when implementing this. Furthermore, this issue could provide guidance with possible workarounds where the documentation is lacking.

As I mentioned in the reproduction steps, all you need to do is try to implement social account merging with the steps laid out in the linked issue, and you will automatically run into the issues mentioned above. With the amount of issues contained in this workflow, it would probably take me a week to construct a minimal, self-contained code example. I am willing to provide further details, though, if needed.

@hanna-becker
Copy link
Author

hanna-becker commented Jun 29, 2023

Okay, let me give a set of instructions, as best as I can:

  1. Set up a new amplify app with Angular.(*)
  2. Choose Cognito userpool for authentication
  3. Add Facebook as a social provider.
  4. Use the Authenticator from the amplify angular-ui npm package.
  5. Merge the Facebook federated identity with a native Cognito user in the PreSignup lambda. Use the code from the linked issue for this (meaning we create a native Cognito user if someone signs up with Facebook for the first time and link it with the Facebook identity. If a native account already existed, because the user created it before, we also link it.)
  6. Try to get the forgot password flow for the native user working even after they signed in with Facebook, which set their email to unverified in the user pool.
  7. Realize that you'll also run into the other issues mentioned above.

Don't hesitate to ask for clarification, if any of these steps are unclear or need more details.

(*) The issues are not specific to Angular, it would probably work the same to set up an example with React or Vue, as long as there is a amplify-ui-package with an authenticator component that supports Facebook login and the forgot password flow.

@abdallahshaban557
Copy link
Contributor

Hi @hanna-becker - this is an absolutely fantastic level of detail - thank you very much for taking the time to right these steps for us! Our support team is taking a look to reproduce this (With and without the Authenticator - this seems like it would impact non-Authenticator users as well). We will also validate these behaviors with the Cognito team.

@nadetastic nadetastic transferred this issue from aws-amplify/amplify-cli Jun 30, 2023
@nadetastic nadetastic added the Auth Related to Auth components/category label Jun 30, 2023
@nadetastic nadetastic self-assigned this Jun 30, 2023
@nadetastic
Copy link
Contributor

Hi @hanna-becker thank you for opening this issue. I took a look at the description and reproduction steps and wanted to get some clarification. I see that part of the flow is to create a new user with a username/password if one doesn't exists in order then run the merge. Is there a specific reason you have implemented this logic, vs just having the one federated user account?

@nadetastic nadetastic added investigating This issue is being investigated pending-response Issue is pending response from the issue requestor and removed pending-triage Issue is pending triage labels Jul 1, 2023
@hanna-becker
Copy link
Author

Hi @nadetastic, I vaguely remember when initially implementing this about a year ago that I ran into issues when sticking with the federated user for federated logins and then trying to merge them with a native user simulating a native signup with the same email address that happened after the federated sign up. Unfortunately, I don't remember the details. It's possible that nowadays it would work without creating a native user under the hood.

@hanna-becker
Copy link
Author

In any case, when creating a Cognito user and then merging the federated identity, I end up with a Cognito user that has the social provider listed in the identities section. This feels predictable to me. When doing it the other way around, I am not sure what kind of user object to expect in Cognito. Would it be a single user object or two separate ones linked to one another?

@nadetastic
Copy link
Contributor

@hanna-becker Cognito recently resolved an issue with linking federated users to an existing user profile in Cognito User Pools, and this may have been part of the issue you experienced a year ago.

As for the linking the users the other way around - it should still work the same, I believe the actual process is to link an existing user profile in a user pool (DestinationUser) to an identity from an external IdP (SourceUser). In other words, if you have a Cognito user profile and an idP user profile, the AdminLinkProviderForUser API will merge the IdP user profile into the Cognito user profile. The recommendation is to use the PreSignUp trigger for this so that you do not end up with two users in your pool (Cognito user and IdP user).

With that said, if you do not already have a Cognito user and create an IdP user, then Im not sure why you are creating a Cognito user for them? Are you facing issues with just having the IdP user?

@hanna-becker
Copy link
Author

hanna-becker commented Jul 3, 2023

It may not be necessary anymore to always create a native user after aws-amplify/amplify-flutter#1716 was closed. In that case, issues 3 and 4 of my list would change to the question on how to trigger the post confirmation hook for federated signups.

Even if I reimplemented this path, there is still the other path where a user first signs up natively and then later also wants to use federated sign in with the same email address. In that case, the remaining issues listed above still apply:

  1. In the case of Facebook as an Idp, they wouldn't be able to use the forgot password flow on their native accounts without my hacky PreTokenGeneration always setting email_verified to true workaround.
  2. To implement the workaround, I'd still have to work around the cyclic dependency issue when trying to give the PreTokenGeneration permissions on auth.

As for the false positive on the "Invalid SourceUser" error, are you suggesting this is a race condition when calling AdminLinkProviderForUser shortly after creating the native user?

@nadetastic
Copy link
Contributor

nadetastic commented Jul 7, 2023

@hanna-becker

Let me address each point from the initial comment in order:

1. Missing Ability to map email_verified

I can confirm that this also works for Sign In with Apple in addition to Google. However, I'm curious which provider's you are seeing this issue with? It could be a case that the provider doesn't provider an attribute that can be used to map to email_verified. In that case, I would recommend to use a combination of PostConfirmation and PostAuthentication triggers (instead of PreTokenGeneration) in order to update the email_verified attribute to true - PostConfirmation for when a user Federates for the first time, and PostAuthentication for subsequent user Federation.

2. Cyclic dependency

Since this issue is documented here, I'm following up with the CLI team for any additional updates.

3. Calling AdminSetUserPassword does not trigger PostConfirmation

This is expected, and if you need to call the PostConfirmation hook you would need to use one of the supported events instead.

4. AdminConfirmSignUp throws "user already confirmed"

This can happen in the case where you use AdminCreateUser to create a user. This action will set the user status to "FORCE_CHANGE_PASSWORD" (docs), however any action to Confirm a user expects the user Status to be "UNCONFIRMED", and is why it fails. More information on user confirmation.

To better understand what your flow is and try to provide some alternative, can you clarify what type of resources you are creating? I'm assuming that you are creating items in a database. If so, can this also be done when you call AdminSetUserPassword instead, or at a different part of your flow?

5. AdminLinkProviderForUser randomy throws an InvalidParameterException

Any additional context on what is happening when this error is thrown? As mentioned in the previous comment, this could be a race condition when calling AdminLinkProviderForUser shortly after creating the native user.

--

Additionally, in the case where a user first signs up natively and then later also wants to use federated sign in with the same email address, it's recommend that you initiate the AdminLinkProviderForUser in the PreSignUpTrigger. Then to keep the email as verified, use a combination of PostConfirmation and PostAuthentication triggers.

@hanna-becker
Copy link
Author

@nadetastic

However, I'm curious which provider's you are seeing this issue with?

There is no option, as far as I am aware, to map the email_verified attribute for Facebook. It would be helpful in this case if there were the option to assign a fixed value in the case where there is no mapping available.

In that case, I would recommend to use a combination of PostConfirmation and PostAuthentication triggers (instead of PreTokenGeneration) in order to update the email_verified attribute to true

Why implement a workaround in both the PostConfirmation and PostAuthentication triggers, vs. just once in the PreTokenGeneration trigger? According to this doc the PreTokenGeneration trigger is called for both the first and all subsequent signins, so it seems like the right place to me.

  1. Cyclic dependency
    Since this issue is documented unexpected stack trace for cyclic dependency error when attempting to grant auth access to lambda amplify-cli#11881, I'm following up with the CLI team for any additional updates.

I saw this issue, but I took from the description that it's only concerned with removing the stack trace from the error output, not solving the cyclic dependency issue itself.

  1. Calling AdminSetUserPassword does not trigger PostConfirmation
    This is expected, and if you need to call the PostConfirmation hook you would need to use one of the supported events instead.

I would argue it is documented, not expected. From a developer perspective, I would expect any API call that confirms a user to also trigger the PostConfirmation trigger.

I'm assuming that you are creating items in a database. If so, can this also be done when you call AdminSetUserPassword instead, or at a different part of your flow?

Yes, I'm creating user specific items in DDB tables. And yes, I believe we can do it in the PostConfirmation trigger, instead. Thanks for the suggestion.

Let me just stress once again that I didn't open this issue looking for workarounds, because I already found them. My point is to surface stumbling blocks when implementing social account merging that probably everybody runs into. I love the Amplify framework! It has helped us in many ways moving fast and releasing things quickly that would otherwise have taken ages of implementing boilerplate code for basic concerns such as authentication and API setup. I want Amplify to stay superb - I just happened to notice that this particular flow could be improved for developers, or at least the workarounds could be documented in a single place, so that it's easier for someone who wants to implement account merging.

@nadetastic nadetastic removed the pending-response Issue is pending response from the issue requestor label Jul 11, 2023
@cwomack cwomack added documentation Related to documentation feature requests and removed investigating This issue is being investigated labels Jul 11, 2023
@nadetastic
Copy link
Contributor

Related to #5104

@guillesha
Copy link

Hi I am having same problem number 5, I checked destination is correct, it could be in fact a racing condition but I am not sure how to avoid it as all seems right at the moment

@balareddy-ec
Copy link

I am also having the same issue with number 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auth Related to Auth components/category documentation Related to documentation feature requests
Projects
None yet
Development

No branches or pull requests

7 participants