From 17eaac32c72efe9899708887842eb3c6338f38a6 Mon Sep 17 00:00:00 2001 From: Jordan Reimer Date: Fri, 3 Nov 2023 09:05:50 -0600 Subject: [PATCH] OIDC/JWT Role Fetch Error Handling Updates (#23908) * updates oidc/jwt role fetch error handling * adds changelog entry --- changelog/23908.txt | 3 + ui/app/components/auth-jwt.js | 125 +++++++++--------- ui/app/templates/components/auth-jwt.hbs | 6 +- ui/tests/acceptance/oidc-auth-method-test.js | 100 ++++++++++---- .../integration/components/auth-jwt-test.js | 2 +- 5 files changed, 141 insertions(+), 95 deletions(-) create mode 100644 changelog/23908.txt diff --git a/changelog/23908.txt b/changelog/23908.txt new file mode 100644 index 0000000000..6bd39bb1a0 --- /dev/null +++ b/changelog/23908.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Updates OIDC/JWT login error handling to surface all role related errors +``` \ No newline at end of file diff --git a/ui/app/components/auth-jwt.js b/ui/app/components/auth-jwt.js index d2b44ce93a..89501afe16 100644 --- a/ui/app/components/auth-jwt.js +++ b/ui/app/components/auth-jwt.js @@ -3,13 +3,11 @@ * SPDX-License-Identifier: BUSL-1.1 */ -import Ember from 'ember'; import { inject as service } from '@ember/service'; // ARG NOTE: Once you remove outer-html after glimmerizing you can remove the outer-html component import Component from './outer-html'; import { task, timeout, waitForEvent } from 'ember-concurrency'; -import { computed } from '@ember/object'; -import { waitFor } from '@ember/test-waiters'; +import { debounce } from '@ember/runloop'; const WAIT_TIME = 500; const ERROR_WINDOW_CLOSED = @@ -28,57 +26,50 @@ export default Component.extend({ roleName: null, role: null, errorMessage: null, + isOIDC: true, + onRoleName() {}, onLoading() {}, onError() {}, onNamespace() {}, didReceiveAttrs() { - this._super(); - const debounce = !this.oldSelectedAuthPath && !this.selectedAuthPath; + this._super(...arguments); + // if mount path or type changes we need to check again for JWT configuration + const didChangePath = this._authPath !== this.selectedAuthPath; + const didChangeType = this._authType !== this.selectedAuthType; - if (this.oldSelectedAuthPath !== this.selectedAuthPath || debounce) { - this.fetchRole.perform(this.roleName, { debounce }); + if (didChangePath || didChangeType) { + // path updates as the user types so we need to debounce that event + const wait = didChangePath ? 500 : 0; + debounce(this, 'fetchRole', wait); } - - this.set('errorMessage', null); - this.set('oldSelectedAuthPath', this.selectedAuthPath); + this._authPath = this.selectedAuthPath; + this._authType = this.selectedAuthType; }, - // 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 () { - return this.errorMessage !== ERROR_JWT_LOGIN; - }), - getWindow() { return this.window || window; }, - fetchRole: task( - waitFor(function* (roleName, options = { debounce: true }) { - if (options.debounce) { - this.onRoleName(roleName); - // debounce - yield timeout(Ember.testing ? 0 : WAIT_TIME); - } - const path = this.selectedAuthPath || this.selectedAuthType; - const id = JSON.stringify([path, roleName]); - let role = null; - try { - role = yield this.store.findRecord('role-jwt', id, { adapterOptions: { namespace: this.namespace } }); - } catch (e) { - // throwing here causes failures in tests - if ((!e.httpStatus || e.httpStatus !== 400) && !Ember.testing) { - throw e; - } - if (e.errors && e.errors.length > 0) { - this.set('errorMessage', e.errors[0]); - } - } + async fetchRole() { + const path = this.selectedAuthPath || this.selectedAuthType; + const id = JSON.stringify([path, this.roleName]); + this.setProperties({ role: null, errorMessage: null, isOIDC: true }); + + try { + const role = await this.store.findRecord('role-jwt', id, { + adapterOptions: { namespace: this.namespace }, + }); this.set('role', role); - }) - ).restartable(), + } catch (e) { + const error = (e.errors || [])[0]; + const errorMessage = + e.httpStatus === 400 ? 'Invalid role. Please try again.' : `Error fetching role: ${error}`; + // assume OIDC until it's known that the mount is configured for JWT authentication via static keys, JWKS, or OIDC discovery. + this.setProperties({ isOIDC: error !== ERROR_JWT_LOGIN, errorMessage }); + } + }, cancelLogin(oidcWindow, errorMessage) { this.closeWindow(oidcWindow); @@ -170,33 +161,20 @@ export default Component.extend({ yield this.onSubmit(null, null, resp.auth.client_token); }), - actions: { - async startOIDCAuth(data, e) { - this.onError(null); - if (e && e.preventDefault) { - e.preventDefault(); - } - try { - await this.fetchRole.perform(this.roleName, { debounce: false }); - } catch (error) { - // this task could be cancelled if the instances in didReceiveAttrs resolve after this was started - if (error?.name !== 'TaskCancelation') { - throw error; - } - } - if (!this.isOIDC || !this.role || !this.role.authUrl) { - let message = this.errorMessage; - if (!this.role) { - message = 'Invalid role. Please try again.'; - } else if (!this.role.authUrl) { - message = - 'Missing auth_url. Please check that allowed_redirect_uris for the role include this mount path.'; - } - this.onError(message); - return; - } - const win = this.getWindow(); + async startOIDCAuth() { + this.onError(null); + await this.fetchRole(); + + const error = + this.role && !this.role.authUrl + ? 'Missing auth_url. Please check that allowed_redirect_uris for the role include this mount path.' + : this.errorMessage || null; + + if (error) { + this.onError(error); + } else { + const win = this.getWindow(); const POPUP_WIDTH = 500; const POPUP_HEIGHT = 600; const left = win.screen.width / 2 - POPUP_WIDTH / 2; @@ -208,6 +186,23 @@ export default Component.extend({ ); this.prepareForOIDC.perform(oidcWindow); + } + }, + + actions: { + onRoleChange(event) { + this.onRoleName(event.target.value); + debounce(this, 'fetchRole', 500); + }, + signIn(event) { + event.preventDefault(); + + if (this.isOIDC) { + this.startOIDCAuth(); + } else { + const { jwt, roleName: role } = this; + this.onSubmit({ role, jwt }); + } }, }, }); diff --git a/ui/app/templates/components/auth-jwt.hbs b/ui/app/templates/components/auth-jwt.hbs index 121b797f9d..104607f894 100644 --- a/ui/app/templates/components/auth-jwt.hbs +++ b/ui/app/templates/components/auth-jwt.hbs @@ -3,20 +3,20 @@ SPDX-License-Identifier: BUSL-1.1 ~}} -
+
diff --git a/ui/tests/acceptance/oidc-auth-method-test.js b/ui/tests/acceptance/oidc-auth-method-test.js index 4dfa296129..cedd5899e3 100644 --- a/ui/tests/acceptance/oidc-auth-method-test.js +++ b/ui/tests/acceptance/oidc-auth-method-test.js @@ -11,6 +11,7 @@ import { setupMirage } from 'ember-cli-mirage/test-support'; import { fakeWindow, buildMessage } from '../helpers/oidc-window-stub'; import sinon from 'sinon'; import { later, _cancelTimers as cancelTimers } from '@ember/runloop'; +import { Response } from 'miragejs'; module('Acceptance | oidc auth method', function (hooks) { setupApplicationTest(hooks); @@ -18,15 +19,41 @@ module('Acceptance | oidc auth method', function (hooks) { hooks.beforeEach(function () { this.openStub = sinon.stub(window, 'open').callsFake(() => fakeWindow.create()); - // OIDC test fails when using fake timestamps, we use the real timestamp.now here - this.server.post('/auth/oidc/oidc/auth_url', () => ({ - data: { auth_url: 'http://example.com' }, - })); + + this.setupMocks = (assert) => { + this.server.post('/auth/oidc/oidc/auth_url', () => ({ + data: { auth_url: 'http://example.com' }, + })); + // there was a bug that would result in the /auth/:path/login endpoint hit with an empty payload rather than lookup-self + // ensure that the correct endpoint is hit after the oidc callback + if (assert) { + this.server.get('/auth/token/lookup-self', (schema, req) => { + assert.ok(true, 'request made to auth/token/lookup-self after oidc callback'); + req.passthrough(); + }); + } + }; + this.server.get('/auth/foo/oidc/callback', () => ({ auth: { client_token: 'root' }, })); + + // select method from dropdown or click auth path tab + this.selectMethod = async (method, useLink) => { + const methodSelector = useLink + ? `[data-test-auth-method-link="${method}"]` + : '[data-test-select="auth-method"]'; + await waitUntil(() => find(methodSelector)); + if (useLink) { + await click(`[data-test-auth-method-link="${method}"]`); + } else { + await fillIn('[data-test-select="auth-method"]', method); + } + }; + // ensure clean state localStorage.removeItem('selectedAuth'); + authPage.logout(); }); hooks.afterEach(function () { @@ -36,14 +63,9 @@ module('Acceptance | oidc auth method', function (hooks) { test('it should login with oidc when selected from auth methods dropdown', async function (assert) { assert.expect(1); - this.server.get('/auth/token/lookup-self', (schema, req) => { - assert.ok(true, 'request made to auth/token/lookup-self after oidc callback'); - req.passthrough(); - }); - authPage.logout(); - // select from dropdown or click auth path tab - await waitUntil(() => find('[data-test-select="auth-method"]')); - await fillIn('[data-test-select="auth-method"]', 'oidc'); + this.setupMocks(assert); + + await this.selectMethod('oidc'); later(() => { window.postMessage(buildMessage().data, window.origin); cancelTimers(); @@ -54,6 +76,8 @@ module('Acceptance | oidc auth method', function (hooks) { test('it should login with oidc from listed auth mount tab', async function (assert) { assert.expect(3); + this.setupMocks(assert); + this.server.get('/sys/internal/ui/mounts', () => ({ data: { auth: { @@ -68,17 +92,8 @@ module('Acceptance | oidc auth method', function (hooks) { assert.ok(true, 'auth_url request made to correct non-standard mount path'); return { data: { auth_url: 'http://example.com' } }; }); - // there was a bug that would result in the /auth/:path/login endpoint hit with an empty payload rather than lookup-self - // ensure that the correct endpoint is hit after the oidc callback - this.server.get('/auth/token/lookup-self', (schema, req) => { - assert.ok(true, 'request made to auth/token/lookup-self after oidc callback'); - req.passthrough(); - }); - authPage.logout(); - // select from dropdown or click auth path tab - await waitUntil(() => find('[data-test-auth-method-link="oidc"]')); - await click('[data-test-auth-method-link="oidc"]'); + await this.selectMethod('oidc', true); later(() => { window.postMessage(buildMessage().data, window.origin); cancelTimers(); @@ -88,10 +103,8 @@ module('Acceptance | oidc auth method', function (hooks) { // 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) { - authPage.logout(); - // select from dropdown or click auth path tab - await waitUntil(() => find('[data-test-select="auth-method"]')); - await fillIn('[data-test-select="auth-method"]', 'oidc'); + this.setupMocks(); + await this.selectMethod('oidc'); later(() => { window.postMessage(buildMessage().data, window.origin); cancelTimers(); @@ -104,4 +117,39 @@ module('Acceptance | oidc auth method', function (hooks) { .dom('[data-test-select="auth-method"]') .hasValue('oidc', 'Previous auth method selected on logout'); }); + + test('it should fetch role when switching between oidc/jwt auth methods and changing the mount path', async function (assert) { + let reqCount = 0; + this.server.post('/auth/:method/oidc/auth_url', (schema, req) => { + reqCount++; + const errors = + req.params.method === 'jwt' ? ['OIDC login is not configured for this mount'] : ['missing role']; + return new Response(400, {}, { errors }); + }); + + await this.selectMethod('oidc'); + assert.dom('[data-test-jwt]').doesNotExist('JWT Token input hidden for OIDC'); + await this.selectMethod('jwt'); + assert.dom('[data-test-jwt]').exists('JWT Token input renders for JWT configured method'); + await click('[data-test-auth-form-options-toggle]'); + await fillIn('[data-test-auth-form-mount-path]', 'foo'); + assert.strictEqual(reqCount, 3, 'Role is fetched when dependant values are changed'); + }); + + test('it should display role fetch errors when signing in with OIDC', async function (assert) { + this.server.post('/auth/:method/oidc/auth_url', (schema, req) => { + const { role } = JSON.parse(req.requestBody); + const status = role ? 403 : 400; + const errors = role ? ['permission denied'] : ['missing role']; + return new Response(status, {}, { errors }); + }); + + await this.selectMethod('oidc'); + await click('[data-test-auth-submit]'); + assert.dom('[data-test-message-error-description]').hasText('Invalid role. Please try again.'); + + await fillIn('[data-test-role]', 'test'); + await click('[data-test-auth-submit]'); + assert.dom('[data-test-message-error-description]').hasText('Error fetching role: permission denied'); + }); }); diff --git a/ui/tests/integration/components/auth-jwt-test.js b/ui/tests/integration/components/auth-jwt-test.js index 494c86a6bf..55302a7bca 100644 --- a/ui/tests/integration/components/auth-jwt-test.js +++ b/ui/tests/integration/components/auth-jwt-test.js @@ -138,7 +138,7 @@ module('Integration | Component | auth jwt', function (hooks) { await component.role('okta'); // 1 for initial render, 1 for each time role changed = 3 - assert.strictEqual(this.server.handledRequests.length, 4, 'fetches the auth_url when the path changes'); + assert.strictEqual(this.server.handledRequests.length, 3, 'fetches the auth_url when the path changes'); assert.strictEqual( component.loginButtonText, 'Sign in with Okta',