From 1afe8af15adef2a919417c4ad449c62a3300ead3 Mon Sep 17 00:00:00 2001
From: Randall Morey <randall@randallmorey.com>
Date: Wed, 17 May 2023 12:24:31 -0700
Subject: [PATCH] Session recording and storage bucket abilities cleanup (#256)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* refactor: 💡 cleanup session recording abilities

* refactor: 💡 use super call instead of duplicating check

* refactor: 💡 cleanup storage bucket abilities
---
 .../api/addon/abilities/session-recording.js  |  9 +--
 addons/api/addon/abilities/storage-bucket.js  | 22 +++---
 ui/admin/app/abilities/scope.js               | 44 +++++++++++
 ui/admin/app/abilities/session-recording.js   | 14 ----
 ui/admin/app/abilities/storage-bucket.js      | 18 +++--
 .../routes/scopes/scope/session-recordings.js |  2 +-
 .../routes/scopes/scope/storage-buckets.js    |  2 +-
 ui/admin/app/templates/scopes/scope.hbs       | 12 +--
 .../scopes/scope/storage-buckets/index.hbs    | 73 ++++++++++---------
 ui/admin/tests/e2e/helpers/boundary-ui.js     |  8 +-
 .../unit/abilities/session-recording-test.js  | 68 ++++++-----------
 .../unit/abilities/storage-bucket-test.js     | 32 ++++----
 12 files changed, 153 insertions(+), 151 deletions(-)
 create mode 100644 ui/admin/app/abilities/scope.js

diff --git a/addons/api/addon/abilities/session-recording.js b/addons/api/addon/abilities/session-recording.js
index 13d2fc4bb..102d48aa6 100644
--- a/addons/api/addon/abilities/session-recording.js
+++ b/addons/api/addon/abilities/session-recording.js
@@ -18,14 +18,7 @@ export default class SessionRecordingAbility extends ModelAbility {
    * @type {boolean}
    */
   get canRead() {
-    return !this.model.isUnknown && this.hasAuthorizedAction('read');
-  }
-
-  get canList() {
-    return (
-      this.hasAuthorizedCollectionAction('list') &&
-      (this.model.isGlobal || this.model.isOrg)
-    );
+    return !this.model.isUnknown && super.canRead;
   }
 
   get canDownload() {
diff --git a/addons/api/addon/abilities/storage-bucket.js b/addons/api/addon/abilities/storage-bucket.js
index d521b8ad4..113519b67 100644
--- a/addons/api/addon/abilities/storage-bucket.js
+++ b/addons/api/addon/abilities/storage-bucket.js
@@ -18,24 +18,22 @@ export default class StorageBucketAbility extends ModelAbility {
    * @type {boolean}
    */
   get canRead() {
-    return !this.model.isUnknown && this.hasAuthorizedAction('read');
+    return !this.model.isUnknown && super.canRead;
   }
 
   /**
-   * Storage buckets can only exist within global and org scopes.
+   * Only "known" storage bucket types may be updated.
    * @type {boolean}
    */
-  get canCreate() {
-    return (
-      this.hasAuthorizedCollectionAction('create') &&
-      (this.model.isGlobal || this.model.isOrg)
-    );
+  get canUpdate() {
+    return !this.model.isUnknown && super.canUpdate;
   }
 
-  get canList() {
-    return (
-      this.hasAuthorizedCollectionAction('list') &&
-      (this.model.isGlobal || this.model.isOrg)
-    );
+  /**
+   * Only "known" storage bucket types may be deleted.
+   * @type {boolean}
+   */
+  get canDelete() {
+    return !this.model.isUnknown && super.canDelete;
   }
 }
diff --git a/ui/admin/app/abilities/scope.js b/ui/admin/app/abilities/scope.js
new file mode 100644
index 000000000..f4d269d77
--- /dev/null
+++ b/ui/admin/app/abilities/scope.js
@@ -0,0 +1,44 @@
+/**
+ * Copyright (c) HashiCorp, Inc.
+ * SPDX-License-Identifier: MPL-2.0
+ */
+
+import OverrideModelAbility from './model';
+import { inject as service } from '@ember/service';
+
+export default class OverrideScopeAbility extends OverrideModelAbility {
+  // =services
+
+  @service features;
+
+  // =attributes
+
+  /**
+   * Creating a resource is allowed only if a create grant is present
+   * and collection-specific criteria are met.
+   * @type {boolean}
+   */
+  get canCreate() {
+    switch (this.collection) {
+      case 'storage-buckets':
+        return this.features.isEnabled('session-recording') && super.canCreate;
+      default:
+        return super.canCreate;
+    }
+  }
+
+  /**
+   * Listing a resource is allowed only if a list grant is present
+   * and collection-specific criteria are met.
+   * @type {boolean}
+   */
+  get canList() {
+    switch (this.collection) {
+      case 'session-recordings':
+      case 'storage-buckets':
+        return this.features.isEnabled('session-recording') && super.canList;
+      default:
+        return super.canList;
+    }
+  }
+}
diff --git a/ui/admin/app/abilities/session-recording.js b/ui/admin/app/abilities/session-recording.js
index 346c97dcd..60664eb26 100644
--- a/ui/admin/app/abilities/session-recording.js
+++ b/ui/admin/app/abilities/session-recording.js
@@ -17,18 +17,4 @@ export default class OverrideSessionRecordingAbility extends SessionRecordingAbi
   get canRead() {
     return this.features.isEnabled('session-recording') ? super.canRead : false;
   }
-
-  /**
-   * This override ensures that session recordings can only be presented in the global scope
-   * and if the session-recording feature flag is enabled.
-   */
-  get canList() {
-    return this.features.isEnabled('session-recording')
-      ? this.hasAuthorizedCollectionAction('list') && this.model.isGlobal
-      : false;
-  }
-
-  get canNavigate() {
-    return this.canList;
-  }
 }
diff --git a/ui/admin/app/abilities/storage-bucket.js b/ui/admin/app/abilities/storage-bucket.js
index c8fae92d6..fb2ed3530 100644
--- a/ui/admin/app/abilities/storage-bucket.js
+++ b/ui/admin/app/abilities/storage-bucket.js
@@ -19,16 +19,22 @@ export default class OverrideStorageBucketAbility extends StorageBucketAbility {
   }
 
   /**
-   * This override ensures that storage buckets can only be presented in the global scope
-   * and if the session-recording feature flag is enabled.
+   * This override ensures that storage buckets may be updated only if the
+   * session-recording feature flag is enabled.
    */
-  get canList() {
+  get canUpdate() {
     return this.features.isEnabled('session-recording')
-      ? this.hasAuthorizedCollectionAction('list') && this.model.isGlobal
+      ? super.canUpdate
       : false;
   }
 
-  get canNavigate() {
-    return this.canCreate || this.canList;
+  /**
+   * This override ensures that storage buckets may be deleted only if the
+   * session-recording feature flag is enabled.
+   */
+  get canDelete() {
+    return this.features.isEnabled('session-recording')
+      ? super.canDelete
+      : false;
   }
 }
diff --git a/ui/admin/app/routes/scopes/scope/session-recordings.js b/ui/admin/app/routes/scopes/scope/session-recordings.js
index 03dc40ed5..9b2918599 100644
--- a/ui/admin/app/routes/scopes/scope/session-recordings.js
+++ b/ui/admin/app/routes/scopes/scope/session-recordings.js
@@ -29,7 +29,7 @@ export default class ScopesScopeSessionRecordingsRoute extends Route {
     const { id: scope_id } = scope;
 
     if (
-      this.can.can('list session-recording', scope, {
+      this.can.can('list scope', scope, {
         collection: 'session-recordings',
       })
     ) {
diff --git a/ui/admin/app/routes/scopes/scope/storage-buckets.js b/ui/admin/app/routes/scopes/scope/storage-buckets.js
index eee836051..730cb6a26 100644
--- a/ui/admin/app/routes/scopes/scope/storage-buckets.js
+++ b/ui/admin/app/routes/scopes/scope/storage-buckets.js
@@ -35,7 +35,7 @@ export default class ScopesScopeStorageBucketsRoute extends Route {
     const scope = this.modelFor('scopes.scope');
     const { id: scope_id } = scope;
     if (
-      this.can.can('list storage-bucket', scope, {
+      this.can.can('list scope', scope, {
         collection: 'storage-buckets',
       })
     ) {
diff --git a/ui/admin/app/templates/scopes/scope.hbs b/ui/admin/app/templates/scopes/scope.hbs
index 64feeb900..e2e9cd27a 100644
--- a/ui/admin/app/templates/scopes/scope.hbs
+++ b/ui/admin/app/templates/scopes/scope.hbs
@@ -28,22 +28,14 @@
           </nav.link>
           {{#if (feature-flag 'session-recording')}}
             {{#if
-              (can
-                'navigate session-recording'
-                @model
-                collection='session-recordings'
-              )
+              (can 'navigate scope' @model collection='session-recordings')
             }}
               <nav.link @route='scopes.scope.session-recordings'>
                 <Rose::Icon @name='flight-icons/svg/film-16' @size='medium' />
                 {{t 'resources.session-recording.title_plural'}}
               </nav.link>
             {{/if}}
-            {{#if
-              (can
-                'navigate storage-bucket' @model collection='storage-buckets'
-              )
-            }}
+            {{#if (can 'navigate scope' @model collection='storage-buckets')}}
               <nav.link @route='scopes.scope.storage-buckets'>
                 <Rose::Icon
                   @name='flight-icons/svg/cloud-upload-16'
diff --git a/ui/admin/app/templates/scopes/scope/storage-buckets/index.hbs b/ui/admin/app/templates/scopes/scope/storage-buckets/index.hbs
index 261566ca4..b402c51d1 100644
--- a/ui/admin/app/templates/scopes/scope/storage-buckets/index.hbs
+++ b/ui/admin/app/templates/scopes/scope/storage-buckets/index.hbs
@@ -17,9 +17,7 @@
   </page.header>
 
   <page.actions>
-    {{#if
-      (can 'create storage-bucket' this.scope collection='storage-buckets')
-    }}
+    {{#if (can 'create scope' this.scope collection='storage-buckets')}}
       {{#if @model}}
         <Hds::Button
           @route='scopes.scope.storage-buckets.new'
@@ -89,28 +87,41 @@
               {{/if}}
             </B.Td>
             <B.Td>
-              {{!TODO: add dropdown for delete actions}}
-              <Hds::Dropdown as |dd|>
-                <dd.ToggleIcon
-                  @icon='more-horizontal'
-                  @text='Overflow Options'
-                  @hasChevron={{false}}
-                  @size='small'
-                />
-                <dd.Interactive
-                  @route='scopes.scope.storage-buckets.storage-bucket'
-                  @model={{B.data.id}}
-                  @text={{t 'actions.edit'}}
-                />
-                {{#if (can 'delete model' B.data)}}
-                  <dd.Separator />
-                  <dd.Interactive
-                    @text={{t 'resources.storage-bucket.actions.delete'}}
-                    @color='critical'
-                    {{on 'click' (route-action 'delete' B.data)}}
-                  />
+              {{#let
+                (hash
+                  canRead=(can 'read storage-bucket' B.data)
+                  canDelete=(can 'delete storage-bucket' B.data)
+                )
+                as |perms|
+              }}
+                {{#if (or perms.canRead perms.canDelete)}}
+                  <Hds::Dropdown as |dd|>
+                    <dd.ToggleIcon
+                      @icon='more-horizontal'
+                      @text='Overflow Options'
+                      @hasChevron={{false}}
+                      @size='small'
+                    />
+                    {{#if perms.canRead}}
+                      <dd.Interactive
+                        @route='scopes.scope.storage-buckets.storage-bucket'
+                        @model={{B.data.id}}
+                        @text={{t 'actions.edit'}}
+                      />
+                    {{/if}}
+                    {{#if (and perms.canRead perms.canDelete)}}
+                      <dd.Separator />
+                    {{/if}}
+                    {{#if perms.canDelete}}
+                      <dd.Interactive
+                        @text={{t 'resources.storage-bucket.actions.delete'}}
+                        @color='critical'
+                        {{on 'click' (route-action 'delete' B.data)}}
+                      />
+                    {{/if}}
+                  </Hds::Dropdown>
                 {{/if}}
-              </Hds::Dropdown>
+              {{/let}}
             </B.Td>
           </B.Tr>
         </:body>
@@ -122,14 +133,10 @@
           as |message|
         >
           <message.description>
-            {{#if
-              (can
-                'list storage-bucket' this.scope collection='storage-buckets'
-              )
-            }}
+            {{#if (can 'list scope' this.scope collection='storage-buckets')}}
               {{t 'resources.storage-bucket.messages.none.description'}}
             {{else if (can
-              'create storage-bucket' this.scope collection='storage-buckets'
+              'create scope' this.scope collection='storage-buckets'
             )}}
               {{t
                 'descriptions.create-but-not-list'
@@ -142,11 +149,7 @@
               }}
             {{/if}}
           </message.description>
-          {{#if
-            (can
-              'create storage-bucket' this.scope collection='storage-buckets'
-            )
-          }}
+          {{#if (can 'create scope' this.scope collection='storage-buckets')}}
             <Hds::Link::Standalone
               @icon='plus-circle'
               @iconPosition='leading'
diff --git a/ui/admin/tests/e2e/helpers/boundary-ui.js b/ui/admin/tests/e2e/helpers/boundary-ui.js
index 5a2ea9ba2..deba9aaf0 100644
--- a/ui/admin/tests/e2e/helpers/boundary-ui.js
+++ b/ui/admin/tests/e2e/helpers/boundary-ui.js
@@ -256,7 +256,11 @@ exports.waitForSessionToBeVisible = async (page, targetName) => {
  * @param {string} targetName Name of the target associated with the session
  * @param {string} credentialName Name of the credentials to be added to the target
  */
-exports.addBrokeredCredentialsToTarget = async (page, targetName, credentialName) => {
+exports.addBrokeredCredentialsToTarget = async (
+  page,
+  targetName,
+  credentialName
+) => {
   await page
     .getByRole('navigation', { name: 'Resources' })
     .getByRole('link', { name: 'Targets' })
@@ -282,4 +286,4 @@ exports.addBrokeredCredentialsToTarget = async (page, targetName, credentialName
   ).toBeVisible();
   await page.getByRole('button', { name: 'Dismiss' }).click();
   await expect(page.getByRole('link', { name: credentialName })).toBeVisible();
-}
+};
diff --git a/ui/admin/tests/unit/abilities/session-recording-test.js b/ui/admin/tests/unit/abilities/session-recording-test.js
index fd4f49b4f..7116cb21b 100644
--- a/ui/admin/tests/unit/abilities/session-recording-test.js
+++ b/ui/admin/tests/unit/abilities/session-recording-test.js
@@ -89,22 +89,18 @@ module('Unit | Abilities | session-recording', function (hooks) {
     });
 
     assert.true(
-      canService.can('list session-recording', scopeModelWithAuthorizedAction, {
+      canService.can('list scope', scopeModelWithAuthorizedAction, {
         collection: 'session-recordings',
       })
     );
     assert.false(
-      canService.can(
-        'list session-recording',
-        scopeModelWithoutAuthorizedAction,
-        {
-          collection: 'session-recordings',
-        }
-      )
+      canService.can('list scope', scopeModelWithoutAuthorizedAction, {
+        collection: 'session-recordings',
+      })
     );
   });
 
-  test('cannot list session recording when in org scope', function (assert) {
+  test('can list session recording when in org scope when authorized', function (assert) {
     assert.expect(2);
 
     const scopeModelWithAuthorizedAction = store.createRecord('scope', {
@@ -118,19 +114,15 @@ module('Unit | Abilities | session-recording', function (hooks) {
       id: 'second',
     });
 
-    assert.false(
-      canService.can('list session-recording', scopeModelWithAuthorizedAction, {
+    assert.true(
+      canService.can('list scope', scopeModelWithAuthorizedAction, {
         collection: 'session-recordings',
       })
     );
     assert.false(
-      canService.can(
-        'list session-recording',
-        scopeModelWithoutAuthorizedAction,
-        {
-          collection: 'session-recordings',
-        }
-      )
+      canService.can('list scope', scopeModelWithoutAuthorizedAction, {
+        collection: 'session-recordings',
+      })
     );
   });
 
@@ -149,22 +141,14 @@ module('Unit | Abilities | session-recording', function (hooks) {
     });
 
     assert.true(
-      canService.can(
-        'navigate session-recording',
-        scopeModelWithAuthorizedAction,
-        {
-          collection: 'session-recordings',
-        }
-      )
+      canService.can('navigate scope', scopeModelWithAuthorizedAction, {
+        collection: 'session-recordings',
+      })
     );
     assert.false(
-      canService.can(
-        'navigate session-recording',
-        scopeModelWithoutAuthorizedAction,
-        {
-          collection: 'session-recordings',
-        }
-      )
+      canService.can('navigate scope', scopeModelWithoutAuthorizedAction, {
+        collection: 'session-recordings',
+      })
     );
   });
 
@@ -184,22 +168,14 @@ module('Unit | Abilities | session-recording', function (hooks) {
     });
 
     assert.false(
-      canService.can(
-        'navigate session-recording',
-        scopeModelWithAuthorizedAction,
-        {
-          collection: 'session-recordings',
-        }
-      )
+      canService.can('navigate scope', scopeModelWithAuthorizedAction, {
+        collection: 'session-recordings',
+      })
     );
     assert.false(
-      canService.can(
-        'navigate session-recording',
-        scopeModelWithoutAuthorizedAction,
-        {
-          collection: 'session-recordings',
-        }
-      )
+      canService.can('navigate scope', scopeModelWithoutAuthorizedAction, {
+        collection: 'session-recordings',
+      })
     );
   });
 });
diff --git a/ui/admin/tests/unit/abilities/storage-bucket-test.js b/ui/admin/tests/unit/abilities/storage-bucket-test.js
index 58b5c4319..2eb8d49c4 100644
--- a/ui/admin/tests/unit/abilities/storage-bucket-test.js
+++ b/ui/admin/tests/unit/abilities/storage-bucket-test.js
@@ -18,10 +18,10 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
   let store;
 
   hooks.beforeEach(function () {
+    store = this.owner.lookup('service:store');
+    canService = this.owner.lookup('service:can');
     features = this.owner.lookup('service:features');
     features.enable('session-recording');
-    canService = this.owner.lookup('service:can');
-    store = this.owner.lookup('service:store');
   });
 
   test('can read storage bucket when authorized and feature is enabled', function (assert) {
@@ -84,7 +84,7 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
     });
 
     assert.true(
-      canService.can('list storage-bucket', scopeModel, {
+      canService.can('list scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
@@ -100,13 +100,13 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
     });
 
     assert.false(
-      canService.can('list storage-bucket', scopeModel, {
+      canService.can('list scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
   });
 
-  test('cannot list storage bucket when authorized and in org scope', function (assert) {
+  test('can list storage bucket when authorized and in org scope', function (assert) {
     assert.expect(1);
 
     const scopeModel = store.createRecord('scope', {
@@ -115,8 +115,8 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
       id: 'o_123',
     });
 
-    assert.false(
-      canService.can('list storage-bucket', scopeModel, {
+    assert.true(
+      canService.can('list scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
@@ -132,7 +132,7 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
     });
 
     assert.true(
-      canService.can('create storage-bucket', scopeModel, {
+      canService.can('create scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
@@ -148,13 +148,13 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
     });
 
     assert.true(
-      canService.can('create storage-bucket', scopeModel, {
+      canService.can('create scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
   });
 
-  test('cannot create storage bucket when authorized and in project scope', function (assert) {
+  test('can create storage bucket when authorized and in project scope', function (assert) {
     assert.expect(1);
 
     const scopeModel = store.createRecord('scope', {
@@ -163,8 +163,8 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
       id: 'p_123',
     });
 
-    assert.false(
-      canService.can('create storage-bucket', scopeModel, {
+    assert.true(
+      canService.can('create scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
@@ -180,7 +180,7 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
     });
 
     assert.false(
-      canService.can('create storage-bucket', scopeModel, {
+      canService.can('create scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
@@ -196,7 +196,7 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
     });
 
     assert.true(
-      canService.can('navigate storage-bucket', scopeModel, {
+      canService.can('navigate scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );
@@ -212,8 +212,8 @@ module('Unit | Abilities | storage-bucket', function (hooks) {
       id: 'global',
     });
 
-    assert.true(
-      canService.can('navigate storage-bucket', scopeModel, {
+    assert.false(
+      canService.can('navigate scope', scopeModel, {
         collection: 'storage-buckets',
       })
     );