Storage events can cause race conditions

[jeroenheijmans]

In the auth-service there is a storage event listener added. This might cause race conditions. For example:

1. Tab A and Tab B both have the app open, not logged in yet
2. Tab A logs in. Internally in auth-service from the library, a function will run that starts with setting the access_token in storage
3. Tab B starts responding to that change before tab A completes with the function
4. Tab B's response is to load the user info, and upon response from the server the sub from the response is checked against the Identity Claims in storage, but Tab A never got to save that yet because it's thread on the CPU didn't get prio.
5. Tab B will now throw an error because the sub from the user info endpoint doesn't match null from storage.

So:

• Workaround: wrap logic in setTimeout to make it very likely that Tab A handles everything before Tab B
• Solution: don't respond to storage events, but make tabs propagate OAuthEvents properly to sibling tabs

[Yanal-Yves]

Hello @jeroenheijmans,

Could you explain what do you mean by

make tabs propagate OAuthEvents properly to sibling tabs

?

[jeroenheijmans]

@Yanal-Yves Ooh ehmm, September 2018, I wrote that.... 😱😅

I think that I intended a solution to be not to sneakily listen to localStorage events (just to see other tabs setting stuff), but instead have a tab do something like:

this.oauthService.events.subscribe(event => {
  // (ab/mis)use localStorage as a lightweight messaging
  // mechanism by shortly setting an item so other tabs
  // may listen to it being set, and immediately pop the
  // message again:
  localStorage.setItem("my-oauth-storage-event", JSON.stringify({
    key: 'oauth-message-for-sibling-tab',
    event,
  });
  localStorage.removeItem("my-oauth-storage-event");
});

Possibly add more data to the message.

And then have other tabs listen to this being set, and respond accordingly.

But, I never got around it, since the race condition I mention in the issue was not really pragmatically encountered in any of my apps.

[Yanal-Yves]

Thank you for your response. It's good to know that the issue is not really pragmatically encountered in any of your apps.

I was wondering if this issue was related to an issue I have in my app but it seems to be unrelated.

I am wondering if that issue is worth remaining open if it could not be reproduced.

[l1b3r]

Alternatively, you could override upstream OAuthService a little bit so that it fires a special storage event only after it finishes writing all the data to that storage. Something like that:

@Injectable()
export class MyOAuthService extends OAuthService {

protected storeAccessTokenResponse(accessToken: string, refreshToken: string, expiresIn: number, grantedScopes: string) {
    super.storeAccessTokenResponse(accessToken, refreshToken, expiresIn, grantedScopes);
    // Signal that the auth data is updated only after __all__ auth-related fields are written to the storage.
    this._storage.setItem('auth_data_updated', accessToken);
    this._storage.removeItem('auth_data_updated');
  }
}

Then in your application you could listen specifically for those 'auth_data_updated' events:

window.addEventListener('storage', (event) => {
    if (event.key === 'auth_data_updated') {
        console.log(`Auth data has been updated in localStorage`);
        // do some stuff here
    }
});

---

EDIT: The same 'auth_data_updated' event should also be fired on logout, since it wipes auth data from the storage and that's something other tabs should probably be also aware of. Also, AFAIK storage API will not fire the event to the other tabs if you don't change the actual value, so it's a good idea to generate a small random string and put it in the event to make sure it's always unique:

    this._storage.setItem('auth_data_updated', getRandomString());
    this._storage.removeItem('auth_data_updated');

[jeroenheijmans]

Thx for the additional option @l1b3r! I'm leaving this issue open for now, it's linked from the sample code too so others might want to use your solution for their apps! <3

[pegma-repo]

@l1b3r I really like your solution. This way the service is not listening to every "access_code" change encountered. I found that extending the condition helps to avoid event firing again after _storage.removeItem() is called and adding an event.key null check helps cover localStorage.clear(). What do you guys think? My 2c.

    window.addEventListener('storage', (event) => {
      if ((event.key === 'auth_data_updated' && event.newValue !== null) || event.key === null) {
        console.log('Auth data has been updated in localStorage');
        this.isAuthenticatedSubject$.next(this.oauthService.hasValidAccessToken());

        if (!this.oauthService.hasValidAccessToken()) {
          this.navigateToLoginPage();
        }
      }
    });

PS: I used this with your storeAccessTokenResponse() override. Thank you for sharing!
Edit: typos & ps

[CharlieGreenman]

@pegma-repo + @l1b3r great stuff

[WarayAmine]

@l1b3r so after calling oauthService logout method, you fire the event "auth_data_updated" again ?

this._storage.setItem('auth_data_updated', getRandomString());
this._storage.removeItem('auth_data_updated');

[JayAhn2]

@WarayAmine

You can wrap logOut method as below.
This logic can be used at any public method of OAuthService that you want to notify to other browser tabs.

import { OAuthService } from 'angular-oauth2-oidc';
import { Injectable } from '@angular/core';
import * as uuid from 'uuid';

@Injectable({ providedIn: 'root' })
export class MyOAuthService extends OAuthService {
  protected storeAccessTokenResponse(
    accessToken: string,
    refreshToken: string,
    expiresIn: number,
    grantedScopes: string
  ) {
    super.storeAccessTokenResponse(
      accessToken,
      refreshToken,
      expiresIn,
      grantedScopes
    );
    this.notifyStorageUpdate();
  }

  public logOut() {
    super.logOut();
    this.notifyStorageUpdate();
  }

  private notifyStorageUpdate() {
    this._storage.setItem('auth_data_updated', uuid.v4());
    this._storage.removeItem('auth_data_updated');
  }
}

[jeroenheijmans]

Thanks for all the workarounds and solutions folks! Extending the library main service to do the notification only exactly when needed is probably the safest solution. I will update my sample to refer to this issue as a "solution" for new folks, so I can actually close this one at the sample's repo off.