Session permissions issue (#1975)

* refactor: 💡 session isCancelable to isAvailable

* refactor: 💡 check for target info for showing target details

* refactor: 💡 add fallback for session data from connect command

* refactor: 💡 remove extra divs

* refactor: 💡 remove await from peekRecord of a session

* test: 💍 add test for no read permission for sessions

* test: 💍 reset global qunit after test is done

addons/api/addon/models/session.js

@@ -128,7 +128,7 @@ export default class SessionModel extends GeneratedSessionModel {
    * @type {boolean}
    */
   @computed('isActive', 'isPending')
-  get isCancelable() {
+  get isAvailable() {
     return this.isActive || this.isPending;
   }
 

addons/api/tests/unit/models/session-test.js

@@ -13,7 +13,7 @@ module('Unit | Model | session', function (hooks) {
     let store = this.owner.lookup('service:store');
     let model = store.createRecord('session', {});
     assert.ok(model);
-    assert.notOk(model.isCancelable);
+    assert.notOk(model.isAvailable);
   });
 
   test('it allows cancellation of an active session', function (assert) {
@@ -21,7 +21,7 @@ module('Unit | Model | session', function (hooks) {
     let model = store.createRecord('session', {
       status: 'active',
     });
-    assert.ok(model.isCancelable);
+    assert.ok(model.isAvailable);
   });
 
   test('it allows cancellation of a pending session', function (assert) {
@@ -29,7 +29,7 @@ module('Unit | Model | session', function (hooks) {
     let model = store.createRecord('session', {
       status: 'pending',
     });
-    assert.ok(model.isCancelable);
+    assert.ok(model.isAvailable);
   });
 
   test('it computes isActive', function (assert) {

ui/admin/app/templates/scopes/scope/sessions/index.hbs

@@ -122,7 +122,7 @@
                 <SessionStatus @model={{sessionAggregate.session}} />
               </row.cell>
               <row.cell>
-                {{#if sessionAggregate.session.isCancelable}}
+                {{#if sessionAggregate.session.isAvailable}}
                   <Rose::Button
                     @style='secondary'
                     {{on

ui/desktop/app/components/session/tabs/index.hbs

@@ -37,45 +37,43 @@
       <bc.Sidebar>
         <div class='details-sidebar'>
           {{#if @model.host}}
-            <div>
-              <div
-                class='title hds-typography-display-200 hds-font-weight-semibold'
-              >
-                {{t 'resources.host.details'}}
-              </div>
-              <Rose::MetadataList @orientation='vertical' as |list|>
-                {{! Host name }}
-                {{#if @model.host.name}}
-                  <list.Item @icon='monitor' @color='faint'>
-                    {{@model.host.name}}
-                  </list.Item>
-                {{/if}}
-                {{! Host description }}
-                {{#if @model.host.description}}
-                  <list.Item @icon='message-circle-fill' @color='faint'>
-                    {{@model.host.description}}
-                  </list.Item>
-                {{/if}}
-                {{! Host address }}
-                {{#if @model.host.address}}
-                  <list.Item @icon='network-alt' @color='faint'>
-                    {{@model.host.address}}
-                  </list.Item>
-                {{/if}}
-                {{! Host ID }}
-                <list.Item @icon='hash' @color='faint'>
-                  <Copyable
-                    @text={{@model.host.id}}
-                    @buttonText={{t 'actions.copy-to-clipboard'}}
-                    @acknowledgeText={{t 'states.copied'}}
-                  >
-                    {{@model.host.id}}
-                  </Copyable>
-                </list.Item>
-              </Rose::MetadataList>
+            <div
+              class='title hds-typography-display-200 hds-font-weight-semibold'
+            >
+              {{t 'resources.host.details'}}
             </div>
+            <Rose::MetadataList @orientation='vertical' as |list|>
+              {{! Host name }}
+              {{#if @model.host.name}}
+                <list.Item @icon='monitor' @color='faint'>
+                  {{@model.host.name}}
+                </list.Item>
+              {{/if}}
+              {{! Host description }}
+              {{#if @model.host.description}}
+                <list.Item @icon='message-circle-fill' @color='faint'>
+                  {{@model.host.description}}
+                </list.Item>
+              {{/if}}
+              {{! Host address }}
+              {{#if @model.host.address}}
+                <list.Item @icon='network-alt' @color='faint'>
+                  {{@model.host.address}}
+                </list.Item>
+              {{/if}}
+              {{! Host ID }}
+              <list.Item @icon='hash' @color='faint'>
+                <Copyable
+                  @text={{@model.host.id}}
+                  @buttonText={{t 'actions.copy-to-clipboard'}}
+                  @acknowledgeText={{t 'states.copied'}}
+                >
+                  {{@model.host.id}}
+                </Copyable>
+              </list.Item>
+            </Rose::MetadataList>
           {{/if}}
-          <div>
+          {{#if @model.target}}
             <div
               class='title hds-typography-display-200 hds-font-weight-semibold'
             >
@@ -125,7 +123,7 @@
                 </Copyable>
               </list.Item>
             </Rose::MetadataList>
-          </div>
+          {{/if}}
         </div>
       </bc.Sidebar>
     </Rose::Layout::BodyContent>

ui/desktop/app/controllers/scopes/scope/projects/sessions/index.js

@@ -20,9 +20,9 @@ export default class ScopesScopeProjectsSessionsIndexController extends Controll
     const sortedSessions = sortBy(sessions, 'session.created_time').reverse();
     return [
       // then move active sessions to the top
-      ...sortedSessions.filter((session) => session.isCancelable),
+      ...sortedSessions.filter((session) => session.isAvailable),
       // and all others to the end
-      ...sortedSessions.filter((session) => !session.isCancelable),
+      ...sortedSessions.filter((session) => !session.isAvailable),
     ];
   }
 }

ui/desktop/app/controllers/scopes/scope/projects/targets/target/sessions.js

@@ -30,7 +30,7 @@ export default class ScopesScopeProjectsTargetsTargetSessionsController extends
     const sessions = this.sessions;
     const userId = this.session.data.authenticated.user_id;
     const filteredSessions = sessions.filter(
-      (session) => session.isCancelable && session.user_id === userId
+      (session) => session.isAvailable && session.user_id === userId
     );
 
     // Sort sessions

ui/desktop/app/routes/scopes/scope/projects/sessions/session.js

@@ -20,9 +20,7 @@ export default class ScopesScopeProjectsSessionsSessionRoute extends Route {
    * @return {SessionModel}
    */
   async model({ session_id }) {
-    const session = await this.store.findRecord('session', session_id, {
-      reload: true,
-    });
+    const session = await this.store.findRecord('session', session_id);
 
     /**
      * If the session has a host_id and the user has grants,

ui/desktop/app/routes/scopes/scope/projects/targets/target.js

@@ -37,9 +37,11 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route {
     });
 
     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
+      /**
+       * 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 }) =>
@@ -69,9 +71,11 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route {
   async afterModel(model, transition) {
     const { isConnecting } = transition.to.queryParams;
 
-    // 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 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);
     }
@@ -120,10 +124,12 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route {
   @action
   @loading
   async preConnect(model, toggleModal) {
-    // 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 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 {
@@ -156,14 +162,35 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route {
       const connectionDetails = await this.ipc.invoke('connect', options);
 
       // Associate the connection details with the session
+      let session;
       const { session_id, address, port, credentials } = connectionDetails;
-      const session = await this.store.findRecord('session', session_id);
+      try {
+        session = await this.store.findRecord('session', session_id);
+      } catch (error) {
+        /**
+         * if the user cannot read or fetch the session we add the important
+         * information returned from the connect command to allow the user
+         * to still continue their work with the information they need
+         */
+        this.store.pushPayload('session', {
+          sessions: [
+            {
+              id: session_id,
+              proxy_address: address,
+              proxy_port: port,
+            },
+          ],
+        });
+        session = this.store.peekRecord('session', session_id);
+      }
 
       // Flag the session has been open in the desktop client
       session.started_desktop_client = true;
-      // Update the session record with proxy information from the CLI
-      // In the future, it may make sense to push this off to the API so that
-      // we don't have to manually persist the proxy details.
+      /**
+       * Update the session record with proxy information from the CLI
+       * In the future, it may make sense to push this off to the API so that
+       * we don't have to manually persist the proxy details.
+       */
       session.proxy_address = address;
       session.proxy_port = port;
       if (credentials) {

ui/desktop/app/templates/scopes/scope/projects/sessions/index.hbs

@@ -130,7 +130,7 @@
         <:body as |B|>
           <B.Tr>
             <B.Td>
-              {{#if (can 'read session' B.data)}}
+              {{#if B.data.isAvailable}}
                 <Hds::Badge
                   @text={{t 'resources.session.title'}}
                   @icon='entry-point'
@@ -171,7 +171,7 @@
             {{!
                   Two if's here because:
                   1) Local proxy info is only relevant if the session is
-                     active or pending (aka isCancelable).  Once the session
+                     active or pending (aka isAvailable).  Once the session
                      is canceled or in the process thereof, the local proxy
                      may no longer be used.
                   2) The session actually has proxy information associated
@@ -180,7 +180,7 @@
                      won't have the local proxy info available here.
                 }}
             <B.Td>
-              {{#if B.data.isCancelable}}
+              {{#if B.data.isAvailable}}
                 {{#if B.data.proxy}}
                   <Copyable
                     @text={{B.data.proxy}}
@@ -201,7 +201,7 @@
               <SessionStatus @model={{B.data}} />
             </B.Td>
             <B.Td>
-              {{#if B.data.isCancelable}}
+              {{#if B.data.isAvailable}}
                 <Hds::Button
                   @text={{t 'actions.cancel'}}
                   @color='secondary'

ui/desktop/tests/acceptance/projects/sessions/session-test.js

@@ -3,6 +3,8 @@
  * SPDX-License-Identifier: MPL-2.0
  */
 
+/* global QUnit */
+
 import { module, test } from 'qunit';
 import { visit, currentURL, click } from '@ember/test-helpers';
 import { setupApplicationTest } from 'ember-qunit';
@@ -18,6 +20,8 @@ module('Acceptance | projects | sessions | session', function (hooks) {
   setupApplicationTest(hooks);
   setupMirage(hooks);
 
+  const TARGET_CONNECT_BUTTON = '[data-test-target-detail-connect-button]';
+
   const instances = {
     scopes: {
       global: null,
@@ -55,6 +59,8 @@ module('Acceptance | projects | sessions | session', function (hooks) {
     clusterUrl.rendererClusterUrl = windowOrigin;
   };
 
+  let originalUncaughtException = QUnit.onUncaughtException;
+
   hooks.beforeEach(function () {
     instances.user = this.server.create('user', {
       scope: instances.scopes.global,
@@ -126,6 +132,11 @@ module('Acceptance | projects | sessions | session', function (hooks) {
     assert.strictEqual(currentURL(), urls.session);
   });
 
+  hooks.afterEach(function () {
+    // reset onUncaughtException to original state
+    QUnit.onUncaughtException = originalUncaughtException;
+  });
+
   test('visiting session with no credentials', async function (assert) {
     assert.expect(4);
     stubs.ipcService.withArgs('cliExists').returns(true);
@@ -137,7 +148,7 @@ module('Acceptance | projects | sessions | session', function (hooks) {
     });
 
     await visit(urls.target);
-    await click('[data-test-target-detail-connect-button]');
+    await click(TARGET_CONNECT_BUTTON);
 
     assert.strictEqual(currentURL(), urls.session);
     assert.dom('credential-panel-header').doesNotExist();
@@ -160,7 +171,30 @@ module('Acceptance | projects | sessions | session', function (hooks) {
     });
 
     await visit(urls.target);
-    await click('[data-test-target-detail-connect-button]');
+    await click(TARGET_CONNECT_BUTTON);
+
+    assert.strictEqual(currentURL(), urls.session);
+  });
+
+  test('visiting a session that does not have read permissions but a successful connect', async function (assert) {
+    assert.expect(2);
+    // verifies second call to sessions/:id is also a 403
+    QUnit.onUncaughtException = (err) => {
+      assert.true(err.errors[0].isForbidden);
+    };
+
+    this.server.get('/sessions/: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(TARGET_CONNECT_BUTTON);
 
     assert.strictEqual(currentURL(), urls.session);
   });