Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

Merged
merged 12 commits into from
Jan 17, 2024
Merged

feat: [M3-7529] – Support VPC in Personal Access Token drawer #10024

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1ce7027
VPCs option in PAT drawer, successful submits possible, 'Read Only' r…
DevDW Jan 2, 2024
f6a06b6
working tooltip
DevDW Jan 2, 2024
9d42a01
rename variable
DevDW Jan 2, 2024
7d4a5bb
Display scope for VPC conditionally based on isFeatureEnabled() logic…
DevDW Jan 3, 2024
1fec282
Added changeset: Support VPC in Access Token drawers
DevDW Jan 3, 2024
4c46984
Unit tests
DevDW Jan 4, 2024
e3eaa66
Passing utils.test.ts
DevDW Jan 4, 2024
cf80e18
Merge in latest develop
DevDW Jan 8, 2024
2f09ba1
Merge in latest develop and resolve conflict
DevDW Jan 12, 2024
576cd97
Take the simplified approach instead
DevDW Jan 12, 2024
286c6d9
Update logic in ViewAPITokenDrawer.tsx as well
DevDW Jan 16, 2024
40c81d0
Adjust getPermsNameMap() and get unit tests passing
DevDW Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-10024-upcoming-features-1704317084918.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Upcoming Features
---

Support VPC in Access Token drawers ([#10024](https://github.com/linode/manager/pull/10024))
22 changes: 20 additions & 2 deletions packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as React from 'react';

import Check from 'src/assets/icons/monitor-ok.svg';
import { Radio } from 'src/components/Radio/Radio';
import { Tooltip } from 'src/components/Tooltip';

interface RadioButton extends HTMLInputElement {
name: string;
Expand All @@ -15,11 +16,20 @@ interface AccessCellProps {
onChange: (e: React.SyntheticEvent<RadioButton>) => void;
scope: string;
scopeDisplay: string;
tooltipText?: string;
viewOnly: boolean;
}

export const AccessCell = React.memo((props: AccessCellProps) => {
const { active, disabled, onChange, scope, scopeDisplay, viewOnly } = props;
const {
active,
disabled,
onChange,
scope,
scopeDisplay,
tooltipText,
viewOnly,
} = props;

if (viewOnly) {
if (!active) {
Expand All @@ -36,7 +46,7 @@ export const AccessCell = React.memo((props: AccessCellProps) => {
);
}

return (
const radioBtn = (
<Radio
inputProps={{
'aria-label': `${scope} for ${scopeDisplay}`,
Expand All @@ -49,6 +59,14 @@ export const AccessCell = React.memo((props: AccessCellProps) => {
value={scope}
/>
);

return tooltipText ? (
<Tooltip placement="top" title={tooltipText}>
<span>{radioBtn}</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, tooltips do not activate on disabled elements. The workaround for this is to add a wrapper such as a <span>. More details here.

</Tooltip>
) : (
radioBtn
);
});

const StyledCheckIcon = styled('span', {
Expand Down
20 changes: 20 additions & 0 deletions packages/manager/src/features/Profile/APITokens/CreateAPITokenDrawer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,26 @@ describe('Create API Token Drawer', () => {
expect(childScope).not.toBeInTheDocument();
});

it('Should show the VPC scope with the VPC feature flag on', () => {
const { getByText } = renderWithTheme(<CreateAPITokenDrawer {...props} />, {
flags: { vpc: true },
});
const vpcScope = getByText('VPCs');
expect(vpcScope).toBeInTheDocument();
});

it('Should not show the VPC scope with the VPC feature flag off', () => {
const { queryByText } = renderWithTheme(
<CreateAPITokenDrawer {...props} />,
{
flags: { vpc: false },
}
);

const vpcScope = queryByText('VPCs');
expect(vpcScope).not.toBeInTheDocument();
});

it('Should close when Cancel is pressed', () => {
const { getByText } = renderWithTheme(<CreateAPITokenDrawer {...props} />);
const cancelButton = getByText(/Cancel/);
Expand Down
37 changes: 34 additions & 3 deletions packages/manager/src/features/Profile/APITokens/CreateAPITokenDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@ import { TableRow } from 'src/components/TableRow';
import { TextField } from 'src/components/TextField';
import { ISO_DATETIME_NO_TZ_FORMAT } from 'src/constants';
import { AccessCell } from 'src/features/ObjectStorage/AccessKeyLanding/AccessCell';
import { VPC_READ_ONLY_TOOLTIP } from 'src/features/VPCs/constants';
import { useFlags } from 'src/hooks/useFlags';
import { useAccount } from 'src/queries/account';
import { useAccountUser } from 'src/queries/accountUsers';
import { useProfile } from 'src/queries/profile';
import { useCreatePersonalAccessTokenMutation } from 'src/queries/tokens';
import { isFeatureEnabled } from 'src/utilities/accountCapabilities';
import { getErrorMap } from 'src/utilities/errorUtils';

import {
Expand All @@ -29,9 +32,9 @@ import {
StyledSelectCell,
} from './APITokenDrawer.styles';
import {
basePermNameMap as _basePermNameMap,
Permission,
allScopesAreTheSame,
basePermNameMap,
permTuplesToScopeString,
scopeStringToPermTuples,
} from './utils';
Expand Down Expand Up @@ -94,6 +97,7 @@ export const CreateAPITokenDrawer = (props: Props) => {
};

const { data: profile } = useProfile();
const { data: account } = useAccount();
const { data: user } = useAccountUser(profile?.username ?? '');

const {
Expand All @@ -102,6 +106,16 @@ export const CreateAPITokenDrawer = (props: Props) => {
mutateAsync: createPersonalAccessToken,
} = useCreatePersonalAccessTokenMutation();

const [basePermNameMap, setBasePermNameMap] = React.useState(
_basePermNameMap
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having state for this, could we derive the value?

Would it work if we had something like getPermsMap that would take basePermNameMap and omit values as needed?

const basePermNameMap = getPermsMap(flags.vpc ? [] : ['vpc']);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be derived. Avoiding the edge case I referenced previously would require having useRef in the mix, but I don't think that edge case is valid at this time: with open beta, API is returning VPCs in the account.capabilities array for all users. That actually makes all of the conditional logic for the PAT unnecessary, but having some conditional logic just to be on the safe side until VPC enters GA wouldn't be bad.

So we could do a simplified approach like

  // @TODO VPC: once VPC enters GA, remove _basePermNameMap logic and references.
  // Just use the basePermNameMap import directly w/o any manipulation.
  const basePermNameMap = getPermsNameMap(_basePermNameMap, {
    name: 'vpc',
    shouldBeIncluded: showVPCs,
  });

where the VPC perm would get removed if showVPCs was false. This would be vulnerable to the edge case, but that edge case would only come into play now if open beta got rolled back and we were testing with our DevTool (or the LD feature flag was toggled at the exact moment a user was interacting with this flow).


const showVPCs = isFeatureEnabled(
'VPCs',
Boolean(flags.vpc),
account?.capabilities ?? []
);

const form = useFormik<{
expiry: string;
label: string;
Expand All @@ -122,8 +136,19 @@ export const CreateAPITokenDrawer = (props: Props) => {
React.useEffect(() => {
if (open) {
form.resetForm({ values: initialValues });

// If VPCs should not be shown, do not display the VPC option
if (!showVPCs) {
const basePermNameMapCopy = Object.assign({}, basePermNameMap);
delete basePermNameMapCopy.vpc;
setBasePermNameMap(basePermNameMapCopy);
}
}
}, [open]);

return () => {
setBasePermNameMap(basePermNameMap);
};
dwiley-akamai marked this conversation as resolved.
Show resolved Hide resolved
}, [open, showVPCs]);

const handleScopeChange = (e: React.SyntheticEvent<RadioButton>): void => {
const newScopes = form.values.scopes;
Expand Down Expand Up @@ -258,6 +283,9 @@ export const CreateAPITokenDrawer = (props: Props) => {
if (!basePermNameMap[scopeTup[0]]) {
return null;
}

const scopeIsForVPC = scopeTup[0] === 'vpc';

return (
<TableRow
data-qa-row={basePermNameMap[scopeTup[0]]}
Expand All @@ -281,8 +309,11 @@ export const CreateAPITokenDrawer = (props: Props) => {
parentColumn="Read Only"
>
<AccessCell
tooltipText={
scopeIsForVPC ? VPC_READ_ONLY_TOOLTIP : undefined
}
active={scopeTup[1] === 1}
disabled={false}
disabled={scopeIsForVPC} // "Read Only" is not a valid scope for VPC
onChange={handleScopeChange}
scope="1"
scopeDisplay={scopeTup[0]}
Expand Down
32 changes: 27 additions & 5 deletions packages/manager/src/features/Profile/APITokens/ViewAPITokenDrawer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ describe('View API Token Drawer', () => {
});

it('should show all permissions as read/write with wildcard scopes', () => {
const { getByTestId } = renderWithTheme(<ViewAPITokenDrawer {...props} />);
const { getByTestId } = renderWithTheme(<ViewAPITokenDrawer {...props} />, {
flags: { vpc: true },
});
for (const permissionName of nonParentPerms) {
expect(getByTestId(`perm-${permissionName}`)).toHaveAttribute(
'aria-label',
Expand All @@ -54,7 +56,7 @@ describe('View API Token Drawer', () => {
it('should show all permissions as none with no scopes', () => {
const { getByTestId } = renderWithTheme(
<ViewAPITokenDrawer {...props} token={limitedToken} />,
{ flags: { parentChildAccountAccess: false } }
{ flags: { parentChildAccountAccess: false, vpc: true } }
);
for (const permissionName of nonParentPerms) {
expect(getByTestId(`perm-${permissionName}`)).toHaveAttribute(
Expand All @@ -69,7 +71,8 @@ describe('View API Token Drawer', () => {
<ViewAPITokenDrawer
{...props}
token={appTokenFactory.build({ scopes: 'account:read_write' })}
/>
/>,
{ flags: { vpc: true } }
);
for (const permissionName of nonParentPerms) {
// We only expect account to have read/write for this test
Expand All @@ -87,9 +90,10 @@ describe('View API Token Drawer', () => {
{...props}
token={appTokenFactory.build({
scopes:
'databases:read_only domains:read_write events:read_write firewall:read_write images:read_write ips:read_write linodes:read_only lke:read_only longview:read_write nodebalancers:read_write object_storage:read_only stackscripts:read_write volumes:read_only',
'databases:read_only domains:read_write events:read_write firewall:read_write images:read_write ips:read_write linodes:read_only lke:read_only longview:read_write nodebalancers:read_write object_storage:read_only stackscripts:read_write volumes:read_only vpc:read_write',
})}
/>
/>,
{ flags: { vpc: true } }
);

const expectedScopeLevels = {
Expand All @@ -107,6 +111,7 @@ describe('View API Token Drawer', () => {
object_storage: 1,
stackscripts: 2,
volumes: 1,
vpc: 2,
} as const;

for (const permissionName of nonParentPerms) {
Expand Down Expand Up @@ -160,4 +165,21 @@ describe('View API Token Drawer', () => {
const childScope = queryByText('Child Account Access');
expect(childScope).not.toBeInTheDocument();
});

it('Should show the VPC scope with the VPC feature flag on', () => {
const { getByText } = renderWithTheme(<ViewAPITokenDrawer {...props} />, {
flags: { vpc: true },
});
const vpcScope = getByText('VPCs');
expect(vpcScope).toBeInTheDocument();
});

it('Should not show the VPC scope with the VPC feature flag off', () => {
const { queryByText } = renderWithTheme(<ViewAPITokenDrawer {...props} />, {
flags: { vpc: false },
});

const vpcScope = queryByText('VPCs');
expect(vpcScope).not.toBeInTheDocument();
});
});
31 changes: 30 additions & 1 deletion packages/manager/src/features/Profile/APITokens/ViewAPITokenDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,20 @@ import { TableHead } from 'src/components/TableHead';
import { TableRow } from 'src/components/TableRow';
import { AccessCell } from 'src/features/ObjectStorage/AccessKeyLanding/AccessCell';
import { useFlags } from 'src/hooks/useFlags';
import { useAccount } from 'src/queries/account';
import { useAccountUser } from 'src/queries/accountUsers';
import { useProfile } from 'src/queries/profile';
import { isFeatureEnabled } from 'src/utilities/accountCapabilities';

import {
StyledAccessCell,
StyledPermissionsCell,
StyledPermsTable,
} from './APITokenDrawer.styles';
import { basePermNameMap, scopeStringToPermTuples } from './utils';
import {
basePermNameMap as _basePermNameMap,
scopeStringToPermTuples,
} from './utils';

interface Props {
onClose: () => void;
Expand All @@ -30,8 +35,32 @@ export const ViewAPITokenDrawer = (props: Props) => {
const flags = useFlags();

const { data: profile } = useProfile();
const { data: account } = useAccount();
const { data: user } = useAccountUser(profile?.username ?? '');

const [basePermNameMap, setBasePermNameMap] = React.useState(
_basePermNameMap
);

const showVPCs = isFeatureEnabled(
'VPCs',
Boolean(flags.vpc),
account?.capabilities ?? []
);

React.useEffect(() => {
if (open && !showVPCs) {
// If VPCs should not be shown, do not display the VPC scope
const basePermNameMapCopy = Object.assign({}, basePermNameMap);
delete basePermNameMapCopy.vpc;
setBasePermNameMap(basePermNameMapCopy);
}

return () => {
setBasePermNameMap(basePermNameMap);
};
}, [open, showVPCs]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually took a slightly different approach which I'm interested in hearing your thoughts. We actually removed them from the list of perms until it no longer needs to be feature flagged: https://github.com/linode/manager/pull/10040/files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that and gave that approach a try earlier in the week, commenting out 'vpc' in basePerms and vpc: 'VPCs' in basePermNameMap in utils.ts and adding the below (+ commenting out the current approach) in CreateAPITokenDrawer.tsx:

  if (showVPCs) {
    const vpcScopeIndex = allPermissions.findIndex(
      ([scope]) => scope === 'vpc'
    );

    if (vpcScopeIndex === -1) {
      allPermissions.push(['vpc', 0]);
    }

    basePermNameMap.vpc = 'VPCs';
  }

It resulted in some weird behavior though where the scope for VPC doesn't appear until another scope is clicked on:

Screen.Recording.2024-01-11.at.1.05.10.PM.mov

Since GA isn't far off and because API is returning VPCs in account.capabilities for all users, I'm leaning towards the simplified approach I described in my previous comment


const allPermissions = scopeStringToPermTuples(token?.scopes ?? '');

// Filter permissions for all users except parent user accounts.
Expand Down
11 changes: 11 additions & 0 deletions packages/manager/src/features/Profile/APITokens/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ describe('APIToken utils', () => {
['object_storage', 2],
['stackscripts', 2],
['volumes', 2],
['vpc', 2],
];
it('should return an array containing a tuple for each type and a permission level of 2', () => {
expect(result).toEqual(expected);
Expand All @@ -64,6 +65,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should return an array containing a tuple for each type and a permission level of 0', () => {
Expand All @@ -89,6 +91,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should return account:0', () => {
Expand All @@ -114,6 +117,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should return account:1', () => {
Expand All @@ -139,6 +143,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should return account:2', () => {
Expand Down Expand Up @@ -166,6 +171,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should domain:1 and longview:2', () => {
Expand Down Expand Up @@ -195,6 +201,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should return the higher value for account.', () => {
Expand Down Expand Up @@ -224,6 +231,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];

it('should return the higher value for account.', () => {
Expand All @@ -249,6 +257,7 @@ describe('APIToken utils', () => {
['object_storage', 0],
['stackscripts', 0],
['volumes', 0],
['vpc', 0],
];
expect(allScopesAreTheSame(scopes)).toBe(0);
});
Expand Down Expand Up @@ -289,6 +298,7 @@ describe('APIToken utils', () => {
['object_storage', 2],
['stackscripts', 2],
['volumes', 2],
['vpc', 2],
];
expect(allScopesAreTheSame(scopes)).toBe(2);
});
Expand All @@ -309,6 +319,7 @@ describe('APIToken utils', () => {
['object_storage', 2],
['stackscripts', 2],
['volumes', 2],
['vpc', 0],
];
expect(allScopesAreTheSame(scopes)).toBe(null);
});
Expand Down
Loading