From 88f4180fcc87e40349f588ce0d67f64be7c022ef Mon Sep 17 00:00:00 2001 From: claire bontempo <68122737+hellobontempo@users.noreply.github.com> Date: Fri, 25 Apr 2025 14:58:09 -0700 Subject: [PATCH] UI: Consolidates form parsing logic to `base.ts` component (#30397) * stuck tests * consolidate form parsing logic --- ui/app/components/auth/form/base.ts | 56 +++++++++++++------ ui/app/components/auth/form/oidc-jwt.ts | 7 +-- ui/app/utils/supported-login-methods.ts | 4 ++ ui/tests/helpers/auth/auth-helpers.ts | 13 ++--- .../components/auth/form-template-test.js | 8 ++- .../components/auth/form/base-test.js | 44 ++++++++++++--- .../components/auth/form/oidc-jwt-test.js | 4 ++ .../components/auth/form/okta-test.js | 4 ++ .../components/auth/form/test-helper.js | 17 ++---- 9 files changed, 106 insertions(+), 51 deletions(-) diff --git a/ui/app/components/auth/form/base.ts b/ui/app/components/auth/form/base.ts index 24c93818c6..68b473fa56 100644 --- a/ui/app/components/auth/form/base.ts +++ b/ui/app/components/auth/form/base.ts @@ -9,10 +9,13 @@ import { service } from '@ember/service'; import { task } from 'ember-concurrency'; import { waitFor } from '@ember/test-waiters'; import { sanitizePath } from 'core/utils/sanitize-path'; +import { POSSIBLE_FIELDS } from 'vault/utils/supported-login-methods'; import type AuthService from 'vault/vault/services/auth'; -import type { AuthData } from 'vault/vault/services/auth'; import type ClusterModel from 'vault/models/cluster'; +import type FlagsService from 'vault/services/flags'; +import type VersionService from 'vault/services/version'; +import type { AuthData } from 'vault/vault/services/auth'; import type { HTMLElementEvent } from 'vault/forms'; /** @@ -33,26 +36,14 @@ interface Args { export default class AuthBase extends Component { @service declare readonly auth: AuthService; + @service declare readonly flags: FlagsService; + @service declare readonly version: VersionService; @action onSubmit(event: HTMLElementEvent) { event.preventDefault(); const formData = new FormData(event.target as HTMLFormElement); - const data: Record = {}; - - for (const key of formData.keys()) { - const value = formData.get(key); - // strip leading or trailing slashes from path for consistency - data[key] = key === 'path' ? sanitizePath(value) : value; - } - - // If path is not included in the submitted form data, - // set it as the auth type which is the default path Vault expects. - // The "token" auth method does not support custom login paths. - if (this.args.authType !== 'token' && !Object.keys(data).includes('path')) { - data['path'] = this.args.authType; - } - + const data = this.parseFormData(formData); this.login.unlinked().perform(data); } @@ -84,4 +75,37 @@ export default class AuthBase extends Component { this.args.onError(errorMessage); } } + + parseFormData(formData: FormData) { + const data: Record = {}; + + // iterate over method specific fields + for (const field of POSSIBLE_FIELDS) { + const value = formData.get(field); + if (value) { + data[field] = value; + } + } + + // path is supported by all auth methods except token + if (this.args.authType !== 'token') { + // strip leading or trailing slashes for consistency. + // fallback to auth type which is the default path Vault expects. + data['path'] = sanitizePath(formData?.get('path')) || this.args.authType; + } + + if (this.version.isEnterprise) { + // strip leading or trailing slashes for consistency + let namespace = sanitizePath(formData?.get('namespace')) || ''; + + const hvdRootNs = this.flags.hvdManagedNamespaceRoot; // if HVD managed, this is "admin" + if (hvdRootNs) { + // HVD managed clusters can only input child namespaces, manually prepend with the hvd root + namespace = namespace ? `${hvdRootNs}/${namespace}` : hvdRootNs; + } + data['namespace'] = namespace; + } + + return data; + } } diff --git a/ui/app/components/auth/form/oidc-jwt.ts b/ui/app/components/auth/form/oidc-jwt.ts index 36c7b8be94..971355ffce 100644 --- a/ui/app/components/auth/form/oidc-jwt.ts +++ b/ui/app/components/auth/form/oidc-jwt.ts @@ -9,7 +9,6 @@ import { tracked } from '@glimmer/tracking'; import { service } from '@ember/service'; import { restartableTask, task, timeout, waitForEvent } from 'ember-concurrency'; import { action } from '@ember/object'; -import { sanitizePath } from 'core/utils/sanitize-path'; import { waitFor } from '@ember/test-waiters'; import errorMessage from 'vault/utils/error-message'; @@ -61,7 +60,7 @@ export default class AuthFormOidcJwt extends AuthBase { ]; // set by form inputs - _formData: FormData | null = null; + _formData: FormData = new FormData(); // set during auth prep and login workflow @tracked fetchedRole: RoleJwtModel | null = null; @@ -102,9 +101,7 @@ export default class AuthFormOidcJwt extends AuthBase { // it will cancel and restart from the beginning. if (wait) await timeout(wait); - const namespace = this._formData?.get('namespace') || ''; - const path = sanitizePath(this._formData?.get('path')) || this.args.authType; - const role = this._formData?.get('role') || ''; + const { namespace = '', path = '', role = '' } = this.parseFormData(this._formData); const id = JSON.stringify([path, role]); // reset state diff --git a/ui/app/utils/supported-login-methods.ts b/ui/app/utils/supported-login-methods.ts index 244dc1d76f..83136da492 100644 --- a/ui/app/utils/supported-login-methods.ts +++ b/ui/app/utils/supported-login-methods.ts @@ -56,3 +56,7 @@ export const ALL_LOGIN_METHODS = [...BASE_LOGIN_METHODS, ...ENTERPRISE_LOGIN_MET export const supportedTypes = (isEnterprise: boolean) => isEnterprise ? ALL_LOGIN_METHODS.map((m) => m.type) : BASE_LOGIN_METHODS.map((m) => m.type); + +// this ensures no unexpected params are injected and submitted in the login form +// 'namespace' and 'path' are intentionally omitted because they are handled explicitly +export const POSSIBLE_FIELDS = ['role', 'jwt', 'password', 'token', 'username']; diff --git a/ui/tests/helpers/auth/auth-helpers.ts b/ui/tests/helpers/auth/auth-helpers.ts index 79bfb5a114..43dfb8d766 100644 --- a/ui/tests/helpers/auth/auth-helpers.ts +++ b/ui/tests/helpers/auth/auth-helpers.ts @@ -8,6 +8,7 @@ import VAULT_KEYS from 'vault/tests/helpers/vault-keys'; import { AUTH_FORM } from 'vault/tests/helpers/auth/auth-form-selectors'; import { GENERAL } from 'vault/tests/helpers/general-selectors'; import { Server } from 'miragejs'; +import { POSSIBLE_FIELDS } from 'vault/utils/supported-login-methods'; const { rootToken } = VAULT_KEYS; @@ -38,15 +39,11 @@ export const loginNs = async (ns: string, token = rootToken) => { return click(AUTH_FORM.login); }; -// LOGIN WITH NON-TOKEN methods -interface LoginFields { - username?: string; - password?: string; - token?: string; - role?: string; +// LOGIN WITH NON-TOKEN METHODS +type LoginFields = Partial> & { path: string; namespace: string; -} +}; interface LoginOptions { authType?: string; @@ -64,7 +61,7 @@ export const loginMethod = async (loginFields: LoginFields, options: LoginOption export const fillInLoginFields = async (loginFields: LoginFields, { toggleOptions = false } = {}) => { if (toggleOptions) await click(AUTH_FORM.moreOptions); - for (const [input, value] of Object.entries(loginFields)) { + for (const [input, value = ''] of Object.entries(loginFields)) { await fillIn(GENERAL.inputByAttr(input), value); } }; diff --git a/ui/tests/integration/components/auth/form-template-test.js b/ui/tests/integration/components/auth/form-template-test.js index df133430b8..214a6d918f 100644 --- a/ui/tests/integration/components/auth/form-template-test.js +++ b/ui/tests/integration/components/auth/form-template-test.js @@ -85,10 +85,14 @@ module('Integration | Component | auth | form template', function (hooks) { }); test('it displays errors', async function (assert) { + const authenticateStub = sinon.stub(this.owner.lookup('service:auth'), 'authenticate'); + authenticateStub.throws('permission denied'); await this.renderComponent(); await click(AUTH_FORM.login); - // this error message text is because the auth service is not stubbed in this test - assert.dom(GENERAL.messageError).hasText('Error Authentication failed: permission denied'); + assert + .dom(GENERAL.messageError) + .hasText('Error Authentication failed: permission denied: Sinon-provided permission denied'); + authenticateStub.restore(); }); module('listing visibility', function (hooks) { diff --git a/ui/tests/integration/components/auth/form/base-test.js b/ui/tests/integration/components/auth/form/base-test.js index 8cb548e52d..576ccf212b 100644 --- a/ui/tests/integration/components/auth/form/base-test.js +++ b/ui/tests/integration/components/auth/form/base-test.js @@ -6,10 +6,11 @@ import { module, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; import hbs from 'htmlbars-inline-precompile'; -import { find, render } from '@ember/test-helpers'; +import { click, fillIn, find, render } from '@ember/test-helpers'; import sinon from 'sinon'; import testHelper from './test-helper'; import { GENERAL } from 'vault/tests/helpers/general-selectors'; +import { AUTH_FORM } from 'vault/tests/helpers/auth/auth-form-selectors'; // These auth types all use the default methods in auth/form/base // Any auth types with custom logic should be in a separate test file, i.e. okta @@ -24,6 +25,10 @@ module('Integration | Component | auth | form | base', function (hooks) { this.onSuccess = sinon.spy(); }); + hooks.afterEach(function () { + this.authenticateStub.restore(); + }); + module('github', function (hooks) { hooks.beforeEach(function () { this.authType = 'github'; @@ -142,11 +147,6 @@ module('Integration | Component | auth | form | base', function (hooks) { hooks.beforeEach(function () { this.authType = 'token'; this.expectedFields = ['token']; - this.expectedSubmit = { - default: { token: 'mytoken' }, - // token doesn't support custom paths, so just test yielding functionality - custom: { token: 'mytoken', yield: 'yield-token' }, - }; this.renderComponent = ({ yieldBlock = false } = {}) => { if (yieldBlock) { return render(hbs` @@ -157,8 +157,8 @@ module('Integration | Component | auth | form | base', function (hooks) { @onSuccess={{this.onSuccess}} > <:advancedSettings> - - + + `); } @@ -172,7 +172,33 @@ module('Integration | Component | auth | form | base', function (hooks) { }; }); - testHelper(test); + testHelper(test, { standardSubmit: false }); + + test('it submits form data with defaults', async function (assert) { + await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('token'), 'mytoken'); + await click(AUTH_FORM.login); + const [actual] = this.authenticateStub.lastCall.args; + assert.propEqual( + actual.data, + { token: 'mytoken' }, + 'auth service "authenticate" method is called with form data' + ); + }); + + test('it submits form data from yielded inputs', async function (assert) { + await this.renderComponent({ yieldBlock: true }); + await fillIn(GENERAL.inputByAttr('token'), 'mytoken'); + // token doesn't support custom paths, so testing path is not sent + await fillIn(GENERAL.inputByAttr('path'), `path-${this.authType}`); + await click(AUTH_FORM.login); + const [actual] = this.authenticateStub.lastCall.args; + assert.propEqual( + actual.data, + { token: 'mytoken' }, + 'auth service "authenticate" method is called without "path"' + ); + }); }); module('userpass', function (hooks) { diff --git a/ui/tests/integration/components/auth/form/oidc-jwt-test.js b/ui/tests/integration/components/auth/form/oidc-jwt-test.js index 31ab4fc29a..1dfd8f9027 100644 --- a/ui/tests/integration/components/auth/form/oidc-jwt-test.js +++ b/ui/tests/integration/components/auth/form/oidc-jwt-test.js @@ -331,6 +331,10 @@ module('Integration | Component | auth | form | oidc-jwt', function (hooks) { }; }); + hooks.afterEach(function () { + this.authenticateStub.restore(); + }); + test('it renders helper text', async function (assert) { await this.renderComponent(); const id = find(GENERAL.inputByAttr('role')).id; diff --git a/ui/tests/integration/components/auth/form/okta-test.js b/ui/tests/integration/components/auth/form/okta-test.js index 591cb2921e..4ba6ee616a 100644 --- a/ui/tests/integration/components/auth/form/okta-test.js +++ b/ui/tests/integration/components/auth/form/okta-test.js @@ -70,6 +70,10 @@ module('Integration | Component | auth | form | okta', function (hooks) { }; }); + hooks.afterEach(function () { + this.authenticateStub.restore(); + }); + testHelper(test, { standardSubmit: false }); test('it submits form data with defaults', async function (assert) { diff --git a/ui/tests/integration/components/auth/form/test-helper.js b/ui/tests/integration/components/auth/form/test-helper.js index bc6a9dfe12..eb9d0fbfb9 100644 --- a/ui/tests/integration/components/auth/form/test-helper.js +++ b/ui/tests/integration/components/auth/form/test-helper.js @@ -3,7 +3,7 @@ * SPDX-License-Identifier: BUSL-1.1 */ -import { click, fillIn } from '@ember/test-helpers'; +import { click, fillIn, findAll } from '@ember/test-helpers'; import { AUTH_FORM } from 'vault/tests/helpers/auth/auth-form-selectors'; import { AUTH_METHOD_MAP } from 'vault/tests/helpers/auth/auth-helpers'; import { GENERAL } from 'vault/tests/helpers/general-selectors'; @@ -19,9 +19,10 @@ export default (test, { standardSubmit = true } = {}) => { test('it renders fields', async function (assert) { await this.renderComponent(); assert.dom(AUTH_FORM.authForm(this.authType)).exists(`${this.authType}: it renders form component`); - this.expectedFields.forEach((field) => { - assert.dom(GENERAL.inputByAttr(field)).exists(`${this.authType}: it renders ${field}`); - }); + const fields = findAll('input'); + for (const field of fields) { + assert.true(this.expectedFields.includes(field.name), `it renders field: ${field.name}`); + } }); test('it fires onError callback', async function (assert) { @@ -75,13 +76,7 @@ export default (test, { standardSubmit = true } = {}) => { for (const [field, value] of Object.entries(loginData)) { await fillIn(GENERAL.inputByAttr(field), value); } - - if (this.authType === 'token') { - // token doesn't support custom paths, so just test yielding functionality - await fillIn(GENERAL.inputByAttr('yield'), `yield-${this.authType}`); - } else { - await fillIn(GENERAL.inputByAttr('path'), `custom-${this.authType}`); - } + await fillIn(GENERAL.inputByAttr('path'), `custom-${this.authType}`); await click(AUTH_FORM.login); const [actual] = this.authenticateStub.lastCall.args;