Remove unauthorized predicates from query

[animesh2049]

All unauthorized predicates will be silently dropped from query and mutation without warning/error.
Unchanged behaviors:

• mutation of ACL predicates is still not allowed.
• alter will still give permission denied error if user doesn't have alter permission on any one of the predicate

This change is 

[manishrjain]

Please add tests and address my comments.

OK from my side to go. Please get it approved by someone and merge.

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @animesh2049)

---

edgraph/access_ee.go, line 466 at r1 (raw file):

	aclOp *acl.Operation) map[string]struct{} {

	blkdPreds := make(map[string]struct{})

blockedPreds

Avoid removing the vowels.

---

edgraph/access_ee.go, line 539 at r1 (raw file):

		}

		unAuthPreds := authorizePreds(userId, groupIds, preds, acl.Modify)

blockedPreds

---

edgraph/access_ee.go, line 652 at r1 (raw file):

	}

	unAuthPreds, err := doAuthorizeMutation()

Let mutation throw an error when trying to mutate a predicate that they don't have access to. Don't apply anything for that mutation, reject the whole thing.

---

edgraph/access_ee.go, line 753 at r1 (raw file):

	unAuthPreds, err := doAuthorizeQuery()
	if len(unAuthPreds) != 0 {
		parsedReq.Query = removeQryPreds(parsedReq.Query, unAuthPreds)

removePreds(...)

---

edgraph/access_ee.go, line 799 at r1 (raw file):

}

func removeQryPreds(gqls []*gql.GraphQuery, unAuthPreds map[string]struct{}) []*gql.GraphQuery {

removePredsFromQuery

---

edgraph/access_ee.go, line 821 at r1 (raw file):

}

func removeMutPreds(mut *gql.Mutation, unAuthPreds map[string]struct{}) *gql.Mutation {

Remove this.

[animesh2049]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @manishrjain)

---

edgraph/access_ee.go, line 466 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

blockedPreds

Avoid removing the vowels.

Done.

---

edgraph/access_ee.go, line 539 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

blockedPreds

Done.

---

edgraph/access_ee.go, line 652 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Let mutation throw an error when trying to mutate a predicate that they don't have access to. Don't apply anything for that mutation, reject the whole thing.

Done.

---

edgraph/access_ee.go, line 753 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

removePreds(...)

changed it to removePredsFromQuery

---

edgraph/access_ee.go, line 799 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

removePredsFromQuery

Done.

---

edgraph/access_ee.go, line 821 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove this.

Done.

[golangcibot]

line is 102 characters (from lll)

[martinmr]

Just a minor comment but it

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @animesh2049 and @manishrjain)

---

