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

AWSMobileClient confirmSignIn success callback not called #1248

Closed
alistra opened this issue Feb 26, 2019 · 15 comments
Closed

AWSMobileClient confirmSignIn success callback not called #1248

alistra opened this issue Feb 26, 2019 · 15 comments
Assignees
Labels
bug Something isn't working mobile client Issues related to AWSMobileClient work in progress Issues was triaged and investigation done

Comments

@alistra
Copy link

alistra commented Feb 26, 2019

Describe the bug
We use the drop-in UI from Cognito (using AWSMobileClient.showSignIn method)
We create a new Cognito user, it starts in the FORCE_CHANGE_PASSWORD state.
We try to log in with the username and temporary password in the drop-in UI.
After tapping the Sign In button nothing happens.
User should be presented with a view controller to change password.

We tried to use the manual sign in methods.
We use AWSMobileClient.signIn method. We get and error with .newPasswordRequired case.
We use AWSMobileClient.confirmSignIn method to try to confirm change the initial password as per documentation here: https://aws-amplify.github.io/docs/ios/authentication
This method doesn't return any success callback for us, so we don't have any way of continuing the flow in the app.
The user is marked as confirmed on the Cognito panel.
It appears as the success callback is not called on some code paths.

It seems like those 2 behaviors are caused by the same bug in the missing success callback.

To Reproduce
A code sample or steps:

func signupScenario() {
       AWSMobileClient.sharedInstance().signIn(username: "testuser", password: "Test123!", validationData: nil) { signInResult, error in

           if let result = signInResult {
               // support this result.codeDetails
               switch result.signInState {
               case .newPasswordRequired:
                   self.setNewPassword("test123123")
               default:
                   break
               }
           } else if let customError = error {

           }
       }
   }

   func setNewPassword(_ password: String) {
       AWSMobileClient.sharedInstance().confirmSignIn(challengeResponse: password) { signInResult, error in
           // <-- this callback never gets called
           if let result = signInResult {
               switch result.signInState {
               case .signedIn:
                   log("user signedIn")
               default:
                   break
               }
           } else if let customError = error {

           }
       }
   }

Which AWS service(s) are affected?
Cognito

Expected behavior

  1. We expect the drop-in UI to handle the case with user in FORCE_CHANGE_STATE correctly, by presenting a view controller that will allow the user to change the password
  2. We expect the AWSMobileClient.confirmSignIn method to call the provided callback upon successful confirmation. Currently the confirmation is performed, but the callback is not called.

Environment(please complete the following information):

  • AWS iOS SDK Version: 2.9.1
  • Dependency Manager: Cocoapods
  • Swift Version : 4.2
  • Device: iPhone X Simulator
  • iOS Version: iOS 12.2
  • Specific to simulators: No

Additional context
We inspected the open source code of AWSMobileClient and it doesn't seem that there is any code path present that would call the block upon success. Maybe it's just a copy-paste error?

CC @anaglik

@alistra
Copy link
Author

alistra commented Feb 26, 2019

case .newPasswordRequired:
               DispatchQueue.main.async {
                   self.setNewPassword("test123123")
               }

If we change the code to the above, the success callback starts working.
This happens because the confirmSignIn method does the following:

       self.userpoolOpsHelper.currentSignInHandlerCallback = completionHandler

This callback is shared between signIn and confirmSignIn methods and if one is called in the callback of another, the callback is cleared.
This should be refactored to separate callbacks.

@desokroshan desokroshan added the bug Something isn't working label Feb 26, 2019
@desokroshan
Copy link
Contributor

@alistra Thanks for pointing it out. We will take a look and make necessary changes. It looks like you have it working for now. Please let us know if that is not the case and you are blocked on it.

@alistra
Copy link
Author

alistra commented Feb 27, 2019

@desokroshan it's a blocker for us, as we would like to use the drop-in UI, which there is still no workaround and we would like to not have to implement our own UI solution in our project.

The workaround only works for the non-UI login methods

@desokroshan desokroshan added the mobile client Issues related to AWSMobileClient label Mar 4, 2019
@rohandubal
Copy link
Contributor

Hello @alistra

This seems more to me like a race condition. The major indication being that once you use the main thread dispatch everything starts working. It might have to do something with the order of the calls. I will investigate and reply back once I identify the root cause.

Best,
Rohan

@rohandubal rohandubal added the investigating This issue is being investigated label Mar 17, 2019
@jamesingham
Copy link
Contributor

jamesingham commented Apr 5, 2019

We use the drop-in UI from Cognito (using AWSMobileClient.showSignIn method)
We create a new Cognito user, it starts in the FORCE_CHANGE_PASSWORD state.
We try to log in with the username and temporary password in the drop-in UI.
After tapping the Sign In button nothing happens.
User should be presented with a view controller to change password.

