Host set permission issue (#1968)

* 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 <cameron.perera@hashicorp.com>
2023-10-30 12:54:37 -05:00
1 parent 9e7852f18d
commit 3f8e39d0e1
@@ -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:
@@ -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;
-  }
-}
@@ -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;
  }
 }
@@ -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;
@@ -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 @@

  <page.body class='details-screen-body'>
    <Rose::Layout::BodyContent as |bc|>
-      {{#if (can 'initiateConnection target' @model.target)}}
-        <Target::HostModal
-          @showModal={{this.isConnecting}}
-          @toggleModal={{this.toggleModal}}
-          @connect={{route-action 'hostConnect'}}
-          @hosts={{@model.hosts}}
-          @target={{@model.target}}
-        />
-      {{/if}}
+      <Target::HostModal
+        @showModal={{this.isConnecting}}
+        @toggleModal={{this.toggleModal}}
+        @connect={{route-action 'hostConnect'}}
+        @hosts={{@model.hosts}}
+        @target={{@model.target}}
+      />
      <bc.Body class='target-details-body'>
        <Hds::ApplicationState as |A|>
-          {{#if (can 'initiateConnection target' @model.target)}}
-            <A.Header
-              @title={{t 'resources.target.messages.connect-more-info.title'}}
-            />
-            <A.Body
-              @text={{t
-                'resources.target.messages.connect-more-info.description'
-              }}
-            />
-          {{else}}
-            <A.Header
-              @title={{t
-                'resources.target.messages.cannot-connect.title'
-                targetDisplayName=@model.target.displayName
-              }}
-            />
-            <A.Body
-              @text={{t 'resources.target.messages.cannot-connect.description'}}
-            />
-          {{/if}}
+          <A.Header
+            @title={{t 'resources.target.messages.connect-more-info.title'}}
+          />
+          <A.Body
+            @text={{t
+              'resources.target.messages.connect-more-info.description'
+            }}
+          />
        </Hds::ApplicationState>
      </bc.Body>
      <bc.Sidebar>
@@ -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');
@@ -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);
  });
 });
@@ -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');
  });
 });
@@ -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'
-    );
-  });
 });
@@ -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));
-  });
-});