From 565871f63c467bb95d57c4893c347dd3efd42e7d Mon Sep 17 00:00:00 2001 From: Chelsea Shaw <82459713+hashishaw@users.noreply.github.com> Date: Thu, 17 Jun 2021 15:56:04 -0500 Subject: [PATCH] UI/fix safari oidc login (#11884) * 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 | 4 +- .../integration/components/auth-jwt-test.js | 100 ++++++++++++------ 5 files changed, 84 insertions(+), 51 deletions(-) create mode 100644 changelog/11884.txt diff --git a/changelog/11884.txt b/changelog/11884.txt new file mode 100644 index 000000000..1dd884c50 --- /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 0dd15a90e..80de616b5 100644 --- a/ui/app/components/auth-jwt.js +++ b/ui/app/components/auth-jwt.js @@ -83,15 +83,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) { @@ -104,6 +108,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(); }), @@ -114,20 +119,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 d03e902a7..6c32ea7f7 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 725d4d88d..c782b341c 100644 --- a/ui/app/templates/components/auth-form.hbs +++ b/ui/app/templates/components/auth-form.hbs @@ -55,7 +55,7 @@ @roleName={{this.roleName}} @selectedAuthType={{this.selectedAuthBackend.type}} @selectedAuthPath={{or this.customPath this.selectedAuthBackend.id}} - @disabled={{authenticate.isRunning}} + @disabled={{or authenticate.isRunning this.isLoading}} > diff --git a/ui/tests/integration/components/auth-jwt-test.js b/ui/tests/integration/components/auth-jwt-test.js index 475b9e906..23d92d77f 100644 --- a/ui/tests/integration/components/auth-jwt-test.js +++ b/ui/tests/integration/components/auth-jwt-test.js @@ -15,6 +15,12 @@ import { ERROR_WINDOW_CLOSED, ERROR_MISSING_PARAMS, ERROR_JWT_LOGIN } from 'vaul const component = create(form); const windows = []; +const buildMessage = opts => ({ + 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'); - }); - - test('oidc: storage event fires with state key, wrong params', async function(assert) { - await renderIt(this); - this.set('selectedAuthPath', 'foo'); - await component.role('test'); - component.login(); - await waitUntil(() => { - return this.openSpy.calledOnce; - }); - this.window.localStorage.getItem.returns(JSON.stringify({})); - this.window.trigger('storage', { storageArea: this.window.localStorage }); - 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'); }); @@ -242,17 +224,67 @@ 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({ + data: { + 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'); 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(); + await settled(); + assert.notOk(this.handler.called, 'should not call the submit handler'); + }); + + test('oidc: fails silently when event is not trusted', 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({ + isTrusted: false, + data: { + path: 'foo', + state: 'state', + code: 'code', + }, + }) + ); + run.cancelTimers(); + await settled(); + assert.notOk(this.handler.called, 'should not call the submit handler'); + }); });