From 75c8672970a506416bde1c1ddc0b289962cb2728 Mon Sep 17 00:00:00 2001 From: Jordan Reimer Date: Fri, 18 Mar 2022 09:40:17 -0600 Subject: [PATCH] OIDC Logout Bug (#14545) * fixes issue with token auth selected after logging out from oidc or jwt methods * adds changelog entry * reverts backendType var name change in auth-form authenticate method --- changelog/14545.txt | 3 + ui/app/components/auth-form.js | 38 +++++++--- ui/app/components/auth-jwt.js | 1 - ui/app/services/auth.js | 11 ++- ui/app/templates/components/auth-form.hbs | 1 - .../acceptance/logout-auth-method-test.js | 42 +++++++++++ ui/tests/helpers/oidc-window-stub.js | 33 +++++++++ .../integration/components/auth-jwt-test.js | 69 +++---------------- 8 files changed, 121 insertions(+), 77 deletions(-) create mode 100644 changelog/14545.txt create mode 100644 ui/tests/acceptance/logout-auth-method-test.js create mode 100644 ui/tests/helpers/oidc-window-stub.js diff --git a/changelog/14545.txt b/changelog/14545.txt new file mode 100644 index 000000000..6b3422132 --- /dev/null +++ b/changelog/14545.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Fixes issue with correct auth method not selected when logging out from OIDC or JWT methods +``` \ No newline at end of file diff --git a/ui/app/components/auth-form.js b/ui/app/components/auth-form.js index a24de2054..490711636 100644 --- a/ui/app/components/auth-form.js +++ b/ui/app/components/auth-form.js @@ -111,6 +111,18 @@ export default Component.extend(DEFAULTS, { this.setProperties(DEFAULTS); }, + getAuthBackend(type) { + const { wrappedToken, methods, selectedAuth, selectedAuthIsPath: keyIsPath } = this; + const selected = type || selectedAuth; + if (!methods && !wrappedToken) { + return {}; + } + if (keyIsPath) { + return methods.findBy('path', selected); + } + return BACKENDS.findBy('type', selected); + }, + selectedAuthIsPath: match('selectedAuth', /\/$/), selectedAuthBackend: computed( 'wrappedToken', @@ -119,14 +131,7 @@ export default Component.extend(DEFAULTS, { 'selectedAuth', 'selectedAuthIsPath', function () { - let { wrappedToken, methods, selectedAuth, selectedAuthIsPath: keyIsPath } = this; - if (!methods && !wrappedToken) { - return {}; - } - if (keyIsPath) { - return methods.findBy('path', selectedAuth); - } - return BACKENDS.findBy('type', selectedAuth); + return this.getAuthBackend(); } ), @@ -208,10 +213,18 @@ export default Component.extend(DEFAULTS, { authenticate: task( waitFor(function* (backendType, data) { - let clusterId = this.cluster.id; + const { + selectedAuth, + cluster: { id: clusterId }, + } = this; try { this.delayAuthMessageReminder.perform(); - const authResponse = yield this.auth.authenticate({ clusterId, backend: backendType, data }); + const authResponse = yield this.auth.authenticate({ + clusterId, + backend: backendType, + data, + selectedAuth, + }); this.onSuccess(authResponse, backendType, data); } catch (e) { this.set('loading', false); @@ -246,7 +259,10 @@ export default Component.extend(DEFAULTS, { this.setProperties({ error: null, }); - let backend = this.selectedAuthBackend || {}; + // if callback from oidc or jwt we have a token at this point + let backend = ['oidc', 'jwt'].includes(this.selectedAuth) + ? this.getAuthBackend('token') + : this.selectedAuthBackend || {}; let backendMeta = BACKENDS.find( (b) => (b.type || '').toLowerCase() === (backend.type || '').toLowerCase() ); diff --git a/ui/app/components/auth-jwt.js b/ui/app/components/auth-jwt.js index cb288b011..6d200fd74 100644 --- a/ui/app/components/auth-jwt.js +++ b/ui/app/components/auth-jwt.js @@ -165,7 +165,6 @@ export default Component.extend({ return this.handleOIDCError(e); } let token = resp.auth.client_token; - this.onSelectedAuth('token'); this.onToken(token); yield this.onSubmit(); }), diff --git a/ui/app/services/auth.js b/ui/app/services/auth.js index ec1db73f0..598f754f0 100644 --- a/ui/app/services/auth.js +++ b/ui/app/services/auth.js @@ -358,7 +358,7 @@ export default Service.extend({ return {}; }, - async authenticate(/*{clusterId, backend, data}*/) { + async authenticate(/*{clusterId, backend, data, selectedAuth}*/) { const [options] = arguments; const adapter = this.clusterAdapter(); @@ -389,6 +389,8 @@ export default Service.extend({ }, async authSuccess(options, response) { + // persist selectedAuth to sessionStorage to rehydrate auth form on logout + sessionStorage.setItem('selectedAuth', options.selectedAuth); const authData = await this.persistAuthData(options, response, this.namespaceService.path); await this.permissions.getPaths.perform(); return authData; @@ -407,8 +409,11 @@ export default Service.extend({ }, getAuthType() { - if (!this.authData) return; - return this.authData.backend.type; + // check sessionStorage first + const selectedAuth = sessionStorage.getItem('selectedAuth'); + if (selectedAuth) return selectedAuth; + // fallback to authData which discerns backend type from token + return this.authData ? this.authData.backend.type : null; }, deleteCurrentToken() { diff --git a/ui/app/templates/components/auth-form.hbs b/ui/app/templates/components/auth-form.hbs index 420d0dd9b..d32dddd99 100644 --- a/ui/app/templates/components/auth-form.hbs +++ b/ui/app/templates/components/auth-form.hbs @@ -68,7 +68,6 @@ @onToken={{action (mut this.token)}} @namespace={{this.namespace}} @onNamespace={{action (mut this.namespace)}} - @onSelectedAuth={{action (mut this.selectedAuth)}} @onSubmit={{action "doSubmit"}} @onRoleName={{action (mut this.roleName)}} @roleName={{this.roleName}} diff --git a/ui/tests/acceptance/logout-auth-method-test.js b/ui/tests/acceptance/logout-auth-method-test.js new file mode 100644 index 000000000..e9f0b48b9 --- /dev/null +++ b/ui/tests/acceptance/logout-auth-method-test.js @@ -0,0 +1,42 @@ +import { module, test } from 'qunit'; +import { setupApplicationTest } from 'ember-qunit'; +import { click, visit, fillIn, settled } from '@ember/test-helpers'; +import { setupMirage } from 'ember-cli-mirage/test-support'; +import { fakeWindow, buildMessage } from '../helpers/oidc-window-stub'; +import sinon from 'sinon'; +import { later, run } from '@ember/runloop'; + +module('Acceptance | logout auth method', function (hooks) { + setupApplicationTest(hooks); + setupMirage(hooks); + + hooks.beforeEach(function () { + this.openStub = sinon.stub(window, 'open').callsFake(() => fakeWindow.create()); + }); + hooks.afterEach(function () { + this.openStub.restore(); + }); + + // coverage for bug where token was selected as auth method for oidc and jwt + test('it should populate oidc auth method on logout', async function (assert) { + this.server.post('/auth/oidc/oidc/auth_url', () => ({ + data: { auth_url: 'http://example.com' }, + })); + this.server.get('/auth/foo/oidc/callback', () => ({ + auth: { client_token: 'root' }, + })); + // ensure clean state + sessionStorage.removeItem('selectedAuth'); + await visit('/vault/auth'); + await fillIn('[data-test-select="auth-method"]', 'oidc'); + later(() => run.cancelTimers(), 50); + await click('[data-test-auth-submit]'); + window.postMessage(buildMessage().data, window.origin); + await settled(); + await click('.nav-user-button button'); + await click('#logout'); + assert + .dom('[data-test-select="auth-method"]') + .hasValue('oidc', 'Previous auth method selected on logout'); + }); +}); diff --git a/ui/tests/helpers/oidc-window-stub.js b/ui/tests/helpers/oidc-window-stub.js new file mode 100644 index 000000000..f318da4cf --- /dev/null +++ b/ui/tests/helpers/oidc-window-stub.js @@ -0,0 +1,33 @@ +import EmberObject, { computed } from '@ember/object'; +import Evented from '@ember/object/evented'; + +export const fakeWindow = EmberObject.extend(Evented, { + init() { + this._super(...arguments); + this.on('close', () => { + this.set('closed', true); + }); + }, + screen: computed(function () { + return { + height: 600, + width: 500, + }; + }), + origin: 'https://my-vault.com', + closed: false, + open() {}, + close() {}, +}); + +export const buildMessage = (opts) => ({ + isTrusted: true, + origin: 'https://my-vault.com', + data: { + source: 'oidc-callback', + path: 'foo', + state: 'state', + code: 'code', + }, + ...opts, +}); diff --git a/ui/tests/integration/components/auth-jwt-test.js b/ui/tests/integration/components/auth-jwt-test.js index 4280884ec..7f573a96f 100644 --- a/ui/tests/integration/components/auth-jwt-test.js +++ b/ui/tests/integration/components/auth-jwt-test.js @@ -1,6 +1,4 @@ import { run } from '@ember/runloop'; -import EmberObject, { computed } from '@ember/object'; -import Evented from '@ember/object/evented'; import Service from '@ember/service'; import { module, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; @@ -12,38 +10,19 @@ import { resolve } from 'rsvp'; import { create } from 'ember-cli-page-object'; import form from '../../pages/components/auth-jwt'; import { ERROR_WINDOW_CLOSED, ERROR_MISSING_PARAMS, ERROR_JWT_LOGIN } from 'vault/components/auth-jwt'; +import { fakeWindow, buildMessage } from '../../helpers/oidc-window-stub'; 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); - this.on('close', () => { - this.set('closed', true); - }); - windows.push(this); - }, - screen: computed(function () { - return { - height: 600, - width: 500, - }; - }), - origin: 'https://my-vault.com', - closed: false, -}); fakeWindow.reopen({ + init() { + this._super(...arguments); + windows.push(this); + }, open() { return fakeWindow.create(); }, - close() { windows.forEach((w) => w.trigger('close')); }, @@ -227,17 +206,7 @@ module('Integration | Component | auth jwt', function (hooks) { await waitUntil(() => { return this.openSpy.calledOnce; }); - this.window.trigger( - 'message', - buildMessage({ - data: { - source: 'oidc-callback', - path: 'foo', - state: 'state', - code: 'code', - }, - }) - ); + this.window.trigger('message', buildMessage()); await settled(); assert.equal(this.selectedAuth, 'token', 'calls onSelectedAuth with token'); assert.equal(this.token, 'token', 'calls onToken with token'); @@ -252,18 +221,7 @@ module('Integration | Component | auth jwt', function (hooks) { await waitUntil(() => { return this.openSpy.calledOnce; }); - this.window.trigger( - 'message', - buildMessage({ - origin: 'http://hackerz.com', - data: { - source: 'oidc-callback', - path: 'foo', - state: 'state', - code: 'code', - }, - }) - ); + this.window.trigger('message', buildMessage({ origin: 'http://hackerz.com' })); run.cancelTimers(); await settled(); assert.notOk(this.handler.called, 'should not call the submit handler'); @@ -277,18 +235,7 @@ module('Integration | Component | auth jwt', function (hooks) { await waitUntil(() => { return this.openSpy.calledOnce; }); - this.window.trigger( - 'message', - buildMessage({ - isTrusted: false, - data: { - source: 'oidc-callback', - path: 'foo', - state: 'state', - code: 'code', - }, - }) - ); + this.window.trigger('message', buildMessage({ isTrusted: false })); run.cancelTimers(); await settled(); assert.notOk(this.handler.called, 'should not call the submit handler');