StartWithSrpAuthAsync Not Sync Safe

[swensorm]

Describe the bug

When authenticating with Cognito credentials according to the basic authentication example on the AWS SDK for .NET documentation using the StartWithSrpAuthAsync function, the overload which does not take a cancellation token lacks a .ConfigureAwait(false) and thus causes a deadlock scenario when used from a Synchronous context.

https://docs.aws.amazon.com/sdk-for-net/v3/developer-guide/cognito-authentication-extension.html

This was discovered while making a service call from a 3rd party CMS which is not asynchronous and requires us to do .Result on the response and block waiting for the call.

Line in question: https://github.com/aws/aws-sdk-net-extensions-cognito/blob/master/src/Amazon.Extensions.CognitoAuthentication/CognitoUserAuthentication.cs#L45

Please let me know if the proposed solution is acceptable and you would like a PR submitted for it.

Thank you kindly!

Expected Behavior

The SDK is usable from both asynchronous and synchronous contexts and returns a successful JWT token.

Current Behavior

The asynchronous thread marshaling back to the calling thread deadlocks the application.

Reproduction Steps

static Main(string[] args) {
  string token = GetCognitoTokenAsync().Result;
  Console.WriteLine(token);
}

private async Task<string> GetCognitoTokenAsync() {
  var provider = new AmazonCognitoIdentityProviderClient(new AnonymousAWSCredentials(), Amazon.RegionEndpoint.USEast2);
  var userPool = new CognitoUserPool(userPoolId, clientId, provider);
  var user = new CognitoUser(serviceUsername, clientId, userPool, provider, serviceClientSecret);

  var authRequest = new InitiateSrpAuthRequest() { Password = userPassword };
  AuthFlowResponse authResponse = await user.StartWithSrpAuthAsync(authRequest).ConfigureAwait(false);

  return authResponse.AuthenticationResult.IdToken;
}

Possible Solution

The async and await on the overload without the cancellation token are unnecessary and can be removed. This would result in using the other overloads proper handling of async contexts without adding additional code.

public virtual Task<AuthFlowResponse> StartWithSrpAuthAsync(InitiateSrpAuthRequest srpRequest)
{
  return StartWithSrpAuthAsync(srpRequest, default);
}

Additional Information/Context

The method overload which does accept a CancellationToken has complete usage of .ConfigureAwait(false) and we were able to make use of this as a workaround for now.

AWS .NET SDK and/or Package version used

Amazon.Extensions.CognitoAuthentication.2.4.2

Targeted .NET Platform

.Net Framework 4.8.2

Operating System and version

Windows 11

[ashishdhingra]

Hi @swensorm,

Good morning.

Thanks for reporting the issue. Could you please try using the overloaded version of StartWithSrpAuthAsync(InitiateSrpAuthRequest srpRequest, CancellationToken cancellationToken) in the version 2.4.2 of this library?

Thanks, Ashish

[swensorm]

Hi @ashishdhingra,

Correct, we are using the overload that accepts the cancellationToken as a workaround. I wanted to raise the issue as it appears the issue is still present in the latest master though and would hopefully prevent others from encountering issues in the future if it could be patched. The async/await on the single parameter overload is unnecessary and only serves to cause this deadlock scenario.

Thank you!

[ashishdhingra]

Hi @ashishdhingra,

Correct, we are using the overload that accepts the cancellationToken as a workaround. I wanted to raise the issue as it appears the issue is still present in the latest master though and would hopefully prevent others from encountering issues in the future if it could be patched. The async/await on the single parameter overload is unnecessary and only serves to cause this deadlock scenario.

Thank you!

@swensorm Thanks for the reply. The single parameter overload was not removed to avoid breaking existing customers. The 2 parameter overload was added recently to per request from one of the users recently. As such this is not a bug since we could not remove the single parameter version to maintain backwards compatibility. I would convert this to Q&A discussion so that it's discoverable by other users. Please confirm before this could be converted into discussion.

[swensorm]

Sorry, I should clarify, I am not suggesting removing the method altogether, merely to remove the async/await keywords. The signature of the method would remain identical and would be completely backwards compatible. Please see the "Proposed Solution" above.

An alternative approach would be to add .ConfigureAwait(false) appended to the call to the other overload. This would also work to solve the deadlock scenario.

[ashishdhingra]

Needs review with the team.

[ashishdhingra]

Fix would be to use .ConfigureAwait(false) to all methods without CancellationToken parameter.

[swensorm]

Hi @ashishdhingra. I have submitted a PR to fix the issue as you described. I also created an alternative PR showing the other method I was talking about (removing async/await keywords) that also fixes the issue in the same way. Please review whichever approach the team would prefer and close the alternative. Thank you!

[swensorm]

PR #130 merged with fix. Thank you all!

[github-actions[bot]]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.