From f8ba33367eac476f821dbbda874aa2e332e57197 Mon Sep 17 00:00:00 2001 From: Chelsea Shaw <82459713+hashishaw@users.noreply.github.com> Date: Tue, 22 Jun 2021 15:28:29 -0600 Subject: [PATCH] UI/fix safari oidc login (#11884) (#11905) * use window.postMessage instead of localStorage on oidc callback --- changelog/11884.txt | 3 + ui/app/components/auth-jwt.js | 26 +++--- ui/app/routes/vault/cluster/oidc-callback.js | 2 +- ui/app/templates/components/auth-form.hbs | 2 +- .../integration/components/auth-jwt-test.js | 90 +++++++++++++------ 5 files changed, 78 insertions(+), 45 deletions(-) create mode 100644 changelog/11884.txt diff --git a/changelog/11884.txt b/changelog/11884.txt new file mode 100644 index 0000000000000..1dd884c50992d --- /dev/null +++ b/changelog/11884.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: fix oidc login with Safari +``` \ No newline at end of file diff --git a/ui/app/components/auth-jwt.js b/ui/app/components/auth-jwt.js index d036fc9f3d191..794031b3273c3 100644 --- a/ui/app/components/auth-jwt.js +++ b/ui/app/components/auth-jwt.js @@ -82,15 +82,19 @@ export default Component.extend({ }, prepareForOIDC: task(function*(oidcWindow) { + const thisWindow = this.getWindow(); // show the loading animation in the parent this.onLoading(true); // start watching the popup window and the current one this.watchPopup.perform(oidcWindow); this.watchCurrent.perform(oidcWindow); - // and then wait for storage event to be fired from the popup - // window setting a value in localStorage when the callback route is loaded - let storageEvent = yield waitForEvent(this.getWindow(), 'storage'); - this.exchangeOIDC.perform(storageEvent, oidcWindow); + // wait for message posted from popup + const event = yield waitForEvent(thisWindow, 'message'); + if (event.origin === thisWindow.origin && event.isTrusted) { + this.exchangeOIDC.perform(event.data, oidcWindow); + } else { + this.handleOIDCError(); + } }), watchPopup: task(function*(oidcWindow) { @@ -103,6 +107,7 @@ export default Component.extend({ }), watchCurrent: task(function*(oidcWindow) { + // when user is about to change pages, close the popup window yield waitForEvent(this.getWindow(), 'beforeunload'); oidcWindow.close(); }), @@ -113,20 +118,13 @@ export default Component.extend({ oidcWindow.close(); }, - exchangeOIDC: task(function*(event, oidcWindow) { - // in non-incognito mode we need to use a timeout because it takes time before oidcState is written to local storage. - let oidcState = Ember.testing - ? event.storageArea.getItem('oidcState') - : yield timeout(1000).then(() => event.storageArea.getItem('oidcState')); - + exchangeOIDC: task(function*(oidcState, oidcWindow) { 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(oidcState); - this.getWindow().localStorage.removeItem('oidcState'); + + let { namespace, path, state, code } = oidcState; // The namespace can be either be passed as a query paramter, or be embedded // in the state param in the format `,ns=`. So if diff --git a/ui/app/routes/vault/cluster/oidc-callback.js b/ui/app/routes/vault/cluster/oidc-callback.js index d03e902a74b78..6c32ea7f7d29c 100644 --- a/ui/app/routes/vault/cluster/oidc-callback.js +++ b/ui/app/routes/vault/cluster/oidc-callback.js @@ -10,7 +10,7 @@ export default Route.extend({ let { namespaceQueryParam: namespace } = this.paramsFor('vault.cluster'); path = window.decodeURIComponent(path); let queryParams = { namespace, path, code, state }; - window.localStorage.setItem('oidcState', JSON.stringify(queryParams)); + window.opener.postMessage(queryParams, window.origin); }, renderTemplate() { this.render(this.templateName, { diff --git a/ui/app/templates/components/auth-form.hbs b/ui/app/templates/components/auth-form.hbs index 8fcf03b58eba4..95a1ff2004cb8 100644 --- a/ui/app/templates/components/auth-form.hbs +++ b/ui/app/templates/components/auth-form.hbs @@ -56,7 +56,7 @@ @roleName={{this.roleName}} @selectedAuthType={{this.selectedAuthBackend.type}} @selectedAuthPath={{or this.customPath this.selectedAuthBackend.id}} - @disabled={{authenticate.isRunning}} + @disabled={{or authenticate.isRunning this.isLoading}} > ({ + isTrusted: true, + origin: 'https://my-vault.com', + data: {}, + ...opts, +}); const fakeWindow = EmberObject.extend(Evented, { init() { this._super(...arguments); @@ -29,12 +35,7 @@ const fakeWindow = EmberObject.extend(Evented, { width: 500, }; }), - localStorage: computed(function() { - return { - getItem: sinon.stub(), - removeItem: sinon.stub(), - }; - }), + origin: 'https://my-vault.com', closed: false, }); @@ -70,7 +71,6 @@ const renderIt = async (context, path = 'jwt') => { context.set('handler', sinon.spy(handler)); context.set('roleName', ''); context.set('selectedAuthPath', path); - await render(hbs` { return this.openSpy.calledOnce; }); - this.window.localStorage.getItem.returns(null); - this.window.trigger('storage', { storageArea: this.window.localStorage }); + this.window.trigger('message', buildMessage({ data: { state: 'state', foo: 'bar' } })); run.cancelTimers(); - assert.ok(this.window.localStorage.getItem.calledOnce, 'calls getItem'); - assert.notOk(this.window.localStorage.removeItem.called, 'never calls removeItem'); + assert.equal(this.error, ERROR_MISSING_PARAMS, 'calls onError with params missing error'); }); - test('oidc: storage event fires with state key, wrong 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'); @@ -226,15 +224,47 @@ module('Integration | Component | auth jwt', function(hooks) { await waitUntil(() => { return this.openSpy.calledOnce; }); - this.window.localStorage.getItem.returns(JSON.stringify({})); - this.window.trigger('storage', { storageArea: this.window.localStorage }); + this.window.trigger( + 'message', + buildMessage({ + data: { + path: 'foo', + state: 'state', + code: 'code', + }, + }) + ); + await settled(); + assert.equal(this.selectedAuth, 'token', 'calls onSelectedAuth with token'); + assert.equal(this.token, 'token', 'calls onToken with token'); + assert.ok(this.handler.calledOnce, 'calls the onSubmit handler'); + }); + + test('oidc: fails silently when event origin does not match window origin', async function(assert) { + await renderIt(this); + this.set('selectedAuthPath', 'foo'); + await component.role('test'); + component.login(); + await waitUntil(() => { + return this.openSpy.calledOnce; + }); + this.window.trigger( + 'message', + buildMessage({ + origin: 'http://hackerz.com', + data: { + path: 'foo', + state: 'state', + code: 'code', + }, + }) + ); run.cancelTimers(); - 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'); + await settled(); + assert.notOk(this.handler.called, 'should not call the submit handler'); }); - test('oidc: storage event fires with state key, correct params', async function(assert) { + test('oidc: fails silently when event is not trusted', async function(assert) { await renderIt(this); this.set('selectedAuthPath', 'foo'); await component.role('test'); @@ -242,17 +272,19 @@ module('Integration | Component | auth jwt', function(hooks) { await waitUntil(() => { return this.openSpy.calledOnce; }); - this.window.localStorage.getItem.returns( - JSON.stringify({ - path: 'foo', - state: 'state', - code: 'code', + this.window.trigger( + 'message', + buildMessage({ + isTrusted: false, + data: { + path: 'foo', + state: 'state', + code: 'code', + }, }) ); - this.window.trigger('storage', { storageArea: this.window.localStorage }); + run.cancelTimers(); await settled(); - assert.equal(this.selectedAuth, 'token', 'calls onSelectedAuth with token'); - assert.equal(this.token, 'token', 'calls onToken with token'); - assert.ok(this.handler.calledOnce, 'calls the onSubmit handler'); + assert.notOk(this.handler.called, 'should not call the submit handler'); }); });