Implement SyncStorageService using broadcast-channel #1197

denysoblohin-okta · 2022-04-25T07:20:36Z

Reimplement SyncStorageService (service to sync authState cross multiple tabs) with using broadcast-channel package instead of using StorageEvent
Added LeaderElectionService (leader election code was a part of ServiceManager)

Implementation notes:

Notes on broadcast-channel issues:

Error Failed to execute 'postMessage' on 'BroadcastChannel': Channel is closed .
See Auth JS 6.4.0 causes issues in Angular apps when testing #1174, Error on redirect from login okta-react#221, "Failed to execute 'postMessage' on 'BroadcastChannel'" error - React 18 okta-react#232
Cause: broadcast-channel's leader election tries to post internal message after closing the channel.
Workaround:

okta-auth-js/lib/services/LeaderElectionService.ts

Line 72 in 4f88408

(this.channel as any).postInternal = () => Promise.resolve();

Detection of Node environment in broadcast-channel is naive:
https://github.com/pubkey/broadcast-channel/blob/773fbc3477d246e96f7566b7ac5ff3164042204d/src/util.js#L61
This leads to wrongly selecting 'node' method (based on fs sockets) for broadcast-channel during unit tests.
Causes tests flakiness.
For downstream SDKs causes error No useable method found in ["native","idb","localstorage"], see Please provide unit test example okta-react#224
Workaround:

okta-auth-js/test/support/jest/jest.setup.js

Lines 33 to 35 in 4f88408

    
           // broadcast-channel should not detect node environment 
        
           // https://github.com/pubkey/broadcast-channel/blob/master/src/util.js#L61 
        
           process[Symbol.toStringTag] = 'Process';

Notes on IE11

Test app was not working on IE11. Need to add polyfills:
https://github.com/okta/okta-auth-js/pull/1197/files#diff-a861a360629a870e92aba3e9458e109f96ad1c9512f8ff339b0a4072f7ce7d4c
(maybe replace multiple features with 'core-js/stable' ?)
And polyfill FormData: https://github.com/okta/okta-auth-js/pull/1197/files#diff-41fbe6af6c0cd49ffacdf69a8831d1a7327ea9d90849bd4bf95c27e6f6033ffcR210
IE11 has known issue with synchronization of localStorage across tabs.
Partial workaround is to set empty event handler for window.onstorage: https://github.com/okta/okta-auth-js/pull/1197/files#diff-4190d70999a6d0023de331545bbec8ba4167f9d6d71faa196061946d5f8822d9R129
But it's not 100% working, sometimes you still get old value from LocalStorage.
So to fix this issue I've added a new type of event EVENT_SET_STORAGE in TokenManager (only for IE11) to update storage explicitly in other tabs.
https://github.com/okta/okta-auth-js/pull/1197/files#diff-f18cf5684cf053b7ffbc2723f4983f1a26432ef9eade50cbfa00a74a7793dedbR280
https://github.com/okta/okta-auth-js/pull/1197/files#diff-b3d0b7dbba2f28b35e943fe28c40cc6b832b43b2986d5d14dd1ce9b7d164f3deR134

Other notes

SyncStorageService works with localStorage and cookie storage.
https://github.com/okta/okta-auth-js/pull/1197/files#diff-6d3aacb7a946c6ca690509a0cc75e9880e5ca6942803b7cdd4059a9ba9a31b2dR356
https://github.com/okta/okta-auth-js/pull/1197/files#diff-2128a2510aeabd863495fcf012a579ef877ad61544805295fff687cfce0728c4R55

codecov-commenter · 2022-04-26T17:13:47Z

Codecov Report

Merging #1197 (d943b27) into master (cceda24) will increase coverage by 0.06%.
The diff coverage is 95.33%.

@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   93.41%   93.48%   +0.06%     
==========================================
  Files         157      159       +2     
  Lines        4586     4681      +95     
  Branches     1058     1081      +23     
==========================================
+ Hits         4284     4376      +92     
- Misses        283      287       +4     
+ Partials       19       18       -1

