From 467fef6ca235b4ed2e6fe182fbf1adb70464f54c Mon Sep 17 00:00:00 2001
From: Angel Garbarino <Monkeychip@users.noreply.github.com>
Date: Thu, 16 Jan 2020 16:55:33 -0700
Subject: [PATCH] Fix oidc callback to check entire storage (#7929) (#8169)

* Fix oidc callback to check entire storage

In some cases, extensions or other unexpected javascript can be executed
on the oidc callback. Sometimes changing the localstorage and generating
a `storage` event before the callback component does. In such cases, the
authorization component will hang indefinitely.

A simple fix is to check for the expected `oidcState` key in the whole
localstorage instead of just the event `key`.

* Fix tests for auth-jwt

Co-authored-by: Angel Garbarino <Monkeychip@users.noreply.github.com>

Co-authored-by: Roberto Pommella Alegro <robertoaall@gmail.com>
---
 ui/app/components/auth-jwt.js                 |  5 ++--
 .../integration/components/auth-jwt-test.js   | 29 +++++++++++--------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/ui/app/components/auth-jwt.js b/ui/app/components/auth-jwt.js
index acaeb98ae011..f349c5cf4060 100644
--- a/ui/app/components/auth-jwt.js
+++ b/ui/app/components/auth-jwt.js
@@ -109,13 +109,14 @@ export default Component.extend({
   },
 
   exchangeOIDC: task(function*(event, oidcWindow) {
-    if (event.key !== 'oidcState') {
+    let oidcState = event.storageArea.getItem('oidcState');
+    if (oidcState === null || oidcState === undefined) {
       return;
     }
     this.onLoading(true);
     // get the info from the event fired by the other window and
     // then remove it from localStorage
-    let { namespace, path, state, code } = JSON.parse(event.newValue);
+    let { namespace, path, state, code } = JSON.parse(oidcState);
     this.getWindow().localStorage.removeItem('oidcState');
 
     // defer closing of the window, but continue executing the task
diff --git a/ui/tests/integration/components/auth-jwt-test.js b/ui/tests/integration/components/auth-jwt-test.js
index 9c2bd9f015f9..f4d1a56c0592 100644
--- a/ui/tests/integration/components/auth-jwt-test.js
+++ b/ui/tests/integration/components/auth-jwt-test.js
@@ -31,6 +31,7 @@ const fakeWindow = EmberObject.extend(Evented, {
   }),
   localStorage: computed(function() {
     return {
+      getItem: sinon.stub(),
       removeItem: sinon.stub(),
     };
   }),
@@ -202,7 +203,7 @@ module('Integration | Component | auth jwt', function(hooks) {
     assert.equal(this.error, ERROR_WINDOW_CLOSED, 'calls onError with error string');
   });
 
-  test('oidc: storage event fires with wrong key', async function(assert) {
+  test('oidc: storage event fires without state key', async function(assert) {
     await renderIt(this);
     this.set('selectedAuthPath', 'foo');
     await component.role('test');
@@ -210,12 +211,14 @@ module('Integration | Component | auth jwt', function(hooks) {
     await waitUntil(() => {
       return this.openSpy.calledOnce;
     });
-    this.window.trigger('storage', { key: 'wrongThing' });
+    this.window.localStorage.getItem.returns(null);
+    this.window.trigger('storage', { storageArea: this.window.localStorage });
     run.cancelTimers();
-    assert.equal(this.window.localStorage.removeItem.callCount, 0, 'never calls removeItem');
+    assert.ok(this.window.localStorage.getItem.calledOnce, 'calls getItem');
+    assert.notOk(this.window.localStorage.removeItem.called, 'never calls removeItem');
   });
 
-  test('oidc: storage event fires with correct key, wrong params', async function(assert) {
+  test('oidc: storage event fires with state key, wrong params', async function(assert) {
     await renderIt(this);
     this.set('selectedAuthPath', 'foo');
     await component.role('test');
@@ -223,13 +226,15 @@ module('Integration | Component | auth jwt', function(hooks) {
     await waitUntil(() => {
       return this.openSpy.calledOnce;
     });
-    this.window.trigger('storage', { key: 'oidcState', newValue: JSON.stringify({}) });
+    this.window.localStorage.getItem.returns(JSON.stringify({}));
+    this.window.trigger('storage', { storageArea: this.window.localStorage });
     run.cancelTimers();
-    assert.equal(this.window.localStorage.removeItem.callCount, 1, 'calls removeItem');
+    assert.ok(this.window.localStorage.getItem.calledOnce, 'calls getItem');
+    assert.ok(this.window.localStorage.removeItem.calledOnce, 'calls removeItem');
     assert.equal(this.error, ERROR_MISSING_PARAMS, 'calls onError with params missing error');
   });
 
-  test('oidc: storage event fires with correct key, correct params', async function(assert) {
+  test('oidc: storage event fires with state key, correct params', async function(assert) {
     await renderIt(this);
     this.set('selectedAuthPath', 'foo');
     await component.role('test');
@@ -237,14 +242,14 @@ module('Integration | Component | auth jwt', function(hooks) {
     await waitUntil(() => {
       return this.openSpy.calledOnce;
     });
-    this.window.trigger('storage', {
-      key: 'oidcState',
-      newValue: JSON.stringify({
+    this.window.localStorage.getItem.returns(
+      JSON.stringify({
         path: 'foo',
         state: 'state',
         code: 'code',
-      }),
-    });
+      })
+    );
+    this.window.trigger('storage', { storageArea: this.window.localStorage });
     await settled();
     assert.equal(this.selectedAuth, 'token', 'calls onSelectedAuth with token');
     assert.equal(this.token, 'token', 'calls onToken with token');