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

fix: Profile Query Keys, User Permissions loading state, and read_only PAT creation #10040

Merged
merged 6 commits into from
Jan 8, 2024
Merged

fix: Profile Query Keys, User Permissions loading state, and read_only PAT creation #10040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
303a6b7
fix profile query keys
bnussman Jan 8, 2024
09636fe
add extra bug fix
bnussman Jan 8, 2024
9d82c8a
Fix loading state for User Permissions page
mjac0bs Jan 8, 2024
7f3e1a9
fix two factor invalidation
bnussman Jan 8, 2024
fc9514d
use `null` insted of empty object
bnussman Jan 8, 2024
e8087f2
Fix error for RO PAT creation
mjac0bs Jan 8, 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
12 changes: 12 additions & 0 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 @@ -168,6 +168,18 @@ export const CreateAPITokenDrawer = (props: Props) => {
const filteredPermissions = allPermissions.filter(
(scopeTup) => basePermNameMap[scopeTup[0]] !== 'Child Account Access'
);
// TODO: Parent/Child - remove this conditional once code is in prod.
// Note: We couldn't include 'child_account' in our list of permissions in utils
// because it needs to be feature-flagged. Therefore, we're manually adding it here.
if (flags.parentChildAccountAccess && user?.user_type !== null) {
const childAccountIndex = allPermissions.findIndex(
([scope]) => scope === 'child_account'
);
if (childAccountIndex === -1) {
allPermissions.push(['child_account', 0]);
}
basePermNameMap.child_account = 'Child Account Access';
}
Comment on lines +171 to +182
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a bug caused by the fact that we can't yet add child_account to the scopes until it is returned by API, so we have to add it in the drawers for now.

image


return (
<Drawer onClose={onClose} open={open} title="Add Personal Access Token">
Expand Down
14 changes: 8 additions & 6 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 @@ -20,7 +20,8 @@ vi.mock('src/queries/accountUsers', async () => {
};
});

const nonParentPerms = basePerms.filter((value) => value !== 'child_account');
// TODO: Parent/Child - add back after API code is in prod. Replace basePerms with nonParentPerms.
// const nonParentPerms = basePerms.filter((value) => value !== 'child_account');

const token = appTokenFactory.build({ label: 'my-token', scopes: '*' });
const limitedToken = appTokenFactory.build({
Expand All @@ -43,7 +44,7 @@ describe('View API Token Drawer', () => {

it('should show all permissions as read/write with wildcard scopes', () => {
const { getByTestId } = renderWithTheme(<ViewAPITokenDrawer {...props} />);
for (const permissionName of nonParentPerms) {
for (const permissionName of basePerms) {
expect(getByTestId(`perm-${permissionName}`)).toHaveAttribute(
'aria-label',
`This token has 2 access for ${permissionName}`
Expand All @@ -56,7 +57,7 @@ describe('View API Token Drawer', () => {
<ViewAPITokenDrawer {...props} token={limitedToken} />,
{ flags: { parentChildAccountAccess: false } }
);
for (const permissionName of nonParentPerms) {
for (const permissionName of basePerms) {
expect(getByTestId(`perm-${permissionName}`)).toHaveAttribute(
'aria-label',
`This token has 0 access for ${permissionName}`
Expand All @@ -71,7 +72,7 @@ describe('View API Token Drawer', () => {
token={appTokenFactory.build({ scopes: 'account:read_write' })}
/>
);
for (const permissionName of nonParentPerms) {
for (const permissionName of basePerms) {
// We only expect account to have read/write for this test
const expectedScopeLevel = permissionName === 'account' ? 2 : 0;
expect(getByTestId(`perm-${permissionName}`)).toHaveAttribute(
Expand Down Expand Up @@ -109,7 +110,7 @@ describe('View API Token Drawer', () => {
volumes: 1,
} as const;

for (const permissionName of nonParentPerms) {
for (const permissionName of basePerms) {
const expectedScopeLevel = expectedScopeLevels[permissionName];
expect(getByTestId(`perm-${permissionName}`)).toHaveAttribute(
'aria-label',
Expand All @@ -136,8 +137,9 @@ describe('View API Token Drawer', () => {
);

const childScope = getByText('Child Account Access');
// TODO: Parent/Child - confirm that this scope level shouldn't be 2
const expectedScopeLevels = {
child_account: 2,
child_account: 0,
} as const;
const childPermissionName = 'child_account';

Expand Down
12 changes: 12 additions & 0 deletions 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 @@ -41,6 +41,18 @@ export const ViewAPITokenDrawer = (props: Props) => {
const filteredPermissions = allPermissions.filter(
(scopeTup) => basePermNameMap[scopeTup[0]] !== 'Child Account Access'
);
// TODO: Parent/Child - remove this conditional once code is in prod.
// Note: We couldn't include 'child_account' in our list of permissions in utils
// because it needs to be feature-flagged. Therefore, we're manually adding it here.
if (flags.parentChildAccountAccess && user?.user_type !== null) {
const childAccountIndex = allPermissions.findIndex(
([scope]) => scope === 'child_account'
);
if (childAccountIndex === -1) {
allPermissions.push(['child_account', 0]);
}
basePermNameMap.child_account = 'Child Account Access';
}

return (
<Drawer onClose={onClose} open={open} title={token?.label ?? 'Token'}>
Expand Down
36 changes: 24 additions & 12 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 @@ -26,7 +26,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('*');
const expected = [
['account', 2],
['child_account', 2],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 2],
['databases', 2],
['domains', 2],
['events', 2],
Expand All @@ -50,7 +51,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('');
const expected = [
['account', 0],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand All @@ -75,7 +77,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('account:none');
const expected = [
['account', 0],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand All @@ -100,7 +103,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('account:read_only');
const expected = [
['account', 1],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand All @@ -125,7 +129,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('account:read_write');
const expected = [
['account', 2],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand All @@ -152,7 +157,8 @@ describe('APIToken utils', () => {
);
const expected = [
['account', 0],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 1],
['events', 0],
Expand Down Expand Up @@ -181,7 +187,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('account:none,tokens:read_write');
const expected = [
['account', 2],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand Down Expand Up @@ -210,7 +217,8 @@ describe('APIToken utils', () => {
const result = scopeStringToPermTuples('account:read_only,tokens:none');
const expected = [
['account', 1],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand All @@ -235,7 +243,8 @@ describe('APIToken utils', () => {
it('should return 0 if all scopes are 0', () => {
const scopes: Permission[] = [
['account', 0],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 0],
['events', 0],
Expand All @@ -255,7 +264,8 @@ describe('APIToken utils', () => {
it('should return 1 if all scopes are 1', () => {
const scopes: Permission[] = [
['account', 1],
['child_account', 1],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 1],
['databases', 1],
['domains', 1],
['events', 1],
Expand All @@ -275,7 +285,8 @@ describe('APIToken utils', () => {
it('should return 2 if all scopes are 2', () => {
const scopes: Permission[] = [
['account', 2],
['child_account', 2],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 2],
['databases', 2],
['domains', 2],
['events', 2],
Expand All @@ -295,7 +306,8 @@ describe('APIToken utils', () => {
it('should return null if all scopes are different', () => {
const scopes: Permission[] = [
['account', 1],
['child_account', 0],
// TODO: Parent/Child - add this scope once code is in prod.
// ['child_account', 0],
['databases', 0],
['domains', 2],
['events', 0],
Expand Down
6 changes: 4 additions & 2 deletions packages/manager/src/features/Profile/APITokens/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ export type Permission = [string, number];

export const basePerms = [
'account',
'child_account',
// TODO: Parent/Child - add this scope once API code is in prod.
// 'child_account',
'databases',
'domains',
'events',
Expand All @@ -24,7 +25,8 @@ export const basePerms = [

export const basePermNameMap: Record<string, string> = {
account: 'Account',
child_account: 'Child Account Access',
// TODO: Parent/Child - add this scope once API code is in prod.
// child_account: 'Child Account Access',
databases: 'Databases',
domains: 'Domains',
events: 'Events',
Expand Down
4 changes: 3 additions & 1 deletion packages/manager/src/features/Users/UserPermissions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { compose as recompose } from 'recompose';

import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel';
import { Box } from 'src/components/Box';
import { CircleProgress } from 'src/components/CircleProgress';
// import { Button } from 'src/components/Button/Button';
import { DocumentTitleSegment } from 'src/components/DocumentTitle';
import { Item } from 'src/components/EnhancedSelect/Select';
Expand Down Expand Up @@ -113,12 +114,13 @@ class UserPermissions extends React.Component<CombinedProps, State> {
}

render() {
const { loading } = this.state;
const { username } = this.props;

return (
<React.Fragment>
<DocumentTitleSegment segment={`${username} - Permissions`} />
{this.renderBody()}
{loading ? <CircleProgress /> : this.renderBody()}
Comment on lines +117 to +123
Copy link
Contributor

@mjac0bs mjac0bs Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This addresses a regression in the User Permissions page introduced here where eliminating the loading state check is causing the User Permissions page for a restricted user to display as Full Account Access as enabled and true and then update to disabled and false once the data loads.

</React.Fragment>
);
}
Expand Down
55 changes: 31 additions & 24 deletions packages/manager/src/queries/profile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,19 @@ import type { RequestOptions } from '@linode/api-v4';

export const queryKey = 'profile';

export const useProfile = ({ headers }: RequestOptions = {}) => {
const key = [queryKey, headers];
return useQuery<Profile, APIError[]>(key, () => getProfile({ headers }), {
...queryPresets.oneTimeFetch,
});
export const useProfile = (options?: RequestOptions) => {
const key = [
queryKey,
options?.headers ? { headers: options.headers } : null,
];

return useQuery<Profile, APIError[]>(
key,
() => getProfile({ headers: options?.headers }),
{
...queryPresets.oneTimeFetch,
}
);
};

export const useMutateProfile = () => {
Expand All @@ -60,28 +68,25 @@ export const updateProfileData = (
newData: Partial<Profile>,
queryClient: QueryClient
): void => {
queryClient.setQueryData([queryKey, undefined], (oldData: Profile) => ({
queryClient.setQueryData([queryKey, null], (oldData: Profile) => ({
...oldData,
...newData,
}));
};

export const useGrants = () => {
const { data: profile } = useProfile();
return useQuery<Grants, APIError[]>(
[queryKey, undefined, 'grants'],
listGrants,
{
...queryPresets.oneTimeFetch,
enabled: Boolean(profile?.restricted),
}
);
return useQuery<Grants, APIError[]>([queryKey, 'grants'], listGrants, {
...queryPresets.oneTimeFetch,
enabled: Boolean(profile?.restricted),
});
};

export const getProfileData = (queryClient: QueryClient) =>
queryClient.getQueryData<Profile>([queryKey, undefined]);
queryClient.getQueryData<Profile>([queryKey, null]);

export const getGrantData = (queryClient: QueryClient) =>
queryClient.getQueryData<Grants>([queryKey, undefined, 'grants']);
queryClient.getQueryData<Grants>([queryKey, 'grants']);

export const useSMSOptOutMutation = () => {
const queryClient = useQueryClient();
Expand All @@ -108,7 +113,7 @@ export const useSSHKeysQuery = (
enabled = true
) =>
useQuery(
[queryKey, {}, 'ssh-keys', 'paginated', params, filter],
[queryKey, 'ssh-keys', 'paginated', params, filter],
() => getSSHKeys(params, filter),
{
enabled,
Expand All @@ -122,7 +127,7 @@ export const useCreateSSHKeyMutation = () => {
createSSHKey,
{
onSuccess() {
queryClient.invalidateQueries([queryKey, undefined, 'ssh-keys']);
queryClient.invalidateQueries([queryKey, 'ssh-keys']);
// also invalidate the /account/users data because that endpoint returns some SSH key data
queryClient.invalidateQueries([accountQueryKey, 'users']);
},
Expand All @@ -136,7 +141,7 @@ export const useUpdateSSHKeyMutation = (id: number) => {
(data) => updateSSHKey(id, data),
{
onSuccess() {
queryClient.invalidateQueries([queryKey, undefined, 'ssh-keys']);
queryClient.invalidateQueries([queryKey, 'ssh-keys']);
// also invalidate the /account/users data because that endpoint returns some SSH key data
queryClient.invalidateQueries([accountQueryKey, 'users']);
},
Expand All @@ -148,7 +153,7 @@ export const useDeleteSSHKeyMutation = (id: number) => {
const queryClient = useQueryClient();
return useMutation<{}, APIError[]>(() => deleteSSHKey(id), {
onSuccess() {
queryClient.invalidateQueries([queryKey, undefined, 'ssh-keys']);
queryClient.invalidateQueries([queryKey, 'ssh-keys']);
// also invalidate the /account/users data because that endpoint returns some SSH key data
queryClient.invalidateQueries([accountQueryKey, 'users']);
},
Expand All @@ -159,14 +164,14 @@ export const sshKeyEventHandler = (event: EventWithStore) => {
// This event handler is a bit agressive and will over-fetch, but UX will
// be great because this will ensure Cloud has up to date data all the time.

event.queryClient.invalidateQueries([queryKey, undefined, 'ssh-keys']);
event.queryClient.invalidateQueries([queryKey, 'ssh-keys']);
// also invalidate the /account/users data because that endpoint returns some SSH key data
event.queryClient.invalidateQueries([accountQueryKey, 'users']);
};

export const useTrustedDevicesQuery = (params?: Params, filter?: Filter) =>
useQuery<ResourcePage<TrustedDevice>, APIError[]>(
[queryKey, {}, 'trusted-devices', 'paginated', params, filter],
[queryKey, 'trusted-devices', 'paginated', params, filter],
() => getTrustedDevices(params, filter),
{
keepPreviousData: true,
Expand All @@ -177,7 +182,7 @@ export const useRevokeTrustedDeviceMutation = (id: number) => {
const queryClient = useQueryClient();
return useMutation<{}, APIError[]>(() => deleteTrustedDevice(id), {
onSuccess() {
queryClient.invalidateQueries([queryKey, undefined, 'trusted-devices']);
queryClient.invalidateQueries([queryKey, 'trusted-devices']);
},
});
};
Expand All @@ -186,7 +191,9 @@ export const useDisableTwoFactorMutation = () => {
const queryClient = useQueryClient();
return useMutation<{}, APIError[]>(disableTwoFactor, {
onSuccess() {
queryClient.invalidateQueries([queryKey, undefined]);
queryClient.invalidateQueries([queryKey, null]);
// also invalidate the /account/users data because that endpoint returns 2FA status for each user
queryClient.invalidateQueries([accountQueryKey, 'users']);
},
});
};