Enable cross tabs communication to sync authState #478

shuowu · 2020-09-18T19:04:26Z

Changes:

Make storage change in the renew process as atomic operation to prevent others tabs read false-positive states when calling updateAuthState()
Listen on storage event in TokenManager
- reset expire timeouts base on new tokens in storage
- compare old&new values, then emit events for downstream (authStateManager) to consume
Handle unreliable localStorage issues for IE (wait for 1s then read)

lib/TokenManager.ts

swiftone · 2020-09-23T20:45:57Z

test/spec/tokenManager.js

+      expect(instance._emitEventsForCrossTabsStorageUpdate).not.toHaveBeenCalled();
+    });
+
+    describe('_emitEventsForCrossTabsStorageUpdate', () => {


Is there a way to test to make sure that we don't cause extra calls? I don't see anything in the code that's wrong, but a test making sure that an emitted event doesn't cause a chain of events continuously bouncing between tabs would help future-proof the code.

Extra calls may happen, as we still want to keep autoRenew running in each single tab. Then we handle it in AuthStateManager to throttle the emitted events by comparing the state and cancelablePromise. https://github.com/okta/okta-auth-js/pull/484/files#diff-c87977be240b146c8cc1ab3c11a6cc66R71
Also, the events just triggered by storage change, but it will not trigger another storage change, then we should be save from the infinite loop.

Overall, yes, extra calls may happen. I'm just saying we should test to make sure that if only one call is expected (because artificial test situation) only one call happens.

That helps make sure we don't shoot ourselves in the foot later (as you say, currently it won't happen. I want to make sure later changes don't break that assumption).

I agree with the future-proof point. I just don't feel it's easy to simulate the multiple tabs env in unit test, if we just trigger the function by emitting a StorageEvent, I don't think it can help too much.

Hmm, that's more involved than I was thinking. More of:

test that when we make a change, we only emit exactly one event, not more than one

test that when we get an event that doesn't require a change, we don't emit an event

StorageEvent can only be triggered outside the current tab, maybe I can try wdio as Aaron suggested.

test that when we get an event that doesn't require a change, we don't emit an event

yep 👍 , looks like more tests are needed.

aarongranick-okta · 2020-09-24T20:16:14Z

test/spec/fingerprint.js

-      expect(name).toEqual('message');
-      listener = fn;
+      // expect(name).toEqual('message');
+      listeners.message = fn;


should this be

if (name === 'message') { listeners.message = fn; }|

aarongranick-okta · 2020-09-24T20:18:56Z

At a high level, I'm not sure we should call it "cross tab communication" since this will also keep multiple instances within a tab in sync, if they have tokens in memory (such as authState). It's handling "storage events", if this storage is shared between tabs then it will affect instances in multiple tabs. But even within a single tab using sessionStorage, this logic still applies.

swiftone · 2020-09-24T20:21:08Z

since this will also keep multiple instances within a tab in sync

Will it? I don't believe the event will be received by other code in the same tab that emits the code.

aarongranick-okta · 2020-09-24T21:11:15Z

lib/TokenManager.ts

+  Object.keys(newTokens).forEach(key => {
+    const oldToken = oldTokens[key];
+    const newToken = newTokens[key];
+    if (JSON.stringify(oldToken) !== JSON.stringify(newTokens)) {


did you mean newToken (instead of newTokens) ?

Thanks for catching it, dammit auto-completion...

That's why I don't use auto-completion, and make boutique, artisanal typos...

That said, if the above didn't trigger a test failure, that signals a test we should add...

aarongranick-okta · 2020-09-24T21:13:26Z

lib/TokenManager.ts

@@ -117,14 +142,23 @@ function setExpireEventTimeoutAll(sdk, tokenMgmtRef, storage) {
  }
 }

-function add(sdk, tokenMgmtRef, storage, key, token: Token) {
-  var tokenStorage = storage.getStorage();
+function resetExpireEventTimeoutAll(sdk, tokenMgmtRef, storage) {


add a comment explaining what clearing and re-adding the timeouts does

aarongranick-okta · 2020-09-24T21:17:06Z

lib/TokenManager.ts

+      // LocalStorage cross tabs update is not synced in IE, set a 1s timer to read latest value
+      // https://stackoverflow.com/questions/24077117/localstorage-in-win8-1-ie11-does-not-synchronize
+      if (isIE11OrLess()) {
+        setTimeout(() => emitEventsAndResetExpireEventTimeouts(), 1000);


I'm concerned what may happen within that 1 second. Will we be emitting adding/removed events that could clobber more recent, authoritative values? Should we check that the current value in storage is the newToken?

Good point, I am thinking how about attach a timestamp to the emitted events, if by any chance an old event comes to updateAuthState, just ignore it.

^ smart catch

aarongranick-okta

I think we need more test coverage on this.
I would like to see how this works with authState: Calling tokenManager.add() should not cause more than one authState event.
I would also like to see testing around this ie11 delay, lets think of all the edges that could be exposed by waiting 1 second.
Our wdio E2E framework does support multiple tabs. I would like to see two app instances started in separate tabs and verify that the cross tab communication is working as we expect.

aarongranick-okta · 2020-10-02T19:48:41Z

lib/AuthStateManager.ts

    });
  }

  getAuthState(): AuthState {
    return this._authState;
  }

-  updateAuthState({ event, key, token }: UpdateAuthStateOptions = {}): void {
+  updateAuthState({ event, key, token, timestamp }: UpdateAuthStateOptions = { timestamp: Date.now() }): void {


I do not think this function should take any parameters or options. None of these parameters should affect the output. Is the output state a function of an event or a key? All of these params (except for timestamp) appear to be here for logging purposes. I would prefer this type of logging was done outside of this method. If an event handler has been fired, that handler can log a message before calling updateAuthState. updateAuthState itself should not take any parameters or return any value, since it should calculate the same state no matter who calls it nor however frequently it is called. Two identical calls in a row should produce the same output state.

Adding time-based logic with timestamp and Date.now() greatly increases the complexity and breaks the assumption of repeatability / idempotence since two subsequent calls with the same parameters would have different results. With the time logic, each call causes an update to internal state.

Calling this method should not produce side-effects other than emitting a state change event with the calculated state object, which is a function of stored token values. It should not matter when I call the method, the current state will always be a reflection of current storage

I think we can still track the timestamp, but instead of tracking it in updateAuthState, we can manage it when AuthStateManager gets the events, then make the decision there.
In this way, the internal evaluation can still benefit from the tracked timestamp, and the external evaluation won't break the assumption of repeatability/idempotence.

aarongranick-okta · 2020-10-07T19:36:13Z

test/e2e/specs/crossTabs.js

+    };
+    if (preToken) {
+      assert(JSON.stringify(preToken) === JSON.stringify(tabTokenMap[handle]));
+    }


should there be an else { here? We only need/want to set preToken once, right?

should not really matter, any unequal will fail the test.

aarongranick-okta · 2020-10-07T19:39:29Z

test/e2e/specs/crossTabs.js

+
+  it('should update tokens cross tabs', async () => {
+    // // login in the latest opened tab
+    // await loginPopup();


comments can be removed?

aarongranick-okta · 2020-10-07T19:44:53Z

lib/AuthStateManager.ts

-      this._setLogOptions({ event: 'added', key, token });
+    // "added" event is emitted in both add and renew process
+    // Only listen on "added" event to update auth state
+    const shouldUpdateAuthState = (timestamp: number): boolean => {


Do we actually need this logic? If updateAuthState is called repeatedly or unnecessarily it should not cause any negative side-effects. If it produces an identical state to the current state it will not emit a change, so will it actually matter if an "older" event was the reason it is being called?

so will it actually matter if an "older" event was the reason it is being called

Yes, the logic here is to handle the 1s delay hack for IE.

Yeah its a hack to call the events later, but does that actually cause a problem? And if it doesn't cause a problem why do we need extra logic to suppress the calls?

Other than the cross tabs update, each tab is still running autoRenew process by itself. In this case, the delayed event will override the value from the tab's own autoRenew result if it comes later.

I don't understand "override the value". Isn't it safe to call updateAuthState at any time? It will read tokens from storage and, if these are different than what is currently in the authState, then it will emit a change event. If there is no change in the state, then nothing is emitted and it is essentially a no-op. If 1 second after an auto renew, the values in storage have changed, then we want this change event, right?

aarongranick-okta · 2020-10-07T19:47:26Z

lib/TokenManager.ts

+
+      // LocalStorage cross tabs update is not synced in IE, set a 1s timer to read latest value
+      // https://stackoverflow.com/questions/24077117/localstorage-in-win8-1-ie11-does-not-synchronize
+      if (isIE11OrLess()) {


What do you think about having this function always called in a setTimeout, but have the time value be configurable. We can set it by default to 0 unless IE11 where we set it by default to 1000. But we could also manually set it to a value in our test/sample apps to verify that the logic works as we expect without needing to fake IE11

codecov-io · 2020-10-08T01:11:15Z

Codecov Report

Merging #478 into master will increase coverage by 0.17%.
The diff coverage is 98.97%.

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
+ Coverage   90.83%   91.00%   +0.17%     
==========================================
  Files          33       32       -1     
  Lines        1767     1834      +67     
  Branches      391      406      +15     
==========================================
+ Hits         1605     1669      +64     
- Misses        162      165       +3

Impacted Files	Coverage Δ
lib/AuthStateManager.ts	`89.32% <92.85%> (-4.23%)`	⬇️
lib/TokenManager.ts	`98.14% <100.00%> (+0.52%)`	⬆️

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 d3cb262...b671d9a. Read the comment docs.

aarongranick-okta · 2020-10-08T17:22:55Z

lib/TokenManager.ts

@@ -303,6 +405,10 @@ export class TokenManager {
      options.storage = 'cookie';
    }

+    if (isIE11OrLess()) {
+      options._storageEventDelay = 1000;


this should be a default if running on IE11, it should not override the value we have set

aarongranick-okta

remove timestamp logic on events, then 🚀

swiftone reviewed Sep 23, 2020

View reviewed changes

lib/TokenManager.ts Outdated Show resolved Hide resolved

swiftone reviewed Sep 23, 2020

View reviewed changes

lib/TokenManager.ts Outdated Show resolved Hide resolved

swiftone reviewed Sep 23, 2020

View reviewed changes

shuowu requested a review from swiftone September 23, 2020 21:45

aarongranick-okta reviewed Sep 24, 2020

View reviewed changes

aarongranick-okta requested changes Sep 24, 2020

View reviewed changes

aarongranick-okta reviewed Oct 2, 2020

View reviewed changes

shuowu-okta force-pushed the dev-4.1 branch from d454eca to 344e8f8 Compare October 5, 2020 18:05

Base automatically changed from dev-4.1 to master October 7, 2020 17:41

shuowu-okta force-pushed the sw-cross-tabs-comm-330042 branch from 37f19fb to 3ceea90 Compare October 7, 2020 18:28

shuowu requested a review from aarongranick-okta October 7, 2020 18:48

aarongranick-okta reviewed Oct 7, 2020

View reviewed changes

shuowu added 10 commits October 7, 2020 18:34

feat: lift up authState and methods from downstream SDKs (#453)

3eab21d

refactor browser public methods (#482)

048e067

fix types and minor updates

e52e6eb

remove options for updateAuthState

bcb7040

revert sample template changes

7e8391b

feat: enable cross tab communication via storage event

54968a2

remove only in test

043a2c8

add changelog

c223579

move storage change listener to tokenManager to keep emitting events

b9e6327

fix lint issue

5c52024

shuowu added 8 commits October 7, 2020 18:36

resolve comments

f21854f

add tests

8967fe2

more tests

b31744f

track timestamp in event listener

10fe072

add e2e tests

e2a4e77

add comment

ba21342

fix karma test

987eb1d

add storageEventDelay option for tokenManager

a9263dd

shuowu-okta force-pushed the sw-cross-tabs-comm-330042 branch from b788356 to a9263dd Compare October 7, 2020 22:36

shuowu added 3 commits October 7, 2020 19:02

remove updateAuthState warnings in test

6d20f6d

fix broken e2e:login

badd156

fix testapp type

b671d9a

shuowu added 3 commits October 8, 2020 10:29

change storageEventDelay as internal option

979c700

add test for _storageEventDelay

6afee0d

remove console log

366625f

shuowu-okta requested a review from aarongranick-okta October 8, 2020 16:47

aarongranick-okta reviewed Oct 8, 2020

View reviewed changes

aarongranick-okta approved these changes Oct 8, 2020

View reviewed changes

remove timestamp in emitted event

ff826cb

shuowu-okta merged commit 4002e08 into master Oct 8, 2020

oleksandrpravosudko-okta mentioned this pull request Nov 25, 2020

AuthSdkError: Oauth flow response state doesn't match request state. When open multiple tabs on browser okta/okta-react#43

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable cross tabs communication to sync authState #478

Enable cross tabs communication to sync authState #478

shuowu commented Sep 18, 2020 •

edited

Loading

swiftone Sep 23, 2020

shuowu Sep 23, 2020

swiftone Sep 24, 2020

shuowu Sep 24, 2020

swiftone Sep 24, 2020

shuowu Sep 25, 2020

aarongranick-okta Sep 24, 2020

aarongranick-okta commented Sep 24, 2020

swiftone commented Sep 24, 2020

aarongranick-okta Sep 24, 2020

shuowu Sep 24, 2020

swiftone Sep 24, 2020

swiftone Sep 24, 2020

aarongranick-okta Sep 24, 2020

aarongranick-okta Sep 24, 2020

shuowu Sep 24, 2020

swiftone Sep 24, 2020

aarongranick-okta left a comment

aarongranick-okta Oct 2, 2020

shuowu Oct 5, 2020

aarongranick-okta Oct 7, 2020

shuowu-okta Oct 7, 2020

aarongranick-okta Oct 7, 2020

aarongranick-okta Oct 7, 2020

shuowu-okta Oct 7, 2020

aarongranick-okta Oct 8, 2020

shuowu-okta Oct 8, 2020

aarongranick-okta Oct 8, 2020

aarongranick-okta Oct 7, 2020

codecov-io commented Oct 8, 2020

aarongranick-okta Oct 8, 2020

aarongranick-okta left a comment

Enable cross tabs communication to sync authState #478

Enable cross tabs communication to sync authState #478

Conversation

shuowu commented Sep 18, 2020 • edited Loading

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 commented Sep 24, 2020

swiftone commented Sep 24, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2020

Codecov Report

Choose a reason for hiding this comment

aarongranick-okta left a comment

Choose a reason for hiding this comment

shuowu commented Sep 18, 2020 •

edited

Loading