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 response of the resend confirmation code request #43

Merged
merged 3 commits into from
Jan 12, 2022

Conversation

lammertw
Copy link
Contributor

@gaebel In PR #36 you asked me to use CodeDeliveryDetails as response type. However I made a mistake in the change there. Instead of wrapping the CodeDeliveryDetails in a new response object it's trying to parse the response json (which contains looks like {"CodeDeliveryDetails":{"AttributeName":"phone_number","DeliveryMedium":"SMS","Destination":"+********0388"}}) directly into a CodeDeliveryDetails. Which now results in an error. So this call currently always fails.

I've fixed it in this PR (and would greatly appreciate it if a new release could be made with this change since we currently have the error in our app on production, tnx!)

@lammertw lammertw force-pushed the fix-resend-confirmation-code-response branch from f02d7c0 to 6a13acd Compare January 11, 2022 18:46
@gaebel
Copy link
Contributor

gaebel commented Jan 11, 2022

Hi @lammertw, I forgot to write a test for this, my bad! I'll merge your PR and publish a new release in the morning. Could you also make the change in the IdentityProviderJS class and write the missing test case?

@lammertw
Copy link
Contributor Author

@gaebel Sure, I've fixed JS as well (basically changed it to match the forgot password response since it's the same). I'd like to add some test, however when I'm running the existing tests none of them work. Not sure if I'm missing anything. I created a new Cognito instance on AWS for this and use that when creating the IdentityProviderClient, however I'm getting this:

actual value is not null expected null, but was:<com.liftric.cognito.idp.core.IdentityProviderException$NotAuthorized: A client attempted to write unauthorized attribute>

@gaebel
Copy link
Contributor

gaebel commented Jan 12, 2022

@gaebel Sure, I've fixed JS as well (basically changed it to match the forgot password response since it's the same). I'd like to add some test, however when I'm running the existing tests none of them work. Not sure if I'm missing anything. I created a new Cognito instance on AWS for this and use that when creating the IdentityProviderClient, however I'm getting this:

actual value is not null expected null, but was:<com.liftric.cognito.idp.core.IdentityProviderException$NotAuthorized: A client attempted to write unauthorized attribute>

I think writable custom attributes have to be manually enabled in the user pool.

@lammertw
Copy link
Contributor Author

@gaebel I finally got the tests to work. I assume you haven't documented anywhere how to exactly set up the Cognito User Pool but perhaps it's good to know for the future. So some of the things I had to do:

  • When creating the user group make sure user account recovery was set to "Email if available, otherwise SMS". This is to ensure that email would not become required since the test suite doesn't use emails.
  • Add target_group as custom attribute
  • Add a Pre Signup Lambda that would automatically confirm the user:
exports.handler = (event, context, callback) => {
    event.response.autoConfirmUser = true;
    context.done(null, event);
};

Because of this setup, it's not possible to add a test case for the resendConfirmationCode that I added. I guess for similar reasons the confirmSignUp is not included in the test cases. So hopefully you're ok with that and can now merge the PR.

@gaebel
Copy link
Contributor

gaebel commented Jan 12, 2022

Yes, that's unfortunately not documented, thanks for the writeup @lammertw – I'll add it to the Contributing.md. Oh... yeah this can't currently be tested in this repo. We're testing the confirmation requests (except resendConfirmationCode) in our private project, though. I may add this missing piece to this repo in the coming weeks.

@gaebel gaebel merged commit 5bf6e03 into Liftric:master Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants