From d2d332b7d1733ab3abe3a08c440685631baeefa3 Mon Sep 17 00:00:00 2001 From: Cameron Perera Date: Thu, 8 Feb 2024 10:01:20 -0600 Subject: [PATCH] =?UTF-8?q?refactor:=20=F0=9F=92=A1=20only=20show=20scopes?= =?UTF-8?q?=20with=20auth=20methods=20at=20authenticate=20(#2129)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: 💡 only show scopes with auth methods at authenticate ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 * test: 💍 verify scopes with no auth methods are hidden when authenticating ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 * refactor: 💡 update logic for fetching scopes and auth methods ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 * refactor: 💡 PR feedback changes ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 * refactor: 💡 added unless block back to template ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 * refactor: 💡 update admin authenticate route and template ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 * test: 💍 add test for orgs with no auth methods admin ui ✅ Closes: https://hashicorp.atlassian.net/browse/ICU-2355 --- .../app/routes/scopes/scope/authenticate.js | 42 ++++++++----------- .../templates/scopes/scope/authenticate.hbs | 13 ++++++ .../tests/acceptance/authentication-test.js | 17 ++++++++ .../app/routes/scopes/scope/authenticate.js | 38 ++++++++++++++--- .../templates/scopes/scope/authenticate.hbs | 1 - .../tests/acceptance/authentication-test.js | 22 ++++++---- 6 files changed, 93 insertions(+), 40 deletions(-) diff --git a/ui/admin/app/routes/scopes/scope/authenticate.js b/ui/admin/app/routes/scopes/scope/authenticate.js index 2e05fab47..4a211b2bd 100644 --- a/ui/admin/app/routes/scopes/scope/authenticate.js +++ b/ui/admin/app/routes/scopes/scope/authenticate.js @@ -5,7 +5,6 @@ import Route from '@ember/routing/route'; import { inject as service } from '@ember/service'; -import { hash } from 'rsvp'; export default class ScopesScopeAuthenticateRoute extends Route { // =services @@ -13,6 +12,7 @@ export default class ScopesScopeAuthenticateRoute extends Route { @service session; @service router; @service resourceFilterStore; + // =methods beforeModel() { @@ -27,16 +27,7 @@ export default class ScopesScopeAuthenticateRoute extends Route { */ async model() { const { id: scope_id } = this.modelFor('scopes.scope'); - // Fetch auth methods for the current scope - const authMethods = await this.resourceFilterStore.queryBy( - 'auth-method', - { - authorized_actions: [{ contains: 'authenticate' }], - }, - { - scope_id, - }, - ); + // Preload all authenticatable auth methods into the store const authMethodsForAllScopes = await this.resourceFilterStore.queryBy( 'auth-method', @@ -48,22 +39,25 @@ export default class ScopesScopeAuthenticateRoute extends Route { recursive: true, }, ); - // Fetch org scopes - // and filter out any that have no auth methods - const scopes = this.modelFor('scopes').filter( - ({ id: scope_id, isOrg }) => - isOrg && - authMethodsForAllScopes.filter((i) => i.scopeID === scope_id).length, + + const scopeIDs = new Set( + authMethodsForAllScopes.map((authMethod) => authMethod.scopeID), ); - return hash({ + + // Fetch org scopes and filter out any that have no auth methods + const scopes = this.modelFor('scopes').filter(({ id: scope_id }) => + scopeIDs.has(scope_id), + ); + + // Filter out auth methods that are not for the current scope + const authMethods = authMethodsForAllScopes.filter( + (authMethod) => authMethod.scopeID === scope_id, + ); + + return { scope: this.modelFor('scopes.scope'), scopes, authMethods, - // for integration testing: - // authMethods: A([{ - // id: 'am_1234567890', - // type: 'password' - // }]) - }); + }; } } diff --git a/ui/admin/app/templates/scopes/scope/authenticate.hbs b/ui/admin/app/templates/scopes/scope/authenticate.hbs index b3336a782..1592e2dd2 100644 --- a/ui/admin/app/templates/scopes/scope/authenticate.hbs +++ b/ui/admin/app/templates/scopes/scope/authenticate.hbs @@ -61,6 +61,19 @@ {{#if @model.authMethods}} {{outlet}} {{/if}} + {{#unless @model.authMethods}} + + + + {{t 'resources.auth-method.messages.none.description'}} + + + + {{/unless}} diff --git a/ui/admin/tests/acceptance/authentication-test.js b/ui/admin/tests/acceptance/authentication-test.js index 9a38c0634..abcd86446 100644 --- a/ui/admin/tests/acceptance/authentication-test.js +++ b/ui/admin/tests/acceptance/authentication-test.js @@ -76,6 +76,15 @@ module('Acceptance | authentication', function (hooks) { }, 'withChildren', ); + // create an emtpy org with no auth methods + this.server.create( + 'scope', + { + type: 'org', + scope: { id: globalScope.id, type: globalScope.type }, + }, + 'withChildren', + ); scope = { id: orgScope.id, type: orgScope.type }; globalAuthMethod = this.server.create('auth-method', { scope: globalScope, @@ -366,4 +375,12 @@ module('Acceptance | authentication', function (hooks) { }); // TODO: test OIDC retry and cancel + + test('org scopes with no auth methods are not visible in dropdown', async function (assert) { + await visit(authMethodGlobalAuthenticateURL); + + await click('.rose-dropdown-trigger'); + + assert.dom('.rose-dropdown-content a').exists({ count: 2 }); + }); }); diff --git a/ui/desktop/app/routes/scopes/scope/authenticate.js b/ui/desktop/app/routes/scopes/scope/authenticate.js index 878a07eb1..56dbe4cb1 100644 --- a/ui/desktop/app/routes/scopes/scope/authenticate.js +++ b/ui/desktop/app/routes/scopes/scope/authenticate.js @@ -5,7 +5,6 @@ import Route from '@ember/routing/route'; import { inject as service } from '@ember/service'; -import { hash } from 'rsvp'; export default class ScopesScopeAuthenticateRoute extends Route { // =services @@ -14,6 +13,7 @@ export default class ScopesScopeAuthenticateRoute extends Route { @service session; @service clusterUrl; @service router; + @service resourceFilterStore; // =methods @@ -28,14 +28,40 @@ export default class ScopesScopeAuthenticateRoute extends Route { * scope and all scopes (for org navigation). * @return {Promise} `{scope, scopes, authMethods}` */ - model() { + async model() { const { id: scope_id } = this.modelFor('scopes.scope'); - return hash({ + // Preload all authenticatable auth methods into the store + const authMethodsForAllScopes = await this.resourceFilterStore.queryBy( + 'auth-method', + { + authorized_actions: [{ contains: 'authenticate' }], + }, + { + scope_id: 'global', + recursive: true, + }, + ); + + const scopeIDs = new Set( + authMethodsForAllScopes.map((authMethod) => authMethod.scopeID), + ); + + // Fetch org scopes and filter out any that have no auth methods + const scopes = this.modelFor('scopes').filter(({ id: scope_id }) => + scopeIDs.has(scope_id), + ); + + // Filter out auth methods that are not for the current scope + const authMethods = authMethodsForAllScopes.filter( + (authMethod) => authMethod.scopeID === scope_id, + ); + + return { scope: this.modelFor('scopes.scope'), - scopes: this.modelFor('scopes').filter((scope) => scope.isOrg), - authMethods: this.store.query('auth-method', { scope_id }), - }); + scopes, + authMethods, + }; } redirect() { diff --git a/ui/desktop/app/templates/scopes/scope/authenticate.hbs b/ui/desktop/app/templates/scopes/scope/authenticate.hbs index 785ea1941..f758b2dec 100644 --- a/ui/desktop/app/templates/scopes/scope/authenticate.hbs +++ b/ui/desktop/app/templates/scopes/scope/authenticate.hbs @@ -70,6 +70,5 @@ {{/unless}} - \ No newline at end of file diff --git a/ui/desktop/tests/acceptance/authentication-test.js b/ui/desktop/tests/acceptance/authentication-test.js index 818c6d197..179ab7d22 100644 --- a/ui/desktop/tests/acceptance/authentication-test.js +++ b/ui/desktop/tests/acceptance/authentication-test.js @@ -31,6 +31,7 @@ module('Acceptance | authentication', function (hooks) { scopes: { global: null, org: null, + emptyOrg: null, project: null, }, authMethods: { @@ -92,6 +93,10 @@ module('Acceptance | authentication', function (hooks) { stubs.project = { id: instances.scopes.project.id, type: 'project' }; // create other resources + instances.scopes.emptyOrg = this.server.create('scope', { + type: 'org', + scope: stubs.global, + }); instances.authMethods.global = this.server.create('auth-method', { scope: instances.scopes.global, type: 'password', @@ -138,15 +143,6 @@ module('Acceptance | authentication', function (hooks) { assert.strictEqual(currentURL(), urls.authenticate.methods.global); }); - test('visiting authenticate route when there no methods shows a message', async function (assert) { - assert.expect(2); - instances.authMethods.global.destroy(); - await visit(urls.authenticate.global); - await a11yAudit(); - assert.strictEqual(currentURL(), urls.authenticate.global); - assert.ok(find('.rose-message')); - }); - test('visiting authenticate route without clusterUrl redirects to clusterUrl index', async function (assert) { assert.expect(1); this.owner.lookup('service:clusterUrl').rendererClusterUrl = null; @@ -255,4 +251,12 @@ module('Acceptance | authentication', function (hooks) { assert.notOk(getRootElement().classList.contains('rose-theme-light')); assert.notOk(getRootElement().classList.contains('rose-theme-dark')); }); + + test('org scopes with no auth methods are not visible in dropdown', async function (assert) { + await visit(urls.authenticate.methods.global); + + await click('.rose-dropdown-trigger'); + + assert.dom('.rose-dropdown-content a').exists({ count: 2 }); + }); });