aws / aws-sdk-net-extensions-cognito

An extension library to assist in the Amazon Cognito User Pools authentication process
Apache License 2.0
102 stars 49 forks source link

Avoid checking _access_ token expiry when trying to refresh access via a refresh token. #76

Closed rib closed 2 years ago

rib commented 3 years ago

Description

Right now there is an awkward chicken and egg situation when trying to refresh expired access tokens...

As a workaround we've been doing the following to create fake user SessionToken state that we know will pass the internal EnsureAuthenticated() check that's done before making any InitiateAuthAsync() request...

// Create a fake expiry time for our current session tokens otherwise the internal call to
// EnsureUserAuthenticated() will see that our access token is expired and won't let us refresh it!
user.SessionTokens = new CognitoUserSession(null, null, refreshToken, DateTime.Now, DateTime.Now.AddHours(1));
var refreshReq = new InitiateRefreshTokenAuthRequest() {
        AuthFlowType = AuthFlowType.REFRESH_TOKEN_AUTH
};

If we don't do this then we get an authentication failure while trying to refresh expired access tokens.

Environment

Resolution

I think perhaps it would make sense to just drop the call to EnsureAuthenticated() from the top of CreateRefreshTokenAuthRequest:

https://github.com/aws/aws-sdk-net-extensions-cognito/blob/ddddb18296f88bd129e5ae52d40c6f021ed1c7db/src/Amazon.Extensions.CognitoAuthentication/CognitoUserAuthentication.cs#L586

Alternatively maybe there could be a check on the expiry of the refresh token instead of the access token in this context.


This is a :bug: bug-report

ashishdhingra commented 3 years ago

Hi @rib,

Good morning.

Thanks for posting the issue. The code that you mentioned is already mentioned as an example in Readme at https://github.com/aws/aws-sdk-net-extensions-cognito#authenticating-using-a-refresh-token-from-a-previous-session. I'm not sure on the design for calling EnsureAuthenticated() at the top of CreateRefreshTokenAuthRequest, may be it's a carry forward from the design of the other methods. Looking at high level it might make sense to remove call to EnsureAuthenticated() for this scenario, but I would rather keep this as it is to conform to the current design. The Cognito API appears to the return the ExpirationTime for the access token when using the sign-in or refresh token scenarios, hence it might not be possible to check the validity of refresh token for this scenario.

I will get this issue triaged with developer and let you know of further updates.

Thanks, Ashish

rib commented 3 years ago

Hi, thanks for the follow up.

Even if you can't validate the refresh token for some reason it still surely doesn't make sense to validate the access token when that's what you're trying to refresh? The validity of the access token is irrelevant here right? This surely can't be an intentional design and that README example is just applying the same necessary workaround to use this API?

It's probably true that the check was copy and pasted from another request implementation, but it looks like that was almost certainly a mistake.

Thanks for forwarding the issue to be triaged with a developer.

ashishdhingra commented 3 years ago

This needs to be investigated and checked with other SDKs for this scenario.

normj commented 2 years ago

I agree the call to EnsureAuthenticated does not make sense in this context and we should remove it. @rib are you interested in making a PR to remove it? Otherwise @ashishdhingra and I will get to this as soon as we can.

rib commented 2 years ago

Thanks for the review @normj - sorry I'm not in a good position to build + test if I make a PR (I'm using in Unity and the integration is a bit of an extra faff).

I mainly just wanted to raise this issue after it caused my application to break due to the recent internal change to assume all DateTime values are UTC. Although it would be good to also address the DateTime timezone issue (#77) it would also be good if there was no need to pass in a fake DateTime value in this context.

In the mean time I've updated my application to make sure I pass in UTC values to work around this issue.

Thanks again for considering this.

ashishdhingra commented 2 years ago

This is fixed in Amazon.Extensions.CognitoAuthentication 2.2.2

github-actions[bot] commented 2 years ago

⚠️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.