Impacted Files	Coverage Δ
lib/browser/browserStorage.ts	`92.80% <66.66%> (-1.18%)`	⬇️
lib/services/LeaderElectionService.ts	`87.50% <87.50%> (ø)`
lib/services/SyncStorageService.ts	`97.33% <96.61%> (+0.55%)`	⬆️
lib/AuthStateManager.ts	`94.00% <100.00%> (ø)`
lib/OktaAuth.ts	`89.45% <100.00%> (+0.06%)`	⬆️
lib/SavedObject.ts	`90.69% <100.00%> (+0.69%)`	⬆️
lib/ServiceManager.ts	`95.45% <100.00%> (+3.04%)`	⬆️
lib/TokenManager.ts	`95.75% <100.00%> (+0.89%)`	⬆️
lib/services/AutoRenewService.ts	`100.00% <100.00%> (ø)`
lib/services/index.ts	`100.00% <100.00%> (ø)`
... and 2 more

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 cceda24...d943b27. Read the comment docs.

jaredperreault-okta · 2022-05-04T13:52:28Z

lib/services/LeaderElectionService.ts

+  onLeader?: OnLeaderHandler;
+};
+
+export class LeaderElectionService implements ServiceInterface {


What is the motivation for moving the tab elector logic to it's own service?

I isolated election-related code from ServiceManager into a separate class for single responsibility, then saw it looks like a service.

jaredperreault-okta · 2022-05-10T14:10:27Z

lib/types/Service.ts

-  {
-    broadcastChannelName?: string;
-  };
+  SyncStorageServiceOptions & LeaderElectionServiceOptions;


changing the name of broadcastChannelName to electionChannelName is a breaking change. Can you have the config respect both options until the next major release

aarongranick-okta · 2022-05-19T22:41:54Z

lib/ServiceManager.ts

@@ -43,14 +44,15 @@ export class ServiceManager implements ServiceManagerInterface {

    // TODO: backwards compatibility, remove in next major version - OKTA-473815
    const { autoRenew, autoRemove, syncStorage } = sdk.tokenManager.getOptions();
+    options.electionChannelName = options.broadcastChannelName;


this should be:

options.electionChannelName = options.electionChannelName || options.broadcastChannelName

to support passing electionChannelName (as is being done in the test app)

aarongranick-okta · 2022-05-19T22:42:23Z

test/spec/ServiceManager.ts

+      const client = createAuth(options);
+      const serviceManagerOptions = (client.serviceManager as any).options;
+      expect(serviceManagerOptions.electionChannelName).toEqual('test-channel');
+    });


Please add a test verifying that electionChannelName option is supported

aarongranick-okta · 2022-05-19T22:45:50Z

lib/ServiceManager.ts

