fix: stricter merge rules for @requiresScopes and @Policy

Current merge policies for `@authenticated`, `@requiresScopes` and `@policy` were inconsistent. If single subgraph declared a field with one of the directives then it would restrict access to this supergraph field regardless which subgraph would resolve this field (results in `AND` rule for any applied auth directive, i.e. `@authenticated` AND `@policy` is required to access this field). If the same auth directive (`@requiresScopes`/`@policy`) were applied across the subgraphs then the resulting supergraph field could be resolved by fullfilling either one of the subgraph requirements (resulting in `OR` rule, i.e. either `@policy` 1 or `@policy` 2 has to be true to access the field). While arguably this allowed for easier schema evolution, it did result in weakening the security requirements. Since `@policy` and `@requiresScopes` values are represent boolean conditions in Disjunctive Normal Form, we can merge them conjunctively to get the final auth requirements, i.e. ```graphql type T @authenticated { # requires scopes (A1 AND A2) OR A3 secret: String @requiresScopes(scopes: [["A1", "A2"], ["A3"]]) } type T { # requires scopes B1 OR B2 secret: String @requiresScopes(scopes: [["B1"], ["B2"]] } type T @authenticated { secret: String @requiresScopes( scopes: [ ["A1", "A2", "B1"], ["A1", "A2", "B2"], ["A3", "B1"], ["A3", "B2"] ]) } ``` This algorithm also deduplicates redundant requirements, e.g. ```graphql type T { # requires A1 AND A2 scopes to access secret: String @requiresScopes(scopes: [["A1", "A2"]]) } type T { # requires only A1 scope to access secret: String @requiresScopes(scopes: [["A1"]]) } type T { # requires only A1 scope to access as A2 is redundant secret: String @requiresScopes(scopes: [["A1"]]) } ```

Author: dariuszkuc

.changeset/tasty-snails-invent.md

@@ -0,0 +1,59 @@
+---
+"@apollo/composition": patch
+"@apollo/federation-internals": patch
+---
+
+Stricter merge rules for @requiresScopes and @policy
+
+Current merge policies for `@authenticated`, `@requiresScopes` and `@policy` were inconsistent.
+
+If a shared field uses the same authorization directives across subgraphs, composition merges them using `OR` logic. However, if a shared field uses different authorization directives across subgraphs composition merges them using `AND` logic. This simplified schema evolution, but weakened security requirements. Therefore, the behavior has been changed to always apply `AND` logic to authorization directives applied to the same field across subgraphs.
+
+Since `@policy` and `@requiresScopes` values represent boolean conditions in Disjunctive Normal Form, we can merge them conjunctively to get the final auth requirements. For example:
+
+```graphql
+# subgraph A
+type T @authenticated {
+ # requires scopes (A1 AND A2) OR A3
+ secret: String @requiresScopes(scopes: [["A1", "A2"], ["A3"]])
+}
+
+# subgraph B
+type T {
+ # requires scopes B1 OR B2
+ secret: String @requiresScopes(scopes: [["B1"], ["B2"]]
+}
+
+# composed supergraph
+type T @authenticated {
+ secret: String @requiresScopes(
+ scopes: [
+ ["A1", "A2", "B1"],
+ ["A1", "A2", "B2"],
+ ["A3", "B1"],
+ ["A3", "B2"]
+ ])
+}
+```
+
+This algorithm also deduplicates redundant requirements, e.g.
+
+```graphql
+# subgraph A
+type T {
+ # requires A1 AND A2 scopes to access
+ secret: String @requiresScopes(scopes: [["A1", "A2"]])
+}
+
+# subgraph B
+type T {
+ # requires only A1 scope to access
+ secret: String @requiresScopes(scopes: [["A1"]])
+}
+
+# composed supergraph
+type T {
+ # requires only A1 scope to access as A2 is redundant
+ secret: String @requiresScopes(scopes: [["A1"]])
+}
+```

composition-js/src/__tests__/compose.directiveArgumentMergeStrategies.test.ts