edgraph/access_ee.go, line 526 at r3 (raw file):

		if x.IsGuardian(groupIds) {
			// Members of guardian group are allowed to alter anything.
			// TODO(Animesh): Check if this could possibly lead to

maybe this todo should link to a github or jira issue. Unless you work on it right after merging this PR.

[animesh2049]

Dismissed @golangcibot from a discussion.
Reviewable status: 3 of 4 files reviewed, 7 unresolved discussions (waiting on @manishrjain and @martinmr)

---

edgraph/access_ee.go, line 526 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

maybe this todo should link to a github or jira issue. Unless you work on it right after merging this PR.

Checked this. We can't modify acl predicates, there is another check at edgraph/server.go:198

[pawanrawal]

Reviewed 3 of 4 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

---

edgraph/access_ee.go, line 753 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

changed it to removePredsFromQuery

Could return error if there is an error and then do this removePreds later.

---

edgraph/access_ee.go, line 484 at r4 (raw file):

// authorizeAlter parses the Schema in the operation and authorizes the operation
// using the aclCachePtr

Add a comment that if any of the predicates is unauthorized, we reject the whole alter operation.

---

edgraph/access_ee.go, line 537 at r4 (raw file):

		unAuthPreds := authorizePreds(userId, groupIds, preds, acl.Modify)
		if len(unAuthPreds) > 0 {
			var preds []string

can pre-allocate

---

edgraph/access_ee.go, line 794 at r4 (raw file):

	filter := gqls[:0]
	for _, gq := range gqls {

We also need to check Order, FilterTree and GroupByAttrs, there might also be others.

Also update the code where this is picked from.

[mangalaman93]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @animesh2049, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 466 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Done.

Would it make more sense to use a list here and run unique on it instead of using a map? List have better GC side effects in terms of memory. We can run unique on it and look for predicates using binary search. I imagine the list to be short in length and binary search could even be faster than using a hadhmap (map in Go).

---

edgraph/access_ee.go, line 539 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Done.

Not done yet.

---

edgraph/access_ee.go, line 477 at r4 (raw file):

			})

			blockedPreds[pred] = struct{}{}

Now, we won't be telling the user why an access was blocked. We do not return the error. This could be useful in cases of mutation and alter where error occurs other than permission denied.

---

edgraph/access_ee.go, line 537 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can pre-allocate

Could use a string builder directly to build the string that is sent in the message? No need to build the list.

---

edgraph/access_ee.go, line 743 at r4 (raw file):

	unAuthPreds, err := doAuthorizeQuery()
	if len(unAuthPreds) != 0 {

we should do this check inside the function removePredsFromQuery

[mangalaman93]

I am not sure about this change altogether. Has any user asked for this? @manishrjain @campoy

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @animesh2049, @manishrjain, @martinmr, and @pawanrawal)

[animesh2049]

Reviewable status: 3 of 4 files reviewed, 12 unresolved discussions (waiting on @animesh2049, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 537 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can pre-allocate

Done.

[animesh2049]

Reviewable status: 3 of 4 files reviewed, 12 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 743 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

we should do this check inside the function removePredsFromQuery

I think it makes more sense to do this check here. removePredsFromQuery function is intended to remove all predicates present in blockedPreds Map, from query. If blockedPreds is empty, this function won't remove any predicate. This behavior seems to be apt for removePredsFromQuery.
We should check if blockedPreds is empty before calling removePredsFromQuery to avoid unnecessary computation.

[animesh2049]

Reviewable status: 3 of 5 files reviewed, 12 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 794 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We also need to check Order, FilterTree and GroupByAttrs, there might also be others.

Also update the code where this is picked from.

Done.

[manishrjain]

Reviewable status: 3 of 5 files reviewed, 19 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 645 at r5 (raw file):

			var msg strings.Builder
			for key := range blockedPreds {
				_, _ = msg.WriteString(key + " ")

Either remove _, _ =, or do x.Check2.

---

edgraph/access_ee.go, line 685 at r5 (raw file):

			predsMap[ord.Attr] = struct{}{}
		}

Remove vertical spaces.

---

edgraph/access_ee.go, line 689 at r5 (raw file):

			predsMap[spec.Attr] = struct{}{}
		}

Remove vertical spaces.

---

edgraph/access_ee.go, line 708 at r5 (raw file):

func parsePredsFromFilter(f *gql.FilterTree) []string {
	preds := make([]string, 0)

Remove vertical space.

---

edgraph/access_ee.go, line 712 at r5 (raw file):

		return preds
	}

Remove vertical space.

---

edgraph/access_ee.go, line 716 at r5 (raw file):

		preds = append(preds, f.Func.Attr)
	}

Remove vertical space.

---

edgraph/access_ee.go, line 720 at r5 (raw file):

		preds = append(preds, parsePredsFromFilter(ch)...)
	}

Remove vertical space.

[mangalaman93]

