change: [M3-7729] - Improve UX of Personal Access Token and Access Key drawers

packages/manager/.changeset/pr-10338-changed-1712685790473.md

@@ -0,0 +1,5 @@
+---
+"@linode/manager": Changed
+---
+
+Improve the UX of Access Token & Access Key drawers ([#10338](https://github.com/linode/manager/pull/10338))

packages/manager/cypress/e2e/core/account/personal-access-tokens.spec.ts

@@ -68,14 +68,42 @@ describe('Personal access tokens', () => {
       .findByTitle('Add Personal Access Token')
       .should('be.visible')
       .within(() => {
-        // Attempt to submit form without specifying a label
+        // Confirm submit button is disabled without specifying scopes.
+        ui.buttonGroup
+          .findButtonByTitle('Create Token')
+          .scrollIntoView()
+          .should('be.visible')
+          .should('be.disabled');
+
+        // Select just one scope.
+        cy.get('[data-qa-row="Account"]').within(() => {
+          cy.get('[type="radio"]').first().click();
+        });
+
+        // Confirm submit button is still disabled without specifying ALL scopes.
+        ui.buttonGroup
+          .findButtonByTitle('Create Token')
+          .scrollIntoView()
+          .should('be.visible')
+          .should('be.disabled');
+
+        // Specify ALL scopes by selecting the "No Access" Select All radio button.
+        cy.get('[data-qa-perm-no-access-radio]').click();
+        cy.get('[data-qa-perm-no-access-radio]').should(
+          'have.attr',
+          'data-qa-radio',
+          'true'
+        );
+
+        // Confirm submit button is enabled; attempt to submit form without specifying a label.
         ui.buttonGroup
           .findButtonByTitle('Create Token')
           .scrollIntoView()
           .should('be.visible')
           .should('be.enabled')
           .click();
 
+        // Confirm validation error.
         cy.findByText('Label must be between 1 and 100 characters.')
           .scrollIntoView()
           .should('be.visible');

packages/manager/cypress/e2e/core/objectStorage/access-keys.smoke.spec.ts

@@ -342,9 +342,14 @@ describe('object storage access keys smoke tests', () => {
             .click()
             .type('{esc}');
 
-          // Enable "Limited Access" toggle for access key, and select access rules.
+          // Enable "Limited Access" toggle for access key and confirm Create button is disabled.
           cy.findByText('Limited Access').should('be.visible').click();
 
+          ui.buttonGroup
+            .findButtonByTitle('Create Access Key')
+            .should('be.disabled');
+
+          // Select access rules for all buckets to enable Create button.
           mockBuckets.forEach((mockBucket) => {
             cy.findByText(mockBucket.label)
               .should('be.visible')

packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessTable.styles.ts

@@ -39,6 +39,14 @@ export const StyledRadioRow = styled(TableRow, {
     }),
 }));
 
+export const StyledSelectAllRadioRow = styled(StyledRadioRow, {
+  label: 'StyledSelectAllRadioRow',
+})(({ theme }) => ({
+  '& td ': {
+    borderBottom: `2px solid ${theme.color.grey2}`,
+  },
+}));
+
 export const StyledRadioCell = styled(TableCell, {
   label: 'StyledRadioCell',
 })(() => ({

packages/manager/src/features/ObjectStorage/AccessKeyLanding/BucketPermissionsTable.tsx

@@ -17,6 +17,7 @@ import {
   StyledClusterCell,
   StyledRadioCell,
   StyledRadioRow,
+  StyledSelectAllRadioRow,
   StyledTableRoot,
 } from './AccessTable.styles';
 
@@ -76,8 +77,9 @@ export const BucketPermissionsTable = React.memo((props: Props) => {
   };
 
   const allScopesEqual = (accessType: AccessType) => {
-    return bucket_access.every(
-      (thisScope) => thisScope.permissions === accessType
+    return (
+      bucket_access.length > 0 &&
+      bucket_access.every((thisScope) => thisScope.permissions === accessType)
     );
   };
 
@@ -105,7 +107,7 @@ export const BucketPermissionsTable = React.memo((props: Props) => {
       </TableHead>
       <TableBody>
         {mode === 'creating' && (
-          <StyledRadioRow data-qa-row="Select All" disabled={disabled}>
+          <StyledSelectAllRadioRow data-qa-row="Select All" disabled={disabled}>
             <TableCell colSpan={2} padding="checkbox" parentColumn="Region">
               <strong>Select All</strong>
             </TableCell>
@@ -151,7 +153,7 @@ export const BucketPermissionsTable = React.memo((props: Props) => {
                 value="read-write"
               />
             </TableCell>
-          </StyledRadioRow>
+          </StyledSelectAllRadioRow>
         )}
         {bucket_access.length === 0 ? (
           <TableRowEmpty

packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx

@@ -35,7 +35,11 @@ import { confirmObjectStorage } from '../utilities';
 import { AccessKeyRegions } from './AccessKeyRegions/AccessKeyRegions';
 import { LimitedAccessControls } from './LimitedAccessControls';
 import { MODE } from './types';
-import { generateUpdatePayload, hasLabelOrRegionsChanged } from './utils';
+import {
+  generateUpdatePayload,
+  hasAccessBeenSelectedForAllBuckets,
+  hasLabelOrRegionsChanged,
+} from './utils';
 
 export interface AccessKeyDrawerProps {
   isRestrictedUser: boolean;
@@ -52,6 +56,11 @@ export interface AccessKeyDrawerProps {
   open: boolean;
 }
 
+// Access key scopes displayed in the drawer can have no permission or "No Access" selected, which are not valid API permissions.
+export interface DisplayedAccessKeyScope extends Omit<Scope, 'permissions'> {
+  permissions: AccessType | null;
+}
+
 export interface FormState {
   bucket_access: Scope[] | null;
   label: string;
@@ -66,8 +75,8 @@ export interface FormState {
  */
 
 export const sortByRegion = (regionLookup: { [key: string]: Region }) => (
-  a: Scope,
-  b: Scope
+  a: DisplayedAccessKeyScope,
+  b: DisplayedAccessKeyScope
 ) => {
   if (!a.region || !b.region) {
     return 0;
@@ -82,12 +91,12 @@ export const sortByRegion = (regionLookup: { [key: string]: Region }) => (
 export const getDefaultScopes = (
   buckets: ObjectStorageBucket[],
   regionLookup: { [key: string]: Region } = {}
-): Scope[] =>
+): DisplayedAccessKeyScope[] =>
   buckets
     .map((thisBucket) => ({
       bucket_name: thisBucket.label,
       cluster: thisBucket.cluster,
-      permissions: 'none' as AccessType,
+      permissions: null,
       region: thisBucket.region,
     }))
     .sort(sortByRegion(regionLookup));
@@ -163,13 +172,15 @@ export const OMC_AccessKeyDrawer = (props: AccessKeyDrawerProps) => {
       // If the user hasn't toggled the Limited Access button,
       // don't include any bucket_access information in the payload.
 
-      // If any/all values are 'none', don't include them in the response.
+      // If any/all permissions are 'none' or null, don't include them in the response.
       const access = values.bucket_access ?? [];
       const payload = limitedAccessChecked
         ? {
             ...values,
             bucket_access: access.filter(
-              (thisAccess) => thisAccess.permissions !== 'none'
+              (thisAccess: DisplayedAccessKeyScope) =>
+                thisAccess.permissions !== 'none' &&
+                thisAccess.permissions !== null
             ),
           }
         : { ...values, bucket_access: null };
@@ -195,7 +206,10 @@ export const OMC_AccessKeyDrawer = (props: AccessKeyDrawerProps) => {
     (mode !== 'creating' &&
       objectStorageKey &&
       objectStorageKey?.regions?.length > 0 &&
-      !hasLabelOrRegionsChanged(formik.values, objectStorageKey));
+      !hasLabelOrRegionsChanged(formik.values, objectStorageKey)) ||
+    (mode === 'creating' &&
+      limitedAccessChecked &&
+      !hasAccessBeenSelectedForAllBuckets(formik.values.bucket_access));
 
   const beforeSubmit = () => {
     confirmObjectStorage<FormState>(

packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.test.ts

@@ -1,7 +1,11 @@
-import { ObjectStorageKey } from '@linode/api-v4/lib/object-storage';
+import {
+  generateUpdatePayload,
+  hasAccessBeenSelectedForAllBuckets,
+  hasLabelOrRegionsChanged,
+} from './utils';
 
-import { FormState } from './OMC_AccessKeyDrawer';
-import { generateUpdatePayload, hasLabelOrRegionsChanged } from './utils';
+import type { DisplayedAccessKeyScope, FormState } from './OMC_AccessKeyDrawer';
+import type { ObjectStorageKey } from '@linode/api-v4/lib/object-storage';
 
 describe('generateUpdatePayload', () => {
   const initialValues: FormState = {
@@ -96,3 +100,42 @@ describe('hasLabelOrRegionsChanged', () => {
     ).toBe(true);
   });
 });
+
+describe('hasAccessBeenSelectedForAllBuckets', () => {
+  const bucketWithoutAccessSelected: DisplayedAccessKeyScope = {
+    bucket_name: 'obj-bucket-1',
+    cluster: 'us-lax-1',
+    permissions: null,
+    region: 'us-lax',
+  };
+
+  const bucketWithAccessSelected: DisplayedAccessKeyScope = {
+    bucket_name: 'obj-bucket-1',
+    cluster: 'us-lax-1',
+    permissions: 'read_only',
+    region: 'us-lax',
+  };
+
+  it('returns false if any buckets have permission set to null', () => {
+    const bucket_access = [
+      bucketWithAccessSelected,
+      bucketWithoutAccessSelected,
+    ];
+    expect(hasAccessBeenSelectedForAllBuckets(bucket_access)).toBe(false);
+  });
+
+  it('returns true if all buckets have permission set to a value other than null', () => {
+    const bucket_access = [bucketWithAccessSelected, bucketWithAccessSelected];
+    expect(hasAccessBeenSelectedForAllBuckets(bucket_access)).toBe(true);
+  });
+
+  it('returns true if buckets are null', () => {
+    const bucket_access = null;
+    expect(hasAccessBeenSelectedForAllBuckets(bucket_access)).toBe(true);
+  });
+
+  it('returns true if there are no buckets', () => {
+    const bucket_access: DisplayedAccessKeyScope[] = [];
+    expect(hasAccessBeenSelectedForAllBuckets(bucket_access)).toBe(true);
+  });
+});

packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.ts

@@ -1,13 +1,12 @@
-import { ObjectStorageKey } from '@linode/api-v4/lib/object-storage';
-
 import { areArraysEqual } from 'src/utilities/areArraysEqual';
 import { sortByString } from 'src/utilities/sort-by';
 
-import { FormState } from './OMC_AccessKeyDrawer';
+import type { DisplayedAccessKeyScope, FormState } from './OMC_AccessKeyDrawer';
+import type { ObjectStorageKey } from '@linode/api-v4/lib/object-storage';
 
 type UpdatePayload =
-  | { label: FormState['label']; regions: FormState['regions'] }
   | { label: FormState['label'] }
+  | { label: FormState['label']; regions: FormState['regions'] }
   | { regions: FormState['regions'] }
   | {};
 
@@ -71,3 +70,19 @@ export const hasLabelOrRegionsChanged = (
 
   return labelChanged || regionsChanged;
 };
+
+/**
+ * Determines whether the selection of access key scopes has been made for every bucket,
+ * since by default, the displayed permissions are set to null.
+ *
+ * @param bucketAccess - The array of bucket objects.
+ * @returns {boolean} True if all buckets have permissions set to none/read_only/read_write or if there are no buckets, false otherwise.
+ */
+export const hasAccessBeenSelectedForAllBuckets = (
+  bucketAccess: DisplayedAccessKeyScope[] | null
+): boolean => {
+  if (!bucketAccess) {
+    return true;
+  }
+  return bucketAccess.every((bucket) => bucket.permissions !== null);
+};

packages/manager/src/features/Profile/APITokens/APITokenDrawer.styles.ts

@@ -12,7 +12,8 @@ export const StyledPermsTable = styled(Table, {
 
 export const StyledSelectCell = styled(TableCell, {
   label: 'StyledSelectCell',
-})(() => ({
+})(({ theme }) => ({
+  borderBottom: `2px solid ${theme.color.grey2}`,
   fontFamily: 'LatoWebBold', // we keep this bold at all times
   fontSize: '.9rem',
 }));
@@ -35,3 +36,9 @@ export const StyledPermissionsCell = styled(TableCell, {
   },
   width: '23%',
 }));
+
+export const StyledSelectAllPermissionsCell = styled(StyledPermissionsCell, {
+  label: 'StyledSelectAllPermissionsCell',
+})(({ theme }) => ({
+  borderBottom: `2px solid ${theme.color.grey2}`,
+}));

packages/manager/src/features/Profile/APITokens/CreateAPITokenDrawer.test.tsx

@@ -54,9 +54,10 @@ describe('Create API Token Drawer', () => {
     const expiry = getByText(/Expiry/);
     expect(expiry).toBeVisible();
 
+    // Submit button will be disabled until scope selection is made.
     const submitBtn = getByTestId('create-button');
     expect(submitBtn).toBeVisible();
-    expect(submitBtn).not.toHaveAttribute('aria-disabled', 'true');
+    expect(submitBtn).toHaveAttribute('aria-disabled', 'true');
 
     const cancelBtn = getByText(/Cancel/);
     expect(cancelBtn).not.toHaveAttribute('aria-disabled', 'true');
@@ -72,26 +73,44 @@ describe('Create API Token Drawer', () => {
       })
     );
 
-    const { getByTestId, getByText } = renderWithTheme(
+    const { getByLabelText, getByTestId, getByText } = renderWithTheme(
       <CreateAPITokenDrawer {...props} />
     );
 
     const labelField = getByTestId('textfield-input');
     await userEvent.type(labelField, 'my-test-token');
-    const submit = getByText('Create Token');
-    await userEvent.click(submit);
+
+    const selectAllNoAccessPermRadioButton = getByLabelText(
+      'Select no access for all'
+    );
+    const submitBtn = getByText('Create Token');
+
+    expect(submitBtn).not.toHaveAttribute('aria-disabled', 'true');
+    await userEvent.click(selectAllNoAccessPermRadioButton);
+    await userEvent.click(submitBtn);
 
     await waitFor(() =>
       expect(props.showSecret).toBeCalledWith('secret-value')
     );
   });
 
-  it('Should default to None for all scopes', () => {
+  it('Should default to no selection for all scopes', () => {
     const { getByLabelText } = renderWithTheme(
       <CreateAPITokenDrawer {...props} />
     );
-    const selectAllNonePermRadioButton = getByLabelText('Select none for all');
-    expect(selectAllNonePermRadioButton).toBeChecked();
+    const selectAllNoAccessPermRadioButton = getByLabelText(
+      'Select no access for all'
+    );
+    const selectAllReadOnlyPermRadioButton = getByLabelText(
+      'Select read-only for all'
+    );
+    const selectAllReadWritePermRadioButton = getByLabelText(
+      'Select read/write for all'
+    );
+
+    expect(selectAllNoAccessPermRadioButton).not.toBeChecked();
+    expect(selectAllReadOnlyPermRadioButton).not.toBeChecked();
+    expect(selectAllReadWritePermRadioButton).not.toBeChecked();
   });
 
   it('Should default to 6 months for expiration', () => {
@@ -157,7 +176,7 @@ describe('Create API Token Drawer', () => {
       <CreateAPITokenDrawer {...props} />
     );
     const vpcPermRadioButtons = getAllByTestId('perm-vpc-radio');
-    const vpcNonePermRadioButton = vpcPermRadioButtons[0].firstChild;
+    const vpcNoAccessPermRadioButton = vpcPermRadioButtons[0].firstChild;
     const vpcReadOnlyPermRadioButton = vpcPermRadioButtons[1].firstChild;
 
     const selectAllReadOnlyPermRadioButton = getByLabelText(
@@ -166,7 +185,7 @@ describe('Create API Token Drawer', () => {
     await userEvent.click(selectAllReadOnlyPermRadioButton);
     expect(selectAllReadOnlyPermRadioButton).toBeChecked();
 
-    expect(vpcNonePermRadioButton).toBeChecked();
+    expect(vpcNoAccessPermRadioButton).toBeChecked();
     expect(vpcReadOnlyPermRadioButton).not.toBeChecked();
     expect(vpcReadOnlyPermRadioButton).toBeDisabled();
   });