From b19431e4a92206703e29aba859a5fc7574b9ef8b Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 5 Nov 2025 21:55:23 -0600 Subject: [PATCH 1/4] fix: handle `@requires` chains when verifying access control Fixed access control verification of transitive requirements (through `@requires` and/or `@fromContext`) to ensure it works with chains of transitive dependencies. --- .changeset/orange-yaks-double.md | 5 + .../src/__tests__/compose.auth.test.ts | 46 ++++++++ composition-js/src/merging/merge.ts | 100 ++++++++++-------- 3 files changed, 104 insertions(+), 47 deletions(-) create mode 100644 .changeset/orange-yaks-double.md diff --git a/.changeset/orange-yaks-double.md b/.changeset/orange-yaks-double.md new file mode 100644 index 000000000..04b911e4b --- /dev/null +++ b/.changeset/orange-yaks-double.md @@ -0,0 +1,5 @@ +--- +"@apollo/composition": patch +--- + +Fixed access control verification of transitive requirements (through `@requires` and/or `@fromContext`) to ensure it works with chains of transitive dependencies. diff --git a/composition-js/src/__tests__/compose.auth.test.ts b/composition-js/src/__tests__/compose.auth.test.ts index 270845735..013ea60b1 100644 --- a/composition-js/src/__tests__/compose.auth.test.ts +++ b/composition-js/src/__tests__/compose.auth.test.ts @@ -473,7 +473,52 @@ describe('authorization tests', () => { ' auth requirements to access the transitive field "T.extra" data from @requires selection set.' ); }) + + it('works with chain of requires', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID + extra: String @external + requiresExtra: String @requires(fields: "extra") @authenticated + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + type T @key(fields: "id") { + id: ID + secret: String @external + extra: String @requires(fields: "secret") @authenticated + } + ` + } + + const subgraph3 = { + name: 'Subgraph3', + url: 'https://Subgraph3', + typeDefs: gql` + type T @key(fields: "id") { + id: ID + secret: String @authenticated @inaccessible + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2, subgraph3]); + assertCompositionSuccess(result); + }) }); + describe("@context", () => { it('works with explicit auth', () => { const subgraph1 = { @@ -711,6 +756,7 @@ describe('authorization tests', () => { ); }) }); + describe("interfaces", () => { it('propagates @authenticated from type', () => { const subgraph1 = { diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index d11219f33..962311ee0 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -3798,21 +3798,24 @@ export class AuthValidator { const errors: GraphQLError[] = [] const joinDirectivesOnRequires = field.appliedDirectivesOf(this.joinFieldDirective); for (const joinDirectiveOnRequires of joinDirectivesOnRequires) { - const requiresFieldSet = joinDirectiveOnRequires.arguments().requires!; - const requiresSelectionSet = parseSelectionSet({parentType: type, source: requiresFieldSet}); - try { - this.verifyAuthRequirementsOnSelectionSet(authRequirementOnRequires, requiresSelectionSet); - } catch (e) { - if (!(e instanceof GraphQLError)) { - throw e; - } - // target subgraph info should always be provided but just in case - const enumSubgraphValue = joinDirectiveOnRequires.arguments().graph; - const subgraph = enumSubgraphValue ? this.joinSpecNamesToSubgraphNames.get(enumSubgraphValue) : undefined; - if (subgraph) { - errors.push(addSubgraphToError(e, subgraph)); - } else { - errors.push(e); + const requiresFieldSet = joinDirectiveOnRequires.arguments().requires; + if (requiresFieldSet) { + // only verify @requires selections if it is defined on @join__field + const requiresSelectionSet = parseSelectionSet({parentType: type, source: requiresFieldSet}); + try { + this.verifyAuthRequirementsOnSelectionSet(authRequirementOnRequires, requiresSelectionSet); + } catch (e) { + if (!(e instanceof GraphQLError)) { + throw e; + } + // target subgraph info should always be provided but just in case + const enumSubgraphValue = joinDirectiveOnRequires.arguments().graph; + const subgraph = enumSubgraphValue ? this.joinSpecNamesToSubgraphNames.get(enumSubgraphValue) : undefined; + if (subgraph) { + errors.push(addSubgraphToError(e, subgraph)); + } else { + errors.push(e); + } } } } @@ -3835,38 +3838,41 @@ export class AuthValidator { const errors: GraphQLError[] = [] const joinDirectivesOnFromContext = field.appliedDirectivesOf(this.joinFieldDirective); for (const joinDirectiveOnFromContext of joinDirectivesOnFromContext) { - const contexts = joinDirectiveOnFromContext.arguments().contextArguments!; - for (const context of contexts) { - const name = context.context; - const contextSelection = context.selection; - - const targetTypeNames = this.contexts.get(name); - assert(targetTypeNames, 'Contexts exists'); - for (const targetTypeName of targetTypeNames) { - // we need to verify against all possible contexts - const targetType = this.schema.type(targetTypeName) as CompositeType; - assert(targetType, 'Context references valid type in the schema'); - try { - const requirementsOnContextType = this.authRequirementsOnElement(targetType); - if (!authRequirementOnContext.satisfies(requirementsOnContextType)) { - const msg = `Field "${field.coordinate}" does not specify necessary @authenticated, @requiresScopes ` - + `and/or @policy auth requirements to access the transitive data in context ${name} from @fromContext selection set.`; - throw ERRORS.MISSING_TRANSITIVE_AUTH_REQUIREMENTS.err(msg); - } + const contexts = joinDirectiveOnFromContext.arguments().contextArguments; + if (contexts) { + // only verify @fromContext selections if they are defined on @join__field directive + for (const context of contexts) { + const name = context.context; + const contextSelection = context.selection; + + const targetTypeNames = this.contexts.get(name); + assert(targetTypeNames, 'Contexts exists'); + for (const targetTypeName of targetTypeNames) { + // we need to verify against all possible contexts + const targetType = this.schema.type(targetTypeName) as CompositeType; + assert(targetType, 'Context references valid type in the schema'); + try { + const requirementsOnContextType = this.authRequirementsOnElement(targetType); + if (!authRequirementOnContext.satisfies(requirementsOnContextType)) { + const msg = `Field "${field.coordinate}" does not specify necessary @authenticated, @requiresScopes ` + + `and/or @policy auth requirements to access the transitive data in context ${name} from @fromContext selection set.`; + throw ERRORS.MISSING_TRANSITIVE_AUTH_REQUIREMENTS.err(msg); + } - const contextSelectionSet = parseSelectionSet({parentType: targetType, source: contextSelection}); - this.verifyAuthRequirementsOnSelectionSet(authRequirementOnContext, contextSelectionSet); - } catch (e) { - if (!(e instanceof GraphQLError)) { - throw e; - } - // target subgraph info should always be provided but just in case - const enumSubgraphValue = joinDirectiveOnFromContext.arguments().graph; - const subgraph = enumSubgraphValue ? this.joinSpecNamesToSubgraphNames.get(enumSubgraphValue) : undefined; - if (subgraph) { - errors.push(addSubgraphToError(e, subgraph)); - } else { - errors.push(e); + const contextSelectionSet = parseSelectionSet({parentType: targetType, source: contextSelection}); + this.verifyAuthRequirementsOnSelectionSet(authRequirementOnContext, contextSelectionSet); + } catch (e) { + if (!(e instanceof GraphQLError)) { + throw e; + } + // target subgraph info should always be provided but just in case + const enumSubgraphValue = joinDirectiveOnFromContext.arguments().graph; + const subgraph = enumSubgraphValue ? this.joinSpecNamesToSubgraphNames.get(enumSubgraphValue) : undefined; + if (subgraph) { + errors.push(addSubgraphToError(e, subgraph)); + } else { + errors.push(e); + } } } } @@ -3875,7 +3881,7 @@ export class AuthValidator { return errors; } - private authRequirementsOnElement(element: NamedType | FieldDefinition): AuthRequirementsOnElement | undefined { + private authRequirementsOnElement(element: NamedType | FieldDefinition | FieldDefinition): AuthRequirementsOnElement | undefined { const requirements = new AuthRequirementsOnElement(); if (this.authenticatedDirective) { const appliedDirective = element.appliedDirectivesOf(this.authenticatedDirective)?.[0]; From 09e596e6a0c753071ca822e84f525d73ada395cf Mon Sep 17 00:00:00 2001 From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 5 Nov 2025 22:34:45 -0600 Subject: [PATCH 2/4] fix: allow interface object fields to specify access control Update composition logic to allow specifying access control directives (`@authenticated`, `@requiresScopes` and `@policy`) on `@interfaceObject` fields. While we disallow access control on interface types and fields, we decided to support it on `@interfaceObject` as it is a useful pattern to define a single resolver (that may need access controls) for common interface fields. Alternative would require our users to explicitly define resolvers for all implementations which defeats the purpose of `@interfaceObject`. This PR refactors in how we propagate access control by providing additional merge sources when merging directives on interfaces, interface fields and object fields. --- .changeset/shaggy-adults-help.md | 9 + .../src/__tests__/compose.auth.test.ts | 339 +++++++++++++++++- composition-js/src/merging/merge.ts | 320 +++++++++++------ .../src/__tests__/subgraphValidation.test.ts | 10 +- internals-js/src/error.ts | 2 +- internals-js/src/federation.ts | 6 +- 6 files changed, 572 insertions(+), 114 deletions(-) create mode 100644 .changeset/shaggy-adults-help.md diff --git a/.changeset/shaggy-adults-help.md b/.changeset/shaggy-adults-help.md new file mode 100644 index 000000000..5c92b4c36 --- /dev/null +++ b/.changeset/shaggy-adults-help.md @@ -0,0 +1,9 @@ +--- +"@apollo/composition": patch +"@apollo/federation-internals": patch +--- +Allow interface object fields to specify access control + +Update composition logic to allow specifying access control directives (`@authenticated`, `@requiresScopes` and `@policy`) on `@interfaceObject` fields. While we disallow access control on interface types and fields, we decided to support it on `@interfaceObject` as it is a useful pattern to define a single resolver (that may need access controls) for common interface fields. Alternative would require our users to explicitly define resolvers for all implementations which defeats the purpose of `@interfaceObject`. + +This PR refactors in how we propagate access control by providing additional merge sources when merging directives on interfaces, interface fields and object fields. diff --git a/composition-js/src/__tests__/compose.auth.test.ts b/composition-js/src/__tests__/compose.auth.test.ts index 013ea60b1..12f14d992 100644 --- a/composition-js/src/__tests__/compose.auth.test.ts +++ b/composition-js/src/__tests__/compose.auth.test.ts @@ -3,7 +3,7 @@ import { assertCompositionSuccess, composeAsFed2Subgraphs, } from "./testHelper"; -import {InterfaceType} from "@apollo/federation-internals"; +import {InterfaceType, ObjectType} from "@apollo/federation-internals"; describe('authorization tests', () => { describe("@requires", () => { @@ -474,6 +474,55 @@ describe('authorization tests', () => { ); }) + it('verifies access control on chain of requires', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID + extra: String @external + requiresExtra: String @requires(fields: "extra") + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + type T @key(fields: "id") { + id: ID + secret: String @external + extra: String @requires(fields: "secret") + } + ` + } + + const subgraph3 = { + name: 'Subgraph3', + url: 'https://Subgraph3', + typeDefs: gql` + type T @key(fields: "id") { + id: ID + secret: String @authenticated @inaccessible + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2, subgraph3]); + expect(result.schema).toBeUndefined(); + expect(result.errors?.length).toBe(1); + expect(result.errors?.[0].message).toBe( + '[Subgraph2] Field "T.extra" does not specify necessary @authenticated, @requiresScopes and/or ' + + '@policy auth requirements to access the transitive field "T.secret" data from @requires selection set.' + ); + }) + it('works with chain of requires', () => { const subgraph1 = { name: 'Subgraph1', @@ -517,6 +566,140 @@ describe('authorization tests', () => { const result = composeAsFed2Subgraphs([subgraph1, subgraph2, subgraph3]); assertCompositionSuccess(result); }) + + it('works with interface objects', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + type I @interfaceObject @key(fields: "id") { + id: ID! + extra: String @external + requiresExtra: String @requires(fields: "extra") @authenticated + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + interface I @key(fields: "id") { + id: ID! + extra: String + } + + type T @key(fields: "id") { + id: ID + extra: String @authenticated + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + const interfaceI = result.schema.type("I") as InterfaceType; + expect(interfaceI).toBeDefined(); + const requiresExtraField = interfaceI.field('requiresExtra'); + expect(requiresExtraField).toBeDefined(); + expect(requiresExtraField?.appliedDirectivesOf("authenticated")).toBeDefined(); + }) + + it('works with interface object chains', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + type I @interfaceObject @key(fields: "id") { + id: ID! + extra: String @external + requiresExtra: String @requires(fields: "extra") @authenticated + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + type I @interfaceObject @key(fields: "id") { + id: ID! + secret: String @external + extra: String @requires(fields: "secret") @authenticated + } + ` + } + + const subgraph3 = { + name: 'Subgraph3', + url: 'https://Subgraph3', + typeDefs: gql` + interface I @key(fields: "id") { + id: ID! + secret: String + } + + type T implements I @key(fields: "id") { + id: ID! + secret: String @authenticated + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2, subgraph3]); + assertCompositionSuccess(result); + }) + + it('verifies requires on interface objects without auth', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + type I @interfaceObject @key(fields: "id") { + id: ID! + extra: String @external + requiresExtra: String @requires(fields: "extra") + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + interface I @key(fields: "id") { + id: ID! + extra: String + } + + type T implements I @key(fields: "id") { + id: ID! + extra: String @authenticated + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + console.log(result.supergraphSdl); + expect(result.schema).toBeUndefined(); + expect(result.errors?.length).toBe(1); + expect(result.errors?.[0].message).toBe( + '[Subgraph1] Field "I.requiresExtra" does not specify necessary @authenticated, @requiresScopes and/or @policy' + + ' auth requirements to access the transitive field "T.extra" data from @requires selection set.' + ); + }) }); describe("@context", () => { @@ -802,7 +985,7 @@ describe('authorization tests', () => { assertCompositionSuccess(result); expect( result.schema.type('I')?.appliedDirectivesOf("authenticated")?.[0] - ); + ).toBeDefined(); }) it('propagates @requiresScopes from type', () => { @@ -1139,5 +1322,157 @@ describe('authorization tests', () => { ] ); }) + + it('works with interface objects', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + type I @interfaceObject @key(fields: "id") { + id: ID! + secret: String @requiresScopes(scopes: [["S1"]]) + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + interface I @key(fields: "id") { + id: ID! + extra: String + } + + type T implements I @key(fields: "id") { + id: ID! + extra: String @authenticated + } + + type U implements I @key(fields: "id") { + id: ID! + extra: String + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + }) + + it('works with shareable interface object fields', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + type I @interfaceObject @key(fields: "id") { + id: ID! + secret: String @requiresScopes(scopes: [["S1"]]) @shareable + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + interface I @key(fields: "id") { + id: ID! + extra: String + } + + type T implements I @key(fields: "id") { + id: ID! + extra: String @authenticated + } + + type U implements I @key(fields: "id") { + id: ID! + extra: String + } + ` + } + + const subgraph3 = { + name: 'Subgraph3', + url: 'https://Subgraph3', + typeDefs: gql` + type T @key(fields: "id") { + id: ID! + secret: String @requiresScopes(scopes: [["S2"]]) @shareable + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2, subgraph3]); + assertCompositionSuccess(result); + // interface I { + // id: ID! + // secret: String @requiresScopes(scopes: [["S1", "S2"]]) + // extra: String @authenticated + // } + const i = result.schema.type("I"); + expect(i).toBeDefined(); + expect(i).toBeInstanceOf(InterfaceType); + const secretI = (i as InterfaceType).field("secret"); + expect(secretI?.appliedDirectivesOf("requiresScopes") + ?.[0]?.arguments()?.["scopes"]).toStrictEqual( + [ + ['S1', 'S2'], + ] + ); + const extraI = (i as InterfaceType).field("extra"); + expect(extraI?.appliedDirectivesOf("authenticated") + ?.[0] + ).toBeDefined(); + + // type T implements I { + // id: ID! + // extra: String @authenticated + // secret: String @requiresScopes(scopes: [["S1", "S2"]]) + // } + const t = result.schema.type("T"); + expect(t).toBeDefined(); + expect(t).toBeInstanceOf(ObjectType); + const secretT = (t as ObjectType).field("secret"); + expect(secretT?.appliedDirectivesOf("requiresScopes") + ?.[0]?.arguments()?.["scopes"]).toStrictEqual( + [ + ['S1', 'S2'], + ] + ); + const extraT = (t as ObjectType).field("extra"); + expect(extraT?.appliedDirectivesOf("authenticated") + ?.[0] + ).toBeDefined(); + + // type U implements I { + // id: ID! + // extra: String + // secret: String @requiresScopes(scopes: [["S1", "S2"]]) + // } + const u = result.schema.type("U"); + expect(u).toBeDefined(); + expect(u).toBeInstanceOf(ObjectType); + const secretU = (u as ObjectType).field("secret"); + expect(secretU?.appliedDirectivesOf("requiresScopes") + ?.[0]?.arguments()?.["scopes"]).toStrictEqual( + [ + ['S1', 'S2'], + ] + ); + const extraU = (u as ObjectType).field("extra"); + expect(extraU?.appliedDirectivesOf("authenticated") + ?.[0] + ).toBeUndefined(); + }) }); }); diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 962311ee0..580130ab0 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -97,7 +97,7 @@ import { SelectionSet, JoinFieldDirectiveArguments, ContextSpecDefinition, - CONTEXT_VERSIONS, + CONTEXT_VERSIONS, FederationDirectiveName, } from "@apollo/federation-internals"; import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; import { @@ -395,8 +395,10 @@ class Merger { private fieldsWithFromContext: Set; private fieldsWithOverride: Set; private fieldsWithRequires: Set; - private authEnabled: boolean; - private interfacesWithAuthRequirements: Set; + private authEnabled: boolean = false; + private accessControlDirectivesInSupergraph: { name: string, nameInSupergraph?: string }[] = []; + private __accessControlSourcesComputed: boolean = false; + private __accessControlAdditionalSources: Map>> = new Map(); constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) { this.latestFedVersionUsed = this.getLatestFederationVersionUsed(); @@ -405,9 +407,6 @@ class Merger { this.fieldsWithFromContext = this.getFieldsWithFromContextDirective(); this.fieldsWithOverride = this.getFieldsWithOverrideDirective(); this.fieldsWithRequires = this.getFieldsWithRequiresDirective(); - const { authEnabled, interfacesWithAuthRequirements } = this.getAuthRequirements(); - this.authEnabled = authEnabled; - this.interfacesWithAuthRequirements = interfacesWithAuthRequirements; this.names = subgraphs.names(); this.composeDirectiveManager = new ComposeDirectiveManager( @@ -622,6 +621,36 @@ class Merger { ) { this.inaccessibleDirectiveInSupergraph = this.merged.directive(nameInSupergraph)!; } + + // If we encounter access control directives, we need to record its definition + // so we can correctly merge them for polymorphic types + let authenticatedDirective = undefined; + if (specInSupergraph.identity === AuthenticatedSpecDefinition.identity && nameInFeature === specInSupergraph.url.name) { + this.authEnabled = true; + authenticatedDirective = this.merged.directive(nameInSupergraph); + } + let requiresScopesDirective = undefined; + if (specInSupergraph.identity === RequiresScopesSpecDefinition.identity && nameInFeature === specInSupergraph.url.name) { + this.authEnabled = true; + requiresScopesDirective = this.merged.directive(nameInSupergraph); + } + let policyDirective = undefined; + if (specInSupergraph.identity === PolicySpecDefinition.identity && nameInFeature === specInSupergraph.url.name) { + this.authEnabled = true; + policyDirective = this.merged.directive(nameInSupergraph); + } + this.accessControlDirectivesInSupergraph.push({ + name: FederationDirectiveName.AUTHENTICATED, + nameInSupergraph: authenticatedDirective?.name, + }); + this.accessControlDirectivesInSupergraph.push({ + name: FederationDirectiveName.REQUIRES_SCOPES, + nameInSupergraph: requiresScopesDirective?.name, + }); + this.accessControlDirectivesInSupergraph.push({ + name: FederationDirectiveName.POLICY, + nameInSupergraph: policyDirective?.name, + }); } } } @@ -766,8 +795,6 @@ class Merger { // we want to make sure everything is ready. this.addMissingInterfaceObjectFieldsToImplementations(); - this.propagateAuthToInterfaces(); - // If we already encountered errors, `this.merged` is probably incomplete. Let's not risk adding errors that // are only an artifact of that incompleteness as it's confusing. if (this.errors.length === 0) { @@ -1013,7 +1040,7 @@ class Merger { this.checkForExtensionWithNoBase(sources, dest); this.mergeDescription(sources, dest); this.addJoinType(sources, dest); - this.recordAppliedDirectivesToMerge(sources, dest); + this.recordTypeAppliedDirectivesToMerge(sources, dest); this.addJoinDirectiveDirectives(sources, dest); switch (dest.kind) { case 'ScalarType': @@ -1769,7 +1796,7 @@ class Merger { // supergraph just because someone fat-fingered the type in an external definition. But after merging the non-external definitions, we // validate the external ones are consistent. this.mergeDescription(withoutExternal, dest); - this.recordAppliedDirectivesToMerge(withoutExternal, dest); + this.recordFieldAppliedDirectivesToMerge(withoutExternal, dest); this.addArgumentsShallow(withoutExternal, dest); for (const destArg of dest.arguments()) { const subgraphArgs = mapSources(withoutExternal, f => f?.argument(destArg.name)); @@ -2967,6 +2994,182 @@ class Merger { }); } + private recordTypeAppliedDirectivesToMerge(sources: Sources>, dest: SchemaElement) { + const inaccessibleName = this.inaccessibleDirectiveInSupergraph?.name; + const names = this.gatherAppliedDirectiveNames(sources); + + if (inaccessibleName && names.has(inaccessibleName)) { + this.mergeAppliedDirective(inaccessibleName, sources, dest); + names.delete(inaccessibleName); + } + + if (dest instanceof InterfaceType) { + // we need to propagate access control from implementing types upward to the interface + for (const { name, nameInSupergraph } of this.accessControlDirectivesInSupergraph) { + if (nameInSupergraph) { + let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); + if (!additionalSources) { + additionalSources = []; + } + // record access control directive IF it is applied on the interface (currently not allowed) + // OR there are some implementations with access control on them + if (names.has(nameInSupergraph) || additionalSources) { + this.appliedDirectivesToMerge.push({ + names: new Set([nameInSupergraph]), + sources: sourcesFromArray([...sources.values(), ...additionalSources]), + dest, + }); + } + names.delete(nameInSupergraph); + } + } + } + + this.appliedDirectivesToMerge.push({ + names, + sources, + dest, + }); + } + + private recordFieldAppliedDirectivesToMerge(sources: Sources>, dest: FieldDefinition) { + const inaccessibleName = this.inaccessibleDirectiveInSupergraph?.name; + const names = this.gatherAppliedDirectiveNames(sources); + + if (inaccessibleName && names.has(inaccessibleName)) { + this.mergeAppliedDirective(inaccessibleName, sources, dest); + names.delete(inaccessibleName); + } + + for (const { name, nameInSupergraph } of this.accessControlDirectivesInSupergraph) { + // we need to propagate auth on fields + // - upwards from object types to interfaces + // - downwards from interface object to object types + if (nameInSupergraph) { + let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); + if (!additionalSources) { + additionalSources = []; + } + // record if access control applied directly OR has some additional sources + if (names.has(nameInSupergraph) || additionalSources) { + this.appliedDirectivesToMerge.push({ + names: new Set([nameInSupergraph]), + sources: sourcesFromArray([...sources.values(), ...additionalSources]), + dest, + }); + } + names.delete(nameInSupergraph); + } + } + + this.appliedDirectivesToMerge.push({ + names, + sources, + dest, + }); + } + + + /** + * We need to propagate access control directives between interfaces and object types. + * - propagate downwards from interface object fields to the implementing object type fields + * - propagate upwards from object types/fields to their implemented interfaces + * + * This method returns a map of schema coordinates (+ access control directive names) and corresponding additional + * sources that should be included when merging directives. + * + * Key format is `_`, e.g. + * - interface `I` and `@authenticated` directive will have a corresponding `I_authenticated` key that maps + * to the list of objects that specify `@authenticated` (upwards propagation) + * - interface `I`, field `f` and `@requiresScopes` directive will have a corresponding `I.f_requiresScopes` key + * that maps to list of implementing object fields that specify `@requiresScopes` requirements (upwards propagation) + * - type `T`, field `f` and `@policy` directive will have a corresponding `T.f_policy` key that maps to a + * corresponding list of interface objects fields that specify `@policy` requirements (downwards propagation) + */ + private accessControlAdditionalSources(): Map>> { + if (this.__accessControlSourcesComputed) { + return this.__accessControlAdditionalSources; + } else { + const intfToImplMap: Map> = new Map(); + const implToIntfMap: Map> = new Map(); + for (const intf of this.merged.interfaceTypes()) { + const implNames = new Set(intf.possibleRuntimeTypes().map((obj) => obj.name)); + intfToImplMap.set(intf.name, implNames); + for (const impl of implNames) { + let interfaces = implToIntfMap.get(impl); + if (!interfaces) { + interfaces = new Set(); + } + interfaces.add(intf.name); + implToIntfMap.set(impl, interfaces); + } + } + + this.subgraphs.values().forEach((subgraph) => { + const metadata = subgraph.metadata(); + for (const accessControlDirective of [metadata.authenticatedDirective(), metadata.requiresScopesDirective(), metadata.policyDirective()]) { + for (const application of accessControlDirective.applications()) { + const candidate = application.parent; + if (candidate instanceof ObjectType) { + // we will be propagating access control from objects up to the interfaces + // - record ALL implementations that define access control and implement an interface + const implementedInterfaces = implToIntfMap.get(candidate.name); + if (implementedInterfaces) { + for (const intf of implementedInterfaces) { + // save implementation as additional source for impl + const key = `${intf}_${accessControlDirective.name}`; + let sources = this.__accessControlAdditionalSources.get(key); + if (!sources) { + sources = []; + } + sources.push(candidate); + this.__accessControlAdditionalSources.set(key, sources); + } + } + } + if (candidate instanceof FieldDefinition && !metadata.isFieldExternal(candidate)) { + const parent = candidate.parent; + if (parent instanceof ObjectType) { + const objectName = parent.name; + if (metadata.isInterfaceObjectType(parent)) { + // we need to propagate field access control downwards from interface object fields to object fields + const implementations = intfToImplMap.get(objectName); + if (implementations) { + for (const impl of implementations) { + const key = `${impl}.${candidate.name}_${accessControlDirective.name}`; + let additionalSources = this.__accessControlAdditionalSources.get(key); + if (!additionalSources) { + additionalSources = []; + } + additionalSources.push(candidate); + this.__accessControlAdditionalSources.set(key, additionalSources); + } + } + } else { + // we need to propagate field access control upwards from object fields to the interface fields + const implementedInterfaces = implToIntfMap.get(objectName); + if (implementedInterfaces) { + for (const intf of implementedInterfaces) { + const key = `${intf}.${candidate.name}_${accessControlDirective.name}`; + let additionalSources = this.__accessControlAdditionalSources.get(key); + if (!additionalSources) { + additionalSources = []; + } + additionalSources.push(candidate); + this.__accessControlAdditionalSources.set(key, additionalSources); + } + } + } + } + } + } + } + }); + this.__accessControlSourcesComputed = true; + return this.__accessControlAdditionalSources; + } + } + // to be called after elements are merged private mergeAllAppliedDirectives() { for (const { names, sources, dest } of this.appliedDirectivesToMerge) { @@ -3634,42 +3837,6 @@ class Merger { ); } - private getAuthRequirements(): { authEnabled: boolean, interfacesWithAuthRequirements: Set } { - const interfacesWithAuthRequirements = new Set(); - let authEnabled = false; - for (const subgraph of this.subgraphs) { - const authenticatedDirective = subgraph.metadata().authenticatedDirective(); - const requiresScopesDirective = subgraph.metadata().requiresScopesDirective(); - const policyDirective = subgraph.metadata().policyDirective(); - - for (const authDirective of [authenticatedDirective, requiresScopesDirective, policyDirective]) { - if (isFederationDirectiveDefinedInSchema(authDirective)) { - authEnabled = authEnabled || authDirective.applications().size > 0; - - for (const application of authDirective.applications()) { - const parent = application.parent; - if (parent instanceof ObjectType) { - // capture interfaces that will need to be updated later on - parent.interfaces().forEach( - (intf) => interfacesWithAuthRequirements.add(intf.name) - ); - } else if (parent instanceof FieldDefinition) { - // we only allow auth on object fields - const parentObjectType = parent.parent; - if (isObjectType(parentObjectType)) { - parentObjectType.interfaces().forEach( - (intf) => interfacesWithAuthRequirements.add(intf.name) - ); - } - } - } - } - } - } - - return { authEnabled, interfacesWithAuthRequirements }; - } - private getFieldsWithAppliedDirective( getDirective: (subgraph: Subgraph) => Post20FederationDirectiveDefinition, getField: (application: Directive>) => FieldDefinition, @@ -3689,55 +3856,6 @@ class Merger { } return fields; } - - propagateAuthToInterfaces() { - if (this.authEnabled) { - const interfacesThatNeedAuth = this.merged.interfaceTypes() - .filter((intf) => this.interfacesWithAuthRequirements.has(intf.name)); - - if (interfacesThatNeedAuth.length > 0) { - const authenticatedFeature = this.merged.coreFeatures?.getByIdentity(AuthenticatedSpecDefinition.identity); - const authenticatedSpec = authenticatedFeature && AUTHENTICATED_VERSIONS.find(authenticatedFeature.url.version); - const authenticatedDirective = authenticatedSpec?.authenticatedDirective(this.merged); - - const requiresScopesFeature = this.merged.coreFeatures?.getByIdentity(RequiresScopesSpecDefinition.identity); - const requiresScopesSpec = requiresScopesFeature && REQUIRES_SCOPES_VERSIONS.find(requiresScopesFeature.url.version); - const requiresScopesDirective = requiresScopesSpec?.requiresScopesDirective(this.merged); - - const policyFeature = this.merged.coreFeatures?.getByIdentity(PolicySpecDefinition.identity); - const policySpec = policyFeature && POLICY_VERSIONS.find(policyFeature.url.version); - const policyDirective = policySpec?.policyDirective(this.merged); - - for (const intf of interfacesThatNeedAuth) { - const impls = intf.possibleRuntimeTypes(); - const implSources = sourcesFromArray(>impls); - if (authenticatedDirective) { - this.mergeAppliedDirective(authenticatedDirective.name, implSources, intf); - } - if (requiresScopesDirective) { - this.mergeAppliedDirective(requiresScopesDirective.name, implSources, intf); - } - if (policyDirective) { - this.mergeAppliedDirective(policyDirective.name, implSources, intf); - } - - for (const intfField of intf.fields()) { - const implFields = impls.map((impl) => impl.field(intfField.name)); - const fieldSources = sourcesFromArray(implFields); - if (authenticatedDirective) { - this.mergeAppliedDirective(authenticatedDirective.name, fieldSources, intfField); - } - if (requiresScopesDirective) { - this.mergeAppliedDirective(requiresScopesDirective.name, fieldSources, intfField); - } - if (policyDirective) { - this.mergeAppliedDirective(policyDirective.name, fieldSources, intfField); - } - } - } - } - } - } } export class AuthValidator { @@ -3785,11 +3903,11 @@ export class AuthValidator { validateRequiresFieldSet(coordinate: string): GraphQLError[] { const fieldCoordinate = coordinate.split('.'); - assert(fieldCoordinate && fieldCoordinate.length == 2,'Valid coordinate for field with @requires'); + assert(fieldCoordinate && fieldCoordinate.length == 2,`Valid coordinate for field "${coordinate}" with @requires`); const type = this.schema.type(fieldCoordinate[0]); - assert(type instanceof ObjectType, 'Type exists in the schema'); + assert(type instanceof ObjectType || type instanceof InterfaceType, 'Type "${fieldCoordinate[0]}" exists in the schema'); const field = type.field(fieldCoordinate[1]); - assert(field instanceof FieldDefinition, 'Field exists in the schema'); + assert(field instanceof FieldDefinition, `Field "${coordinate}" exists in the schema`); const authRequirementOnRequires = new AuthRequirements(coordinate, '@requires'); authRequirementOnRequires.type = this.authRequirementsOnElement(type); @@ -3825,11 +3943,11 @@ export class AuthValidator { validateFromContext(coordinate: string): GraphQLError[] { const fieldCoordinate = coordinate.split('.'); - assert(fieldCoordinate && fieldCoordinate.length == 2,'Valid coordinate for field with @fromContext'); + assert(fieldCoordinate && fieldCoordinate.length == 2,`Valid coordinate for field "${coordinate}" with @requires`); const type = this.schema.type(fieldCoordinate[0]); - assert(type instanceof ObjectType, 'Type exists in the schema'); + assert(type instanceof ObjectType || type instanceof InterfaceType, 'Type "${fieldCoordinate[0]}" exists in the schema'); const field = type.field(fieldCoordinate[1]); - assert(field instanceof FieldDefinition, 'Field exists in the schema'); + assert(field instanceof FieldDefinition, `Field "${coordinate}" exists in the schema`); const authRequirementOnContext = new AuthRequirements(coordinate, '@fromContext'); authRequirementOnContext.type = this.authRequirementsOnElement(type); diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 9513d4d34..9742bcfc0 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -1764,19 +1764,15 @@ describe('authentication validations', () => { expect(buildForErrors(doc, { includeAllImports: true })).toStrictEqual([ [ 'AUTHENTICATION_APPLIED_ON_INTERFACE', - `[S] Invalid use of ${directiveName} on field "I.x": ${directiveName} cannot be applied on interfaces, interface objects or their fields`, + `[S] Invalid use of ${directiveName} on field "I.x": ${directiveName} cannot be applied on interfaces, interface fields and interface objects`, ], [ 'AUTHENTICATION_APPLIED_ON_INTERFACE', - `[S] Invalid use of ${directiveName} on interface "I": ${directiveName} cannot be applied on interfaces, interface objects or their fields`, + `[S] Invalid use of ${directiveName} on interface "I": ${directiveName} cannot be applied on interfaces, interface fields and interface objects`, ], [ 'AUTHENTICATION_APPLIED_ON_INTERFACE', - `[S] Invalid use of ${directiveName} on field "O.y": ${directiveName} cannot be applied on interfaces, interface objects or their fields`, - ], - [ - 'AUTHENTICATION_APPLIED_ON_INTERFACE', - `[S] Invalid use of ${directiveName} on interface object "O": ${directiveName} cannot be applied on interfaces, interface objects or their fields`, + `[S] Invalid use of ${directiveName} on interface object "O": ${directiveName} cannot be applied on interfaces, interface fields and interface objects`, ], ]); }, diff --git a/internals-js/src/error.ts b/internals-js/src/error.ts index cbb59291c..fd5f1d105 100644 --- a/internals-js/src/error.ts +++ b/internals-js/src/error.ts @@ -635,7 +635,7 @@ const MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED = makeCodeDefinition( const AUTHENTICATION_APPLIED_ON_INTERFACE = makeCodeDefinition( 'AUTHENTICATION_APPLIED_ON_INTERFACE', - 'The @authenticated, @requiresScopes and @policy directive cannot be applied on interface, interface object or their fields.', + 'The @authenticated, @requiresScopes and @policy directive cannot be applied on interface, interface fields and interface object', { addedIn: '2.9.4' }, ); diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index d8d9fbe3d..4827ed065 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -1846,7 +1846,7 @@ export class FederationBlueprint extends SchemaBlueprint { validateSizedFieldsAreValidLists(application, parent, errorCollector); } - // Validate @authenticated, @requireScopes and @policy + // Validate @authenticated, @requireScopes and @policy usage on interfaces and interface objects validateNoAuthenticationOnInterfaces(metadata, errorCollector); return errorCollector; @@ -2912,7 +2912,7 @@ function validateNoAuthenticationOnInterfaces(metadata: FederationMetadata, erro return isInterfaceType(type) || isInterfaceObjectType(baseType(type)); } function isAppliedOnInterfaceField(elem: SchemaElement) { - return isFieldDefinition(elem) && isAppliedOnInterface(elem.parent); + return isFieldDefinition(elem) && isInterfaceType(elem.parent); } if (isAppliedOnInterface(element) || isAppliedOnInterfaceField(element)) { @@ -2929,7 +2929,7 @@ function validateNoAuthenticationOnInterfaces(metadata: FederationMetadata, erro break; } errorCollector.push(ERRORS.AUTHENTICATION_APPLIED_ON_INTERFACE.err( - `Invalid use of @${directive.name} on ${kind} "${element.coordinate}": @${directive.name} cannot be applied on interfaces, interface objects or their fields`, + `Invalid use of @${directive.name} on ${kind} "${element.coordinate}": @${directive.name} cannot be applied on interfaces, interface fields and interface objects`, {nodes: sourceASTs(application, element.parent)}, )); } From 61c3838cf9619fee26f1fe7149d39cadec0a3c85 Mon Sep 17 00:00:00 2001 From: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com> Date: Wed, 12 Nov 2025 14:51:13 -0600 Subject: [PATCH 3/4] fix: addressing feedback on updated access control logic (#3337) Fixes: * we now correctly sort all conditions so we can properly deduplicate entries in `dnfConjunction` * added extra type checks when validating usage of access control on interfaces * fixed `AuthRequirementsOnElement.isSubset()` to verify that `@requires`/`@fromContext` field auth requirements satisfy transitive requirements * fixed `AuthRequirements.satisfies()` logic to use DNF conjunction of auth requirements from type and field to get the final requirements to validate against * cleaned up computation of additional access control sources * fixed handling of renamed access control directives * added comments around our usage of additional sources for access control * updated logic to handle `@interfaceObject` -> object type -> other interface access control propagation chains * `copyNonJoinAppliedDirectives()` was updated to pull additional sources from `accessControlAdditionalSources()` for auth directives, instead of just copying the interface field directives (which were already merged) * update DNF handling logic to convert empty `[]` into `[[]]` to ensure we correctly calculate conjunctions for empty cases * update `dnfConjunction()` to copy its inputs as it mutates the values when sorting --------- Co-authored-by: Sachin D. Shinde --- .../src/__tests__/compose.auth.test.ts | 274 +++++++++++- composition-js/src/merging/merge.ts | 390 +++++++++++------- .../src/__tests__/subgraphValidation.test.ts | 6 +- .../src/argumentCompositionStrategies.ts | 42 +- internals-js/src/definitions.ts | 4 + internals-js/src/error.ts | 6 +- internals-js/src/federation.ts | 22 +- internals-js/src/specs/authenticatedSpec.ts | 4 + internals-js/src/specs/policySpec.ts | 4 + internals-js/src/specs/requiresScopesSpec.ts | 4 + 10 files changed, 576 insertions(+), 180 deletions(-) diff --git a/composition-js/src/__tests__/compose.auth.test.ts b/composition-js/src/__tests__/compose.auth.test.ts index 12f14d992..1fcb57934 100644 --- a/composition-js/src/__tests__/compose.auth.test.ts +++ b/composition-js/src/__tests__/compose.auth.test.ts @@ -4,6 +4,7 @@ import { composeAsFed2Subgraphs, } from "./testHelper"; import {InterfaceType, ObjectType} from "@apollo/federation-internals"; +import {composeServices} from "../compose"; describe('authorization tests', () => { describe("@requires", () => { @@ -116,7 +117,7 @@ describe('authorization tests', () => { id: ID extra: I @external requiresExtra: String @requires(fields: "extra { i ... on I1 { i1 } ... on I2 { i2 } }") - @requiresScopes(scopes: [["S1"]["S2"]]) @policy(policies: [["P1"]]) + @requiresScopes(scopes: [["S1", "S2"]]) @policy(policies: [["P1"]]) } interface I { @@ -352,7 +353,7 @@ describe('authorization tests', () => { expect(result.errors?.length).toBe(1); expect(result.errors?.[0].message).toBe( '[Subgraph1] Field "T.requiresExtra" does not specify necessary @authenticated, @requiresScopes and/or @policy ' + - 'auth requirements to access the transitive field "I1.i" data from @requires selection set.' + 'auth requirements to access the transitive field "I.i" data from @requires selection set.' ); }) @@ -692,14 +693,45 @@ describe('authorization tests', () => { } const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); - console.log(result.supergraphSdl); expect(result.schema).toBeUndefined(); expect(result.errors?.length).toBe(1); expect(result.errors?.[0].message).toBe( '[Subgraph1] Field "I.requiresExtra" does not specify necessary @authenticated, @requiresScopes and/or @policy' + - ' auth requirements to access the transitive field "T.extra" data from @requires selection set.' + ' auth requirements to access the transitive field "I.extra" data from @requires selection set.' ); }) + + it('works if field specifies additional auth', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID + extra: String @external + requiresExtra: String @requires(fields: "extra") @requiresScopes(scopes: [["S1", "S2"]]) + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + type T @key(fields: "id") { + id: ID + extra: String @requiresScopes(scopes: [["S1"]]) + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + }) }); describe("@context", () => { @@ -767,7 +799,7 @@ describe('authorization tests', () => { type U @key(fields: "id") { id: ID! - field(a: String @fromContext(field: "$context { prop }")): Int! @requiresScopes(scopes: [["S1"], ["S2"]]) + field(a: String @fromContext(field: "$context { prop }")): Int! @requiresScopes(scopes: [["S1", "S2"]]) } `, }; @@ -1457,7 +1489,7 @@ describe('authorization tests', () => { // type U implements I { // id: ID! // extra: String - // secret: String @requiresScopes(scopes: [["S1", "S2"]]) + // secret: String @requiresScopes(scopes: [["S1"]]) // } const u = result.schema.type("U"); expect(u).toBeDefined(); @@ -1466,7 +1498,7 @@ describe('authorization tests', () => { expect(secretU?.appliedDirectivesOf("requiresScopes") ?.[0]?.arguments()?.["scopes"]).toStrictEqual( [ - ['S1', 'S2'], + ['S1'], ] ); const extraU = (u as ObjectType).field("extra"); @@ -1474,5 +1506,233 @@ describe('authorization tests', () => { ?.[0] ).toBeUndefined(); }) + + it('propagates access control on chains of interfaces', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + node(id: ID!): Node + } + + interface Node { + id: ID! + } + + interface I implements Node { + id: ID! + intf: String + } + + type T implements Node & I @key(fields: "id") @policy(policies: [["P1"]]) { + id: ID! + intf: String + vT: String + } + + type U implements Node & I @key(fields: "id") @policy(policies: [["P2"]]) { + id: ID! + intf: String + vU: String + } + + type V implements Node & I @key(fields: "id") { + id: ID! + intf: String + vV: String + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID! + other: Int + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + expect( + result.schema.type('Node') + ?.appliedDirectivesOf("policy") + ?.[0]?.arguments()?.["policies"]).toStrictEqual( + [ + ['P1', 'P2'], + ] + ); + expect( + result.schema.type('I') + ?.appliedDirectivesOf("policy") + ?.[0]?.arguments()?.["policies"]).toStrictEqual( + [ + ['P1', 'P2'], + ] + ); + }) + + it('propagates access control on chains with interface object', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + type Query { + i: I + } + + type I @interfaceObject @key(fields: "id") { + id: ID! + secret: String @requiresScopes(scopes: [["S1"]]) + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + interface Node { + id: ID! + secret: String + } + + interface I implements Node @key(fields: "id") { + id: ID! + extra: String + secret: String + } + + type T implements Node & I @key(fields: "id") { + id: ID! + extra: String @authenticated + secret: String @external + } + + type U implements Node & I @key(fields: "id") { + id: ID! + extra: String + secret: String @external + } + ` + } + + const result = composeAsFed2Subgraphs([subgraph1, subgraph2]); + assertCompositionSuccess(result); + const node = result.schema.type('Node'); + expect(node).toBeDefined(); + expect(node).toBeInstanceOf(InterfaceType); + const secretNode = (node as InterfaceType).field("secret"); + expect( + secretNode + ?.appliedDirectivesOf("requiresScopes") + ?.[0]?.arguments()?.["scopes"]).toStrictEqual( + [ + ['S1'], + ] + ); + const i = result.schema.type('I'); + expect(i).toBeDefined(); + expect(i).toBeInstanceOf(InterfaceType); + const secretI = (i as InterfaceType).field("secret"); + expect( + secretI + ?.appliedDirectivesOf("requiresScopes") + ?.[0]?.arguments()?.["scopes"]).toStrictEqual( + [ + ['S1'], + ] + ); + const extraI = (i as InterfaceType).field("extra"); + expect( + extraI + ?.appliedDirectivesOf("authenticated") + ?.[0] + ).toBeDefined(); + }) + + it('works with renames', () => { + const subgraph1 = { + name: 'Subgraph1', + url: 'https://Subgraph1', + typeDefs: gql` + extend schema @link( + url: "https://specs.apollo.dev/federation/v2.9", + import: [ "@key", { name: "@policy", as: "@apolloPolicy" }] + ) + + type Query { + i: I! + } + + interface I { + id: ID + } + + type T implements I @key(fields: "id") @apolloPolicy(policies: [["P1"]]) { + id: ID + vT: String + } + + type U implements I @key(fields: "id") @apolloPolicy(policies: [["P2"]]) { + id: ID + vU: String + } + + type V implements I @key(fields: "id") { + id: ID + vV: String + } + ` + } + + const subgraph2 = { + name: 'Subgraph2', + url: 'https://Subgraph2', + typeDefs: gql` + extend schema @link( + url: "https://specs.apollo.dev/federation/v2.9", + import: [ "@key", { name: "@authenticated", as: "@apolloAuthenticated" }] + ) + + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID! + other: Int @apolloAuthenticated + } + ` + } + + const result = composeServices([subgraph1, subgraph2]); + assertCompositionSuccess(result); + expect( + result.schema.type('I') + ?.appliedDirectivesOf("apolloPolicy") + ?.[0]?.arguments()?.["policies"]).toStrictEqual( + [ + ['P1', 'P2'], + ] + ); + const t = result.schema.type('T'); + expect(t).toBeDefined(); + expect(t).toBeInstanceOf(ObjectType); + const otherT = (t as ObjectType).field("other"); + expect( + otherT + ?.appliedDirectivesOf("apolloAuthenticated") + ?.[0] + ).toBeDefined(); + }) }); }); diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 580130ab0..bde4755c9 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -97,19 +97,18 @@ import { SelectionSet, JoinFieldDirectiveArguments, ContextSpecDefinition, - CONTEXT_VERSIONS, FederationDirectiveName, + CONTEXT_VERSIONS, + FederationDirectiveName, + dnfConjunction, + convertEmptyToTrue, } from "@apollo/federation-internals"; -import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; -import { - CompositionHint, - HintCodeDefinition, - HINTS, -} from "../hints"; -import { ComposeDirectiveManager } from '../composeDirectiveManager'; -import { MismatchReporter } from './reporter'; -import { inspect } from "util"; -import { collectCoreDirectivesToCompose, CoreDirectiveInSubgraphs } from "./coreDirectiveCollector"; -import { CompositionOptions } from "../compose"; +import {ASTNode, DirectiveLocation, GraphQLError} from "graphql"; +import {CompositionHint, HintCodeDefinition, HINTS,} from "../hints"; +import {ComposeDirectiveManager} from '../composeDirectiveManager'; +import {MismatchReporter} from './reporter'; +import {inspect} from "util"; +import {collectCoreDirectivesToCompose, CoreDirectiveInSubgraphs} from "./coreDirectiveCollector"; +import {CompositionOptions} from "../compose"; // A Sources map represents the contributions from each subgraph of the given // element type T. The numeric keys correspond to the indexes of the subgraphs @@ -395,10 +394,8 @@ class Merger { private fieldsWithFromContext: Set; private fieldsWithOverride: Set; private fieldsWithRequires: Set; - private authEnabled: boolean = false; - private accessControlDirectivesInSupergraph: { name: string, nameInSupergraph?: string }[] = []; - private __accessControlSourcesComputed: boolean = false; - private __accessControlAdditionalSources: Map>> = new Map(); + private accessControlDirectivesInSupergraph: { name: string, nameInSupergraph: string }[] = []; + private __accessControlAdditionalSources?: Map>>; constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) { this.latestFedVersionUsed = this.getLatestFederationVersionUsed(); @@ -624,33 +621,33 @@ class Merger { // If we encounter access control directives, we need to record its definition // so we can correctly merge them for polymorphic types - let authenticatedDirective = undefined; if (specInSupergraph.identity === AuthenticatedSpecDefinition.identity && nameInFeature === specInSupergraph.url.name) { - this.authEnabled = true; - authenticatedDirective = this.merged.directive(nameInSupergraph); + const authenticatedDirective = this.merged.directive(nameInSupergraph); + if (authenticatedDirective) { + this.accessControlDirectivesInSupergraph.push({ + name: FederationDirectiveName.AUTHENTICATED, + nameInSupergraph: authenticatedDirective.name, + }); + } } - let requiresScopesDirective = undefined; if (specInSupergraph.identity === RequiresScopesSpecDefinition.identity && nameInFeature === specInSupergraph.url.name) { - this.authEnabled = true; - requiresScopesDirective = this.merged.directive(nameInSupergraph); + const requiresScopesDirective = this.merged.directive(nameInSupergraph); + if (requiresScopesDirective) { + this.accessControlDirectivesInSupergraph.push({ + name: FederationDirectiveName.REQUIRES_SCOPES, + nameInSupergraph: requiresScopesDirective.name, + }); + } } - let policyDirective = undefined; if (specInSupergraph.identity === PolicySpecDefinition.identity && nameInFeature === specInSupergraph.url.name) { - this.authEnabled = true; - policyDirective = this.merged.directive(nameInSupergraph); + const policyDirective = this.merged.directive(nameInSupergraph); + if (policyDirective) { + this.accessControlDirectivesInSupergraph.push({ + name: FederationDirectiveName.POLICY, + nameInSupergraph: policyDirective.name, + }); + } } - this.accessControlDirectivesInSupergraph.push({ - name: FederationDirectiveName.AUTHENTICATED, - nameInSupergraph: authenticatedDirective?.name, - }); - this.accessControlDirectivesInSupergraph.push({ - name: FederationDirectiveName.REQUIRES_SCOPES, - nameInSupergraph: requiresScopesDirective?.name, - }); - this.accessControlDirectivesInSupergraph.push({ - name: FederationDirectiveName.POLICY, - nameInSupergraph: policyDirective?.name, - }); } } } @@ -1285,14 +1282,49 @@ class Merger { } } + /** + * This method is used to copy "user provided" applied directives from + * interface fields to an implementing object field when those fields are only + * provided by `@interfaceObject`s. + * + * However, we shouldn't copy the `join` spec directive as those are for the + * interface field but are invalid for the implementation field. Also, we + * do explicitly compute access control directives for the implementing field + * based on the access control directives for the `@interfaceObject` fields. + */ private copyNonJoinAppliedDirectives(source: SchemaElement, dest: SchemaElement) { - // This method is used to copy "user provided" applied directives from interface fields to some - // implementation type when @interfaceObject is used. But we shouldn't copy the `join` spec directive - // as those are for the interface field but are invalid for the implementation field. + // Note this function may take in the implementing object field, but it also + // may take in arguments to that field. + if (dest instanceof FieldDefinition) { + for (const { name, nameInSupergraph } of this.accessControlDirectivesInSupergraph) { + let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); + if (!additionalSources) { + additionalSources = []; + } + // WARNING: In order to propagate access control directive requirements, we are hijacking existing merge + // directive logic by providing "additional sources" from their interfaces/implementations. This means + // that their corresponding subgraphIndex will be incorrect and shouldn't be used/relied on when merging + // access control directives. + if (additionalSources.length > 0) { + this.mergeAppliedDirective( + nameInSupergraph, + sourcesFromArray(additionalSources), + dest, + ); + } + } + } + source.appliedDirectives.forEach((d) => { - if (!this.joinSpec.isSpecDirective(d.definition!)) { - dest.applyDirective(d.name, {...d.arguments()}) + if (this.joinSpec.isSpecDirective(d.definition!)) { + return; + } + if (this.accessControlDirectivesInSupergraph.some( + ({ nameInSupergraph }) => d.name === nameInSupergraph + )) { + return; } + dest.applyDirective(d.name, {...d.arguments()}); }); } @@ -3006,22 +3038,24 @@ class Merger { if (dest instanceof InterfaceType) { // we need to propagate access control from implementing types upward to the interface for (const { name, nameInSupergraph } of this.accessControlDirectivesInSupergraph) { - if (nameInSupergraph) { - let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); - if (!additionalSources) { - additionalSources = []; - } + let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); + if (!additionalSources) { + additionalSources = []; + } + // WARNING: In order to propagate access control directive requirements, we are hijacking existing merge + // directive logic by providing "additional sources" from their interfaces/implementations. This means + // that their corresponding subgraphIndex will be incorrect and shouldn't be used/relied on when merging + // access control directives. + if (names.has(nameInSupergraph) || additionalSources.length > 0) { // record access control directive IF it is applied on the interface (currently not allowed) // OR there are some implementations with access control on them - if (names.has(nameInSupergraph) || additionalSources) { - this.appliedDirectivesToMerge.push({ - names: new Set([nameInSupergraph]), - sources: sourcesFromArray([...sources.values(), ...additionalSources]), - dest, - }); - } - names.delete(nameInSupergraph); + this.appliedDirectivesToMerge.push({ + names: new Set([nameInSupergraph]), + sources: sourcesFromArray([...sources.values(), ...additionalSources]), + dest, + }); } + names.delete(nameInSupergraph); } } @@ -3042,24 +3076,27 @@ class Merger { } for (const { name, nameInSupergraph } of this.accessControlDirectivesInSupergraph) { + // WARNING: In order to propagate access control directive requirements, we are hijacking existing merge + // directive logic by providing "additional sources" from their interfaces/implementations. This means + // that their corresponding subgraphIndex will be incorrect and shouldn't be used/relied on when merging + // access control directives. + // we need to propagate auth on fields // - upwards from object types to interfaces // - downwards from interface object to object types - if (nameInSupergraph) { - let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); - if (!additionalSources) { - additionalSources = []; - } + let additionalSources = this.accessControlAdditionalSources().get(`${dest.coordinate}_${name}`); + if (!additionalSources) { + additionalSources = []; + } + if (names.has(nameInSupergraph) || additionalSources.length > 0) { // record if access control applied directly OR has some additional sources - if (names.has(nameInSupergraph) || additionalSources) { - this.appliedDirectivesToMerge.push({ - names: new Set([nameInSupergraph]), - sources: sourcesFromArray([...sources.values(), ...additionalSources]), - dest, - }); - } - names.delete(nameInSupergraph); + this.appliedDirectivesToMerge.push({ + names: new Set([nameInSupergraph]), + sources: sourcesFromArray([...sources.values(), ...additionalSources]), + dest, + }); } + names.delete(nameInSupergraph); } this.appliedDirectivesToMerge.push({ @@ -3087,9 +3124,10 @@ class Merger { * corresponding list of interface objects fields that specify `@policy` requirements (downwards propagation) */ private accessControlAdditionalSources(): Map>> { - if (this.__accessControlSourcesComputed) { - return this.__accessControlAdditionalSources; - } else { + if (this.__accessControlAdditionalSources === undefined) { + const sourceMap = new Map(); + // we first populate interface -> implementations and implementation -> interface maps + // we'll use this info to know where to propagate the requirements const intfToImplMap: Map> = new Map(); const implToIntfMap: Map> = new Map(); for (const intf of this.merged.interfaceTypes()) { @@ -3107,8 +3145,22 @@ class Merger { this.subgraphs.values().forEach((subgraph) => { const metadata = subgraph.metadata(); - for (const accessControlDirective of [metadata.authenticatedDirective(), metadata.requiresScopesDirective(), metadata.policyDirective()]) { - for (const application of accessControlDirective.applications()) { + const accessControlDirectives = [ + { + name: FederationDirectiveName.AUTHENTICATED, + directive: metadata.authenticatedDirective(), + }, + { + name: FederationDirectiveName.REQUIRES_SCOPES, + directive: metadata.requiresScopesDirective(), + }, + { + name: FederationDirectiveName.POLICY, + directive: metadata.policyDirective(), + }, + ]; + for (const { name, directive } of accessControlDirectives) { + for (const application of directive.applications()) { const candidate = application.parent; if (candidate instanceof ObjectType) { // we will be propagating access control from objects up to the interfaces @@ -3117,13 +3169,8 @@ class Merger { if (implementedInterfaces) { for (const intf of implementedInterfaces) { // save implementation as additional source for impl - const key = `${intf}_${accessControlDirective.name}`; - let sources = this.__accessControlAdditionalSources.get(key); - if (!sources) { - sources = []; - } - sources.push(candidate); - this.__accessControlAdditionalSources.set(key, sources); + const key = `${intf}_${name}`; + this.recordAdditionalSource(sourceMap, key, candidate); } } } @@ -3135,14 +3182,27 @@ class Merger { // we need to propagate field access control downwards from interface object fields to object fields const implementations = intfToImplMap.get(objectName); if (implementations) { + const otherInterfaces: Set = new Set(); for (const impl of implementations) { - const key = `${impl}.${candidate.name}_${accessControlDirective.name}`; - let additionalSources = this.__accessControlAdditionalSources.get(key); - if (!additionalSources) { - additionalSources = []; + const key = `${impl}.${candidate.name}_${name}`; + this.recordAdditionalSource(sourceMap, key, candidate); + + const implementedInterfaces = implToIntfMap.get(impl); + if (implementedInterfaces) { + for (const intf of implementedInterfaces) { + if (objectName == intf) { + // skip current @interfaceObject + continue; + } + otherInterfaces.add(intf); + } } - additionalSources.push(candidate); - this.__accessControlAdditionalSources.set(key, additionalSources); + } + // we now need to propagate field access control upwards from @interfaceObject fields to any + // other interfaces implemented by the given type + for (const intf of otherInterfaces) { + const key = `${intf}.${candidate.name}_${name}`; + this.recordAdditionalSource(sourceMap, key, candidate); } } } else { @@ -3150,13 +3210,8 @@ class Merger { const implementedInterfaces = implToIntfMap.get(objectName); if (implementedInterfaces) { for (const intf of implementedInterfaces) { - const key = `${intf}.${candidate.name}_${accessControlDirective.name}`; - let additionalSources = this.__accessControlAdditionalSources.get(key); - if (!additionalSources) { - additionalSources = []; - } - additionalSources.push(candidate); - this.__accessControlAdditionalSources.set(key, additionalSources); + const key = `${intf}.${candidate.name}_${name}`; + this.recordAdditionalSource(sourceMap, key, candidate); } } } @@ -3165,9 +3220,22 @@ class Merger { } } }); - this.__accessControlSourcesComputed = true; - return this.__accessControlAdditionalSources; + this.__accessControlAdditionalSources = sourceMap; } + return this.__accessControlAdditionalSources; + } + + private recordAdditionalSource( + sourceMap: Map>>, + key: string, + source: ObjectType | FieldDefinition + ) { + let additionalSources = sourceMap.get(key); + if (!additionalSources) { + additionalSources = []; + } + additionalSources.push(source); + sourceMap.set(key, additionalSources); } // to be called after elements are merged @@ -3207,6 +3275,13 @@ class Merger { // but when an argument is an input object type, we should (?) ignore those fields that will not be included in the supergraph // due the intersection merging of input types, otherwise the merged value may be invalid for the supergraph. let perSource: { directives: Directive[], subgraphIndex: number}[] = []; + + // WARNING: Access control directives (@authenticated, @requiresScopes, and @policy) need to be propagated + // - upwards from object types and fields to interface types and fields + // - downwards from interface object fields to implementing object type fields + // When merging access control directives, we hijack the existing merge mechanism by providing additional + // implementations/interfaces/fields as additional sources. This means that subgraphIndex SHOULD NOT be used + // for merging access control directives. sources.forEach((source, index) => { if (source) { const directives: Directive[] = source.appliedDirectivesOf(name); @@ -3645,7 +3720,7 @@ class Merger { // auth verification on the supergraph // need to verify usage of @requires on fields that require authorization - if (this.authEnabled) { + if (this.accessControlDirectivesInSupergraph.length > 0) { const authValidator = new AuthValidator(this.merged, this.joinSpec, this.subgraphNamesToJoinSpecName); for (const coordinate of this.fieldsWithRequires) { const errors = authValidator.validateRequiresFieldSet(coordinate); @@ -3909,9 +3984,9 @@ export class AuthValidator { const field = type.field(fieldCoordinate[1]); assert(field instanceof FieldDefinition, `Field "${coordinate}" exists in the schema`); - const authRequirementOnRequires = new AuthRequirements(coordinate, '@requires'); - authRequirementOnRequires.type = this.authRequirementsOnElement(type); - authRequirementOnRequires.field = this.authRequirementsOnElement(field); + const typeRequirements = this.authRequirementsOnElement(type); + const fieldRequirements = this.authRequirementsOnElement(field); + const authRequirementOnRequires = new AuthRequirements(coordinate, '@requires', typeRequirements, fieldRequirements); const errors: GraphQLError[] = [] const joinDirectivesOnRequires = field.appliedDirectivesOf(this.joinFieldDirective); @@ -3949,9 +4024,9 @@ export class AuthValidator { const field = type.field(fieldCoordinate[1]); assert(field instanceof FieldDefinition, `Field "${coordinate}" exists in the schema`); - const authRequirementOnContext = new AuthRequirements(coordinate, '@fromContext'); - authRequirementOnContext.type = this.authRequirementsOnElement(type); - authRequirementOnContext.field = this.authRequirementsOnElement(field); + const typeRequirements = this.authRequirementsOnElement(type); + const fieldRequirements = this.authRequirementsOnElement(field); + const authRequirementOnContext = new AuthRequirements(coordinate, '@fromContext', typeRequirements, fieldRequirements); const errors: GraphQLError[] = [] const joinDirectivesOnFromContext = field.appliedDirectivesOf(this.joinFieldDirective); @@ -3999,7 +4074,7 @@ export class AuthValidator { return errors; } - private authRequirementsOnElement(element: NamedType | FieldDefinition | FieldDefinition): AuthRequirementsOnElement | undefined { + private authRequirementsOnElement(element: NamedType | FieldDefinition): AuthRequirementsOnElement | undefined { const requirements = new AuthRequirementsOnElement(); if (this.authenticatedDirective) { const appliedDirective = element.appliedDirectivesOf(this.authenticatedDirective)?.[0]; @@ -4032,27 +4107,9 @@ export class AuthValidator { } private verifyAuthRequirementsOnSelectionSet(authRequirements: AuthRequirements, selectionSet: SelectionSet) { - const parentType = this.schema.type(selectionSet.parentType.name); for (const selection of selectionSet.selections()) { if (selection instanceof FieldSelection) { - if (parentType instanceof InterfaceType) { - // if we are referencing an interface field we need to check against all implementations - for (const implType of parentType.possibleRuntimeTypes()) { - const requirementsOnImplType = this.authRequirementsOnElement(implType); - if (!authRequirements.satisfies(requirementsOnImplType)) { - const msg = `Field "${authRequirements.fieldCoordinate}" does not specify necessary @authenticated, @requiresScopes ` - + `and/or @policy auth requirements to access the transitive interface "${parentType}" data from ${authRequirements.directive} selection set.`; - throw ERRORS.MISSING_TRANSITIVE_AUTH_REQUIREMENTS.err(msg); - } - // re-check the implementation auth condition - this.verifyAuthOnFieldSelection(implType, selection, authRequirements); - } - } - - if (parentType instanceof ObjectType) { - this.verifyAuthOnFieldSelection(parentType, selection, authRequirements); - } - + this.verifyAuthOnFieldSelection(selection, authRequirements); if (selection.selectionSet) { this.verifyAuthRequirementsOnSelectionSet(authRequirements, selection.selectionSet); } @@ -4071,9 +4128,8 @@ export class AuthValidator { } } - private verifyAuthOnFieldSelection(parentType: ObjectType, selection: FieldSelection, authRequirements: AuthRequirements) { - const field = parentType.field(selection.element.name); - assert(field, 'field exists in the schema'); + private verifyAuthOnFieldSelection(selection: FieldSelection, authRequirements: AuthRequirements) { + const field = selection.element.definition; const fieldAuthReqs = this.authRequirementsOnElement(field); const returnType = baseType(field.type!); const fieldReturnAuthReqs = this.authRequirementsOnElement(returnType); @@ -4089,21 +4145,51 @@ export class AuthValidator { class AuthRequirements { fieldCoordinate: string; directive: string; - type?: AuthRequirementsOnElement; - field?: AuthRequirementsOnElement; + requirements: AuthRequirementsOnElement - constructor(coordinate: string, directive: string) { + constructor( + coordinate: string, + directive: string, + typeRequirements: AuthRequirementsOnElement | undefined, + fieldRequirements: AuthRequirementsOnElement | undefined) { this.fieldCoordinate = coordinate; this.directive = directive; + + // we need to combine auth requirements from type and field to get the final reqs + // e.g. if type access requires being @authenticated with scope S1 and field requires scope S2 then we need + // to be @authenticated and have both S1 and S2 scopes to access this field + const requirements = new AuthRequirementsOnElement(); + requirements.isAuthenticated = (typeRequirements?.isAuthenticated ?? false) || (fieldRequirements?.isAuthenticated ?? false); + + const scopesToMerge = []; + if (typeRequirements?.scopes) { + scopesToMerge.push(typeRequirements.scopes); + } + if (fieldRequirements?.scopes) { + scopesToMerge.push(fieldRequirements.scopes); + } + if (scopesToMerge.length > 0) { + requirements.scopes = dnfConjunction(scopesToMerge); + } + + const policiesToMerge = []; + if (typeRequirements?.policies) { + policiesToMerge.push(typeRequirements.policies); + } + if (fieldRequirements?.policies) { + policiesToMerge.push(fieldRequirements.policies); + } + if (policiesToMerge.length > 0) { + requirements.policies = dnfConjunction(policiesToMerge); + } + + this.requirements = requirements; } satisfies(authOnElement: AuthRequirementsOnElement | undefined): boolean { if (authOnElement) { - const typeSatisfiesRequirements = this.type && this.type.satisfies(authOnElement); - const fieldSatisfiesRequirements = this.field && this.field.satisfies(authOnElement); - if (!typeSatisfiesRequirements && !fieldSatisfiesRequirements) { - return false; - } + // auth requirements on element have to be an implication of type + field requirements + return this.requirements.satisfies(authOnElement); } return true; } @@ -4116,28 +4202,28 @@ class AuthRequirementsOnElement { satisfies(other: AuthRequirementsOnElement): boolean { const authenticatedSatisfied = this.isAuthenticated || !other.isAuthenticated; - const scopesSatisfied = this.isSubset(this.scopes, other.scopes); - const policiesSatisfied = this.isSubset(this.policies, other.policies); + const scopesSatisfied = this.isImplication(this.scopes, other.scopes); + const policiesSatisfied = this.isImplication(this.policies, other.policies); return authenticatedSatisfied && scopesSatisfied && policiesSatisfied; } - isSubset(first: string[][] | undefined, second: string[][] | undefined): boolean { - if (second) { - if (first) { - // outer elements follow OR rules so we just need one element to match - // inner elements follow AND rules so ALL has to match - return first.some((firstInner) => second.some((secondInner) => { - const s = new Set(secondInner); - return firstInner.length == secondInner.length && firstInner.every((elem) => s.has(elem)); - })); - } else { - // we don't specify any auth requirements but second one requires it - return false; - } - } else { - // second one does not specify any auth requirements so we are good - return true; - } + // Whether the left DNF expression materially implies the right one. See: + // https://en.wikipedia.org/wiki/Material_conditional + private isImplication(first: string[][] | undefined, second: string[][] | undefined): boolean { + // No auth requirements are the same as auth requirements that are always + // true. We also run `convertEmptyToTrue()`; see its doc string to + // understand why this is necessary. + const firstNormalized = convertEmptyToTrue(first ?? [[]]); + const secondNormalized = convertEmptyToTrue(second ?? [[]]); + + // outer elements follow OR rules so we need all conditions to match as we don't know which one will be provided at runtime + return firstNormalized.every((firstInner) => secondNormalized.some((secondInner) => { + // inner elements follow AND rules which means that + // ALL elements from secondInner has to be present in the firstInner + const firstSet = new Set(firstInner); + const secondSet = new Set(secondInner); + return firstSet.size >= secondSet.size && secondInner.every((elem) => firstSet.has(elem)); + })); } toString(): string { diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 9742bcfc0..56b0e6617 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -1763,15 +1763,15 @@ describe('authentication validations', () => { expect(buildForErrors(doc, { includeAllImports: true })).toStrictEqual([ [ - 'AUTHENTICATION_APPLIED_ON_INTERFACE', + 'AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE', `[S] Invalid use of ${directiveName} on field "I.x": ${directiveName} cannot be applied on interfaces, interface fields and interface objects`, ], [ - 'AUTHENTICATION_APPLIED_ON_INTERFACE', + 'AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE', `[S] Invalid use of ${directiveName} on interface "I": ${directiveName} cannot be applied on interfaces, interface fields and interface objects`, ], [ - 'AUTHENTICATION_APPLIED_ON_INTERFACE', + 'AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE', `[S] Invalid use of ${directiveName} on interface object "O": ${directiveName} cannot be applied on interfaces, interface fields and interface objects`, ], ]); diff --git a/internals-js/src/argumentCompositionStrategies.ts b/internals-js/src/argumentCompositionStrategies.ts index 5a9c77180..4f507dfa7 100644 --- a/internals-js/src/argumentCompositionStrategies.ts +++ b/internals-js/src/argumentCompositionStrategies.ts @@ -65,8 +65,9 @@ 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 + * Each 2D array is interpreted as follows + * * Inner array is interpreted as the conjunction (an AND) of the conditions in the array. + * * Outer array is interpreted as the disjunction (an OR) of the inner arrays. * * Algorithm * * filter out duplicate entries to limit the amount of necessary computations @@ -74,12 +75,19 @@ function unionValues(values: any[]): any { * * 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(values: T[][][]): T[][] { +export function dnfConjunction(values: T[][][]): T[][] { // should never be the case if (values.length == 0) { return []; } + // Copy the 2D arrays, as we'll be modifying them below (due to sorting). + for (let i = 0; i < values.length; i++) { + // See the doc string for `convertEmptyToTrue()` to understand why this is + // necessary. + values[i] = convertEmptyToTrue(dnfCopy(values[i])); + } + // we first filter out duplicate values from candidates // this avoids exponential computation of exactly the same conditions const filtered = filterNestedArrayDuplicates(values); @@ -119,7 +127,14 @@ function filterNestedArrayDuplicates(values: T[][][]): T[][][] { const filtered: T[][][] = []; const seen = new Set; values.forEach((value) => { - value.sort(); + value.forEach((inner) => { + inner.sort(); + }) + value.sort((a, b) => { + const left = JSON.stringify(a); + const right = JSON.stringify(b); + return left > right ? 1 : left < right ? -1 : 0; + }); const key = JSON.stringify(value); if (!seen.has(key)) { seen.add(key); @@ -160,6 +175,25 @@ function deduplicateSubsumedValues(values: T[][]): T[][] { return result; } +function dnfCopy(value: T[][]): T[][] { + const newValue = new Array(value.length); + for (let i = 0; i < value.length; i++) { + newValue[i] = value[i].slice(); + } + return newValue; +} + +/** + * Normally for DNF, you'd consider [] to be always false and [[]] to be always + * true, and code that uses some()/every() needs no special-casing to work with + * these definitions. However, router special-cases [] to also mean true, and so + * if we're about to do any evaluation on DNFs, we need to do these conversions + * beforehand. + */ +export function convertEmptyToTrue(value: T[][]): T[][] { + return value.length === 0 ? [[]] : value; +} + export const ARGUMENT_COMPOSITION_STRATEGIES = { MAX: { name: 'MAX', diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index cdd8e09dd..2c1b82558 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -3825,3 +3825,7 @@ function copyDirectiveDefinitionInner( export function isFieldDefinition(elem: SchemaElement): elem is FieldDefinition { return elem instanceof FieldDefinition; } + +export function isElementNamedType(elem: SchemaElement): elem is NamedType { + return elem instanceof BaseNamedType; +} diff --git a/internals-js/src/error.ts b/internals-js/src/error.ts index fd5f1d105..84d327bcf 100644 --- a/internals-js/src/error.ts +++ b/internals-js/src/error.ts @@ -633,8 +633,8 @@ const MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED = makeCodeDefinition( { addedIn: '2.8.0' }, ); -const AUTHENTICATION_APPLIED_ON_INTERFACE = makeCodeDefinition( - 'AUTHENTICATION_APPLIED_ON_INTERFACE', +const AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE = makeCodeDefinition( + 'AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE', 'The @authenticated, @requiresScopes and @policy directive cannot be applied on interface, interface fields and interface object', { addedIn: '2.9.4' }, ); @@ -746,7 +746,7 @@ export const ERRORS = { LIST_SIZE_INVALID_SIZED_FIELD, LIST_SIZE_INVALID_SLICING_ARGUMENT, MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED, - AUTHENTICATION_APPLIED_ON_INTERFACE, + AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE, MISSING_TRANSITIVE_AUTH_REQUIREMENTS, }; diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index 4827ed065..a77dcf099 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -37,7 +37,7 @@ import { isWrapperType, possibleRuntimeTypes, isIntType, - Type, isFieldDefinition, + Type, isFieldDefinition, isElementNamedType, } from "./definitions"; import { assert, MultiMap, printHumanReadableList, OrderedMap, mapValues, assertUnreachable } from "./utils"; import { SDLValidationRule } from "graphql/validation/ValidationContext"; @@ -2907,15 +2907,15 @@ function validateNoAuthenticationOnInterfaces(metadata: FederationMetadata, erro const policyDirective = metadata.policyDirective(); [authenticatedDirective, requiresScopesDirective, policyDirective].forEach((directive) => { for (const application of directive.applications()) { - const element = application.parent; - function isAppliedOnInterface(type: Type) { - return isInterfaceType(type) || isInterfaceObjectType(baseType(type)); - } - function isAppliedOnInterfaceField(elem: SchemaElement) { - return isFieldDefinition(elem) && isInterfaceType(elem.parent); - } - - if (isAppliedOnInterface(element) || isAppliedOnInterfaceField(element)) { + const element: SchemaElement = application.parent; + if ( + // Is it applied on interface or interface object types? + (isElementNamedType(element) && + (isInterfaceType(element) || isInterfaceObjectType(element)) + ) || + // Is it applied on interface fields? + (isFieldDefinition(element) && isInterfaceType(element.parent)) + ) { let kind = ''; switch (element.kind) { case 'FieldDefinition': @@ -2928,7 +2928,7 @@ function validateNoAuthenticationOnInterfaces(metadata: FederationMetadata, erro kind = 'interface object'; break; } - errorCollector.push(ERRORS.AUTHENTICATION_APPLIED_ON_INTERFACE.err( + errorCollector.push(ERRORS.AUTH_REQUIREMENTS_APPLIED_ON_INTERFACE.err( `Invalid use of @${directive.name} on ${kind} "${element.coordinate}": @${directive.name} cannot be applied on interfaces, interface fields and interface objects`, {nodes: sourceASTs(application, element.parent)}, )); diff --git a/internals-js/src/specs/authenticatedSpec.ts b/internals-js/src/specs/authenticatedSpec.ts index b6efe6001..41a3f1e5a 100644 --- a/internals-js/src/specs/authenticatedSpec.ts +++ b/internals-js/src/specs/authenticatedSpec.ts @@ -24,6 +24,10 @@ export class AuthenticatedSpecDefinition extends FeatureDefinition { minimumFederationVersion, ); + // WARNING: we cannot declare staticArgumentTransform() as access control merge logic needs to propagate + // requirements upwards/downwards between types and interfaces. We hijack the merge process by providing + // implementations/interfaces as "additional sources". This means that we cannot apply staticArgumentTransform() + // as subgraph index index will be wrong/undefined. this.registerDirective(createDirectiveSpecification({ name: AuthenticatedSpecDefinition.directiveName, locations: [ diff --git a/internals-js/src/specs/policySpec.ts b/internals-js/src/specs/policySpec.ts index 378cd4fbc..ca03acb2e 100644 --- a/internals-js/src/specs/policySpec.ts +++ b/internals-js/src/specs/policySpec.ts @@ -31,6 +31,10 @@ export class PolicySpecDefinition extends FeatureDefinition { this.registerType(createScalarTypeSpecification({ name: PolicyTypeName.POLICY })); + // WARNING: we cannot declare staticArgumentTransform() as access control merge logic needs to propagate + // requirements upwards/downwards between types and interfaces. We hijack the merge process by providing + // implementations/interfaces as "additional sources". This means that we cannot apply staticArgumentTransform() + // as subgraph index index will be wrong/undefined. this.registerDirective(createDirectiveSpecification({ name: PolicySpecDefinition.directiveName, args: [{ diff --git a/internals-js/src/specs/requiresScopesSpec.ts b/internals-js/src/specs/requiresScopesSpec.ts index 6d93a9848..a06254709 100644 --- a/internals-js/src/specs/requiresScopesSpec.ts +++ b/internals-js/src/specs/requiresScopesSpec.ts @@ -32,6 +32,10 @@ export class RequiresScopesSpecDefinition extends FeatureDefinition { this.registerType(createScalarTypeSpecification({ name: RequiresScopesTypeName.SCOPE })); + // WARNING: we cannot declare staticArgumentTransform() as access control merge logic needs to propagate + // requirements upwards/downwards between types and interfaces. We hijack the merge process by providing + // implementations/interfaces as "additional sources". This means that we cannot apply staticArgumentTransform() + // as subgraph index index will be wrong/undefined. this.registerDirective(createDirectiveSpecification({ name: RequiresScopesSpecDefinition.directiveName, args: [{ From ac1ed2946c48e0fef4b413b192d8c5fbdb2370ae Mon Sep 17 00:00:00 2001 From: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com> Date: Tue, 11 Nov 2025 22:46:42 -0600 Subject: [PATCH 4/4] fix: fix demand control validations (#3336) `@listSize` validations were not correctly unwrapping non-nullable composite types and fields. --- .changeset/smart-crabs-jump.md | 5 ++ .../src/__tests__/subgraphValidation.test.ts | 83 +++++++++++++++---- internals-js/src/federation.ts | 11 +-- 3 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 .changeset/smart-crabs-jump.md diff --git a/.changeset/smart-crabs-jump.md b/.changeset/smart-crabs-jump.md new file mode 100644 index 000000000..355706ec1 --- /dev/null +++ b/.changeset/smart-crabs-jump.md @@ -0,0 +1,5 @@ +--- +"@apollo/federation-internals": patch +--- + +Fixed demand control validations to unwrap non-nullable composite types and fields when performing validations. \ No newline at end of file diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 56b0e6617..457b57b2e 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -1,6 +1,6 @@ import { DocumentNode } from 'graphql'; import gql from 'graphql-tag'; -import { Subgraph } from '..'; +import { asFed2SubgraphDocument, Subgraph } from '..'; import { buildSubgraph } from '../federation'; import { defaultPrintOptions, printSchema } from '../print'; import { buildForErrors } from './testUtils'; @@ -733,9 +733,18 @@ describe('custom error message for misnamed directives', () => { }); }); -function buildAndValidate(doc: DocumentNode): Subgraph { - const name = 'S'; - return buildSubgraph(name, `http://${name}`, doc).validate(); +function buildAndValidate( + doc: DocumentNode, + options?: { + subgraphName?: string; + asFed2?: boolean; + includeAllImports?: boolean; + }, +): Subgraph { + const subgraphDoc = + options?.asFed2 ?? true ? asFed2SubgraphDocument(doc, { ...options }) : doc; + const name = options?.subgraphName ?? 'S'; + return buildSubgraph(name, `http://${name}`, subgraphDoc).validate(); } describe('@core/@link handling', () => { @@ -824,7 +833,7 @@ describe('@core/@link handling', () => { } `; - validateFullSchema(buildAndValidate(doc)); + validateFullSchema(buildAndValidate(doc, { asFed2: false })); }); it('expands definitions if both the federation spec and link spec are linked', () => { @@ -838,11 +847,13 @@ describe('@core/@link handling', () => { } `; - validateFullSchema(buildAndValidate(doc)); + validateFullSchema(buildAndValidate(doc, { asFed2: false })); }); it('is valid if a schema is complete from the get-go', () => { - validateFullSchema(buildAndValidate(gql(expectedFullSchema))); + validateFullSchema( + buildAndValidate(gql(expectedFullSchema), { asFed2: false }), + ); }); it('expands missing definitions when some are partially provided', () => { @@ -929,7 +940,7 @@ describe('@core/@link handling', () => { // calls the graphQL-js validation, so we can be somewhat sure that if something necessary wasn't expanded // properly, we would have an issue. The main reason we did validate the full schema in prior tests is // so we had at least one full example of a subgraph expansion in the tests. - docs.forEach((doc) => buildAndValidate(doc)); + docs.forEach((doc) => buildAndValidate(doc, { asFed2: false })); }); it('allows known directives with incomplete but compatible definitions', () => { @@ -1033,7 +1044,7 @@ describe('@core/@link handling', () => { ]; // Like above, we really only care that the examples validate. - docs.forEach((doc) => buildAndValidate(doc)); + docs.forEach((doc) => buildAndValidate(doc, { asFed2: false })); }); it('errors on invalid known directive location', () => { @@ -1187,7 +1198,7 @@ describe('@core/@link handling', () => { `; // Just making sure this don't error out. - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('allows defining a repeatable directive as non-repeatable but validates usages', () => { @@ -1229,7 +1240,7 @@ describe('federation 1 schema', () => { directive @requires on FIELD_DEFINITION `; - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('accepts federation directive definitions with nullable arguments', () => { @@ -1251,7 +1262,7 @@ describe('federation 1 schema', () => { directive @requires(fields: String) on FIELD_DEFINITION `; - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('accepts federation directive definitions with "FieldSet" type instead of "_FieldSet"', () => { @@ -1268,7 +1279,7 @@ describe('federation 1 schema', () => { directive @key(fields: FieldSet) on OBJECT | INTERFACE `; - buildAndValidate(doc); + buildAndValidate(doc, { asFed2: false }); }); it('rejects federation directive definition with unknown arguments', () => { @@ -1507,10 +1518,54 @@ describe('@cost', () => { }); describe('@listSize', () => { + it('works on lists', () => { + const doc = gql` + type Query { + a1: [A]! @listSize(assumedSize: 5) + a2: [A] @listSize(assumedSize: 5) + } + + type A { + field: String + } + `; + + expect(buildAndValidate(doc, { includeAllImports: true })); + }); + + it('works on valid sizedFields', () => { + const doc = gql` + type Query { + a1: A @listSize(assumedSize: 10, sizedFields: ["nullableList"]) + a2: A + @listSize( + assumedSize: 10 + sizedFields: ["nullableListOfNullableInts"] + ) + a3: A @listSize(assumedSize: 10, sizedFields: ["ints"]) + a4: A! @listSize(assumedSize: 10, sizedFields: ["nullableList"]) + a5: A! + @listSize( + assumedSize: 10 + sizedFields: ["nullableListOfNullableInts"] + ) + a6: A! @listSize(assumedSize: 10, sizedFields: ["ints"]) + } + + type A { + nullableListOfNullableInts: [Int] + nullableList: [Int!] + ints: [Int!]! + } + `; + + expect(buildAndValidate(doc, { includeAllImports: true })); + }); + it('rejects applications on non-lists (unless it uses sizedFields)', () => { const doc = gql` type Query { - a1: A @listSize(assumedSize: 5) + a1: A! @listSize(assumedSize: 5) a2: A @listSize(assumedSize: 10, sizedFields: ["ints"]) } diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index a77dcf099..7b62796c9 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -1071,7 +1071,7 @@ function validateListSizeAppliedToList( ) { const { sizedFields = [] } = application.arguments(); // @listSize must be applied to a list https://ibm.github.io/graphql-specs/cost-spec.html#sec-Valid-List-Size-Target - if (!sizedFields.length && parent.type && !isListType(parent.type)) { + if (!sizedFields.length && parent.type && !isListType(parent.type) && !isNonNullListType(parent.type)) { errorCollector.push(ERRORS.LIST_SIZE_APPLIED_TO_NON_LIST.err( `"${parent.coordinate}" is not a list`, { nodes: sourceASTs(application, parent) }, @@ -1141,8 +1141,9 @@ function validateSizedFieldsAreValidLists( ) { const { sizedFields = [] } = application.arguments(); // Validate sizedFields https://ibm.github.io/graphql-specs/cost-spec.html#sec-Valid-Sized-Fields-Target - if (sizedFields.length) { - if (!parent.type || !isCompositeType(parent.type)) { + if (sizedFields.length && parent.type) { + const baseParentType = baseType(parent.type); + if (!isCompositeType(baseParentType)) { // The output type must have fields errorCollector.push(ERRORS.LIST_SIZE_INVALID_SIZED_FIELD.err( `Sized fields cannot be used because "${parent.type}" is not a composite type`, @@ -1150,11 +1151,11 @@ function validateSizedFieldsAreValidLists( )); } else { for (const sizedFieldName of sizedFields) { - const sizedField = parent.type.field(sizedFieldName); + const sizedField = baseParentType.field(sizedFieldName); if (!sizedField) { // Sized fields must be present on the output type errorCollector.push(ERRORS.LIST_SIZE_INVALID_SIZED_FIELD.err( - `Sized field "${sizedFieldName}" is not a field on type "${parent.type.coordinate}"`, + `Sized field "${sizedFieldName}" is not a field on type "${baseParentType.coordinate}"`, { nodes: sourceASTs(application, parent) } )); } else if (!sizedField.type || !(isListType(sizedField.type) || isNonNullListType(sizedField.type))) {