diff --git a/.gitignore b/.gitignore index d45a04118c3..7a9a1a13e99 100644 --- a/.gitignore +++ b/.gitignore @@ -28,6 +28,7 @@ vendor .minio.sys # Local test files +.local/ contrib/local-test/dql-data.json contrib/local-test/dql-data.rdf contrib/local-test/gql-data.json diff --git a/NESTED_AUTH_FIX.md b/NESTED_AUTH_FIX.md new file mode 100644 index 00000000000..8d8c6fc71e3 --- /dev/null +++ b/NESTED_AUTH_FIX.md @@ -0,0 +1,99 @@ +# Nested Insert @auth Fix + +This branch fixes a **false accept** security gap in Dgraph GraphQL mutations: nested inserts and `@hasInverse` edge linking could modify protected nodes without running the parent type's update authorization rules. + +## Problem + +When a mutation links a new child to an **existing parent** via `@hasInverse`, Dgraph mutates the parent's inverse predicate but only ran `@auth add` checks on **newly allocated UIDs**. Existing parents were never validated, so non-admin users could run mutations like: + +```graphql +mutation { + addFooItem(input: [{ parent: { id: "foo1" } }]) { numUids } +} +``` + +…even when `ProtectedFoo` requires admin for both add and update. + +Deep nested adds could also create leaf nodes whose stricter `@auth add` rules were not enforced consistently. + +## Fix + +1. **`mutation_rewriter.go`**: track `AffectedNodes` when `asIDReference()` links through an inverse edge to an existing UID. +2. **`mutation.go`**: after `authorizeNewNodes()` (add rules on new UIDs), run `authorizeAffectedNodes()` using **update rules** on affected existing nodes. This runs for **add mutations only** to avoid breaking update mutation flows. + +## Verify locally + +### Unit tests + +```bash +go test ./graphql/resolve -run 'TestAuthQueryRewriting|TestAddChildRecordsAffectedProtectedParent|TestNestedAddRecordsDeepNewNodes' -count=1 +``` + +### E2E integration tests + +Build the patched binary and local Docker image, then run auth e2e tests: + +```bash +make dgraph +make local-image +docker tag dgraph/dgraph:local dgraph/dgraph:nested-auth-fix + +# From graphql/e2e/auth (uses dgraph/dgraph:local via docker-compose.yml) +cd graphql/e2e/auth +docker compose up -d +go test -tags=integration -run TestNestedAdd -count=1 . +docker compose down +``` + +Or via the repo Makefile: + +```bash +make test TAGS=integration PKG=graphql/e2e/auth TEST=TestNestedAdd +``` + +### Manual repro harness + +1. Start Dgraph standalone on port 8080 (see Docker section below). +2. Run: + +```bash +chmod +x .local/mint-jwt.sh .local/repro.sh +./.local/repro.sh +``` + +JWT helper: + +```bash +./.local/mint-jwt.sh USER user@example.com false # non-admin token +./.local/mint-jwt.sh ADMIN user@example.com true # admin token +``` + +Secret lives in `.local/jwt-secret` (gitignored). + +## Docker image + +Build and tag the patched image: + +```bash +make local-image +docker tag dgraph/dgraph:local dgraph/dgraph:nested-auth-fix +``` + +Use in docker-compose (override snippet): + +```yaml +services: + dgraph: + image: dgraph/dgraph:nested-auth-fix + ports: + - "8080:8080" + - "9080:9080" + command: dgraph zero # use standalone or zero+alpha layout for your setup +``` + +For raggen-style standalone on port 8080, point the `dgraph` service image at `dgraph/dgraph:nested-auth-fix` instead of `dgraph/dgraph:v25.2.0`. + +## References + +- [Parent update rules bypassed via child + hasInverse](https://discuss.dgraph.io/t/bug-auth-rules-of-parent-not-respected-when-child-with-hasinverse-is-added/12955) +- Branch: `fix/nested-auth-insertions` (based on `v25.3.5`) diff --git a/docker-compose.override.example.yml b/docker-compose.override.example.yml new file mode 100644 index 00000000000..ddab7ebecf0 --- /dev/null +++ b/docker-compose.override.example.yml @@ -0,0 +1,5 @@ +# Example override for projects using Dgraph standalone (e.g. raggen). +# Copy to docker-compose.override.yml or merge into your compose file. +services: + dgraph: + image: dgraph/dgraph:nested-auth-fix diff --git a/graphql/e2e/auth/nested_add_mutation_test.go b/graphql/e2e/auth/nested_add_mutation_test.go new file mode 100644 index 00000000000..917ce931cc0 --- /dev/null +++ b/graphql/e2e/auth/nested_add_mutation_test.go @@ -0,0 +1,141 @@ +//go:build integration + +/* + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package auth + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/dgraph-io/dgraph/v25/graphql/e2e/common" +) + +func TestNestedAddHasInverseParentUpdateAuthBlocked(t *testing.T) { + adminHeaders := common.GetJWT(t, "admin", "ADMIN", metaInfo) + userHeaders := common.GetJWT(t, "regular", "USER", metaInfo) + + addFooParams := &common.GraphQLParams{ + Headers: adminHeaders, + Query: `mutation { + addProtectedFoo(input: [{ id: "nested-auth-foo1", items: [] }]) { + numUids + } + }`, + } + gqlResponse := addFooParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) + + addItemParams := &common.GraphQLParams{ + Headers: userHeaders, + Query: `mutation { + addFooItem(input: [{ parent: { id: "nested-auth-foo1" } }]) { + numUids + } + }`, + } + gqlResponse = addItemParams.ExecuteAsPost(t, common.GraphqlURL) + require.NotEmpty(t, gqlResponse.Errors) + require.Contains(t, gqlResponse.Errors[0].Message, "authorization failed") + + queryParams := &common.GraphQLParams{ + Headers: adminHeaders, + Query: `query { + queryProtectedFoo(filter: { id: { eq: "nested-auth-foo1" } }) { + items { __typename } + } + }`, + } + gqlResponse = queryParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) + require.JSONEq(t, `{"queryProtectedFoo":[{"items":[]}]}`, string(gqlResponse.Data)) + + deleteParams := &common.GraphQLParams{ + Headers: adminHeaders, + Query: `mutation { + deleteProtectedFoo(filter: { id: { eq: "nested-auth-foo1" } }) { + msg + } + }`, + } + gqlResponse = deleteParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) +} + +func TestNestedAddDeepLeafAuthBlocked(t *testing.T) { + userHeaders := common.GetJWT(t, "member", "USER", metaInfo) + + addParams := &common.GraphQLParams{ + Headers: userHeaders, + Query: `mutation { + addGuardedBase(input: [{ + id: "nested-auth-base1", + files: [{}] + }]) { + numUids + } + }`, + } + gqlResponse := addParams.ExecuteAsPost(t, common.GraphqlURL) + require.NotEmpty(t, gqlResponse.Errors) + require.Contains(t, gqlResponse.Errors[0].Message, "authorization failed") +} + +func TestNestedAddDeepLeafAuthAllowedForAdmin(t *testing.T) { + adminHeaders := common.GetJWT(t, "admin", "ADMIN", metaInfo) + + addParams := &common.GraphQLParams{ + Headers: adminHeaders, + Query: `mutation { + addGuardedBase(input: [{ + id: "nested-auth-base2", + files: [{}] + }]) { + guardedBase { id } + } + }`, + } + gqlResponse := addParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) + + deleteParams := &common.GraphQLParams{ + Headers: adminHeaders, + Query: `mutation { + deleteGuardedBase(filter: { id: { eq: "nested-auth-base2" } }) { + msg + } + }`, + } + gqlResponse = deleteParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) +} + +func TestNestedAddDeepLeafAuthAllowedForUserWithoutNestedFiles(t *testing.T) { + userHeaders := common.GetJWT(t, "member", "USER", metaInfo) + + addParams := &common.GraphQLParams{ + Headers: userHeaders, + Query: `mutation { + addGuardedBase(input: [{ id: "nested-auth-base3", files: [] }]) { + guardedBase { id } + } + }`, + } + gqlResponse := addParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) + + deleteParams := &common.GraphQLParams{ + Headers: userHeaders, + Query: `mutation { + deleteGuardedBase(filter: { id: { eq: "nested-auth-base3" } }) { + msg + } + }`, + } + gqlResponse = deleteParams.ExecuteAsPost(t, common.GraphqlURL) + common.RequireNoGQLErrors(t, gqlResponse) +} diff --git a/graphql/e2e/auth/schema.graphql b/graphql/e2e/auth/schema.graphql index 4148b373968..e103efc79b5 100644 --- a/graphql/e2e/auth/schema.graphql +++ b/graphql/e2e/auth/schema.graphql @@ -974,3 +974,32 @@ type Home @auth( favouriteMember: HomeMember } # union testing - end + +type ProtectedFoo + @auth( + add: { rule: "{ $ROLE: { eq: \"ADMIN\" } }" } + update: { rule: "{ $ROLE: { eq: \"ADMIN\" } }" } + ) { + id: String! @id + items: [FooItem!]! @hasInverse(field: parent) +} + +type FooItem { + parent: ProtectedFoo +} + +type GuardedBase + @auth(add: { + or: [ + { rule: "{ $ROLE: { eq: \"USER\" } }" } + { rule: "{ $ROLE: { eq: \"ADMIN\" } }" } + ] + }) { + id: String! @id + files: [GuardedFile!]! @hasInverse(field: base) +} + +type GuardedFile @auth(add: { rule: "{ $ROLE: { eq: \"ADMIN\" } }" }) { + id: ID! + base: GuardedBase! +} diff --git a/graphql/resolve/auth_add_test.yaml b/graphql/resolve/auth_add_test.yaml index d9dc0de5a4a..cd9f39fbb79 100644 --- a/graphql/resolve/auth_add_test.yaml +++ b/graphql/resolve/auth_add_test.yaml @@ -206,6 +206,22 @@ "Column": [ { "uid": "0x456" } ], "Ticket": [ { "uid": "0x789" } ] } + authquerysec: |- + query { + Project(func: uid(Project_1)) @filter(uid(Project_Auth2)) { + uid + } + Project_1 as var(func: uid(0x123)) + Project_Auth2 as var(func: uid(Project_1)) @cascade { + Project.roles : Project.roles @filter(eq(Role.permission, "ADMIN")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + } + authjsonsec: | + { + "Project": [ { "uid": "0x123" } ] + } - name: Add multiple nodes of different types that fails auth gqlquery: | @@ -348,6 +364,22 @@ "Column": [ { "uid": "0x456" }, { "uid": "0x459" } ], "Ticket": [ { "uid": "0x789" }, { "uid": "0x799" } ] } + authquerysec: |- + query { + Project(func: uid(Project_1)) @filter(uid(Project_Auth2)) { + uid + } + Project_1 as var(func: uid(0x123)) + Project_Auth2 as var(func: uid(Project_1)) @cascade { + Project.roles : Project.roles @filter(eq(Role.permission, "ADMIN")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + } + authjsonsec: | + { + "Project": [ { "uid": "0x123" } ] + } - name: Add multiples of multiple nodes of different types that fails auth gqlquery: | @@ -509,6 +541,36 @@ { "Column": [ { "uid": "0x456" } ] } + authquerysec: |- + query { + Project(func: uid(Project_1)) @filter(uid(Project_Auth2)) { + uid + } + Project_1 as var(func: uid(0x123)) + Project_Auth2 as var(func: uid(Project_1)) @cascade { + Project.roles : Project.roles @filter(eq(Role.permission, "ADMIN")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + Ticket(func: uid(Ticket_3)) @filter(uid(Ticket_Auth4)) { + uid + } + Ticket_3 as var(func: uid(0x789)) + Ticket_Auth4 as var(func: uid(Ticket_3)) @cascade { + Ticket.onColumn : Ticket.onColumn { + Column.inProject : Column.inProject { + Project.roles : Project.roles @filter(eq(Role.permission, "EDIT")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + } + } + } + authjsonsec: | + { + "Project": [ { "uid": "0x123" } ], + "Ticket": [ { "uid": "0x789" } ] + } - name: Add with auth on additional delete that fails gqlquery: | @@ -589,7 +651,37 @@ { "Column": [ { "uid": "0x456" } ] } - error: { "message": couldn't rewrite query for mutation addColumn because authorization failed } + authquerysec: |- + query { + Project(func: uid(Project_1)) @filter(uid(Project_Auth2)) { + uid + } + Project_1 as var(func: uid(0x123)) + Project_Auth2 as var(func: uid(Project_1)) @cascade { + Project.roles : Project.roles @filter(eq(Role.permission, "ADMIN")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + Ticket(func: uid(Ticket_3)) @filter(uid(Ticket_Auth4)) { + uid + } + Ticket_3 as var(func: uid(0x789)) + Ticket_Auth4 as var(func: uid(Ticket_3)) @cascade { + Ticket.onColumn : Ticket.onColumn { + Column.inProject : Column.inProject { + Project.roles : Project.roles @filter(eq(Role.permission, "EDIT")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + } + } + } + authjsonsec: | + { + "Project": [], + "Ticket": [] + } + error: { "message": mutation failed because authorization failed } - name: Add with deep auth on additional delete gqlquery: | @@ -681,6 +773,26 @@ "Column": [ { "uid": "0x456" } ], "Project": [ { "uid": "0x123" } ] } + authquerysec: |- + query { + Ticket(func: uid(Ticket_1)) @filter(uid(Ticket_Auth2)) { + uid + } + Ticket_1 as var(func: uid(0x789)) + Ticket_Auth2 as var(func: uid(Ticket_1)) @cascade { + Ticket.onColumn : Ticket.onColumn { + Column.inProject : Column.inProject { + Project.roles : Project.roles @filter(eq(Role.permission, "EDIT")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + } + } + } + authjsonsec: | + { + "Ticket": [ { "uid": "0x789" } ] + } - name: Add with deep auth on additional delete that fails gqlquery: | @@ -771,7 +883,27 @@ "Column": [ { "uid": "0x456" } ], "Project": [ { "uid": "0x123" } ] } - error: { "message": couldn't rewrite query for mutation addProject because authorization failed } + authquerysec: |- + query { + Ticket(func: uid(Ticket_1)) @filter(uid(Ticket_Auth2)) { + uid + } + Ticket_1 as var(func: uid(0x789)) + Ticket_Auth2 as var(func: uid(Ticket_1)) @cascade { + Ticket.onColumn : Ticket.onColumn { + Column.inProject : Column.inProject { + Project.roles : Project.roles @filter(eq(Role.permission, "EDIT")) { + Role.assignedTo : Role.assignedTo @filter(eq(User.username, "user1")) + } + } + } + } + } + authjsonsec: | + { + "Ticket": [] + } + error: { "message": mutation failed because authorization failed } - name: Add with top level RBAC false gqlquery: | diff --git a/graphql/resolve/auth_test.go b/graphql/resolve/auth_test.go index 5a80dc6ad65..ba6b87547ae 100644 --- a/graphql/resolve/auth_test.go +++ b/graphql/resolve/auth_test.go @@ -59,6 +59,10 @@ type AuthQueryRewritingCase struct { AuthQuery string AuthJson string + // Second post-mutation auth round (e.g. update rules on existing nodes linked via @hasInverse) + AuthQuerySec string + AuthJsonSec string + // Indicates if we should skip auth query verification when using authExecutor. // Example: Top level RBAC rules is true. SkipAuth bool @@ -81,8 +85,8 @@ type authExecutor struct { uids string // auth - authQuery string - authJson string + authQueries []string + authJsons []string skipAuth bool } @@ -114,37 +118,23 @@ func (ex *authExecutor) Execute(ctx context.Context, req *dgoapi.Request, // Check query generated along with mutation. require.Equal(ex.t, ex.dgQuerySec, req.Query) - if len(assigned) == 0 { - // skip state 3, there's no new nodes to apply auth to - ex.state++ - } - - // For rules that don't require auth, it should directly go to step 4. - if ex.skipAuth { - ex.state++ - } - return &dgoapi.Response{ Json: []byte(ex.json), Uids: assigned, Metrics: &dgoapi.Metrics{NumUids: map[string]uint64{touchedUidsKey: 0}}, }, nil - case 3: - // auth - - // check that we got the expected auth query - require.Equal(ex.t, ex.authQuery, req.Query) - - // respond to query - return &dgoapi.Response{ - Json: []byte(ex.authJson), - Metrics: &dgoapi.Metrics{NumUids: map[string]uint64{touchedUidsKey: 0}}, - }, nil - - case 4: - // final result + default: + authIdx := ex.state - 3 + if authIdx < len(ex.authQueries) { + require.Equal(ex.t, ex.authQueries[authIdx], req.Query) + return &dgoapi.Response{ + Json: []byte(ex.authJsons[authIdx]), + Metrics: &dgoapi.Metrics{NumUids: map[string]uint64{touchedUidsKey: 0}}, + }, nil + } + // final result query from FromMutationResult return &dgoapi.Response{ Json: []byte(`{"done": "and done"}`), Metrics: &dgoapi.Metrics{NumUids: map[string]uint64{touchedUidsKey: 0}}, @@ -772,10 +762,18 @@ func checkAddUpdateCase( dgQuerySec: tcase.DGQuerySec, uids: tcase.Uids, dgQuery: tcase.DGQuery, - authQuery: tcase.AuthQuery, - authJson: tcase.AuthJson, skipAuth: tcase.SkipAuth, } + if !tcase.SkipAuth { + if tcase.AuthQuery != "" { + ex.authQueries = append(ex.authQueries, tcase.AuthQuery) + ex.authJsons = append(ex.authJsons, tcase.AuthJson) + } + if tcase.AuthQuerySec != "" { + ex.authQueries = append(ex.authQueries, tcase.AuthQuerySec) + ex.authJsons = append(ex.authJsons, tcase.AuthJsonSec) + } + } resolver := NewDgraphResolver(rewriter(), ex) // -- Act -- diff --git a/graphql/resolve/mutation.go b/graphql/resolve/mutation.go index 5a5093779ae..22ed7dd3351 100644 --- a/graphql/resolve/mutation.go +++ b/graphql/resolve/mutation.go @@ -127,9 +127,10 @@ type DgraphExecutor interface { // The node types is a blank node name -> Type mapping of nodes that could // be created by the upsert. type UpsertMutation struct { - Query []*dql.GraphQuery - Mutations []*dgoapi.Mutation - NewNodes map[string]schema.Type + Query []*dql.GraphQuery + Mutations []*dgoapi.Mutation + NewNodes map[string]schema.Type + AffectedNodes map[string]schema.Type } // DgraphExecutorFunc is an adapter that allows us to compose dgraph execution and @@ -403,6 +404,7 @@ func (mr *dgraphResolver) rewriteAndExecute( result := make(map[string]interface{}) newNodes := make(map[string]schema.Type) + affectedNodes := make(map[string]schema.Type) mutationTimer := newtimer(ctx, &dgraphMutationDuration.OffsetDuration) mutationTimer.Start() @@ -471,6 +473,7 @@ func (mr *dgraphResolver) rewriteAndExecute( } copyTypeMap(upsert.NewNodes, newNodes) + copyAffectedNodeMap(upsert.AffectedNodes, affectedNodes) } mutationTimer.Stop() @@ -479,6 +482,13 @@ func (mr *dgraphResolver) rewriteAndExecute( return emptyResult(schema.GQLWrapf(authErr, "mutation failed")), resolverFailed } + if mutation.MutationType() == schema.AddMutation { + authErr = authorizeAffectedNodes(ctx, mutation, affectedNodes, mr.executor, mutResp.Txn) + if authErr != nil { + return emptyResult(schema.GQLWrapf(authErr, "mutation failed")), resolverFailed + } + } + var dgQuery []*dql.GraphQuery dgQuery, err = mr.mutationRewriter.FromMutationResult(ctx, mutation, mutResp.GetUids(), result) queryErrs = schema.AppendGQLErrs(queryErrs, schema.GQLWrapf(err, @@ -603,21 +613,7 @@ func authorizeNewNodes( queryExecutor DgraphExecutor, txn *dgoapi.TxnContext) error { - customClaims, err := m.GetAuthMeta().ExtractCustomClaims(ctx) - if err != nil { - return schema.GQLWrapf(err, "authorization failed") - } - authVariables := customClaims.AuthVariables - newRw := &authRewriter{ - authVariables: authVariables, - varGen: NewVariableGenerator(), - selector: addAuthSelector, - hasAuthRules: true, - } - - // Collect all the newly created nodes in type groups - - newByType := make(map[string][]uint64) + byType := make(map[string][]uint64) namesToType := make(map[string]schema.Type) for nodeName, nodeTyp := range newNodeTypes { if uidStr, created := uids[nodeName]; created { @@ -629,22 +625,75 @@ func authorizeNewNodes( nodeTyp = nodeTyp.ListType() } namesToType[nodeTyp.Name()] = nodeTyp - newByType[nodeTyp.Name()] = append(newByType[nodeTyp.Name()], uid) + byType[nodeTyp.Name()] = append(byType[nodeTyp.Name()], uid) + } + } + + return authorizeNodesByType(ctx, m, byType, namesToType, addAuthSelector, queryExecutor, txn) +} + +// authorizeAffectedNodes checks update authorization rules on existing nodes that were +// modified by nested insertions linking through @hasInverse. When a new child references +// an existing parent, the inverse edge mutates the parent without creating a new UID for +// it, so authorizeNewNodes never sees the parent. +func authorizeAffectedNodes( + ctx context.Context, + m schema.Mutation, + affectedNodeTypes map[string]schema.Type, + queryExecutor DgraphExecutor, + txn *dgoapi.TxnContext) error { + + if len(affectedNodeTypes) == 0 { + return nil + } + + byType := make(map[string][]uint64) + namesToType := make(map[string]schema.Type) + for uidStr, nodeTyp := range affectedNodeTypes { + uid, err := strconv.ParseUint(uidStr, 0, 64) + if err != nil { + return schema.GQLWrapf(err, "authorization failed") } + if nodeTyp.ListType() != nil { + nodeTyp = nodeTyp.ListType() + } + namesToType[nodeTyp.Name()] = nodeTyp + byType[nodeTyp.Name()] = append(byType[nodeTyp.Name()], uid) } - // sort to get a consistent query rewriting - var createdTypes []string - for typeName := range newByType { - createdTypes = append(createdTypes, typeName) + return authorizeNodesByType(ctx, m, byType, namesToType, updateAuthSelector, queryExecutor, txn) +} + +func authorizeNodesByType( + ctx context.Context, + m schema.Mutation, + nodesByType map[string][]uint64, + namesToType map[string]schema.Type, + authSelector func(schema.Type) *schema.RuleNode, + queryExecutor DgraphExecutor, + txn *dgoapi.TxnContext) error { + + customClaims, err := m.GetAuthMeta().ExtractCustomClaims(ctx) + if err != nil { + return schema.GQLWrapf(err, "authorization failed") + } + authVariables := customClaims.AuthVariables + newRw := &authRewriter{ + authVariables: authVariables, + varGen: NewVariableGenerator(), + selector: authSelector, + hasAuthRules: true, } - sort.Strings(createdTypes) - // Write auth queries for each set of node types + var typeNames []string + for typeName := range nodesByType { + typeNames = append(typeNames, typeName) + } + sort.Strings(typeNames) var needsAuth []string authQrys := make(map[string][]*dql.GraphQuery) - for _, typeName := range createdTypes { + for _, typeName := range typeNames { typ := namesToType[typeName] varName := newRw.varGen.Next(typ, "", "", false) newRw.varName = varName @@ -666,13 +715,6 @@ func authorizeNewNodes( continue } - // Generate query blocks like this for each node type - // - // Todo(func: uid(Todo1)) @filter(uid(Todo2) AND uid(Todo3)) { uid } - // Todo1 as var(func: uid(...new uids of this type...) ) - // Todo2 as var(func: uid(Todo1)) @cascade { ...auth query 1... } - // Todo3 as var(func: uid(Todo1)) @cascade { ...auth query 2... } - typQuery := &dql.GraphQuery{ Attr: typ.Name(), Func: &dql.Function{ @@ -681,7 +723,7 @@ func authorizeNewNodes( Filter: authFilter, Children: []*dql.GraphQuery{{Attr: "uid"}}} - nodes := newByType[typeName] + nodes := nodesByType[typeName] sort.Slice(nodes, func(i, j int) bool { return nodes[i] < nodes[j] }) varQry := &dql.GraphQuery{ Var: varName, @@ -694,15 +736,12 @@ func authorizeNewNodes( needsAuth = append(needsAuth, typeName) authQrys[typeName] = append([]*dql.GraphQuery{typQuery, varQry}, authQueries...) - } if len(needsAuth) == 0 { - // no auth to apply return nil } - // create the query in order so we get a stable query sort.Strings(needsAuth) var qs []*dql.GraphQuery for _, typeName := range needsAuth { @@ -726,12 +765,6 @@ func authorizeNewNodes( for _, typeName := range needsAuth { check, ok := authResult[typeName] if !ok || check == nil { - // We needed auth on this type, but it wasn't even in the response. That - // means Dgraph found no matching nodes and returned nothing for this field. - // So all the nodes failed auth. - - // FIXME: what do we actually want to return to users when auth failed? - // Is this too much? return x.GqlErrorf("authorization failed") } @@ -740,14 +773,10 @@ func authorizeNewNodes( return x.GqlErrorf("authorization failed") } - if len(newByType[typeName]) != len(foundUIDs) { - // Some of the created nodes passed auth and some failed. + if len(nodesByType[typeName]) != len(foundUIDs) { return x.GqlErrorf("authorization failed") } } - // By now either there were no types that needed auth, or all nodes passed the - // auth checks. So the mutation as a whole passed authorization. - return nil } diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index d9dd2d39c66..3ca79888c7b 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -77,12 +77,13 @@ type deleteRewriter struct { // of add a new object for the XID and another for link to an existing XID, depending // on what condition evaluates to true in the upsert. type mutationFragment struct { - queries []*dql.GraphQuery - conditions []string - fragment interface{} - deletes []interface{} - check resultChecker - newNodes map[string]schema.Type + queries []*dql.GraphQuery + conditions []string + fragment interface{} + deletes []interface{} + check resultChecker + newNodes map[string]schema.Type + affectedNodes map[string]schema.Type } // xidMetadata is used to handle cases where we get multiple objects which have same xid value in a @@ -446,6 +447,7 @@ func (arw *AddRewriter) Rewrite( // Example // newNodes["Project3"] = schema.Type(Project) newNodes := make(map[string]schema.Type) + affectedNodes := make(map[string]schema.Type) // mutationsAll stores mutations computed from fragment. These are returned as Mutation parameter // of UpsertMutation var mutationsAll []*dgoapi.Mutation @@ -540,13 +542,15 @@ func (arw *AddRewriter) Rewrite( } queries = append(queries, frag.queries...) copyTypeMap(frag.newNodes, newNodes) + copyAffectedNodeMap(frag.affectedNodes, affectedNodes) } if len(mutationsAll) > 0 { ret = append(ret, &UpsertMutation{ - Query: queries, - Mutations: mutationsAll, - NewNodes: newNodes, + Query: queries, + Mutations: mutationsAll, + NewNodes: newNodes, + AffectedNodes: affectedNodes, }) } @@ -605,6 +609,7 @@ func (urw *UpdateRewriter) Rewrite( // Example // newNodes["Project3"] = schema.Type(Project) newNodes := make(map[string]schema.Type) + affectedNodes := make(map[string]schema.Type) // mutations stores mutations computed from fragment. These are returned as Mutation parameter // of UpsertMutation var mutations []*dgoapi.Mutation @@ -732,16 +737,19 @@ func (urw *UpdateRewriter) Rewrite( if urw.setFrag != nil { copyTypeMap(urw.setFrag.newNodes, newNodes) + copyAffectedNodeMap(urw.setFrag.affectedNodes, affectedNodes) } if urw.delFrag != nil { copyTypeMap(urw.delFrag.newNodes, newNodes) + copyAffectedNodeMap(urw.delFrag.affectedNodes, affectedNodes) } if len(mutations) > 0 { ret = append(ret, &UpsertMutation{ - Query: queries, - Mutations: mutations, - NewNodes: newNodes, + Query: queries, + Mutations: mutations, + NewNodes: newNodes, + AffectedNodes: affectedNodes, }) } return ret, retErrors @@ -1301,6 +1309,7 @@ func asIDReference( result["uid"] = val // val will contain the UID string. addInverseLink(result, srcField, srcUID) + recordAffectedNode(frag, srcField, val.(string), isRemove) // Delete any additional old edges from inverse nodes in case this is not a remove // as part of an Update Mutation. @@ -1653,9 +1662,10 @@ func rewriteObject( updateFromChildren := func(parentFragment, childFragment *mutationFragment) { copyTypeMap(childFragment.newNodes, parentFragment.newNodes) - frag.queries = append(parentFragment.queries, childFragment.queries...) - frag.deletes = append(parentFragment.deletes, childFragment.deletes...) - frag.check = func(lcheck, rcheck resultChecker) resultChecker { + copyAffectedNodeMap(childFragment.affectedNodes, parentFragment.affectedNodes) + parentFragment.queries = append(parentFragment.queries, childFragment.queries...) + parentFragment.deletes = append(parentFragment.deletes, childFragment.deletes...) + parentFragment.check = func(lcheck, rcheck resultChecker) resultChecker { return func(m map[string]interface{}) error { return schema.AppendGQLErrs(lcheck(m), rcheck(m)) } @@ -2427,9 +2437,10 @@ func addInverseLink(obj map[string]interface{}, srcField schema.FieldDefinition, func newFragment(f interface{}) *mutationFragment { return &mutationFragment{ - fragment: f, - check: func(m map[string]interface{}) error { return nil }, - newNodes: make(map[string]schema.Type), + fragment: f, + check: func(m map[string]interface{}) error { return nil }, + newNodes: make(map[string]schema.Type), + affectedNodes: make(map[string]schema.Type), } } @@ -2439,6 +2450,26 @@ func copyTypeMap(from, to map[string]schema.Type) { } } +// recordAffectedNode tracks an existing node that will be modified through an inverse +// edge when a nested object links to it. +func recordAffectedNode( + frag *mutationFragment, + srcField schema.FieldDefinition, + parentUID string, + isRemove bool) { + + if isRemove || srcField == nil || srcField.Inverse() == nil { + return + } + frag.affectedNodes[parentUID] = srcField.Type() +} + +func copyAffectedNodeMap(from, to map[string]schema.Type) { + for uid, typ := range from { + to[uid] = typ + } +} + func extractVal(xidVal interface{}, xidName, typeName string) (string, error) { switch typeName { case "Int": diff --git a/graphql/resolve/nested_auth_schema.graphql b/graphql/resolve/nested_auth_schema.graphql new file mode 100644 index 00000000000..d0c2605de5d --- /dev/null +++ b/graphql/resolve/nested_auth_schema.graphql @@ -0,0 +1,28 @@ +type ProtectedFoo + @auth( + add: { rule: "{ $ROLE: { eq: \"ADMIN\" } }" } + update: { rule: "{ $ROLE: { eq: \"ADMIN\" } }" } + ) { + id: String! @id + items: [FooItem!]! @hasInverse(field: parent) +} + +type FooItem { + parent: ProtectedFoo +} + +type GuardedBase + @auth(add: { + or: [ + { rule: "{ $ROLE: { eq: \"USER\" } }" } + { rule: "{ $ROLE: { eq: \"ADMIN\" } }" } + ] + }) { + id: String! @id + files: [GuardedFile!]! @hasInverse(field: base) +} + +type GuardedFile @auth(add: { rule: "{ $ROLE: { eq: \"ADMIN\" } }" }) { + id: ID! + base: GuardedBase! +} diff --git a/graphql/resolve/nested_auth_test.go b/graphql/resolve/nested_auth_test.go new file mode 100644 index 00000000000..9f1f23a026c --- /dev/null +++ b/graphql/resolve/nested_auth_test.go @@ -0,0 +1,110 @@ +/* + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package resolve + +import ( + "context" + "os" + "testing" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" + + "github.com/dgraph-io/dgraph/v25/graphql/schema" + "github.com/dgraph-io/dgraph/v25/graphql/test" + "github.com/dgraph-io/dgraph/v25/testutil" +) + +func loadNestedAuthSchema(t *testing.T) schema.Schema { + t.Helper() + sch, err := os.ReadFile("nested_auth_schema.graphql") + require.NoError(t, err) + authSchema, err := testutil.AppendAuthInfo(sch, jwt.SigningMethodHS256.Name, "", false) + require.NoError(t, err) + return test.LoadSchemaFromString(t, string(authSchema)) +} + +func TestAddChildRecordsAffectedProtectedParent(t *testing.T) { + gqlSchema := loadNestedAuthSchema(t) + authMeta := &testutil.AuthMeta{ + Namespace: "https://xyz.io/jwt/claims", + Algo: jwt.SigningMethodHS256.Name, + AuthVars: map[string]interface{}{ + "ROLE": "USER", + }, + } + + query := ` + mutation { + addFooItem(input: [{ parent: { id: "foo1" } }]) { + numUids + } + }` + + op, err := gqlSchema.Operation(&schema.Request{Query: query}) + require.NoError(t, err) + mut := test.GetMutation(t, op) + + ctx, err := authMeta.AddClaimsToContext(context.Background()) + require.NoError(t, err) + + arw := NewAddRewriter().(*AddRewriter) + idExistence := map[string]string{ + "ProtectedFoo_1": "0xabc", + } + _, _, _ = arw.RewriteQueries(ctx, mut) + upserts, err := arw.Rewrite(ctx, mut, idExistence) + require.NoError(t, err) + require.Len(t, upserts, 1) + require.Equal(t, "ProtectedFoo", upserts[0].AffectedNodes["0xabc"].Name()) +} + +func TestNestedAddRecordsDeepNewNodes(t *testing.T) { + gqlSchema := loadNestedAuthSchema(t) + authMeta := &testutil.AuthMeta{ + Namespace: "https://xyz.io/jwt/claims", + Algo: jwt.SigningMethodHS256.Name, + AuthVars: map[string]interface{}{ + "ROLE": "USER", + }, + } + + query := ` + mutation { + addGuardedBase(input: [{ + id: "base1", + files: [{}] + }]) { + numUids + } + }` + + op, err := gqlSchema.Operation(&schema.Request{Query: query}) + require.NoError(t, err) + mut := test.GetMutation(t, op) + + ctx, err := authMeta.AddClaimsToContext(context.Background()) + require.NoError(t, err) + + arw := NewAddRewriter().(*AddRewriter) + idExistence := map[string]string{} + _, _, _ = arw.RewriteQueries(ctx, mut) + upserts, err := arw.Rewrite(ctx, mut, idExistence) + require.NoError(t, err) + require.Len(t, upserts, 1) + + var hasBase, hasFile bool + for _, typ := range upserts[0].NewNodes { + switch typ.Name() { + case "GuardedBase": + hasBase = true + case "GuardedFile": + hasFile = true + } + } + require.True(t, hasBase, "expected nested GuardedBase in newNodes") + require.True(t, hasFile, "expected nested GuardedFile in newNodes") +}