-
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
Block delete if predicate permission is zero #4349
Conversation
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.
apart from a couple of small comments.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @animesh2049 and @manishrjain)
edgraph/access_ee.go, line 584 at r1 (raw file):
setPreds := parsePredsFromMutation(gmu.Set) delPreds := parsePredsFromMutation(gmu.Del) preds := append(setPreds, delPreds...)
this can be written as:
preds := parsePredsFromMutation(gmu.Set)
preds = append(preds, parsePredsFromMutation(gmu.Del)...)
ee/acl/acl_test.go, line 524 at r1 (raw file):
} _, err = txn.Mutate(ctx, mutation) require.NotNil(t, err)
use require.Error
instead.
Also, it might be useful to check the error message is what you expect. It's possible that it fails because of an unrelated error. You could use
require.Contains(t, err.Error(), "whatever error message needs to be checked")
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: all files reviewed, 2 unresolved discussions (waiting on @manishrjain and @martinmr)
edgraph/access_ee.go, line 584 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
setPreds := parsePredsFromMutation(gmu.Set) delPreds := parsePredsFromMutation(gmu.Del) preds := append(setPreds, delPreds...)
this can be written as:
preds := parsePredsFromMutation(gmu.Set) preds = append(preds, parsePredsFromMutation(gmu.Del)...)
Done.
ee/acl/acl_test.go, line 524 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
use
require.Error
instead.Also, it might be useful to check the error message is what you expect. It's possible that it fails because of an unrelated error. You could use
require.Contains(t, err.Error(), "whatever error message needs to be checked")
I think we should first check with require.Error
and then do require.Contains
. If err
turns out to be nil
, err.Error()
will cause segfault.
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.
A bit surprised that Del wasn't already handled. So, check around a bit more. And add a comment as well.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @animesh2049 and @martinmr)
edgraph/access_ee.go, line 602 at r2 (raw file):
if userId == x.GrootId { // groot is allowed to mutate anything except the permission of the acl predicates if isAclPredMutation(gmu.Set) {
Also check gmu.Del.
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.
I think this was a bug since the beginning, from f115de2eb6a40d882a86c64da68bf5c2a33ef69a
commit.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @martinmr)
edgraph/access_ee.go, line 602 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Also check gmu.Del.
Done.
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: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @martinmr)
Add predicates of delete mutation in
authorizeMutation
Fixes #4265
This change is