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.

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

Unify access-control errors, and fix access-control types #7914

Merged
merged 19 commits into from
Oct 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 .changeset/errors-now-equal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@keystone-6/core': major
---

Changes access-control error messages to only show the list key and operation
6 changes: 3 additions & 3 deletions examples/ecommerce/tests/mutations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ describe(`Custom mutations`, () => {
expect(data).toEqual({ addToCart: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
`An error occured while resolving relationship fields.\n - CartItem.product: Access denied: You cannot perform the 'connect' operation on the item '{"id":"${FAKE_ID}"}'. It may not exist.\n - CartItem.user: Access denied: You cannot perform the 'connect' operation on the item '{"id":"${FAKE_ID}"}'. It may not exist.`
`An error occured while resolving relationship fields.\n - CartItem.product: Access denied: You cannot connect that Product - it may not exist\n - CartItem.user: Access denied: You cannot connect that User - it may not exist`
);
});

Expand Down Expand Up @@ -192,7 +192,7 @@ describe(`Custom mutations`, () => {
expect(data).toEqual({ addToCart: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
`An error occured while resolving relationship fields.\n - CartItem.product: Access denied: You cannot perform the 'connect' operation on the item '{"id":"${product.id}"}'. It may not exist.`
`An error occured while resolving relationship fields.\n - CartItem.product: Access denied: You cannot connect that Product - it may not exist`
);
});

Expand All @@ -216,7 +216,7 @@ describe(`Custom mutations`, () => {
expect(data).toEqual({ addToCart: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
`An error occured while resolving relationship fields.\n - CartItem.product: Access denied: You cannot perform the 'connect' operation on the item '{"id":"${product.id}"}'. It may not exist.`
`An error occured while resolving relationship fields.\n - CartItem.product: Access denied: You cannot connect that Product - it may not exist`
);
});

Expand Down
4 changes: 2 additions & 2 deletions examples/testing/example.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ test('Check access control by running updateTask as a specific user via context.
expect(errors).toHaveLength(1);
expect(errors![0].path).toEqual(['updateTask']);
expect(errors![0].message).toEqual(
`Access denied: You cannot perform the 'update' operation on the item '{"id":"${task.id}"}'. It may not exist.`
`Access denied: You cannot update that Task - it may not exist`
);
}

Expand Down Expand Up @@ -123,7 +123,7 @@ test('Check access control by running updateTask as a specific user via context.
expect(errors).toHaveLength(1);
expect(errors![0].path).toEqual(['updateTask']);
expect(errors![0].message).toEqual(
`Access denied: You cannot perform the 'update' operation on the item '{"id":"${task.id}"}'. It may not exist.`
`Access denied: You cannot update that Task - it may not exist`
);
}
});
159 changes: 80 additions & 79 deletions packages/core/src/lib/core/access-control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ import { accessReturnError, extensionError } from './graphql-errors';
import { InitialisedList } from './types-for-lists';
import { InputFilter } from './where-inputs';

export function cannotForItem(operation: string, list: InitialisedList) {
return (
`You cannot ${operation} that ${list.listKey}` +
(operation === 'create' ? '' : ' - it may not exist')
);
}

export function cannotForItemFields(
operation: string,
list: InitialisedList,
fieldsDenied: string[]
) {
return `You cannot ${operation} that ${
list.listKey
} - you cannot ${operation} the fields ${JSON.stringify(fieldsDenied)}`;
}

export async function getOperationAccess(
list: InitialisedList,
context: KeystoneContext,
Expand Down Expand Up @@ -56,60 +73,77 @@ export async function getAccessFilters(
context: KeystoneContext,
operation: 'update' | 'query' | 'delete'
): Promise<boolean | InputFilter> {
const args = { operation, session: context.session, listKey: list.listKey, context };
// Check the mutation access
const access = list.access.filter[operation];
try {
// @ts-ignore
let filters = typeof access === 'function' ? await access(args) : access;
if (typeof filters === 'boolean') {
return filters;
}
const filters = await access({
operation,
session: context.session,
list: list.listKey,
context,
} as any); // TODO: FIXME
if (typeof filters === 'boolean') return filters;

const schema = context.sudo().graphql.schema;
const whereInput = assertInputObjectType(schema.getType(getGqlNames(list).whereInputName));
const result = coerceAndValidateForGraphQLInput(schema, whereInput, filters);
if (result.kind === 'valid') {
return result.value;
}
if (result.kind === 'valid') return result.value;
throw result.error;
} catch (error: any) {
throw extensionError('Access control', [
{ error, tag: `${args.listKey}.access.filter.${args.operation}` },
{ error, tag: `${list.listKey}.access.filter.${operation}` },
]);
}
}

export type ResolvedFieldAccessControl = {
create: IndividualFieldAccessControl<FieldCreateItemAccessArgs<BaseListTypeInfo>>;
read: IndividualFieldAccessControl<FieldReadItemAccessArgs<BaseListTypeInfo>>;
update: IndividualFieldAccessControl<FieldUpdateItemAccessArgs<BaseListTypeInfo>>;
};

export function parseFieldAccessControl(
access: FieldAccessControl<BaseListTypeInfo> | undefined
): ResolvedFieldAccessControl {
if (typeof access === 'boolean' || typeof access === 'function') {
if (typeof access === 'function') {
dcousens marked this conversation as resolved.
Show resolved Hide resolved
return { read: access, create: access, update: access };
}
// note i'm intentionally not using spread here because typescript can't express an optional property which cannot be undefined so spreading would mean there is a possibility that someone could pass {access: undefined} or {access:{read: undefined}} and bad things would happen

return {
read: access?.read ?? (() => true),
create: access?.create ?? (() => true),
update: access?.update ?? (() => true),
// delete: not supported
};
}

export type ResolvedFieldAccessControl = {
read: IndividualFieldAccessControl<FieldReadItemAccessArgs<BaseListTypeInfo>>;
create: IndividualFieldAccessControl<FieldCreateItemAccessArgs<BaseListTypeInfo>>;
update: IndividualFieldAccessControl<FieldUpdateItemAccessArgs<BaseListTypeInfo>>;
export type ResolvedListAccessControl = {
operation: {
query: ListOperationAccessControl<'query', BaseListTypeInfo>;
create: ListOperationAccessControl<'create', BaseListTypeInfo>;
update: ListOperationAccessControl<'update', BaseListTypeInfo>;
delete: ListOperationAccessControl<'delete', BaseListTypeInfo>;
};
filter: {
query: ListFilterAccessControl<'query', BaseListTypeInfo>;
// create: not supported
update: ListFilterAccessControl<'update', BaseListTypeInfo>;
delete: ListFilterAccessControl<'delete', BaseListTypeInfo>;
};
item: {
// query: not supported
create: CreateListItemAccessControl<BaseListTypeInfo>;
update: UpdateListItemAccessControl<BaseListTypeInfo>;
delete: DeleteListItemAccessControl<BaseListTypeInfo>;
};
};

export function parseListAccessControl(
access: ListAccessControl<BaseListTypeInfo>
): ResolvedListAccessControl {
let item, filter, operation;

if (typeof access === 'function') {
return {
operation: {
create: access,
query: access,
create: access,
update: access,
delete: access,
},
Expand All @@ -126,67 +160,34 @@ export function parseListAccessControl(
};
}

if (typeof access?.operation === 'function') {
let { operation, filter, item } = access;
if (typeof operation === 'function') {
operation = {
create: access.operation,
query: access.operation,
update: access.operation,
delete: access.operation,
};
} else {
// Note I'm intentionally not using spread here because typescript can't express
// an optional property which cannot be undefined so spreading would mean there
// is a possibility that someone could pass { access: undefined } or
// { access: { read: undefined } } and bad things would happen.
operation = {
create: access?.operation?.create ?? (() => true),
query: access?.operation?.query ?? (() => true),
update: access?.operation?.update ?? (() => true),
delete: access?.operation?.delete ?? (() => true),
query: operation,
create: operation,
update: operation,
delete: operation,
};
}

if (typeof access?.filter === 'boolean' || typeof access?.filter === 'function') {
filter = { query: access.filter, update: access.filter, delete: access.filter };
} else {
filter = {
return {
operation: {
query: operation.query ?? (() => true),
create: operation.create ?? (() => true),
update: operation.update ?? (() => true),
delete: operation.delete ?? (() => true),
},
filter: {
query: filter?.query ?? (() => true),
// create: not supported
query: access?.filter?.query ?? (() => true),
update: access?.filter?.update ?? (() => true),
delete: access?.filter?.delete ?? (() => true),
};
}

if (typeof access?.item === 'boolean' || typeof access?.item === 'function') {
item = { create: access.item, update: access.item, delete: access.item };
} else {
item = {
create: access?.item?.create ?? (() => true),
// read: not supported
update: access?.item?.update ?? (() => true),
delete: access?.item?.delete ?? (() => true),
};
}
return { operation, filter, item };
}

export type ResolvedListAccessControl = {
operation: {
create: ListOperationAccessControl<'create', BaseListTypeInfo>;
query: ListOperationAccessControl<'query', BaseListTypeInfo>;
update: ListOperationAccessControl<'update', BaseListTypeInfo>;
delete: ListOperationAccessControl<'delete', BaseListTypeInfo>;
};
filter: {
// create: not supported
query: ListFilterAccessControl<'query', BaseListTypeInfo>;
update: ListFilterAccessControl<'update', BaseListTypeInfo>;
delete: ListFilterAccessControl<'delete', BaseListTypeInfo>;
update: filter?.update ?? (() => true),
delete: filter?.delete ?? (() => true),
},
item: {
// query: not supported
create: item?.create ?? (() => true),
update: item?.update ?? (() => true),
delete: item?.delete ?? (() => true),
},
};
item: {
create: CreateListItemAccessControl<BaseListTypeInfo>;
// query: not supported
update: UpdateListItemAccessControl<BaseListTypeInfo>;
delete: DeleteListItemAccessControl<BaseListTypeInfo>;
};
};
}
Loading