From 3f8e39d0e18675e71ae5191de0d082f03d556675 Mon Sep 17 00:00:00 2001 From: Carlos Alcaide Corvo Date: Mon, 30 Oct 2023 18:54:37 +0100 Subject: [PATCH] Host set permission issue (#1968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: 💡 allow for graceful error handling when connecting to a target where host-sets cannot be fetched * refactor: 💡 update for loop to for...of loop * Move fetching host from target to session domain * Decouple getting target host-sets and getting hosts from the host-sets, now this code is extracted in 2 methods. Now if the user does not have permissions to fetch host or host-sets we do not show an error and we allow for quick-connect approach * Fix a check within getHostsFromHostSets * Change a for loop for Promise.all within getTargetHostSets * Change a for loop for Promise.All within getHostsFromHostSets * Delete unnecessary condition * Delete unnecessary condition * chore: 🤖 remove host sources from target mirage factory by default it uses address when using 'withAssociations' * test: 💍 update target address tests to use 'withOneHost' * test: 💍 remove connect tests from scopes acceptance tests * refactor: 💡 remove can initiate connect target ability and associated code and tests * refactor: 💡 update target model afterModel hook, and preconnect * refactor: 💡 revert 'withAssociations' mirage change * test: 💍 add tests for permissions issues for host-sets and host * test: 💍 visiting session without permissions to read hosts --------- Co-authored-by: cameronperera --- addons/core/translations/resources/en-us.yaml | 3 - ui/desktop/app/abilities/target.js | 16 ---- .../scopes/scope/projects/sessions/session.js | 19 ++++- .../scopes/scope/projects/targets/target.js | 58 +++++++------ .../scopes/scope/projects/targets/target.hbs | 45 ++++------ .../projects/sessions/session-test.js | 18 ++++ .../acceptance/projects/targets/index-test.js | 35 +++----- .../projects/targets/target-test.js | 85 ++++++++++++++----- ui/desktop/tests/acceptance/scopes-test.js | 49 ----------- .../tests/unit/abilities/target-test.js | 30 ------- 10 files changed, 157 insertions(+), 201 deletions(-) delete mode 100644 ui/desktop/app/abilities/target.js delete mode 100644 ui/desktop/tests/unit/abilities/target-test.js diff --git a/addons/core/translations/resources/en-us.yaml b/addons/core/translations/resources/en-us.yaml index 58874b299..7e3e775e7 100644 --- a/addons/core/translations/resources/en-us.yaml +++ b/addons/core/translations/resources/en-us.yaml @@ -551,9 +551,6 @@ target: connect-more-info: title: Connect for more info description: Additional information for this target will be provided here after your session is established. - cannot-connect: - title: Cannot connect to {targetDisplayName} - description: There are no host sources and no address configured for this target. titles: new: New Target actions: diff --git a/ui/desktop/app/abilities/target.js b/ui/desktop/app/abilities/target.js deleted file mode 100644 index 40c420212..000000000 --- a/ui/desktop/app/abilities/target.js +++ /dev/null @@ -1,16 +0,0 @@ -/** - * Copyright (c) HashiCorp, Inc. - * SPDX-License-Identifier: MPL-2.0 - */ - -import TargetAbility from 'api/abilities/target'; - -export default class OverrideTargetAbility extends TargetAbility { - /** - * - * @type {boolean} - */ - get canInitiateConnection() { - return this.model.address || this.model.host_sources?.length; - } -} diff --git a/ui/desktop/app/routes/scopes/scope/projects/sessions/session.js b/ui/desktop/app/routes/scopes/scope/projects/sessions/session.js index fa8236465..5fd899689 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/sessions/session.js +++ b/ui/desktop/app/routes/scopes/scope/projects/sessions/session.js @@ -19,9 +19,24 @@ export default class ScopesScopeProjectsSessionsSessionRoute extends Route { * @param {string} params.session_id * @return {SessionModel} */ - model({ session_id }) { - return this.store.findRecord('session', session_id, { + async model({ session_id }) { + const session = await this.store.findRecord('session', session_id, { reload: true, }); + + /** + * If the session has a host_id and the user has grants, + * we retrieve the host and aggregate it to the session model. + */ + if (session.host_id) { + try { + const host = await this.store.findRecord('host', session.host_id); + session.addHost(host); + } catch (error) { + // no operation + } + } + + return session; } } diff --git a/ui/desktop/app/routes/scopes/scope/projects/targets/target.js b/ui/desktop/app/routes/scopes/scope/projects/targets/target.js index c4913e7ea..fae406ea9 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/targets/target.js +++ b/ui/desktop/app/routes/scopes/scope/projects/targets/target.js @@ -36,24 +36,31 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route { reload: true, }); - if (target.host_sources) { - const hostSets = await Promise.all( - target.host_sources.map(({ host_source_id }) => - this.store.findRecord('host-set', host_source_id) - ) - ); + if (target.host_sources.length >= 1) { + // if user does not have permissions to fetch host-sets or hosts + // this will catch the error and return an empty array + // from the model hook for hosts + try { + const hostSets = await Promise.all( + target.host_sources.map(({ host_source_id }) => + this.store.findRecord('host-set', host_source_id) + ) + ); - // Extract host ids from all host sets - const hostIds = hostSets.flatMap(({ host_ids }) => host_ids); + // Extract host ids from all host sets + const hostIds = hostSets.flatMap(({ host_ids }) => host_ids); - // Load unique hosts - const uniqueHostIds = new Set(hostIds); + // Load unique hosts + const uniqueHostIds = new Set(hostIds); - hosts = await Promise.all( - [...uniqueHostIds].map((hostId) => - this.store.findRecord('host', hostId) - ) - ); + hosts = await Promise.all( + [...uniqueHostIds].map((hostId) => + this.store.findRecord('host', hostId) + ) + ); + } catch (error) { + // no operation + } } return { target, hosts }; @@ -62,10 +69,11 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route { async afterModel(model, transition) { const { isConnecting } = transition.to.queryParams; - if (isConnecting) { - if (model.target.address || model.hosts.length === 1) { - await this.connect(model.target); - } + // if connecting and hosts length is 1 or less we will try to + // connect, even if there is no address on the target and + // rely on the CLI to give the user the proper error + if (isConnecting && model.hosts.length <= 1) { + await this.connect(model.target); } } @@ -112,7 +120,11 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route { @action @loading async preConnect(model, toggleModal) { - if (model.target.address || model.hosts.length === 1) { + // if hosts length is 1 or less we will try to + // connect, even if there is no address on the target and + // rely on the CLI to give the user the proper error or if + // there are 2 or more hosts we show the modal for host selection + if (model.hosts.length <= 1) { await this.connect(model.target); } else { toggleModal(true); @@ -157,14 +169,10 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route { if (credentials) { credentials.forEach((cred) => session.addCredential(cred)); } - if (session.host_id) { - const host = await this.store.findRecord('host', session.host_id); - session.addHost(host); - } await this.router.transitionTo( 'scopes.scope.projects.sessions.session', - session + session_id ); } catch (e) { this.isConnectionError = true; diff --git a/ui/desktop/app/templates/scopes/scope/projects/targets/target.hbs b/ui/desktop/app/templates/scopes/scope/projects/targets/target.hbs index 89d953afa..c4cb8819c 100644 --- a/ui/desktop/app/templates/scopes/scope/projects/targets/target.hbs +++ b/ui/desktop/app/templates/scopes/scope/projects/targets/target.hbs @@ -21,7 +21,6 @@ @text={{t 'resources.session.actions.connect'}} @icon='entry-point' @iconPosition='trailing' - disabled={{not (can 'initiateConnection target' @model.target)}} {{on 'click' (route-action 'preConnect' @model this.toggleModal)}} /> {{/if}} @@ -29,37 +28,23 @@ - {{#if (can 'initiateConnection target' @model.target)}} - - {{/if}} + - {{#if (can 'initiateConnection target' @model.target)}} - - - {{else}} - - - {{/if}} + + diff --git a/ui/desktop/tests/acceptance/projects/sessions/session-test.js b/ui/desktop/tests/acceptance/projects/sessions/session-test.js index 7761a0d6e..ee7f408f6 100644 --- a/ui/desktop/tests/acceptance/projects/sessions/session-test.js +++ b/ui/desktop/tests/acceptance/projects/sessions/session-test.js @@ -147,6 +147,24 @@ module('Acceptance | projects | sessions | session', function (hooks) { .hasText(`Connected You can now access ${instances.target.name}`); }); + test('visiting a session that does not have permissions to read a host', async function (assert) { + assert.expect(1); + this.server.get('/hosts/:id', () => new Response(403)); + stubs.ipcService.withArgs('cliExists').returns(true); + stubs.ipcService.withArgs('connect').returns({ + session_id: instances.session.id, + host_id: 'h_123', + address: 'a_123', + port: 'p_123', + protocol: 'tcp', + }); + + await visit(urls.target); + await click('[data-test-target-detail-connect-button]'); + + assert.strictEqual(currentURL(), urls.session); + }); + test('cancelling a session shows success alert', async function (assert) { assert.expect(1); stubs.ipcService.withArgs('stop'); diff --git a/ui/desktop/tests/acceptance/projects/targets/index-test.js b/ui/desktop/tests/acceptance/projects/targets/index-test.js index 9cdce1bef..19e991ea1 100644 --- a/ui/desktop/tests/acceptance/projects/targets/index-test.js +++ b/ui/desktop/tests/acceptance/projects/targets/index-test.js @@ -29,7 +29,6 @@ module('Acceptance | projects | targets', function (hooks) { let getTargetCount; const ROSE_APP_STATE_TITLE = '.rose-message-title'; - const APP_STATE_TITLE = '.hds-application-state__title'; const TARGET_DETAILS_ROUTE_NAME = 'scopes.scope.projects.targets.target.index'; const ROSE_DIALOG_MODAL = '.rose-dialog-error'; @@ -103,14 +102,10 @@ module('Acceptance | projects | targets', function (hooks) { { scope: instances.scopes.project }, 'withChildren' ); - instances.target = this.server.create( - 'target', - { - scope: instances.scopes.project, - address: '127.0.0.1', - }, - 'withAssociations' - ); + instances.target = this.server.create('target', { + scope: instances.scopes.project, + address: '127.0.0.1', + }); instances.session = this.server.create( 'session', { @@ -231,28 +226,18 @@ module('Acceptance | projects | targets', function (hooks) { }); test('user is redirected to target details page when unable to connect from list view', async function (assert) { - assert.expect(2); - instances.target.address = ''; - instances.target.update({ address: '', hostSets: [] }); - await visit(urls.projects); - - await click(`[href="${urls.targets}"]`); - await click(`[data-test-targets-connect-button="${instances.target.id}"]`); - - assert.strictEqual(currentRouteName(), TARGET_DETAILS_ROUTE_NAME); - assert.dom(APP_STATE_TITLE).includesText('Cannot connect'); - }); - - test('choose host modal does not render when there is a connect error', async function (assert) { - assert.expect(2); + assert.expect(3); + stubs.ipcService.withArgs('cliExists').returns(true); + stubs.ipcService.withArgs('connect').rejects(); const confirmService = this.owner.lookup('service:confirm'); confirmService.enabled = true; - await visit(urls.projects); - await click(`[href="${urls.targets}"]`); + await visit(urls.targets); + await click(`[data-test-targets-connect-button="${instances.target.id}"]`); assert.dom(ROSE_DIALOG_MODAL).exists(); assert.dom(CHOOSE_HOST_MODAL).doesNotExist(); + assert.strictEqual(currentRouteName(), TARGET_DETAILS_ROUTE_NAME); }); }); diff --git a/ui/desktop/tests/acceptance/projects/targets/target-test.js b/ui/desktop/tests/acceptance/projects/targets/target-test.js index 8d3750273..96f0447bf 100644 --- a/ui/desktop/tests/acceptance/projects/targets/target-test.js +++ b/ui/desktop/tests/acceptance/projects/targets/target-test.js @@ -4,7 +4,7 @@ */ import { module, test } from 'qunit'; -import { visit, click, find } from '@ember/test-helpers'; +import { visit, currentURL, click, find } from '@ember/test-helpers'; import { setupApplicationTest } from 'ember-qunit'; import setupMirage from 'ember-cli-mirage/test-support/setup-mirage'; import sinon from 'sinon'; @@ -91,14 +91,10 @@ module('Acceptance | projects | targets | target', function (hooks) { { scope: instances.scopes.project }, 'withChildren' ); - instances.target = this.server.create( - 'target', - { - scope: instances.scopes.project, - address: 'localhost', - }, - 'withAssociations' - ); + instances.target = this.server.create('target', { + scope: instances.scopes.project, + address: 'localhost', + }); instances.targetWithOneHost = this.server.create( 'target', { scope: instances.scopes.project }, @@ -140,13 +136,12 @@ module('Acceptance | projects | targets | target', function (hooks) { stubs.ipcService.withArgs('cliExists').returns(true); stubs.ipcService.withArgs('connect').returns({ session_id: instances.session.id, - address: 'a_123', + address: 'localhost', port: 'p_123', protocol: 'tcp', }); - await visit(urls.targets); - await click(`[href="${urls.target}"]`); + await visit(urls.target); assert.dom(TARGET_CONNECT_BUTTON).isEnabled(); assert.dom(APP_STATE_TITLE).includesText('Connect for more info'); @@ -194,9 +189,9 @@ module('Acceptance | projects | targets | target', function (hooks) { stubs.ipcService.withArgs('cliExists').rejects(); const confirmService = this.owner.lookup('service:confirm'); confirmService.enabled = true; - await visit(urls.targets); - await click(`[href="${urls.target}"]`); + await visit(urls.target); + await click(TARGET_CONNECT_BUTTON); const firstErrorDialog = find(ROSE_DIALOG_MODAL); await click(ROSE_DIALOG_RETRY_BUTTON, 'Retry'); @@ -249,15 +244,63 @@ module('Acceptance | projects | targets | target', function (hooks) { assert.dom(APP_STATE_TITLE).hasText('Connected'); }); - test('user cannot connect to a target without an address and no host sources', async function (assert) { - assert.expect(2); - instances.target.address = ''; - instances.target.update({ address: '', hostSets: [] }); + test('user can visit target details screen without read permissions for host-set', async function (assert) { + assert.expect(1); + this.server.get('/host-sets/:id', () => new Response(403)); + await visit(urls.targets); - await click(`[href="${urls.target}"]`); + await click(`[href="${urls.targetWithOneHost}"]`); - assert.dom(TARGET_CONNECT_BUTTON).isDisabled(); - assert.dom(APP_STATE_TITLE).includesText('Cannot connect'); + assert.strictEqual(currentURL(), urls.targetWithOneHost); + }); + + test('user can visit target details screen without read permissions for host', async function (assert) { + assert.expect(1); + this.server.get('/hosts/:id', () => new Response(403)); + + await visit(urls.targets); + + await click(`[href="${urls.targetWithOneHost}"]`); + + assert.strictEqual(currentURL(), urls.targetWithOneHost); + }); + + test('user can connect to a target without read permissions for host-set', async function (assert) { + assert.expect(1); + this.server.get('/host-sets/:id', () => new Response(403)); + stubs.ipcService.withArgs('cliExists').returns(true); + stubs.ipcService.withArgs('connect').returns({ + session_id: instances.session.id, + address: 'a_123', + port: 'p_123', + protocol: 'tcp', + }); + + await visit(urls.targets); + + await click(`[href="${urls.targetWithOneHost}"]`); + await click(TARGET_CONNECT_BUTTON); + + assert.dom(APP_STATE_TITLE).hasText('Connected'); + }); + + test('user can connect to a target without read permissions for host', async function (assert) { + assert.expect(1); + this.server.get('/hosts/:id', () => new Response(403)); + stubs.ipcService.withArgs('cliExists').returns(true); + stubs.ipcService.withArgs('connect').returns({ + session_id: instances.session.id, + address: 'a_123', + port: 'p_123', + protocol: 'tcp', + }); + + await visit(urls.targets); + + await click(`[href="${urls.targetWithOneHost}"]`); + await click(TARGET_CONNECT_BUTTON); + + assert.dom(APP_STATE_TITLE).hasText('Connected'); }); }); diff --git a/ui/desktop/tests/acceptance/scopes-test.js b/ui/desktop/tests/acceptance/scopes-test.js index 9dac8462c..43fcf585c 100644 --- a/ui/desktop/tests/acceptance/scopes-test.js +++ b/ui/desktop/tests/acceptance/scopes-test.js @@ -282,53 +282,4 @@ module('Acceptance | scopes', function (hooks) { 'Cannot retry' ); }); - - test('handles cli error on connect', async function (assert) { - assert.expect(4); - stubs.ipcService.withArgs('cliExists').returns(false); - const confirmService = this.owner.lookup('service:confirm'); - confirmService.enabled = true; - - await visit(urls.scopes.global); - await click('[data-test-targets-connect-button]'); - - assert.ok(find('.rose-dialog-error'), 'Error dialog'); - const dialogButtons = findAll('.rose-dialog-footer button'); - assert.strictEqual(dialogButtons.length, 2); - assert.strictEqual( - dialogButtons[0].textContent.trim(), - 'Retry', - 'Can retry' - ); - assert.strictEqual( - dialogButtons[1].textContent.trim(), - 'Cancel', - 'Can cancel' - ); - }); - - test('handles connect error', async function (assert) { - assert.expect(4); - stubs.ipcService.withArgs('cliExists').returns(true); - stubs.ipcService.withArgs('connect').rejects(); - const confirmService = this.owner.lookup('service:confirm'); - confirmService.enabled = true; - - await visit(urls.targets); - await click('[data-test-targets-connect-button]'); - - assert.ok(find('.rose-dialog-error'), 'Error dialog'); - const dialogButtons = findAll('.rose-dialog-footer button'); - assert.strictEqual(dialogButtons.length, 2); - assert.strictEqual( - dialogButtons[0].textContent.trim(), - 'Retry', - 'Can retry' - ); - assert.strictEqual( - dialogButtons[1].textContent.trim(), - 'Cancel', - 'Can cancel' - ); - }); }); diff --git a/ui/desktop/tests/unit/abilities/target-test.js b/ui/desktop/tests/unit/abilities/target-test.js deleted file mode 100644 index 2299590ab..000000000 --- a/ui/desktop/tests/unit/abilities/target-test.js +++ /dev/null @@ -1,30 +0,0 @@ -/** - * Copyright (c) HashiCorp, Inc. - * SPDX-License-Identifier: MPL-2.0 - */ - -import { module, test } from 'qunit'; -import { setupTest } from 'ember-qunit'; - -module('Unit | Abilities | target', function (hooks) { - setupTest(hooks); - - test('it reflects when a target can initiate connection or not', function (assert) { - assert.expect(5); - const service = this.owner.lookup('service:can'); - const model = { - address: '', - }; - assert.false(service.can('initiateConnection target', model)); - model.host_sources = []; - assert.false(service.can('initiateConnection target', model)); - model.address = 'localhost'; - assert.true(service.can('initiateConnection target', model)); - model.address = ''; - model.host_sources = ['host_123']; - assert.true(service.can('initiateConnection target', model)); - model.address = 'localhost'; - model.host_sources = ['host_123']; - assert.true(service.can('initiateConnection target', model)); - }); -});