We are experiencing the exact same behaviour (AWS iOS SDK Version: 2.9.3). When the user state is FORCE_CHANGE_PASSWORD and the user has entered the correct credentials, pressing the Sign In button doesn't appear to do anything. Is this a bug or are we expected to manually handle the UI for the user to set a new password?

@rohandubal
Copy link
Contributor

Hello all,

There are 2 issues here mentioned in this thread.

  1. The drop in UI handling of new password required case.

This case is currently not supported by the drop in UI. We are tracking it as feature request.

  1. The API for confirmSignIn is running into some race condition.

I am investigating that issue and working on a fix.

Thanks,
Rohan

@jamesingham
Copy link
Contributor

OK thanks for the update.

@rohandubal
Copy link
Contributor

Hello @alistra and @jamesingham

I have been investigating this issue and have some findings to share. I wrote an app which does exactly what @alistra has mentioned and found that:

  • If the call to confirmSignIn() is made on the same thread as the one on which callback to signIn is received, it could result in the race condition which causes the described issue.
  • If the call to confirmSignIn() is made separately / on a different thread, then the callback is triggered correctly

From what I can see, the confirmSignIn API should not be called from signIn completion handler as it requires some user interaction. So in normal cases when the signIn completion handler exits, confirmSignIn calls would not be impacted by it. Can you de-couple the code in UI and see if it fixes your issue?

I will be trying to work on decoupling two callbacks so that invoking confirmSignIn on the same thread is also not an issue.

Thanks,
Rohan

@rohandubal
Copy link
Contributor

On investigating this further, I identified that the code changes would be more involved and would require plenty of edge cases and race conditions if the APIs are accessed incorrectly.

I am going to mark this as an enhancement which we can revisit in the future.

But currently, my recommendation would be to use the APIs independently from user interactions rather calling the confirmSignIn from completionHandler of signIn.

Could you please confirm if the issue is resolved if handled via separate user interactions?

Thanks,
Rohan

@rohandubal rohandubal added pending-response Issue is pending response from the issue requestor and removed bug Something isn't working investigating This issue is being investigated labels Jun 6, 2019
@sourabhardwaj
Copy link

sourabhardwaj commented Jun 12, 2019

@rohandubal After decoupling the confirmSignIn call from signIn, I'm able to get the callback but it is always resulting in AWSMobileClientError:

[Error] [NSOperationQueue 0x600002ddb060 (QOS: UNSPECIFIED)] [LoginViewController.swift:87] setNewPassword(_:) > invalidParameter(message: "Invalid attributes given, name is missing")

It doesn't seem like confirmSignIn method requires us to provide attributes. Kindly suggest.

Many thanks,

EDIT:
FYI, the name and email are required parameters for user's signup. I tried passing name in validationData in signIn parameters but doesn't seem to work.

@jamesingham
Copy link
Contributor

  1. The drop in UI handling of new password required case.
    This case is currently not supported by the drop in UI. We are tracking it as feature request.

Interestingly I have just been using the Android equivalent sdk and that handles this by providing a set new password view.

@palpatim palpatim added question General question and removed pending-response Issue is pending response from the issue requestor labels Jun 24, 2019
@royjit
Copy link
Contributor

royjit commented Jul 19, 2019

@sourabhardwaj We need to pass required attributes (if you have set them) while making confirmSignIn API call. In your case "name" is a required attribute, the code change will be:

AWSMobileClient.sharedInstance().confirmSignIn(challengeResponse: newPassword,
                                                       userAttributes: ["name": nameOfUser]) {_,_ in}

I have raised a PR #1672 to support attributes on confirmSignIn method.

Thanks,
Jithin

@royjit royjit added the bug Something isn't working label Jul 19, 2019
@royjit royjit self-assigned this Jul 19, 2019
@royjit royjit removed the question General question label Jul 19, 2019
@palpatim palpatim added the work in progress Issues was triaged and investigation done label Jul 22, 2019
@sourabhardwaj
Copy link

Awesome @royjit , much needed change in the SDK.

@royjit
Copy link
Contributor

royjit commented Jul 25, 2019

PR #1672 is released, which fixes passing user attributes to confirmSignIn.

  1. Race condition when calling confirmSignIn from sign callback. Tracked in AWSMobileClient race condition in calling confirmSignIn from sign callback #1686
  2. No callback when using drop-in UI AWSMobileClient.showSignIn with FORCE_CHANGE_PASSWORD state. Tracked in New password required state not propagated to drop-in UI #1711
  3. No drop-in UI handling of new password required case. Also tracked in New password required state not propagated to drop-in UI #1711

@royjit
Copy link
Contributor

royjit commented Sep 6, 2019

Closing this issue since we are tracking the sub issues in the related github issues (#1686 #1711 ). The original posted issue is tracked here #1686 and is waiting release.

@royjit royjit closed this as completed Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mobile client Issues related to AWSMobileClient work in progress Issues was triaged and investigation done
Projects
None yet
Development

No branches or pull requests

8 participants