Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed #1171
[Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed #1171
Changes from 4 commits
9097789
78dd3f0
ad0c215
2eeed45
b0d82c9
8f8b0c5
ffd6ebf
f09ba06
ef654db
b1157d9
5f6df2d
04733e8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expires is for cookies, it doesn't work for localstorage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question: What does this look like if both the original and copy are stored in local store? They're stored separately still correct? Otherwise, since they have the same name, one would override the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vcua-mobify The original and copy would both point to LocalStorage only in case of standalone setups in which case the token values will be the same and
hasSFRAAuthStateChanged
does not check values it only compares the keys. which again will be the same in this case. So we should be good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to be actively comparing the values since we open ourselves up to an edge case if we just look at the keys.
On a PWA Kit only environment (the only scenario where both point the original and copy are in local store) I think it's fine if we compare the value with itself or if the copy overrides the original since they should never diverge in this scenario in the first place.
On a hybrid environment, the original will be in the cookie store while the copy is in local store so we should have different values then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if user stays on PWA kit and keeps refreshing the page? (no SFRA)
It seems to me
hasSFRAAuthStateChanged
always evaluate totrue
and PWA will always use refresh token, and never re-use access token?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cookie doesn't exist, this function should return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, let's add a test case for when refresh cookies does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing by no SFRA you mean in case of a PWA Kit only setup:
In this case, the
storage
andstorageCopy
would both be pointing toLocalStorage
andrefreshTokenKey
===refreshTokenKeyCopy
sohasSFRAAuthStateChanged
will befalse
If by no SFRA you mean Hybrid Setup but the user hasn't yet navigated to SFRA,
Then
storage
will point to CookieStorage andstorageCopy
will point to LocalStorage in which case PWA Kit itself will write the refresh_token in both places. And if the user keeps refreshing on PWA kit without navigating to SFRA againrefreshTokenKey
===refreshTokenKeyCopy
sohasSFRAAuthStateChanged
will befalse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be a stickler but why are we only comparing the keys and not the values of the refresh token?
By comparing just the keys we open ourselves to the following edge case in hybrid:
In the above, SFRA has changed the login state a couple of times but settled on a cc-nx-g when transitioning to PWA. If we compare only the keys, in this case we are not discarding the old access token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
hasSFRALoginStateChanged(...)
to compare values if refershToken keys are same.Thanks @vcua-mobify for catching that 🎉