-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ACL: Allow users to query data for their groups, username, and permissions #4774
Conversation
addUserFilter should only work for graphql queries. Also non graphql query in test to make sure it returns empty result for acl queryies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after the current changes are made and the tests pass
Reviewed 1 of 3 files at r1, 1 of 3 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @animesh2049 and @manishrjain)
edgraph/access_ee.go, line 789 at r1 (raw file):
addUserFilterToQuery(gq, userId, groupIds) } for _, pred := range x.AllACLPredicates() {
Add a comment here that the query could have ACL predicates which could be part of the blocked preds and we want to allow access to them, hence we delete them from the blocked preds map.
edgraph/access_ee.go, line 786 at r3 (raw file):
if len(blockedPreds) != 0 { if graphql {
Add a comment saying
For GraphQL requests, we allow filtered access to the ACL predicates. Filter for user_id and group_id is applied for the currently logged in user.
edgraph/access_ee.go, line 833 at r3 (raw file):
// addUserFilterToQuery applies makes sure that a user can access only its own // acl info by applying filter of userid and groupid to acl predicates
me(func: type(Group))
me(func: type(Group)) @filter(eq(dgraph.xid, ["group1", "group2",..]))
similarly for User.
edgraph/access_ee.go, line 857 at r3 (raw file):
case "dgraph.user.group": newFilter := groupFilter(groupIds) gq.Filter = parentFilter(newFilter, gq.Filter)
No need to return the pointer here, the function can just update gq.Filter
.
edgraph/access_ee.go, line 860 at r3 (raw file):
case "~dgraph.user.group": newFilter := userFilter(userId) gq.Filter = parentFilter(newFilter, gq.Filter)
same here
edgraph/access_ee.go, line 916 at r3 (raw file):
} */ func addUserFilterToFilter(filter *gql.FilterTree, userId string,
We can update filter in place
edgraph/access_ee.go
Outdated
Op: "AND", | ||
Child: []*gql.FilterTree{filter, newFilter}, | ||
} | ||
filter = parentFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ineffectual assignment to filter
(from ineffassign
)
Reverting this commit because updating pointers inside function doesn't look a neat way. Method used earlier seems more appropriate. This reverts commit 5b16b15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 6 files reviewed, 9 unresolved discussions (waiting on @filter, @golangcibot, @manishrjain, and @pawanrawal)
edgraph/access_ee.go, line 789 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Add a comment here that the query could have ACL predicates which could be part of the blocked preds and we want to allow access to them, hence we delete them from the blocked preds map.
Done.
edgraph/access_ee.go, line 852 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt
-ed with-s
(fromgofmt
){Value: userId},
Done.
edgraph/access_ee.go, line 849 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
File is not
gofmt
-ed with-s
(fromgofmt
)Args: []gql.Arg{{Value: userId}},
Done.
edgraph/access_ee.go, line 786 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Add a comment saying
For GraphQL requests, we allow filtered access to the ACL predicates. Filter for user_id and group_id is applied for the currently logged in user.
Done.
edgraph/access_ee.go, line 833 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
me(func: type(Group))
me(func: type(Group)) @filter(eq(dgraph.xid, ["group1", "group2",..]))
similarly for User.
Done.
edgraph/access_ee.go, line 857 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
No need to return the pointer here, the function can just update
gq.Filter
.
for that I will have to do something like this
updateFilter(newFilter, &gq.Filter)
func updateFilter(newFilter *gql.Filter, filter **gql.Filter) {
if **filter == nil {
*filter = newFilter
...
The current version seems more appropriate to me, so leaving reverted my commit.
edgraph/access_ee.go, line 860 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
same here
Same as above.
edgraph/access_ee.go, line 916 at r3 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
We can update filter in place
Same as above.
edgraph/access_ee.go, line 886 at r4 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
ineffectual assignment to
filter
(fromineffassign
)
Done.
This PR enables users to query their groups, username, permissions. Earlier only
guardians
could do any of that.This change is
Docs Preview: