From 733e7aa0b304530950cc253cd7085ef297520b36 Mon Sep 17 00:00:00 2001 From: Tomer Eskenazi Date: Thu, 2 Jul 2020 00:27:12 +0300 Subject: [PATCH] Policy directive - accept only a single policy (#146) * change policy directive to accept only a single policy * Refactored PolicyExecutor API to only expose static methods --- .../directives/policy/policy-executor.ts | 73 +++++++++++-------- .../modules/directives/policy/policy-query.ts | 3 +- .../src/modules/directives/policy/policy.ts | 14 +--- .../authorization_with_queries.spec.ts.snap | 18 ++--- .../tests/authorization_with_queries.spec.ts | 9 +-- services/src/tests/helpers/authzSchema.ts | 14 ++-- 6 files changed, 65 insertions(+), 66 deletions(-) diff --git a/services/src/modules/directives/policy/policy-executor.ts b/services/src/modules/directives/policy/policy-executor.ts index f2198c16..e2caecd7 100644 --- a/services/src/modules/directives/policy/policy-executor.ts +++ b/services/src/modules/directives/policy/policy-executor.ts @@ -1,7 +1,7 @@ import {GraphQLResolveInfo, graphql} from 'graphql'; import {RequestContext} from '../../context'; import {Policy, GraphQLArguments, QueryResults} from './types'; -import {Policy as PolicyDefinition, PolicyArgsObject, PolicyAttachments, PolicyQuery} from '../../resource-repository'; +import {Policy as PolicyDefinition, PolicyArgsObject, PolicyAttachments} from '../../resource-repository'; import {evaluate as evaluateOpa} from './opa'; import {injectParameters, resolveParameters} from '../../paramInjection'; @@ -10,36 +10,53 @@ const typeEvaluators = { }; export class PolicyExecutor { - private policyDefinitions: PolicyDefinition[]; + private policyDefinition: PolicyDefinition; private policyAttachments: PolicyAttachments; - constructor( - protected policies: Policy[], + private constructor( + protected policy: Policy, protected parent: any, protected args: GraphQLArguments, protected context: RequestContext, protected info: GraphQLResolveInfo ) { - this.policyDefinitions = context.policies; + this.policyDefinition = this.getPolicyDefinition(context.policies, this.policy.namespace, this.policy.name); this.policyAttachments = context.policyAttachments; } - async validatePolicies() { - await Promise.all(this.policies.map(r => this.validatePolicy(r))); + static async evaluatePolicy( + policy: Policy, + parent: any, + args: GraphQLArguments, + context: RequestContext, + info: GraphQLResolveInfo + ): Promise { + const executor = new PolicyExecutor(policy, parent, args, context, info); + return executor.evaluatePolicy(); } - async evaluatePolicy(policy: Policy): Promise { - const policyDefinition = this.getPolicyDefinition(policy.namespace, policy.name); - - const args = policyDefinition.args && this.preparePolicyArgs(policyDefinition.args, policy); + static async validatePolicy( + policy: Policy, + parent: any, + args: GraphQLArguments, + context: RequestContext, + info: GraphQLResolveInfo + ): Promise { + const executor = new PolicyExecutor(policy, parent, args, context, info); + const allow = await executor.evaluatePolicy(); + if (!allow) + throw new Error(`Unauthorized by policy ${executor.policy.name} in namespace ${executor.policy.namespace}`); + } - const query = policyDefinition.query && (await this.evaluatePolicyQuery(policyDefinition.query, args)); + private async evaluatePolicy(): Promise { + const args = this.preparePolicyArgs(); + const query = await this.evaluatePolicyQuery(args); - const evaluate = typeEvaluators[policyDefinition.type]; - if (!evaluate) throw new Error(`Unsupported policy type ${policyDefinition.type}`); + const evaluate = typeEvaluators[this.policyDefinition.type]; + if (!evaluate) throw new Error(`Unsupported policy type ${this.policyDefinition.type}`); const {done, allow} = await evaluate({ - ...policy, + ...this.policy, args, query, policyAttachments: this.policyAttachments, @@ -48,20 +65,18 @@ export class PolicyExecutor { return allow || false; } - async validatePolicy(policy: Policy): Promise { - const allow = await this.evaluatePolicy(policy); - if (!allow) throw new Error(`Unauthorized by policy ${policy.name} in namespace ${policy.namespace}`); - } + private preparePolicyArgs(): PolicyArgsObject | undefined { + const supportedPolicyArgs = this.policyDefinition.args; + if (!supportedPolicyArgs) return; - private preparePolicyArgs(supportedPolicyArgs: PolicyArgsObject, policy: Policy): PolicyArgsObject { return Object.entries(supportedPolicyArgs).reduce( (policyArgs, [policyArgName, policyArgType]) => { - if (policy?.args?.[policyArgName] === undefined) + if (this.policy?.args?.[policyArgName] === undefined) throw new Error( - `Missing arg ${policyArgName} for policy ${policy.name} in namespace ${policy.namespace}` + `Missing arg ${policyArgName} for policy ${this.policy.name} in namespace ${this.policy.namespace}` ); - let policyArgValue = policy.args[policyArgName]; + let policyArgValue = this.policy.args[policyArgName]; if (typeof policyArgValue === 'string') { if (policyArgType === 'String') { policyArgValue = injectParameters( @@ -92,8 +107,8 @@ export class PolicyExecutor { ); } - private getPolicyDefinition(namespace: string, name: string) { - const policyDefinition = this.policyDefinitions.find(({metadata}) => { + private getPolicyDefinition(policyDefinitions: PolicyDefinition[], namespace: string, name: string) { + const policyDefinition = policyDefinitions.find(({metadata}) => { return metadata.namespace === namespace && metadata.name === name; }); @@ -101,10 +116,10 @@ export class PolicyExecutor { return policyDefinition; } - private async evaluatePolicyQuery( - query: PolicyQuery, - args: PolicyArgsObject = {} - ): Promise { + private async evaluatePolicyQuery(args: PolicyArgsObject = {}): Promise { + const query = this.policyDefinition.query; + if (!query) return; + let variableValues = query.variables && Object.entries(query.variables).reduce<{[key: string]: any}>((policyArgs, [varName, varValue]) => { diff --git a/services/src/modules/directives/policy/policy-query.ts b/services/src/modules/directives/policy/policy-query.ts index da1cca72..1cb274c7 100644 --- a/services/src/modules/directives/policy/policy-query.ts +++ b/services/src/modules/directives/policy/policy-query.ts @@ -19,8 +19,7 @@ export class PolicyQueryDirective extends SchemaDirectiveVisitor { args: args, }; - const executor = new PolicyExecutor([], parent, args, context, info); - const allow = await executor.evaluatePolicy(policy); + const allow = await PolicyExecutor.evaluatePolicy(policy, parent, args, context, info); return {allow}; }; } diff --git a/services/src/modules/directives/policy/policy.ts b/services/src/modules/directives/policy/policy.ts index ea780268..d5e5db16 100644 --- a/services/src/modules/directives/policy/policy.ts +++ b/services/src/modules/directives/policy/policy.ts @@ -4,16 +4,16 @@ import {SchemaDirectiveVisitor} from 'graphql-tools'; import {GraphQLField, defaultFieldResolver} from 'graphql'; import {gql} from 'apollo-server-core'; import {PolicyExecutor} from './policy-executor'; +import {Policy} from './types'; export class PolicyDirective extends SchemaDirectiveVisitor { visitFieldDefinition(field: GraphQLField) { const originalResolve = field.resolve || defaultFieldResolver; - const policies = this.args.policies; + const policy = this.args as Policy; field.resolve = async (parent: any, args: any, context: RequestContext, info: GraphQLResolveInfo) => { if (!context.ignorePolicies) { - const executor = new PolicyExecutor(policies, parent, args, context, info); - await executor.validatePolicies(); + await PolicyExecutor.validatePolicy(policy, parent, args, context, info); } return originalResolve.call(field, parent, args, context, info); @@ -22,11 +22,5 @@ export class PolicyDirective extends SchemaDirectiveVisitor { } export const sdl = gql` - input PolicyDirectivePolicy { - namespace: String! - name: String! - args: JSONObject - } - - directive @policy(policies: [PolicyDirectivePolicy!]!) on FIELD_DEFINITION + directive @policy(namespace: String!, name: String!, args: JSONObject) on FIELD_DEFINITION `; diff --git a/services/src/tests/e2e/tests/__snapshots__/authorization_with_queries.spec.ts.snap b/services/src/tests/e2e/tests/__snapshots__/authorization_with_queries.spec.ts.snap index f7a47bcb..d0c4dab3 100644 --- a/services/src/tests/e2e/tests/__snapshots__/authorization_with_queries.spec.ts.snap +++ b/services/src/tests/e2e/tests/__snapshots__/authorization_with_queries.spec.ts.snap @@ -10,10 +10,8 @@ Object { "exception": Object { "stacktrace": Array [ "Error: Unauthorized by policy alwaysDenied in namespace namespace", - " at PolicyExecutor.validatePolicy (/service/dist/modules/directives/policy/policy-executor.js:42:19)", - " at async Promise.all (index 0)", - " at async PolicyExecutor.validatePolicies (/service/dist/modules/directives/policy/policy-executor.js:20:9)", - " at async field.resolve (/service/dist/modules/directives/policy/policy.js:14:17)", + " at Function.validatePolicy (/service/dist/modules/directives/policy/policy-executor.js:27:19)", + " at async field.resolve (/service/dist/modules/directives/policy/policy.js:13:17)", ], }, }, @@ -58,10 +56,8 @@ Object { "exception": Object { "stacktrace": Array [ "Error: Unauthorized by policy notClassified in namespace namespace", - " at PolicyExecutor.validatePolicy (/service/dist/modules/directives/policy/policy-executor.js:42:19)", - " at async Promise.all (index 0)", - " at async PolicyExecutor.validatePolicies (/service/dist/modules/directives/policy/policy-executor.js:20:9)", - " at async field.resolve (/service/dist/modules/directives/policy/policy.js:14:17)", + " at Function.validatePolicy (/service/dist/modules/directives/policy/policy-executor.js:27:19)", + " at async field.resolve (/service/dist/modules/directives/policy/policy.js:13:17)", ], }, }, @@ -99,10 +95,8 @@ Object { "exception": Object { "stacktrace": Array [ "Error: Unauthorized by policy notClassified in namespace namespace", - " at PolicyExecutor.validatePolicy (/service/dist/modules/directives/policy/policy-executor.js:42:19)", - " at async Promise.all (index 0)", - " at async PolicyExecutor.validatePolicies (/service/dist/modules/directives/policy/policy-executor.js:20:9)", - " at async field.resolve (/service/dist/modules/directives/policy/policy.js:14:17)", + " at Function.validatePolicy (/service/dist/modules/directives/policy/policy-executor.js:27:19)", + " at async field.resolve (/service/dist/modules/directives/policy/policy.js:13:17)", ], }, }, diff --git a/services/src/tests/e2e/tests/authorization_with_queries.spec.ts b/services/src/tests/e2e/tests/authorization_with_queries.spec.ts index 11404173..96602417 100644 --- a/services/src/tests/e2e/tests/authorization_with_queries.spec.ts +++ b/services/src/tests/e2e/tests/authorization_with_queries.spec.ts @@ -82,14 +82,14 @@ const schema: Schema = { name: String hireDate: Int department: Department! - address: String @policy(policies: [{ + address: String @policy( namespace: "namespace", name: "notClassified", args: { departmentId: "{source.department.id}", hireDate: "{source.hireDate}" } - }]) + ) } type Query { @@ -126,10 +126,7 @@ const schema: Schema = { classifiedDepartments: [Department!]! @stub(value: [{ id: "D1000" name: "VIP" - }]) @policy(policies: [{ - namespace: "namespace", - name: "alwaysDenied" - }]) + }]) @policy(namespace: "namespace", name: "alwaysDenied") } `, }; diff --git a/services/src/tests/helpers/authzSchema.ts b/services/src/tests/helpers/authzSchema.ts index 6bf40e9d..86752cf2 100644 --- a/services/src/tests/helpers/authzSchema.ts +++ b/services/src/tests/helpers/authzSchema.ts @@ -39,19 +39,19 @@ export const getSchema = () => ({ schema: ` type User { firstName: String - lastName: String @policy(policies: [ - { namespace: "ns", name: "onlyAdmin", args: { role: "{source.role}" } } - ]) + lastName: String @policy(namespace: "ns", name: "onlyAdmin", args: { role: "{source.role}" }) role: String } type ArbitraryData { - arbitraryField: String @policy(policies: [ - { namespace: "ns", name: "jwtName", args: { + arbitraryField: String @policy( + namespace: "ns", + name: "jwtName", + args: { jwtName: "{jwt.name}", allowedName: "Varg" - }} - ]) + } + ) } type Query {