fix(UserManager): handle concurrent token refresh requests via leader election

[DASPRiD]

Introduces leader election for concurrent token refresh requests. This adds a new dependency (broadcast-channel) which takes care of the election process.

Closes #430

[codecov[bot]]

Codecov Report

Merging #434 (f760441) into main (c9fcaa0) will increase coverage by 0.15%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
+ Coverage   75.25%   75.40%   +0.15%     
==========================================
  Files          44       44              
  Lines        1572     1590      +18     
  Branches      288      292       +4     
==========================================
+ Hits         1183     1199      +16     
- Misses        363      364       +1     
- Partials       26       27       +1     

Flag	Coverage Δ
unittests	75.40% <100.00%> (+0.15%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/UserManager.ts	57.14% <100.00%> (+2.63%)	:arrow_up:
src/ResponseValidator.ts	90.26% <0.00%> (-1.70%)	:arrow_down:
src/OidcClient.ts	94.23% <0.00%> (ø)
src/SigninRequest.ts	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c9fcaa0...f760441. Read the comment docs.

[pamapa]

Great work , thanks for the fast response!

Do we really need the broadcast-channel library, modern browsers support the broadcast-channel feature natively. When i look into https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API:

• IE has no support -> OK
• messageerror event is not all over supported -> we do not need it -> OK

The leader election feature could be taken, simplified and converted into TypeScript from the broadcast-channel library.

The main idea behind this is to have as few external dependencies as really needed....

[DASPRiD]

I suppose it should be no problem to use the native broadcast-channel. I'll have a look at this.

[DASPRiD]

@pamapa Just wondering, as this would ease implementation a lot: Is the browser support for the LockManager API too thin/new to be used?

https://developer.mozilla.org/en-US/docs/Web/API/Web_Locks_API

[pamapa]

Good question. We only care about modern browsers (primary chrome, firefox, edge, safari + mobile versions, not IE) anyway.

In that list i only see Safari to be a problem for us now. Maybe we could first detect if that API exists and then use it, if not found we keep current way, which was the default for many years in the predecessor library...

@kherock What is your opinion here?

[DASPRiD]

That sounds like a sane way to me to implement it!

[kherock]

Web locks seem like a more appropriate API to use since we don't have any information to publish between tabs.

Also, I have concerns over using the _userStoreKey as the resource to lock onto. It seems like it we should only lock when the web storage backend is shared across tabs, since normally it isn't when sesssionStorage is being used. Today, if I open up a new tab and assume the same identity with a silent refresh, the new tab will have its own unique refresh token and access token in sessionStorage. By always locking onto _userStoreKey, we'd be unnecessarily holding up the other tabs with matching client IDs. The resource string should be derived from the refresh token itself, or locking should be conditional depending on the way storage persists across tabs.

Are you replacing the webstorage backend with localStorage @DASPRiD? If so, I think a better way to do this would be something like this

// extend with a method for obtaining a lock and running a task
interface StateStore {
  async runTask(key: string, task: () => Promise<void>);
}

class WebStorageStateStore implements StateStore {
  /** whether tasks run against this store require synchronization from other instances of oidc-client */
  private _sync: boolean;

  public constructor({ prefix = "oidc.", store = localStorage, sync } = {}) {
        this._store = store;
        this._prefix = prefix;
        this._sync = sync ?? store === localStorage
  }

  public async runTask(key: string, task: () => void | Promise<void>) {
    if (!this._sync) return await task();
    await navigator.locks.request(this._prefix + key, async lock => {
      // The lock has been acquired.
      await task();
      // Now the lock will be released.
    });
  }
}

This locking utility probably doesn't need to be in WebStorageStateStore, but in my mind, the storage backend should influence its default behavior regardless of where it is.

[DASPRiD]

I am using localStorage, yes. But Wouldn't sessionStorage suffer from the same issue, two tabs still access the same sessionStorage value with the same key. Only memory storage wouldn't suffer from this issue.

I agree that we could limit this to the refresh token function and locking onto the refresh token. I assume we wouldn't need a BroadcastChannel in this case, as after acquiring the lock, we could potentially just verify if the access token was just refreshed recently (e.g. in the last X seconds).

[kherock]

Page sessions are unique per tab! From MDN.

Whenever a document is loaded in a particular tab in the browser, a unique page session gets created and assigned to that particular tab. That page session is valid only for that particular tab.

In my case, I'm writing an enterprise application where sessions don't persist beyond the lifespan of a tab. So for me separate tabs for the same application shouldn't have to ever interact or conflict with each other since they never shared any state to begin with. But it does seem like there is an exception where a browser may duplicate a tab and its associated session storage:

Duplicating a tab copies the tab's sessionStorage into the new tab.

So it does make sense that we need to provide some synchronization mechanism independent of the storage backend.

[DASPRiD]

Uh, so this becomes kinda tricky. Basically when you duplicate a tab using session storage while using refresh tokens, they become permanently linked to each other.

I guess the following method could be implemented (given that LockManager is available, otherwise we don't handle it):

1. Acquire a lock on the refresh token.
2. When lock is acquired, perform the token refresh and broadcast the new user object via BroadcastChannel.
3. When a user is received via the BroadcastChannel while waiting for a lock, cancel the lock request.
4. When a user is received via the BroadcastChannel outside of acquiring a lock, update the local user and reset the refresh timer.

Does this sound right to you?

[DASPRiD]

Well, I went ahead and implemented this to solve it for localStorage, which was relatively easy. I still have to think how exactly I want to handle that for sessionStorage. Any input is welcome!

[pamapa]

Thanks for working on this, looks much simpler now with locking. For easier review could you please squash your changes into a single commit?

[Badisi]

@DASPRiD thanks for raising this issue as I also faced it a few months ago and was yet to find an ideal solution.

---

In my case, I'm using a simple web app which stores its tokens in memory (no local/session storage of any sort).

Opening this same web app in two different tabs, results in the following:

Using [email protected]

Revoke Refresh Token: ON (ie. activating refresh token rotation)

When ON, Keycloak revokes refresh tokens and issues another token that the client must use. This action applies to OIDC clients performing the refresh token flow.

1. App is opened in TabA and receives a Refresh Token (RTA1)
2. App is opened in TabB and receives RTB1
3. TabA is about to expire and request a renew
   • RTA1 is used and revoked, and RTA2 is given in exchange
4. TabB is about to expire and request a renew
   • RTB1 is used but as TabA already did a rotation, TabB is perceived as a potential threat and all outstanding tokens are invalidated
   • Response from oidc-client is Error: Stale token
   • TabB now requires a full login
5. TabA is about to expire again and request a renew
   • RTA2 is used but it was revoked during 4) so TabA is also blocked and requires a full login

Using Auth0

Refresh Token Rotation: ON

When enabled, as a result of exchanging a refresh token, a new refresh token will be issued and the existing token will be invalidated. This allows for automatic detection of token reuse if the token is leaked. In addition, an absolute expiration lifetime must be set.

No error whatsoever, both tabs can renew their tokens.

Conclusion

I'm not familiar enough with the OIDC spec so I don't know if this could relates to a bug in one or another. But I think this PR could solve the first case with Keycloak, if only the Refresh Token is not used as the resource to lock on (because both tabs receive different refresh tokens and thus the BroadcastChannel wouldn't work).

[kherock]

@Badisi thanks for sharing this, I was also seeing a similar error with my Keycloak application and decided to simply disable refresh tokens, but your explanation makes sense.

Rather than locking on a refresh token, what happens if we lock onto session_state instead? And then subscribe to broadcast channels using that state as the topic name?

[Badisi]

That would work because session_state is unique and thus identical for each tabs. But it would work only for Keycloak (at least) because there is no such info returned by Auth0...

The only common data I'm thinking of is the client-id.

[Badisi]

The more I think about it, the more I realize it should be an issue on Keycloak side.

Auth0 reasoning (see Automatic-Reuse-Detection) seems way more logical. They invalidates the RT family only if an already invalided RT is being reused.

Whereas Keycloak is invalidating the RT family if any previously delivered RT (before the rotation) is being reused. Which makes no sense to me...

[DASPRiD]

Whereas Keycloak is invalidating the RT family if any previously delivered RT (before the rotation) is being reused. Which makes no sense to me...

Yeah, this seems to be a Keycloak issue. The way their implementation works, you could never have two independent clients authenticated at the same time, because they invalidate each others tokens. The way auth0 works makes a lot more sense.

[DASPRiD]

Thanks for working on this, looks much simpler now with locking. For easier review could you please squash your changes into a single commit?

I just squashed it all down to a single commit. Regarding linked sessionStorage sessions, I think we should consider that a different issue.

[DASPRiD]

For some reason, I cannot answer to your review comments:

Yes, the check for navigator.locks covers testing for the existence of the BroadcastChannel feature, as the latter is supported much longer already.

[pamapa]

LGTM

[pamapa]

@kherock I leave the final approval to you :-)

[pamapa]

@Badisi Thanks for helping reviewing. Valid concerns, which must be answered/resolved before we merge...

[DASPRiD]

@pamapa are we waiting for @kherock to shime in here?

[pamapa]

@DASPRiD Yes we are waiting for @kherock for his approval and please also answer to @Badisi questions...

[DASPRiD]

@pamapa I thought I answered all his questions :)

