From fa40df2bc7f0f42173fbcdfc2b1f6cc22afd444e Mon Sep 17 00:00:00 2001
From: Harry Hogg <hhogg@spotify.com>
Date: Thu, 6 Oct 2022 12:04:31 +0100
Subject: [PATCH] Made changs to allow params and schemas to be defaulted and
 required only when there is params defined.

Co-authored-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Co-authored-by: Mike Lewis <mlewis@spotify.com>

Signed-off-by: Harry Hogg <hhogg@spotify.com>
---
 plugins/catalog-backend/api-report.md         |  2 +-
 plugins/permission-common/api-report.md       |  6 ++-
 plugins/permission-common/src/types/api.ts    |  6 ++-
 plugins/permission-node/api-report.md         | 12 ++---
 .../src/integration/createConditionExports.ts |  4 +-
 .../createConditionFactory.test.ts            |  1 +
 .../src/integration/createConditionFactory.ts | 26 ++++++-----
 .../integration/createConditionTransformer.ts |  6 +--
 .../createPermissionIntegrationRouter.test.ts | 44 ++-----------------
 .../createPermissionIntegrationRouter.ts      | 10 ++---
 .../src/integration/createPermissionRule.ts   |  4 +-
 .../src/integration/util.test.ts              |  3 --
 plugins/permission-node/src/types.ts          |  4 +-
 plugins/playlist-backend/api-report.md        |  2 +-
 .../DefaultPlaylistPermissionPolicy.test.ts   |  4 +-
 .../DefaultPlaylistPermissionPolicy.ts        |  2 +-
 .../playlist-backend/src/permissions/rules.ts |  1 -
 17 files changed, 53 insertions(+), 84 deletions(-)

diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md
index ea9b0b2277..6935243cf5 100644
--- a/plugins/catalog-backend/api-report.md
+++ b/plugins/catalog-backend/api-report.md
@@ -303,7 +303,7 @@ export const createCatalogConditionalDecision: (
 
 // @alpha
 export const createCatalogPermissionRule: <
-  TParams extends PermissionRuleParams = PermissionRuleParams,
+  TParams extends PermissionRuleParams = undefined,
 >(
   rule: PermissionRule<Entity, EntitiesSearchFilter, 'catalog-entity', TParams>,
 ) => PermissionRule<Entity, EntitiesSearchFilter, 'catalog-entity', TParams>;
diff --git a/plugins/permission-common/api-report.md b/plugins/permission-common/api-report.md
index b51b82c907..1c2476d20d 100644
--- a/plugins/permission-common/api-report.md
+++ b/plugins/permission-common/api-report.md
@@ -177,7 +177,7 @@ export type PermissionCondition<
 > = {
   resourceType: TResourceType;
   rule: string;
-  params: TParams;
+  params?: TParams;
 };
 
 // @public
@@ -208,7 +208,9 @@ export type PermissionMessageBatch<T> = {
 export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[];
 
 // @public
-export type PermissionRuleParams = Record<string, PermissionRuleParam>;
+export type PermissionRuleParams =
+  | undefined
+  | Record<string, PermissionRuleParam>;
 
 // @public
 export type PolicyDecision =
diff --git a/plugins/permission-common/src/types/api.ts b/plugins/permission-common/src/types/api.ts
index 77d6206c07..5e7a2bb349 100644
--- a/plugins/permission-common/src/types/api.ts
+++ b/plugins/permission-common/src/types/api.ts
@@ -106,7 +106,7 @@ export type PermissionCondition<
 > = {
   resourceType: TResourceType;
   rule: string;
-  params: TParams;
+  params?: TParams;
 };
 
 /**
@@ -161,7 +161,9 @@ export type PermissionRuleParam = undefined | JsonPrimitive | JsonPrimitive[];
  *
  * @public
  */
-export type PermissionRuleParams = Record<string, PermissionRuleParam>;
+export type PermissionRuleParams =
+  | undefined
+  | Record<string, PermissionRuleParam>;
 
 /**
  * An individual request sent to the permission backend.
diff --git a/plugins/permission-node/api-report.md b/plugins/permission-node/api-report.md
index b272aa9b1c..4bc654cd73 100644
--- a/plugins/permission-node/api-report.md
+++ b/plugins/permission-node/api-report.md
@@ -55,7 +55,9 @@ export type Condition<TRule> = TRule extends PermissionRule<
   infer TResourceType,
   infer TParams
 >
-  ? (params: TParams) => PermissionCondition<TResourceType, TParams>
+  ? undefined extends TParams
+    ? () => PermissionCondition<TResourceType, TParams>
+    : (params: TParams) => PermissionCondition<TResourceType, TParams>
   : never;
 
 // @public
@@ -98,7 +100,7 @@ export const createConditionFactory: <
   TParams extends PermissionRuleParams = PermissionRuleParams,
 >(
   rule: PermissionRule<unknown, unknown, TResourceType, TParams>,
-) => (params: TParams) => PermissionCondition<TResourceType, TParams>;
+) => (args_0: TParams) => PermissionCondition<TResourceType, TParams>;
 
 // @public
 export const createConditionTransformer: <
@@ -129,7 +131,7 @@ export const createPermissionRule: <
   TResource,
   TQuery,
   TResourceType extends string,
-  TParams extends PermissionRuleParams = PermissionRuleParams,
+  TParams extends PermissionRuleParams = undefined,
 >(
   rule: PermissionRule<TResource, TQuery, TResourceType, TParams>,
 ) => PermissionRule<TResource, TQuery, TResourceType, TParams>;
@@ -154,7 +156,7 @@ export const makeCreatePermissionRule: <
   TResource,
   TQuery,
   TResourceType extends string,
->() => <TParams extends PermissionRuleParams = PermissionRuleParams>(
+>() => <TParams extends PermissionRuleParams = undefined>(
   rule: PermissionRule<TResource, TQuery, TResourceType, TParams>,
 ) => PermissionRule<TResource, TQuery, TResourceType, TParams>;
 
@@ -177,7 +179,7 @@ export type PermissionRule<
   name: string;
   description: string;
   resourceType: TResourceType;
-  paramsSchema: PermissionRuleSchema<TParams>;
+  paramsSchema?: PermissionRuleSchema<TParams>;
   apply(
     resource: TResource,
     params: NoInfer<z.input<PermissionRuleSchema<TParams>>>,
diff --git a/plugins/permission-node/src/integration/createConditionExports.ts b/plugins/permission-node/src/integration/createConditionExports.ts
index 3921c2dfe3..1c82434add 100644
--- a/plugins/permission-node/src/integration/createConditionExports.ts
+++ b/plugins/permission-node/src/integration/createConditionExports.ts
@@ -36,7 +36,9 @@ export type Condition<TRule> = TRule extends PermissionRule<
   infer TResourceType,
   infer TParams
 >
-  ? (params: TParams) => PermissionCondition<TResourceType, TParams>
+  ? undefined extends TParams
+    ? () => PermissionCondition<TResourceType, TParams>
+    : (params: TParams) => PermissionCondition<TResourceType, TParams>
   : never;
 
 /**
diff --git a/plugins/permission-node/src/integration/createConditionFactory.test.ts b/plugins/permission-node/src/integration/createConditionFactory.test.ts
index 2b3acff342..520fcc01f9 100644
--- a/plugins/permission-node/src/integration/createConditionFactory.test.ts
+++ b/plugins/permission-node/src/integration/createConditionFactory.test.ts
@@ -37,6 +37,7 @@ describe('createConditionFactory', () => {
   describe('return value', () => {
     it('constructs a condition with the rule name and supplied params', () => {
       const conditionFactory = createConditionFactory(testRule);
+
       expect(
         conditionFactory({
           foo: 'bar',
diff --git a/plugins/permission-node/src/integration/createConditionFactory.ts b/plugins/permission-node/src/integration/createConditionFactory.ts
index 8edeb6230f..3a31e07dac 100644
--- a/plugins/permission-node/src/integration/createConditionFactory.ts
+++ b/plugins/permission-node/src/integration/createConditionFactory.ts
@@ -36,15 +36,17 @@ import { PermissionRule } from '../types';
  *
  * @public
  */
-export const createConditionFactory =
-  <
-    TResourceType extends string,
-    TParams extends PermissionRuleParams = PermissionRuleParams,
-  >(
-    rule: PermissionRule<unknown, unknown, TResourceType, TParams>,
-  ) =>
-  (params: TParams): PermissionCondition<TResourceType, TParams> => ({
-    rule: rule.name,
-    resourceType: rule.resourceType,
-    params,
-  });
+export const createConditionFactory = <
+  TResourceType extends string,
+  TParams extends PermissionRuleParams = PermissionRuleParams,
+>(
+  rule: PermissionRule<unknown, unknown, TResourceType, TParams>,
+) => {
+  return (...args: [TParams]): PermissionCondition<TResourceType, TParams> => {
+    return {
+      rule: rule.name,
+      resourceType: rule.resourceType,
+      params: args[0],
+    };
+  };
+};
diff --git a/plugins/permission-node/src/integration/createConditionTransformer.ts b/plugins/permission-node/src/integration/createConditionTransformer.ts
index dc0b11e90e..4b6f933f79 100644
--- a/plugins/permission-node/src/integration/createConditionTransformer.ts
+++ b/plugins/permission-node/src/integration/createConditionTransformer.ts
@@ -47,13 +47,13 @@ const mapConditions = <TQuery>(
   }
 
   const rule = getRule(criteria.rule);
-  const result = rule.paramsSchema.safeParse(criteria.params);
+  const result = rule.paramsSchema?.safeParse(criteria.params);
 
-  if (rule.paramsSchema && !result.success) {
+  if (result && !result.success) {
     throw new InputError(`Parameters to rule are invalid`, result.error);
   }
 
-  return rule.toQuery(criteria.params);
+  return rule.toQuery(criteria.params ?? {});
 };
 
 /**
diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts
index 55d4d5f4cf..32d814465f 100644
--- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts
+++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.test.ts
@@ -52,11 +52,8 @@ const testRule2 = createPermissionRule({
   name: 'test-rule-2',
   description: 'Test rule 2',
   resourceType: 'test-resource',
-  paramsSchema: z.object({
-    foo: z.string().describe('foo'),
-  }),
-  apply: (_resource: any, _foo) => false,
-  toQuery: _foo => ({}),
+  apply: (_resource: any) => false,
+  toQuery: () => ({}),
 });
 
 describe('createPermissionIntegrationRouter', () => {
@@ -105,7 +102,6 @@ describe('createPermissionIntegrationRouter', () => {
           {
             rule: 'test-rule-2',
             resourceType: 'test-resource',
-            params: { foo: 'b' },
           },
         ],
       },
@@ -113,9 +109,6 @@ describe('createPermissionIntegrationRouter', () => {
         not: {
           rule: 'test-rule-2',
           resourceType: 'test-resource',
-          params: {
-            foo: 'c',
-          },
         },
       },
       {
@@ -133,9 +126,6 @@ describe('createPermissionIntegrationRouter', () => {
               {
                 rule: 'test-rule-2',
                 resourceType: 'test-resource',
-                params: {
-                  foo: 'b',
-                },
               },
             ],
           },
@@ -153,9 +143,6 @@ describe('createPermissionIntegrationRouter', () => {
                 {
                   rule: 'test-rule-2',
                   resourceType: 'test-resource',
-                  params: {
-                    foo: 'c',
-                  },
                 },
               ],
             },
@@ -191,9 +178,6 @@ describe('createPermissionIntegrationRouter', () => {
       {
         rule: 'test-rule-2',
         resourceType: 'test-resource',
-        params: {
-          foo: 'a',
-        },
       },
       {
         allOf: [
@@ -208,7 +192,6 @@ describe('createPermissionIntegrationRouter', () => {
           {
             rule: 'test-rule-2',
             resourceType: 'test-resource',
-            params: { foo: 'b' },
           },
         ],
       },
@@ -227,9 +210,6 @@ describe('createPermissionIntegrationRouter', () => {
               {
                 rule: 'test-rule-2',
                 resourceType: 'test-resource',
-                params: {
-                  foo: 'b',
-                },
               },
             ],
           },
@@ -248,9 +228,6 @@ describe('createPermissionIntegrationRouter', () => {
                   not: {
                     rule: 'test-rule-2',
                     resourceType: 'test-resource',
-                    params: {
-                      foo: 'd',
-                    },
                   },
                 },
               ],
@@ -309,9 +286,6 @@ describe('createPermissionIntegrationRouter', () => {
                 conditions: {
                   rule: 'test-rule-2',
                   resourceType: 'test-resource',
-                  params: {
-                    foo: 'b',
-                  },
                 },
               },
               {
@@ -337,9 +311,6 @@ describe('createPermissionIntegrationRouter', () => {
                   not: {
                     rule: 'test-rule-2',
                     resourceType: 'test-resource',
-                    params: {
-                      foo: 'c',
-                    },
                   },
                 },
               },
@@ -360,9 +331,6 @@ describe('createPermissionIntegrationRouter', () => {
                     {
                       rule: 'test-rule-2',
                       resourceType: 'test-resource',
-                      params: {
-                        foo: 'd',
-                      },
                     },
                   ],
                 },
@@ -626,13 +594,7 @@ describe('createPermissionIntegrationRouter', () => {
             paramsSchema: {
               $schema: 'http://json-schema.org/draft-07/schema#',
               additionalProperties: false,
-              properties: {
-                foo: {
-                  description: 'foo',
-                  type: 'string',
-                },
-              },
-              required: ['foo'],
+              properties: {},
               type: 'object',
             },
           },
diff --git a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts
index c2d99ac4a4..52f34d7e6a 100644
--- a/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts
+++ b/plugins/permission-node/src/integration/createPermissionIntegrationRouter.ts
@@ -47,7 +47,7 @@ const permissionCriteriaSchema: z.ZodSchema<
     z.object({
       rule: z.string(),
       resourceType: z.string(),
-      params: z.record(z.any()),
+      params: z.record(z.any()).optional(),
     }),
   ]),
 );
@@ -127,13 +127,13 @@ const applyConditions = <TResourceType extends string, TResource>(
   }
 
   const rule = getRule(criteria.rule);
-  const result = rule.paramsSchema.safeParse(criteria.params);
+  const result = rule.paramsSchema?.safeParse(criteria.params);
 
-  if (rule.paramsSchema && !result.success) {
+  if (result && !result.success) {
     throw new InputError(`Parameters to rule are invalid`, result.error);
   }
 
-  return rule.apply(resource, criteria.params);
+  return rule.apply(resource, criteria.params ?? {});
 };
 
 /**
@@ -195,7 +195,7 @@ export const createPermissionIntegrationRouter = <
       name: rule.name,
       description: rule.description,
       resourceType: rule.resourceType,
-      paramsSchema: zodToJsonSchema(rule.paramsSchema),
+      paramsSchema: zodToJsonSchema(rule.paramsSchema ?? z.object({})),
     }));
 
     return res.json({ permissions, rules: serializableRules });
diff --git a/plugins/permission-node/src/integration/createPermissionRule.ts b/plugins/permission-node/src/integration/createPermissionRule.ts
index ea2e37a671..44187e58a6 100644
--- a/plugins/permission-node/src/integration/createPermissionRule.ts
+++ b/plugins/permission-node/src/integration/createPermissionRule.ts
@@ -26,7 +26,7 @@ export const createPermissionRule = <
   TResource,
   TQuery,
   TResourceType extends string,
-  TParams extends PermissionRuleParams = PermissionRuleParams,
+  TParams extends PermissionRuleParams = undefined,
 >(
   rule: PermissionRule<TResource, TQuery, TResourceType, TParams>,
 ) => rule;
@@ -41,7 +41,7 @@ export const createPermissionRule = <
  */
 export const makeCreatePermissionRule =
   <TResource, TQuery, TResourceType extends string>() =>
-  <TParams extends PermissionRuleParams = PermissionRuleParams>(
+  <TParams extends PermissionRuleParams = undefined>(
     rule: PermissionRule<TResource, TQuery, TResourceType, TParams>,
   ) =>
     createPermissionRule(rule);
diff --git a/plugins/permission-node/src/integration/util.test.ts b/plugins/permission-node/src/integration/util.test.ts
index b1961ae532..6c1c270953 100644
--- a/plugins/permission-node/src/integration/util.test.ts
+++ b/plugins/permission-node/src/integration/util.test.ts
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import { z } from 'zod';
 import { createPermissionRule } from './createPermissionRule';
 import {
   createGetRule,
@@ -31,7 +30,6 @@ describe('permission integration utils', () => {
       name: 'test-rule-1',
       description: 'Test rule 1',
       resourceType: 'test-resource',
-      paramsSchema: z.object({}),
       apply: jest.fn(),
       toQuery: jest.fn(),
     });
@@ -40,7 +38,6 @@ describe('permission integration utils', () => {
       name: 'test-rule-2',
       description: 'Test rule 2',
       resourceType: 'test-resource',
-      paramsSchema: z.object({}),
       apply: jest.fn(),
       toQuery: jest.fn(),
     });
diff --git a/plugins/permission-node/src/types.ts b/plugins/permission-node/src/types.ts
index df21889fb9..6b15df407e 100644
--- a/plugins/permission-node/src/types.ts
+++ b/plugins/permission-node/src/types.ts
@@ -64,9 +64,9 @@ export type PermissionRule<
   resourceType: TResourceType;
 
   /**
-   * A ZodSchema that documents the parameters that this rule accepts.
+   * A ZodSchema that reflects the structure of the parameters that are passed to
    */
-  paramsSchema: PermissionRuleSchema<TParams>;
+  paramsSchema?: PermissionRuleSchema<TParams>;
 
   /**
    * Apply this rule to a resource already loaded from a backing data source. The params are
diff --git a/plugins/playlist-backend/api-report.md b/plugins/playlist-backend/api-report.md
index faae97a734..6b51f316c4 100644
--- a/plugins/playlist-backend/api-report.md
+++ b/plugins/playlist-backend/api-report.md
@@ -79,7 +79,7 @@ export const playlistConditions: Conditions<{
     PlaylistMetadata,
     ListPlaylistsFilter,
     'playlist-list',
-    PermissionRuleParams
+    undefined
   >;
 }>;
 
diff --git a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts
index 3d761bfd35..17ee4a840a 100644
--- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts
+++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.test.ts
@@ -70,7 +70,7 @@ describe('DefaultPlaylistPermissionPolicy', () => {
           playlistConditions.isOwner({
             owners: ['user:default/me', 'group:default/owner'],
           }),
-          playlistConditions.isPublic({}),
+          playlistConditions.isPublic(),
         ],
       },
     });
@@ -91,7 +91,7 @@ describe('DefaultPlaylistPermissionPolicy', () => {
           playlistConditions.isOwner({
             owners: ['user:default/me', 'group:default/owner'],
           }),
-          playlistConditions.isPublic({}),
+          playlistConditions.isPublic(),
         ],
       },
     });
diff --git a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts
index 317f5da9c8..b4556fbb4f 100644
--- a/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts
+++ b/plugins/playlist-backend/src/permissions/DefaultPlaylistPermissionPolicy.ts
@@ -71,7 +71,7 @@ export class DefaultPlaylistPermissionPolicy implements PermissionPolicy {
           playlistConditions.isOwner({
             owners: user?.identity.ownershipEntityRefs ?? [],
           }),
-          playlistConditions.isPublic({}),
+          playlistConditions.isPublic(),
         ],
       });
     }
diff --git a/plugins/playlist-backend/src/permissions/rules.ts b/plugins/playlist-backend/src/permissions/rules.ts
index cd6711befb..99f4725972 100644
--- a/plugins/playlist-backend/src/permissions/rules.ts
+++ b/plugins/playlist-backend/src/permissions/rules.ts
@@ -47,7 +47,6 @@ const isPublic = createPlaylistPermissionRule({
   name: 'IS_PUBLIC',
   description: 'Should allow only if the playlist is public',
   resourceType: PLAYLIST_LIST_RESOURCE_TYPE,
-  paramsSchema: z.object({}),
   apply: (list: PlaylistMetadata) => list.public,
   toQuery: () => ({ key: 'public', values: [true] }),
 });