Skip to content

Commit dcd1998

Browse files
angrykoalaangrykoala
authored
Merge pull request #6801 from neo4j/6797-variable-this0-not-defined-on-create-with-authorization
Fix authorization in create
2 parents 1025d3a + 73a57ef commit dcd1998

File tree

11 files changed

+293
-115
lines changed

11 files changed

+293
-115
lines changed

‎packages/graphql/src/translate/queryAST/ast/operations/CreateOperation.ts‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,11 +88,10 @@ export class CreateOperation extends MutationOperation {
8888
this.projectionOperations.push(...operations);
8989
}
9090

91-
/** Post subqueries */
9291
public getAuthorizationSubqueries(_context: QueryASTContext): Cypher.Clause[] {
9392
const nestedContext = this.nestedContext;
9493

95-
if (!nestedContext) {
94+
if (!nestedContext || !nestedContext.hasTarget()) {
9695
throw new Error(
9796
"Error parsing query, nested context not available, need to call transpile first. Please contact support"
9897
);
@@ -136,11 +135,11 @@ export class CreateOperation extends MutationOperation {
136135

137136
const createClause = new Cypher.Create(createPattern);
138137

139-
const setParams = Array.from(this.inputFields.values()).flatMap((input) => {
138+
const setParams = this.inputFields.flatMap((input) => {
140139
return input.getSetParams(nestedContext);
141140
});
142141

143-
const mutationSubqueries = Array.from(this.inputFields.values()).flatMap((input) => {
142+
const mutationSubqueries = this.inputFields.flatMap((input) => {
144143
return input.getSubqueries(nestedContext);
145144
});
146145

@@ -171,19 +170,15 @@ export class CreateOperation extends MutationOperation {
171170
return { projectionExpr: nestedContext.target, clauses: [clauses] };
172171
}
173172

174-
private getAuthorizationClauses(context: QueryASTContext): Cypher.Clause[] {
175-
const { selections, subqueries, predicates, validations } = this.transpileAuthClauses(context);
176-
const predicate = Cypher.and(...predicates);
177-
const lastSelection = selections[selections.length - 1];
173+
private getAuthorizationClauses(context: QueryASTContext<Cypher.Node>): Cypher.Clause[] {
174+
const { selections, subqueries, validations } = this.transpileAuthClauses(context);
178175

179-
if (!predicates.length && !validations.length) {
176+
if (!validations.length) {
180177
return [];
181178
} else {
182-
if (lastSelection) {
183-
lastSelection.where(predicate);
184-
return [...subqueries, new Cypher.With("*"), ...selections, ...validations];
185-
}
186-
return [...subqueries, new Cypher.With("*").where(predicate), ...selections, ...validations];
179+
return [
180+
Cypher.utils.concat(...subqueries.map((sq) => new Cypher.Call(sq, "*")), ...selections, ...validations),
181+
];
187182
}
188183
}
189184

‎packages/graphql/src/translate/queryAST/ast/operations/TopLevelCreateMutationOperation.ts‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class TopLevelCreateMutationOperation extends Operation {
3232
// The response fields in the mutation, currently only READ operations are supported in the MutationResponse
3333
private readonly projectionOperations: OperationField[];
3434

35-
private readonly createOperations: CreateOperation[] = [];
35+
private readonly topLevelCreateOperations: CreateOperation[] = [];
3636

3737
constructor({
3838
createOperations,
@@ -42,36 +42,50 @@ export class TopLevelCreateMutationOperation extends Operation {
4242
projectionOperations: OperationField[];
4343
}) {
4444
super();
45-
this.createOperations = createOperations;
45+
this.topLevelCreateOperations = createOperations;
4646
this.projectionOperations = projectionOperations;
4747
}
4848

4949
public getChildren(): QueryASTNode[] {
50-
return filterTruthy([...this.createOperations, ...this.projectionOperations]);
50+
return filterTruthy([...this.topLevelCreateOperations, ...this.projectionOperations]);
5151
}
5252

5353
public transpile(context: QueryASTContext): OperationTranspileResult {
5454
if (!context.hasTarget()) {
5555
throw new Error("No parent node found!");
5656
}
57-
const subqueries = this.createOperations.map((field) => {
58-
const { clauses, projectionExpr } = field.transpile(context);
57+
const operationQueries = this.topLevelCreateOperations.map((createOperation) => {
58+
const { clauses, projectionExpr } = createOperation.transpile(context);
59+
60+
const authSubqueries = this.getAuthorizationSubqueriesForCreateOperation(createOperation, context);
5961

6062
return Cypher.utils.concat(
6163
...clauses,
62-
...field.getAuthorizationSubqueries(context),
64+
...authSubqueries,
6365
new Cypher.Return([projectionExpr, context.returnVariable])
6466
);
6567
});
6668

67-
const unionStatement = new Cypher.Call(new Cypher.Union(...subqueries));
69+
const unionStatement = new Cypher.Call(new Cypher.Union(...operationQueries));
6870
const projection: Cypher.Clause = this.getProjectionClause(context);
6971
return {
7072
projectionExpr: context.returnVariable,
7173
clauses: [unionStatement, projection],
7274
};
7375
}
7476

77+
private getAuthorizationSubqueriesForCreateOperation(
78+
operation: CreateOperation,
79+
context: QueryASTContext<Cypher.Node>
80+
): Cypher.Clause[] {
81+
const authSubqueries = operation.getAuthorizationSubqueries(context).map((sq) => new Cypher.Call(sq, "*"));
82+
if (authSubqueries.length > 0) {
83+
return [new Cypher.With("*"), ...authSubqueries];
84+
}
85+
86+
return [];
87+
}
88+
7589
private getProjectionClause(context: QueryASTContext<Cypher.Node>): Cypher.Clause {
7690
const projectionOperation = this.projectionOperations[0]; // TODO: multiple projection operations not supported
7791

‎packages/graphql/tests/integration/deprecations/aggregations/where/authorization-with-aggregation-filter.int.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,10 @@ describe("authorization-with-aggregation-filter", () => {
194194
expect((gqlResult.errors as any[])[0].message).toBe("Forbidden");
195195
});
196196

197-
test("should authorize update operations on post with exactly two likes", async () => {
197+
// Test disabled due to flakyness. Enable once `validatePredicate` has been removed from update operations.
198+
// The flakyness is caused by the `AND` operation, that doesn't guarantee shortcircuit of each predicate
199+
// eslint-disable-next-line jest/no-disabled-tests
200+
test.skip("should authorize update operations on post with exactly two likes", async () => {
198201
const typeDefs = /* GraphQL */ `
199202
type ${User} @node {
200203
id: ID!

‎packages/graphql/tests/integration/issues/6797.int.test.ts‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
20+
import type { UniqueType } from "../../utils/graphql-types";
21+
import { TestHelper } from "../../utils/tests-helper";
22+
23+
describe("https://github.com/neo4j/graphql/issues/6797", () => {
24+
const testHelper = new TestHelper();
25+
let typeDefs: string;
26+
const secret = "sssh";
27+
28+
let Group: UniqueType;
29+
let Invitee: UniqueType;
30+
31+
beforeAll(async () => {
32+
Group = testHelper.createUniqueType("Group");
33+
Invitee = testHelper.createUniqueType("Invitee");
34+
35+
typeDefs = /* GraphQL */ `
36+
type ${Group} @node {
37+
id: ID! @id
38+
name: String!
39+
invitees: [${Invitee}!]! @relationship(type: "INVITED_TO", direction: IN, aggregate: true)
40+
}
41+
42+
enum InviteeStatus {
43+
PENDING
44+
ACCEPTED
45+
}
46+
47+
type ${Invitee}
48+
@node
49+
@authorization(
50+
validate: [
51+
{
52+
operations: [CREATE]
53+
where: { node: { group_ALL: { inviteesAggregate: { count_LT: 5 } } } }
54+
}
55+
]
56+
) {
57+
id: ID! @id
58+
group: [${Group}!]! @relationship(type: "INVITED_TO", direction: OUT)
59+
email: String!
60+
status: InviteeStatus! @default(value: PENDING)
61+
}
62+
`;
63+
64+
await testHelper.initNeo4jGraphQL({
65+
typeDefs,
66+
features: {
67+
authorization: {
68+
key: secret,
69+
},
70+
},
71+
});
72+
});
73+
74+
afterAll(async () => {
75+
await testHelper.close();
76+
});
77+
78+
test("create and connect invitees to groups", async () => {
79+
await testHelper.executeCypher(`
80+
CREATE (:${Group} { id: "an-id", name: "groupymcgroupface" });
81+
`);
82+
83+
const mutation = /* GraphQL */ `
84+
mutation {
85+
${Group.operations.create}(
86+
input: [
87+
{
88+
name: "My Name"
89+
invitees: {
90+
create: [
91+
{
92+
node: {
93+
email: "an email"
94+
group: { connect: [{ where: { node: { id_EQ: "an-id" } } }] }
95+
}
96+
}
97+
]
98+
}
99+
}
100+
]
101+
) {
102+
${Group.plural} {
103+
invitees {
104+
email
105+
group {
106+
id
107+
}
108+
}
109+
}
110+
}
111+
}
112+
`;
113+
114+
const token = testHelper.createBearerToken(secret);
115+
const queryResult = await testHelper.executeGraphQLWithToken(mutation, token);
116+
117+
expect(queryResult.errors).toBeUndefined();
118+
expect(queryResult.data).toEqual({
119+
[Group.operations.create]: {
120+
[Group.plural]: [
121+
{
122+
invitees: [
123+
{
124+
email: "an email",
125+
group: expect.toIncludeSameMembers([
126+
{
127+
id: "an-id",
128+
},
129+
{
130+
id: expect.toBeString(),
131+
},
132+
]),
133+
},
134+
],
135+
},
136+
],
137+
},
138+
});
139+
140+
await testHelper.expectRelationship(Invitee, Group, "INVITED_TO").count(2);
141+
});
142+
});

‎packages/graphql/tests/tck/directives/authorization/arguments/roles-where.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -891,7 +891,9 @@ describe("Cypher Auth Where with Roles", () => {
891891
} AND ($jwt.roles IS NOT NULL AND $param7 IN $jwt.roles)) OR ($isAuthenticated = true AND ($jwt.roles IS NOT NULL AND $param8 IN $jwt.roles))), \\"@neo4j/graphql/FORBIDDEN\\", [0])
892892
}
893893
WITH *
894-
CALL apoc.util.validate(NOT (($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub) AND ($jwt.roles IS NOT NULL AND $param9 IN $jwt.roles)) OR ($isAuthenticated = true AND ($jwt.roles IS NOT NULL AND $param10 IN $jwt.roles))), \\"@neo4j/graphql/FORBIDDEN\\", [0])
894+
CALL (*) {
895+
CALL apoc.util.validate(NOT (($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub) AND ($jwt.roles IS NOT NULL AND $param9 IN $jwt.roles)) OR ($isAuthenticated = true AND ($jwt.roles IS NOT NULL AND $param10 IN $jwt.roles))), \\"@neo4j/graphql/FORBIDDEN\\", [0])
896+
}
895897
RETURN this0 AS this
896898
}
897899
WITH this
@@ -973,7 +975,9 @@ describe("Cypher Auth Where with Roles", () => {
973975
} AND ($jwt.roles IS NOT NULL AND $param8 IN $jwt.roles)) OR ($isAuthenticated = true AND ($jwt.roles IS NOT NULL AND $param9 IN $jwt.roles))), \\"@neo4j/graphql/FORBIDDEN\\", [0])
974976
}
975977
WITH *
976-
CALL apoc.util.validate(NOT (($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub) AND ($jwt.roles IS NOT NULL AND $param10 IN $jwt.roles)) OR ($isAuthenticated = true AND ($jwt.roles IS NOT NULL AND $param11 IN $jwt.roles))), \\"@neo4j/graphql/FORBIDDEN\\", [0])
978+
CALL (*) {
979+
CALL apoc.util.validate(NOT (($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub) AND ($jwt.roles IS NOT NULL AND $param10 IN $jwt.roles)) OR ($isAuthenticated = true AND ($jwt.roles IS NOT NULL AND $param11 IN $jwt.roles))), \\"@neo4j/graphql/FORBIDDEN\\", [0])
980+
}
977981
RETURN this0 AS this
978982
}
979983
WITH this

‎packages/graphql/tests/tck/directives/authorization/arguments/validate/interface-relationships/implementation-bind.test.ts‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -134,14 +134,18 @@ describe("Cypher Auth Allow", () => {
134134
SET
135135
this1.id = $param3
136136
WITH *
137-
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
138-
WITH *
139-
CALL apoc.util.validate(NOT ($isAuthenticated = true AND EXISTS {
140-
MATCH (this1)<-[:HAS_CONTENT]-(this5:User)
141-
WHERE ($jwt.sub IS NOT NULL AND this5.id = $jwt.sub)
142-
}), \\"@neo4j/graphql/FORBIDDEN\\", [0])
143-
WITH *
144-
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this2.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
137+
CALL (*) {
138+
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
139+
}
140+
CALL (*) {
141+
CALL apoc.util.validate(NOT ($isAuthenticated = true AND EXISTS {
142+
MATCH (this1)<-[:HAS_CONTENT]-(this5:User)
143+
WHERE ($jwt.sub IS NOT NULL AND this5.id = $jwt.sub)
144+
}), \\"@neo4j/graphql/FORBIDDEN\\", [0])
145+
}
146+
CALL (*) {
147+
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this2.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
148+
}
145149
RETURN this0 AS this
146150
}
147151
WITH this
@@ -221,9 +225,12 @@ describe("Cypher Auth Allow", () => {
221225
SET
222226
this1.id = $param3
223227
WITH *
224-
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
225-
WITH *
226-
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this2.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
228+
CALL (*) {
229+
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this0.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
230+
}
231+
CALL (*) {
232+
CALL apoc.util.validate(NOT ($isAuthenticated = true AND ($jwt.sub IS NOT NULL AND this2.id = $jwt.sub)), \\"@neo4j/graphql/FORBIDDEN\\", [0])
233+
}
227234
RETURN this0 AS this
228235
}
229236
WITH this

0 commit comments

Comments
 (0)