[pamapa]

I can not see the answers to https://github.com/authts/oidc-client-ts/pull/434#discussion_r835527285 + https://github.com/authts/oidc-client-ts/pull/434#discussion_r835532113

[DASPRiD]

Oh, stupid me, I had a review running instead of commenting directly, so my comments were pending :D

The second one is not really a question to me, is it? Technically @kherock already said that we should lock on the refresh token, if you guys want a slightly different naming though, we can change that.

[Badisi]

@DASPRiD , as I explained here, using the refresh token as a lock key would be useless because each browser tabs receives a different refresh token.

To me the only data that is commonly shared by each tabs is the client-id. Just waiting on @kherock thoughts about that 🙂.

[DASPRiD]

@kherock Can you have a look at the inline conversation? :)

[Badisi]

~~It might help summarizing a bit because this discussion is quite long :)~~ ~~(so correct me if I'm wrong or if something is missing)~~

~~Pending required changes:~~ ~~- [ ] Change the name of the key for the BroadcastChannel and the Lock~~ ~~=> use session_state when available or refresh_token as a fallback~~ ~~- [ ] Eliminate potential race conditions~~ ~~=> @kherock proposal here~~ ~~OR~~ ~~=> @DASPRiD proposal : "use a 250ms delay in the leader function"~~