Reviewable status: 3 of 5 files reviewed, 23 unresolved discussions (waiting on @animesh2049, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 539 at r5 (raw file):

			var msg strings.Builder
			for key := range blockedPreds {
				_, _ = msg.WriteString(key + " ")

Call to two WriteString

---

edgraph/access_ee.go, line 622 at r5 (raw file):

		userData, err := extractUserAndGroups(ctx)
		if err != nil {
			// We don't follow fail open approach anymore.

What is fail open approach?

---

edgraph/access_ee.go, line 682 at r5 (raw file):

		}

		for _, ord := range gq.Order {

Would it make more sense to define a internal function to do this and call the function 3 times?

---

edgraph/access_ee.go, line 855 at r5 (raw file):

}

func removeFilters(f *gql.FilterTree, blockedPreds map[string]struct{}) *gql.FilterTree {

We should add test case for each scenario. Currently, I don't see any test cases.

---

edgraph/access_ee.go, line 882 at r5 (raw file):

}

func removeGroupBy(groupAttrs []gql.GroupByAttr,

It may be pretty strange that I put a group by in the query and results are not grouped by because I didn't have access to the predicate in the group by. same would be true for order by and filter. It makes sense to remove predicates from the result, it would be pretty weird to modify the query in filters, orderby and groupby. @manishrjain @pawanrawal

---

testutil/client.go, line 300 at r5 (raw file):

		}
	} else {
		require.True(t, output.Data != nil,

why change this?

[animesh2049]

Dismissed @mangalaman93 from a discussion.
Reviewable status: 3 of 5 files reviewed, 23 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 539 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Call to two WriteString

Done.

---

edgraph/access_ee.go, line 622 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

What is fail open approach?

If there is no policy then anyone can have all access.

---

edgraph/access_ee.go, line 645 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Either remove _, _ =, or do x.Check2.

Done.

---

edgraph/access_ee.go, line 682 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Would it make more sense to define a internal function to do this and call the function 3 times?

I don't think so.

---

edgraph/access_ee.go, line 685 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove vertical spaces.

Done.

---

edgraph/access_ee.go, line 689 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove vertical spaces.

Done.

---

edgraph/access_ee.go, line 712 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove vertical space.

Done.

---

edgraph/access_ee.go, line 716 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove vertical space.

Done.

---

edgraph/access_ee.go, line 720 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove vertical space.

Done.

---

testutil/client.go, line 300 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

why change this?

Sometimes output.Data is not nil but its length is 0.

[animesh2049]

Reviewable status: 2 of 5 files reviewed, 23 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 484 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment that if any of the predicates is unauthorized, we reject the whole alter operation.

Done.

---

edgraph/access_ee.go, line 708 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove vertical space.

Done.

---

edgraph/access_ee.go, line 855 at r5 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

We should add test case for each scenario. Currently, I don't see any test cases.

Done.

[mangalaman93]

Reviewable status: 2 of 5 files reviewed, 22 unresolved discussions (waiting on @animesh2049, @manishrjain, @martinmr, and @pawanrawal)

---

edgraph/access_ee.go, line 683 at r6 (raw file):

			predsMap[gq.Attr] = struct{}{}
		}
		for _, ord := range gq.Order {

we could use single letter names in this and following for loop.

---

edgraph/access_ee.go, line 689 at r6 (raw file):

			predsMap[spec.Attr] = struct{}{}
		}
		for _, pred := range parsePredsFromFilter(gq.Filter) {

seems like we are creating a list and then iterating over the list to write to map. We could just write to map directly.

---

edgraph/access_ee.go, line 820 at r6 (raw file):

	blockedPreds map[string]struct{}) []*gql.GraphQuery {

	filteredQuery := gqls[:0]

call it filteredGqs instead

---

ee/acl/acl_test.go, line 612 at r6 (raw file):

		output      string
		description string
		err         error

error is not used, can be removed.

---

ee/acl/acl_test.go, line 654 at r6 (raw file):

			`,
			`{"me1":[{"name":"RandomGuy"},{"name":"RandomGuy2"}],"me2":[{"name":"RandomGuy"},{"name":"RandomGuy2"}]}`,
			`me1, me2 will have same resul, can't order by <age> since it is unauthorized`,

results*

[animesh2049]

Reviewable status: 2 of 5 files reviewed, 22 unresolved discussions (waiting on @animesh2049, @mangalaman93, @manishrjain, @martinmr, and @pawanrawal)

---

ee/acl/acl_test.go, line 654 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

results*

Done.