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

Co-authored-by: Daniel Cousens <dcousens@users.noreply.github.com>

.changeset/errors-now-equal.md

@@ -0,0 +1,5 @@
+---
+'@keystone-6/core': major
+---
+
+Changes access-control error messages to only show the list key and operation

examples/ecommerce/tests/mutations.test.ts

@@ -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`
       );
     });
 
@@ -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`
       );
     });
 
@@ -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`
       );
     });
 

examples/testing/example.test.ts

@@ -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`
     );
   }
 
@@ -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`
     );
   }
 });

packages/core/src/lib/core/access-control.ts

@@ -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,
@@ -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') {
     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,
       },
@@ -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>;
-  };
-};
+}