@@ -159,6 +159,22 @@ describe('composition of directive with non-trivial argument strategies', () =>
  resultValues: {
  t: ['foo', 'bar'], k: ['v1', 'v2'], b: ['x'],
  },
+ },
+ {
+ name: 'dnf_conjunction',
+ // [[String!]!]!
+ type: (schema: Schema) => new NonNullType(new ListType(
+ new NonNullType(new ListType(
+ new NonNullType(schema.stringType())))
+ )),
+ compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.DNF_CONJUNCTION,
+ argValues: {
+ s1: { t: [['foo'], ['bar']], k: [['v1']] },
+ s2: { t: [['foo'], ['bar'], ['baz']], k: [['v2', 'v3']], b: [['x']] },
+ },
+ resultValues: {
+ t: [['bar'], ['foo']], k: [['v1', 'v2', 'v3']], b: [['x']],
+ },
  }])('works for $name', ({ name, type, compositionStrategy, argValues, resultValues }) => {
  createTestFeature({
  url: 'https://specs.apollo.dev',

composition-js/src/__tests__/compose.test.ts

@@ -4421,14 +4421,14 @@ describe('composition', () => {
  expect(result2.schema.type('A')?.hasAppliedDirective(directiveName.slice(1))).toBeTruthy();
  });
 
- it.each(testsToRun)('merges ${directiveName} lists (simple union)', ({ directiveName, argName }) => {
+ it.each(testsToRun)('merges $directiveName lists (outer sum)', ({ directiveName, argName }) => {
  const a1 = {
  typeDefs: gql`
  type Query {
  a: A
  }
 
- type A ${directiveName}(${argName}: ["a"]) @key(fields: "id") {
+ type A ${directiveName}(${argName}: [["a1", "a2"], ["a3"]]) @key(fields: "id") {
  id: String!
  a1: String
  }
@@ -4437,7 +4437,7 @@ describe('composition', () => {
  };
  const a2 = {
  typeDefs: gql`
- type A ${directiveName}(${argName}: ["b"]) @key(fields: "id") {
+ type A ${directiveName}(${argName}: [["b1"], ["b2"]]) @key(fields: "id") {
  id: String!
  a2: String
  }
@@ -4450,18 +4450,24 @@ describe('composition', () => {
  expect(
  result.schema.type('A')
  ?.appliedDirectivesOf(directiveName.slice(1))
- ?.[0]?.arguments()?.[argName]).toStrictEqual(['a', 'b']
+ ?.[0]?.arguments()?.[argName]).toStrictEqual(
+ [
+ ['a3', 'b1'],
+ ['a3', 'b2'],
+ ['a1', 'a2', 'b1'],
+ ['a1', 'a2', 'b2']
+ ]
  );
  });
 
- it.each(testsToRun)('merges ${directiveName} lists (deduplicates intersecting scopes)', ({ directiveName, argName }) => {
+ it.each(testsToRun)('merges $directiveName lists (deduplicates redundant scopes)', ({ directiveName, argName }) => {
  const a1 = {
  typeDefs: gql`
  type Query {
  a: A
  }
 
- type A ${directiveName}(${argName}: ["a", "b"]) @key(fields: "id") {
+ type A ${directiveName}(${argName}: [["a"], ["c"]]) @key(fields: "id") {
  id: String!
  a1: String
  }
@@ -4470,28 +4476,41 @@ describe('composition', () => {
  };
  const a2 = {
  typeDefs: gql`
- type A ${directiveName}(${argName}: ["b", "c"]) @key(fields: "id") {
+ type A ${directiveName}(${argName}: [["a"], ["b"], ["c"]]) @key(fields: "id") {
  id: String!
  a2: String
  }
  `,
  name: 'a2',
  };
+ const a3 = {
+ typeDefs: gql`
+ type A ${directiveName}(${argName}: [["a"], ["b", "c"]]) @key(fields: "id") {
+ id: String!
+ a3: String
+ }
+ `,
+ name: 'a3',
+ };
 
- const result = composeAsFed2Subgraphs([a1, a2]);
+ const result = composeAsFed2Subgraphs([a1, a2, a3]);
  assertCompositionSuccess(result);
  expect(
  result.schema.type('A')
  ?.appliedDirectivesOf(directiveName.slice(1))
- ?.[0]?.arguments()?.[argName]).toStrictEqual(['a', 'b', 'c']
+ ?.[0]?.arguments()?.[argName]).toStrictEqual(
+ [
+ ['a'],
+ ['b', 'c'],
+ ]
  );
  });
 
  it.each(testsToRun)('${directiveName} has correct definition in the supergraph', ({ directiveName, argName, argType, identity }) => {
  const a = {
  typeDefs: gql`
  type Query {
- x: Int ${directiveName}(${argName}: ["a", "b"])
+ x: Int ${directiveName}(${argName}: [["a"], ["b"]])
  }
  `,
  name: 'a',

internals-js/src/argumentCompositionStrategies.ts

@@ -1,4 +1,4 @@
-import { InputType, NonNullType, Schema, isListType, isNonNullType } from "./definitions"
+import {InputType, NonNullType, Schema, isListType, isNonNullType} from "./definitions"
 import { sameType } from "./types";
 import { valueEquals } from "./values";
 
@@ -19,6 +19,14 @@ function supportFixedTypes(types: (schema: Schema) => InputType[]): TypeSupportV
  };
 }
 
+function supportAnyNonNullNestedArray(): TypeSupportValidator {
+ return (_, type) =>
+ isNonNullType(type) && isListType(type.ofType)
+ && isNonNullType(type.ofType.ofType) && isListType(type.ofType.ofType.ofType)
+ ? { valid: true }
+ : { valid: false, supportedMsg: 'non nullable nested list types of any type' }
+}
+
 function supportAnyNonNullArray(): TypeSupportValidator {
  return (_, type) => isNonNullType(type) && isListType(type.ofType)
  ? { valid: true }
@@ -54,6 +62,104 @@ function unionValues(values: any[]): any {
  }, []);
 }
 
+/**
+ * Performs conjunction of 2d arrays that represent conditions in Disjunctive Normal Form.
+ *
+ * * Each inner array is interpreted as the conjunction of the conditions in the array.
+ * * The top-level array is interpreted as the disjunction of the inner arrays
+ *
+ * Algorithm
+ * * filter out duplicate entries to limit the amount of necessary computations
+ * * calculate cartesian product of the arrays to find all possible combinations
+ * * simplify combinations by dropping duplicate conditions (i.e. p ^ p = p, p ^ q = q ^ p)
+ * * eliminate entries that are subsumed by others (i.e. (p ^ q) subsumes (p ^ q ^ r))
+ */
+function dnfConjunction<T>(values: T[][][]): T[][] {
+ // should never be the case
+ if (values.length == 0) {
+ return [];
+ }
+
+ // we first filter out duplicate values from candidates
+ // this avoids exponential computation of exactly the same conditions
+ const filtered = filterNestedArrayDuplicates(values);
+
+ // initialize with first entry
+ let result: T[][] = filtered[0];
+ // perform cartesian product to find all possible entries
+ for (let i = 1; i < filtered.length; i++) {
+ const current = filtered[i];
+ const accumulator: T[][] = [];
+ const seen = new Set<string>;
+
+ for (const accElement of result) {
+ for (const currentElement of current) {
+ // filter out elements that are already present in accElement
+ const filteredElement = currentElement.filter((e) => !accElement.includes(e));
+ const candidate = [...accElement, ...filteredElement].sort();
+ const key = JSON.stringify(candidate);
+ // only add entries which has not been seen yet
+ if (!seen.has(key)) {
+ seen.add(key);
+ accumulator.push(candidate);
+ }
+ }
+ }
+ // Now we need to deduplicate the results. Given that
+ // - outer array implies OR requirements
+ // - inner array implies AND requirements
+ // We can filter out any inner arrays that fully contain other inner arrays, i.e.
+ // A OR B OR (A AND B) OR (A AND B AND C) => A OR B
+ result = deduplicateSubsumedValues(accumulator);
+ }
+ return result;
+}
+
+function filterNestedArrayDuplicates<T>(values: T[][][]): T[][][] {
+ const filtered: T[][][] = [];
+ const seen = new Set<string>;
+ values.forEach((value) => {
+ value.sort();
+ const key = JSON.stringify(value);
+ if (!seen.has(key)) {
+ seen.add(key);
+ filtered.push(value);
+ }
+ });
+ return filtered;
+}
+
+function deduplicateSubsumedValues<T>(values: T[][]): T[][] {
+ const result: T[][] = [];
+ // we first sort by length as the longer ones might be dropped
+ values.sort((first, second) => {
+ if (first.length < second.length) {
+ return -1;
+ } else if (first.length > second.length) {
+ return 1;
+ } else {
+ return 0;
+ }
+ });
+
+ for (const candidate of values) {
+ const entry = new Set(candidate);
+ let redundant = false;
+ for (const r of result) {
+ if (r.every(e => entry.has(e))) {
+ // if `r` is a subset of a `candidate` then it means `candidate` is redundant
+ redundant = true;
+ break;
+ }
+ }
+
+ if (!redundant) {
+ result.push(candidate);
+ }
+ }
+ return result;
+}
+
 export const ARGUMENT_COMPOSITION_STRATEGIES = {
  MAX: {
  name: 'MAX',
@@ -95,7 +201,8 @@ export const ARGUMENT_COMPOSITION_STRATEGIES = {
  schema.booleanType(),
  new NonNullType(schema.booleanType())
  ]),
- mergeValues: mergeNullableValues(
+ mergeValues:
+ mergeNullableValues(
  (values: boolean[]) => values.every((v) => v)
  ),
  },
@@ -113,5 +220,10 @@ export const ARGUMENT_COMPOSITION_STRATEGIES = {
  name: 'NULLABLE_UNION',
  isTypeSupported: supportAnyArray(),
  mergeValues: mergeNullableValues(unionValues),
+ },
+ DNF_CONJUNCTION: {
+ name: 'DNF_CONJUNCTION',
+ isTypeSupported: supportAnyNonNullNestedArray(),
+ mergeValues: dnfConjunction
  }
 }

internals-js/src/specs/policySpec.ts

@@ -42,7 +42,7 @@ export class PolicySpecDefinition extends FeatureDefinition {
  assert(PolicyType, () => `Expected "${policyName}" to be defined`);
  return new NonNullType(new ListType(new NonNullType(new ListType(new NonNullType(PolicyType)))));
  },
- compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.UNION,
+ compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.DNF_CONJUNCTION,
  }],
  locations: [
  DirectiveLocation.FIELD_DEFINITION,

internals-js/src/specs/requiresScopesSpec.ts

@@ -43,7 +43,7 @@ export class RequiresScopesSpecDefinition extends FeatureDefinition {
  assert(scopeType, () => `Expected "${scopeName}" to be defined`);
  return new NonNullType(new ListType(new NonNullType(new ListType(new NonNullType(scopeType)))));
  },
- compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.UNION,
+ compositionStrategy: ARGUMENT_COMPOSITION_STRATEGIES.DNF_CONJUNCTION,
  }],
  locations: [
  DirectiveLocation.FIELD_DEFINITION,