Setting Store Tokens should only clear tokens that no longer have a value #13584
Conversation
…alue. This resolves thrashing events when a consumer is listening to storage events, preventing them from seeing a 'clear' followed immediatly by a 'set' value.
jon-armen
requested review from
ukhan-amazon,
haverchuck,
cshfang,
jimblanc and
HuiSF
as code owners
|
Hello, @jon-armen and thanks for taking the time to submit this PR! We'll get this reviewed by the team internally and follow up with any questions or feedback that we have. |
|
Thanks for working on this issue @jon-armen, could you also add some tests for the behavior? |
|
@joon-won I'll see if I can get some tests together in the next few days. @itsramiel , do you have thoughts on tests you might like to see to validate your use case? |
One way that comes to mind is to call // Start storeTokens and loadTokens simultaneously
const storePromise = tokenStore.storeTokens(tokens);
const loadPromise = tokenStore.loadTokens();
const [storeResult, loadResult] = await Promise.allSettled([
storePromise,
loadPromise,
]);However I am not sure this will work always, half of the times, or never in tests since it really depends on how the js env handles async calls and running other functions while sync calls are awaited. But that would be my approach |
There was a problem hiding this comment.
Typewise, tokens.username, tokens.accessToken and tokens.clockDrift are non optional types so technically no need for the if...else on line 100, 110, and 165 since also in the before changes they were just being set.
Maybe we can remove the if...else from those ones?
There was a problem hiding this comment.
I've removed the extra if/else statements.
| ); | ||
| } else { | ||
| await this.getKeyValueStorage().removeItem(authKeys.clockDrift); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
do we need a statement to removeItem(authKeys.oauthMetdata)?
There was a problem hiding this comment.
Yes, good catch. I don't think that was in tokens when I put this together a few months ago.
There was a problem hiding this comment.
Don't think we should set/remove oauthMetdata here. It's separately managed on line 243.
There was a problem hiding this comment.
Regarding oauthMetadata, I agree with @Samaritan1011001. Do we know if we need to set/clear it in storeTokens here?
| @@ -95,31 +95,43 @@ export class DefaultTokenStore implements AuthTokenStore { | |||
|
|
|||
| async storeTokens(tokens: CognitoAuthTokens): Promise<void> { | |||
| assert(tokens !== undefined, TokenProviderErrorCode.InvalidAuthTokens); | |||
| await this.clearTokens(); | |||
There was a problem hiding this comment.
we should assert that in the absence of this all keys in https://github.com/aws-amplify/amplify-js/blob/main/packages/auth/src/providers/cognito/tokenProvider/types.ts#L27 are cleared if unspecified.
Description of changes
Setting Store Tokens should only clear tokens that no longer have a value. This resolves thrashing events when a consumer is listening to storage events, preventing them from seeing a 'clear' followed immediately by a 'set' value.
Issue #, if available
#13583
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.