+      { autoRenew, autoRemove, syncStorage }, 
+      {
+        electionChannelName: `${sdk.options.clientId}-election`,
+        syncChannelName: `${sdk.options.clientId}-sync`,


please add a test for these default values in test/spec/ServiceManager

also please add a test verifying syncChannelName can be set

aarongranick-okta · 2022-05-19T22:51:44Z

lib/types/TokenManager.ts


 export declare type AccessTokenCallback = (key: string, token: AccessToken) => void;
 export declare type IDTokenCallback = (key: string, token: IDToken) => void;
 export declare type RefreshTokenCallback = (key: string, token: RefreshToken) => void;

 // only add methods needed internally
 export interface TokenManagerInterface {
-  on: (event: string, handler: TokenManagerErrorEventHandler | TokenManagerEventHandler, context?: object) => void;
-  off: (event: string, handler?: TokenManagerErrorEventHandler | TokenManagerEventHandler) => void;
+  on: (event: string, handler: TokenManagerAnyEventHandler, context?: object) => void;


Do you think we could handle this better with function overloads, like we are doing on SIW: https://github.com/okta/okta-signin-widget/blob/okta-signin-widget-6.3.3/src/types/api.ts#L10
Then it will be clear which type of event has which type of callback. As is, I'm not sure as a developer how to implement my handler function - it seems to just have one of 3 shapes, so I have to be prepared for any of the variations. Would be better I think to know which type of callback I should implement for a specific event.

aarongranick-okta

I have tested the functionality and it seems to work great! Just a few comments

aarongranick-okta

please add missing tests and fix logic with electionChannelName

. .

.

. .

…A-473815)

async .

pzi · 2022-06-04T05:33:28Z

what happened here?

denysoblohin-okta · 2022-06-06T16:09:47Z

@pzi This PR has been merged to master

pzi · 2022-06-07T05:42:55Z

@denysoblohin-okta oh ok, I kinda expected the PR to be merged instead of closed :D thanks for that

denysoblohin-okta force-pushed the od-sync-service-broadcast-OKTA-476153 branch from 2bcc941 to 6fa57a4 Compare April 25, 2022 12:49

denysoblohin-okta marked this pull request as ready for review May 4, 2022 12:51

denysoblohin-okta requested review from shuowu and aarongranick-okta May 4, 2022 12:52

jaredperreault-okta reviewed May 4, 2022

View reviewed changes

denysoblohin-okta force-pushed the od-sync-service-broadcast-OKTA-476153 branch from fa79c0b to b4d565c Compare May 5, 2022 19:03

jaredperreault-okta reviewed May 10, 2022

View reviewed changes

jaredperreault-okta approved these changes May 10, 2022

View reviewed changes

aarongranick-okta reviewed May 19, 2022

View reviewed changes

aarongranick-okta suggested changes May 19, 2022

View reviewed changes

denysoblohin-okta requested a review from aarongranick-okta May 24, 2022 18:34

aarongranick-okta approved these changes Jun 1, 2022

View reviewed changes

denysoblohin-okta force-pushed the od-sync-service-broadcast-OKTA-476153 branch 2 times, most recently from 07b4484 to c609644 Compare June 1, 2022 21:53

denysoblohin-okta added 11 commits June 2, 2022 23:37

Added LeaderElectionService

419224d

broadcastChannelName -> electionChannelName

d795662

New impl of SyncStorageService

20c5d10

fix message loop

0f461e8

allow syncStorage for localStorage and cookie

5991101

. .

.

bd6ac2d

fixes

e890015

fix

ca08809

support IE11

095faf4

IE11 workaround

714ed25

Use event set_storage for IE11

a4adcfe

denysoblohin-okta added 22 commits June 2, 2022 23:37

Added polyfills for IE11

5850351

lint fix

3453ebb

fix Channel is closed

85138a4

.

added tests LES

e442b2c

added test for starting leaderElection service

34acc6f

added tests for services

246b2af

added test

34e6ff4

added tests

f9294ee

added SM tests

b156e62

fix: post set_storage event when storage is cleared on logout

353ba5e

. .

fix: SM should be started before TM

3949afb

fix test 'can use memory token storage' (OKTA-464122)

0cf1982

Support broadcastChannelName as old name for electionChannelName (OKT…

f8ecc6e

…A-473815)

adress comments

afa1461

Overload token manager on/off

19de428

fix

c336d3f

fix

4e4a02f

async fix after rebase

97eb8d9

async .

fix clearPendingRemoveTokens

049f7ed

test

32faade

fix

fce8c44

chlog

35be52a

denysoblohin-okta force-pushed the od-sync-service-broadcast-OKTA-476153 branch from 498382d to d943b27 Compare June 2, 2022 20:38

await updateAuthState in start

45fc202

denysoblohin-okta force-pushed the od-sync-service-broadcast-OKTA-476153 branch from d943b27 to 45fc202 Compare June 3, 2022 08:03

fix test memory storage

8897c98

eng-services-aperture-github-bot-okta closed this Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SyncStorageService using broadcast-channel #1197

Implement SyncStorageService using broadcast-channel #1197

denysoblohin-okta commented Apr 25, 2022 •

edited

Loading

codecov-commenter commented Apr 26, 2022 •

edited

Loading

jaredperreault-okta May 4, 2022

denysoblohin-okta May 4, 2022

jaredperreault-okta May 10, 2022

aarongranick-okta May 19, 2022

aarongranick-okta May 19, 2022

aarongranick-okta May 19, 2022

aarongranick-okta May 19, 2022

aarongranick-okta May 19, 2022

aarongranick-okta left a comment

aarongranick-okta left a comment

pzi commented Jun 4, 2022

denysoblohin-okta commented Jun 6, 2022

pzi commented Jun 7, 2022

	// broadcast-channel should not detect node environment
	// https://github.com/pubkey/broadcast-channel/blob/master/src/util.js#L61
	process[Symbol.toStringTag] = 'Process';

Implement SyncStorageService using broadcast-channel #1197

Implement SyncStorageService using broadcast-channel #1197

Conversation

denysoblohin-okta commented Apr 25, 2022 • edited Loading

Implementation notes:

codecov-commenter commented Apr 26, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongranick-okta left a comment

Choose a reason for hiding this comment

aarongranick-okta left a comment

Choose a reason for hiding this comment

pzi commented Jun 4, 2022

denysoblohin-okta commented Jun 6, 2022

pzi commented Jun 7, 2022

denysoblohin-okta commented Apr 25, 2022 •

edited

Loading

codecov-commenter commented Apr 26, 2022 •

edited

Loading