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
fix(UserManager): handle concurrent token refresh requests via leader election #434
base: main
Are you sure you want to change the base?
fix(UserManager): handle concurrent token refresh requests via leader election #434
Changes from 1 commit
fb75619
f760441
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.
Still needs to decide what key should be best for the
BroadcastChannel
and theLock
.@kherock
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.
The way that this code handles competing requests seems leaky. It looks like when two tabs both ask for a lock on the refresh token state, they can still both send refresh tokens sequentially. It seems to make an assumption that sending a message informing the other tab of the new user via the broadcast channel is a synchronous operation, but if this were the case, no web locks would be necessary at all. I doubt that there will ever be a case where releasing and re-obtaining the lock takes longer than sending a BroadcastChannel message, but I think the possibility is there.
Instead, I think it would be better to use the Web Locks API to perform leader election:
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 like this implementation best.
@kherock, just a few questions:
storeUser(user)
only performed by the leader ? (and not by everyone at the end, like_events.load
)postMessage
never occurred ? (ex:refreshUser()
taking too long and in the meantime user closes the leader tab)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 didn't think about the scenario of the tab being interrupted, but more realistically, my code doesn't handle when the leader encounters an error either.
storeUser
writes the user tolocalStorage
/sessionStorage
which is presumably shared by both tabs in this scenario, so it only needs to be serialized and stored once. The user actually doesn't even need to be sent over the channel at all, the other tab could just as well doreturn await this._loadUser()
after receiving the notification from the leader (or by waiting for the lock to release, though this doesn't account for the lock releasing because of a failure or interruption).I think think finding a "proper" lock key to use in a separate issue. My hunch is that we should lock onto
session_state
when the IDP implements OIDC sessions (e.g. Keycloak) and fall back to the refresh 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.
Agreed that some error handling needs to be made.
If I'm not mistaken,
sessionStorage
is not shared between tabs - so it won't work in that case.I've seen
session_state
being returned by Keycloak (but not Auth0), so yes we could use it if we have it or fall back otherwise. But I'm still not confident in usingrefresh token
as the fall back. Each tab receives a different refresh token, which makes the purpose of the lock completely useless.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.
In instances where sessionStorage isn't shared, how would two tabs obtain the same refresh token in the first place? Also, see this comment: #434 (comment)
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.
@DASPRiD, I'm using neither LocalStorage nor SessionStorage, I'm keeping everything in memory. But as I see it, this concurrency issue exists no matter the type of storage, so I would opt for a way to solve it once and for all (and not only for LocalStorage). Why do you think your proposal does only cover LocalStorage ? I was convinced that simply using a common lock key would be enough to solved it for SessionStorage/MemoryStorage too. Don't you think ?
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.
Actually no, this issue does not affect memory storage, as memory storage is always bound to it's own session without sharing it with any other process.
The problem with SessionStorage is the part about session storage duplication when a tab is duplicated. In that instance, they will share the same session (from the perspective of the IDP), but not from their own perspective.
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.
@kherock Actually, I do see an issue with using client ID + sub as the lock key: It would block unrelated tabs with different sessions but same client ID + sub from performing their refresh, that's why I was using the refresh 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.
Ok, then we are talking about two different concurrency issues.
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.
Well yeah, the issue is absolutely on their side. Solving it with a shared lock key like that is a hacky way to solve your issue though. Especially since it would only really solve it as long as your are using local storage. If you are using anything else (based on Kheros asked change of not using the BroadcastChannel), this would not help with your issue.
It would, as mentioned above, also introduce the issue that it would block possibly unrelated tabs from performing their refresh. I really only see one way here to address your issue without affecting all other users:
@kherock Actually, instead of using the refresh token, we could also add a randomly generated ID to the store which is used as common identifier for the lock, but when
session_state
is available, store that as the common identifier. That way it would address @Badisi issue, and make us not rely on the value of the refresh token.Although to ultimately address their issue, we'd need to use the BroadcastChannel. Thinking about that, we could add a delay of maybe 250ms or so into the leader function. That would address any possible common race condition. We probably might not even need such a long delay.