From 0244cad050bd0e1c6f5b30a10653bec90388ca02 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 | 66 ++++++++----- ui/app/components/auth-jwt.js | 14 +-- ui/app/services/auth.js | 39 ++++---- 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 | 99 +++++-------------- 8 files changed, 172 insertions(+), 125 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 000000000000..6b3422132b0d --- /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 9e24fc3c9a2a..030752f2d78b 100644 --- a/ui/app/components/auth-form.js +++ b/ui/app/components/auth-form.js @@ -109,6 +109,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', @@ -116,19 +128,12 @@ export default Component.extend(DEFAULTS, { 'methods.[]', '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); + function () { + return this.getAuthBackend(); } ), - providerName: computed('selectedAuthBackend.type', function() { + providerName: computed('selectedAuthBackend.type', function () { if (!this.selectedAuthBackend) { return; } @@ -142,22 +147,24 @@ export default Component.extend(DEFAULTS, { cspErrorText: `This is a standby Vault node but can't communicate with the active node via request forwarding. Sign in at the active node to use the Vault UI.`, - allSupportedMethods: computed('methodsToShow', 'hasMethodsWithPath', function() { + allSupportedMethods: computed('methodsToShow', 'hasMethodsWithPath', function () { let hasMethodsWithPath = this.hasMethodsWithPath; let methodsToShow = this.methodsToShow; return hasMethodsWithPath ? methodsToShow.concat(BACKENDS) : methodsToShow; }), - hasMethodsWithPath: computed('methodsToShow', function() { + hasMethodsWithPath: computed('methodsToShow', function () { return this.methodsToShow.isAny('path'); }), - methodsToShow: computed('methods', function() { + methodsToShow: computed('methods', function () { let methods = this.methods || []; - let shownMethods = methods.filter(m => BACKENDS.find(b => b.type.toLowerCase() === m.type.toLowerCase())); + let shownMethods = methods.filter((m) => + BACKENDS.find((b) => b.type.toLowerCase() === m.type.toLowerCase()) + ); return shownMethods.length ? shownMethods : BACKENDS; }), - unwrapToken: task(function*(token) { + unwrapToken: task(function* (token) { // will be using the Token Auth Method, so set it here this.set('selectedAuth', 'token'); let adapter = this.store.adapterFor('tools'); @@ -170,7 +177,7 @@ export default Component.extend(DEFAULTS, { } }).withTestWaiter(), - fetchMethods: task(function*() { + fetchMethods: task(function* () { let store = this.store; try { let methods = yield store.findAll('auth-method', { @@ -180,7 +187,7 @@ export default Component.extend(DEFAULTS, { }); this.set( 'methods', - methods.map(m => { + methods.map((m) => { const method = m.serialize({ includeId: true }); return { ...method, @@ -202,7 +209,7 @@ export default Component.extend(DEFAULTS, { this.set('loading', false); let errors; if (e.errors) { - errors = e.errors.map(error => { + errors = e.errors.map((error) => { if (error.detail) { return error.detail; } @@ -215,13 +222,21 @@ export default Component.extend(DEFAULTS, { this.set('error', `${message}${errors.join('.')}`); }, - authenticate: task(function*(backendType, data) { - let clusterId = this.cluster.id; + authenticate: task(function* (backendType, data) { + const { + selectedAuth, + cluster: { id: clusterId }, + } = this; try { if (backendType === 'okta') { this.delayAuthMessageReminder.perform(); } - let authResponse = yield this.auth.authenticate({ clusterId, backend: backendType, data }); + let authResponse = yield this.auth.authenticate({ + clusterId, + backend: backendType, + data, + selectedAuth, + }); let { isRoot, namespace } = authResponse; let transition; @@ -248,7 +263,7 @@ export default Component.extend(DEFAULTS, { } }).withTestWaiter(), - delayAuthMessageReminder: task(function*() { + delayAuthMessageReminder: task(function* () { if (Ember.testing) { this.showLoading = true; yield timeout(0); @@ -272,9 +287,12 @@ 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() + (b) => (b.type || '').toLowerCase() === (backend.type || '').toLowerCase() ); let attributes = (backendMeta || {}).formAttributes || []; diff --git a/ui/app/components/auth-jwt.js b/ui/app/components/auth-jwt.js index 2417754eda0e..43eb3e1c83b5 100644 --- a/ui/app/components/auth-jwt.js +++ b/ui/app/components/auth-jwt.js @@ -29,6 +29,7 @@ export default Component.extend({ onNamespace() {}, didReceiveAttrs() { +this._super(); let { oldSelectedAuthPath, selectedAuthPath } = this; let shouldDebounce = !oldSelectedAuthPath && !selectedAuthPath; if (oldSelectedAuthPath !== selectedAuthPath) { @@ -44,7 +45,7 @@ export default Component.extend({ // Assumes authentication using OIDC until it's known that the mount is // configured for JWT authentication via static keys, JWKS, or OIDC discovery. - isOIDC: computed('errorMessage', function() { + isOIDC: computed('errorMessage', function () { return this.errorMessage !== ERROR_JWT_LOGIN; }), @@ -52,7 +53,7 @@ export default Component.extend({ return this.window || window; }, - fetchRole: task(function*(roleName, options = { debounce: true }) { + fetchRole: task(function* (roleName, options = { debounce: true }) { if (options.debounce) { this.onRoleName(roleName); // debounce @@ -82,7 +83,7 @@ export default Component.extend({ this.onError(err); }, - prepareForOIDC: task(function*(oidcWindow) { + prepareForOIDC: task(function* (oidcWindow) { const thisWindow = this.getWindow(); // show the loading animation in the parent this.onLoading(true); @@ -104,7 +105,7 @@ export default Component.extend({ } }), - watchPopup: task(function*(oidcWindow) { + watchPopup: task(function* (oidcWindow) { while (true) { yield timeout(WAIT_TIME); if (!oidcWindow || oidcWindow.closed) { @@ -113,7 +114,7 @@ export default Component.extend({ } }), - watchCurrent: task(function*(oidcWindow) { + watchCurrent: task(function* (oidcWindow) { // when user is about to change pages, close the popup window yield waitForEvent(this.getWindow(), 'beforeunload'); oidcWindow.close(); @@ -125,7 +126,7 @@ export default Component.extend({ oidcWindow.close(); }, - exchangeOIDC: task(function*(oidcState, oidcWindow) { + exchangeOIDC: task(function* (oidcState, oidcWindow) { if (oidcState === null || oidcState === undefined) { return; } @@ -163,7 +164,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 8b12ab586ac0..c558e59f5734 100644 --- a/ui/app/services/auth.js +++ b/ui/app/services/auth.js @@ -33,7 +33,7 @@ export default Service.extend({ return getOwner(this).lookup('adapter:cluster'); }, - tokens: computed(function() { + tokens: computed(function () { return this.getTokensFromStorage() || []; }), @@ -92,7 +92,7 @@ export default Service.extend({ return fetch(url, { method: opts.method || 'GET', headers: opts.headers || {}, - }).then(response => { + }).then((response) => { if (response.status === 204) { return resolve(); } else if (response.status >= 200 && response.status < 300) { @@ -144,7 +144,7 @@ export default Service.extend({ let currentBackend = BACKENDS.findBy('type', backend); let displayName; if (isArray(currentBackend.displayNamePath)) { - displayName = currentBackend.displayNamePath.map(name => get(resp, name)).join('/'); + displayName = currentBackend.displayNamePath.map((name) => get(resp, name)).join('/'); } else { displayName = get(resp, currentBackend.displayNamePath); } @@ -217,7 +217,7 @@ export default Service.extend({ return this.storage(token).removeItem(token); }, - tokenExpirationDate: computed('currentTokenName', 'expirationCalcTS', function() { + tokenExpirationDate: computed('currentTokenName', 'expirationCalcTS', function () { const tokenName = this.currentTokenName; if (!tokenName) { return; @@ -232,7 +232,7 @@ export default Service.extend({ return expiration ? this.now() >= expiration : null; }, - renewAfterEpoch: computed('currentTokenName', 'expirationCalcTS', function() { + renewAfterEpoch: computed('currentTokenName', 'expirationCalcTS', function () { const tokenName = this.currentTokenName; let { expirationCalcTS } = this; const data = this.getTokenData(tokenName); @@ -252,18 +252,18 @@ export default Service.extend({ } this.isRenewing = true; return this.renewCurrentToken().then( - resp => { + (resp) => { this.isRenewing = false; return this.persistAuthData(tokenName, resp.data || resp.auth); }, - e => { + (e) => { this.isRenewing = false; throw e; } ); }, - checkShouldRenew: task(function*() { + checkShouldRenew: task(function* () { while (true) { if (Ember.testing) { return; @@ -304,7 +304,7 @@ export default Service.extend({ getTokensFromStorage(filterFn) { return this.storage() .keys() - .reject(key => { + .reject((key) => { return key.indexOf(TOKEN_PREFIX) !== 0 || (filterFn && filterFn(key)); }); }, @@ -314,7 +314,7 @@ export default Service.extend({ return; } - this.getTokensFromStorage().forEach(key => { + this.getTokensFromStorage().forEach((key) => { const data = this.getTokenData(key); if (data && data.policies && data.policies.includes('root')) { this.removeTokenData(key); @@ -322,19 +322,24 @@ export default Service.extend({ }); }, - async authenticate(/*{clusterId, backend, data}*/) { + async authenticate(/*{clusterId, backend, data, selectedAuth}*/) { const [options] = arguments; const adapter = this.clusterAdapter(); let resp = await adapter.authenticate(options); + // persist selectedAuth to sessionStorage to rehydrate auth form on logout + sessionStorage.setItem('selectedAuth', options.selectedAuth); let authData = await this.persistAuthData(options, resp.auth || resp.data, this.namespaceService.path); await this.permissions.getPaths.perform(); return authData; }, 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() { @@ -350,19 +355,19 @@ export default Service.extend({ }, // returns the key for the token to use - currentTokenName: computed('activeCluster', 'tokens', 'tokens.[]', function() { + currentTokenName: computed('activeCluster', 'tokens', 'tokens.[]', function () { const regex = new RegExp(this.activeCluster); - return this.tokens.find(key => regex.test(key)); + return this.tokens.find((key) => regex.test(key)); }), - currentToken: computed('currentTokenName', function() { + currentToken: computed('currentTokenName', function () { const name = this.currentTokenName; const data = name && this.getTokenData(name); // data.token is undefined so that's why it returns current token undefined return name && data ? data.token : null; }), - authData: computed('currentTokenName', function() { + authData: computed('currentTokenName', function () { const token = this.currentTokenName; if (!token) { return; diff --git a/ui/app/templates/components/auth-form.hbs b/ui/app/templates/components/auth-form.hbs index c782b341c635..9b57685aa337 100644 --- a/ui/app/templates/components/auth-form.hbs +++ b/ui/app/templates/components/auth-form.hbs @@ -49,7 +49,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 000000000000..e9f0b48b9d82 --- /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 000000000000..f318da4cfedd --- /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 bcf49e797750..7f573a96f62a 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,40 +10,21 @@ 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, { + +fakeWindow.reopen({ 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({ open() { return fakeWindow.create(); }, - close() { - windows.forEach(w => w.trigger('close')); + windows.forEach((w) => w.trigger('close')); }, }); @@ -86,17 +65,17 @@ const renderIt = async (context, path = 'jwt') => { /> `); }; -module('Integration | Component | auth jwt', function(hooks) { +module('Integration | Component | auth jwt', function (hooks) { setupRenderingTest(hooks); - hooks.beforeEach(function() { + hooks.beforeEach(function () { this.openSpy = sinon.spy(fakeWindow.proto(), 'open'); this.owner.register('service:router', routerStub); - this.server = new Pretender(function() { - this.get('/v1/auth/:path/oidc/callback', function() { + this.server = new Pretender(function () { + this.get('/v1/auth/:path/oidc/callback', function () { return [200, { 'Content-Type': 'application/json' }, JSON.stringify(OIDC_AUTH_RESPONSE)]; }); - this.post('/v1/auth/:path/oidc/auth_url', request => { + this.post('/v1/auth/:path/oidc/auth_url', (request) => { let body = JSON.parse(request.requestBody); if (body.role === 'test') { return [ @@ -125,17 +104,17 @@ module('Integration | Component | auth jwt', function(hooks) { }); }); - hooks.afterEach(function() { + hooks.afterEach(function () { this.openSpy.restore(); this.server.shutdown(); }); - test('it renders the yield', async function(assert) { + test('it renders the yield', async function (assert) { await render(hbs`Hello!`); assert.equal(component.yieldContent, 'Hello!', 'yields properly'); }); - test('jwt: it renders and makes auth_url requests', async function(assert) { + test('jwt: it renders and makes auth_url requests', async function (assert) { await renderIt(this); await settled(); assert.ok(component.jwtPresent, 'renders jwt field'); @@ -152,13 +131,13 @@ module('Integration | Component | auth jwt', function(hooks) { ); }); - test('jwt: it calls passed action on login', async function(assert) { + test('jwt: it calls passed action on login', async function (assert) { await renderIt(this); await component.login(); assert.ok(this.handler.calledOnce); }); - test('oidc: test role: it renders', async function(assert) { + test('oidc: test role: it renders', async function (assert) { await renderIt(this); await settled(); this.set('selectedAuthPath', 'foo'); @@ -173,7 +152,7 @@ module('Integration | Component | auth jwt', function(hooks) { assert.equal(component.loginButtonText, 'Sign in with Okta', 'recognizes auth methods with certain urls'); }); - test('oidc: it calls window.open popup window on login', async function(assert) { + test('oidc: it calls window.open popup window on login', async function (assert) { await renderIt(this); this.set('selectedAuthPath', 'foo'); await component.role('test'); @@ -190,7 +169,7 @@ module('Integration | Component | auth jwt', function(hooks) { ); }); - test('oidc: it calls error handler when popup is closed', async function(assert) { + test('oidc: it calls error handler when popup is closed', async function (assert) { await renderIt(this); this.set('selectedAuthPath', 'foo'); await component.role('test'); @@ -203,7 +182,7 @@ module('Integration | Component | auth jwt', function(hooks) { assert.equal(this.error, ERROR_WINDOW_CLOSED, 'calls onError with error string'); }); - test('oidc: shows error when message posted with state key, wrong params', async function(assert) { + test('oidc: shows error when message posted with state key, wrong params', async function (assert) { await renderIt(this); this.set('selectedAuthPath', 'foo'); await component.role('test'); @@ -219,7 +198,7 @@ module('Integration | Component | auth jwt', function(hooks) { assert.equal(this.error, ERROR_MISSING_PARAMS, 'calls onError with params missing error'); }); - test('oidc: storage event fires with state 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'); @@ -227,24 +206,14 @@ 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'); assert.ok(this.handler.calledOnce, 'calls the onSubmit handler'); }); - test('oidc: fails silently when event origin does not match window origin', async function(assert) { + 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'); @@ -252,24 +221,13 @@ 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'); }); - test('oidc: fails silently when event is not trusted', 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'); @@ -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');