[BREAKING] ACL schema change #4725

animesh2049 · 2020-02-04T10:48:32Z

This change is

pawanrawal

Reviewed 1 of 11 files at r1.
Reviewable status: 1 of 11 files reviewed, 12 unresolved discussions (waiting on @animesh2049)

dgraph/cmd/bulk/systest/test-bulk-schema.sh, line 195 at r1 (raw file):

  LC_ALL=C sort all_dbs.out | uniq -c
  diff <(LC_ALL=C sort all_dbs.out | uniq -c) - <<EOF
      1 dgraph.acl.permission

Should these be dgraph.rule.permission and dgraph.rule.predicate?

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

  allAcls(func: type(Group)) {
    dgraph.xid
	dgraph.group.acl

Why do we still query this?

ee/acl/acl.go, line 420 at r1 (raw file):

	defer cancel()

	ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Minute)

Wow, why is this 10 Minute?

ee/acl/acl.go, line 429 at r1 (raw file):

	}()

	ruleQuery := fmt.Sprintf(`

Also have a test for when group doesn't exist, then that should be an error. You'll have to fetch the guid in another query block to check that.

ee/acl/acl.go, line 437 at r1 (raw file):

	}`, groupId, predicate)

	ruleUpsert := &api.Mutation{

Add comments about the 4 cases possible here.

ee/acl/acl.go, line 437 at r1 (raw file):

	}`, groupId, predicate)

	ruleUpsert := &api.Mutation{

updateRule

ee/acl/acl.go, line 445 at r1 (raw file):

			},
		},
		Cond: "@if(gt(len(rUID), 0) AND eq(len(gUID), 1))",

This condition should be eq(len(rUID), 1)

ee/acl/acl.go, line 448 at r1 (raw file):

	}

	ruleMutation := &api.Mutation{

createRule

ee/acl/acl.go, line 453 at r1 (raw file):

		}
	}
	newAcls, updated := updateAcl(currentAcls, newAcl)

TestUpdateAcl can be removed from groups_test.go and please ensure we have integration tests for all the scenarios tested for it.

ee/acl/acl.go, line 466 at r1 (raw file):

			},
		},
		Cond: "@if(eq(len(rUID), 0) AND eq(len(gUID), 1))",

Also, according to updateAcl if perm is <0 then the rule is deleted, please ensure we do that here as well and have a test for that.

ee/acl/acl.go, line 475 at r1 (raw file):

	})

	if err != nil {

return err

schema/schema.go, line 514 at r1 (raw file):

				ValueType: pb.Posting_STRING,
				Directive: pb.SchemaUpdate_INDEX,
				Tokenizer: []string{"exact"}, // Should we have upsert directive for this ?

Should have upsert directive here, yes.

animesh2049

Reviewable status: 0 of 11 files reviewed, 12 unresolved discussions (waiting on @animesh2049 and @pawanrawal)

dgraph/cmd/bulk/systest/test-bulk-schema.sh, line 195 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should these be dgraph.rule.permission and dgraph.rule.predicate?

Done.

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

Previously, pawanrawal (Pawan Rawal) wrote…

Why do we still query this?

Done.

ee/acl/acl.go, line 420 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Wow, why is this 10 Minute?

Done.

ee/acl/acl.go, line 437 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

updateRule

Done.

ee/acl/acl.go, line 445 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This condition should be eq(len(rUID), 1)

Done.

ee/acl/acl.go, line 448 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

createRule

Done.

schema/schema.go, line 514 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should have upsert directive here, yes.

I think upsert directive should be there for dgraph.acl.rule ... ??

golangcibot · 2020-02-06T14:40:24Z

x/x.go

-{"predicate":"dgraph.group.acl","type":"string"}
+{"predicate":"dgraph.user.group","list":true, "reverse":true, "type":"uid"},
+{"predicate":"dgraph.acl.rule","type":"uid","list":true},
+{"predicate":"dgraph.rule.predicate","type":"string","index":true,"tokenizer":["exact"],"upsert":true},


line is 103 characters (from lll)

pawanrawal

Mostly LGTM now apart from failing CI tests.

Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @animesh2049 and @pawanrawal)

ee/acl/acl.go, line 437 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add comments about the 4 cases possible here.

This still needs to be done.

ee/acl/acl.go, line 489 at r2 (raw file):

	}
	resp, err := txn.Do(ctx, upsertRequest)
	fmt.Println(string(resp.GetJson()))

Should be removed

schema/schema.go, line 514 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

I think upsert directive should be there for dgraph.acl.rule ... ??

Upsert directive makes sense on scalar predicates where we compare value and then add it. If you try and add a rule for the same predicate twice for the same dgraph.acl.rule concurrently, one of them would fail.

animesh2049

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

ee/acl/acl.go, line 453 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

TestUpdateAcl can be removed from groups_test.go and please ensure we have integration tests for all the scenarios tested for it.

Done.

ee/acl/acl.go, line 475 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

return err

Done.

schema/schema.go, line 514 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Upsert directive makes sense on scalar predicates where we compare value and then add it. If you try and add a rule for the same predicate twice for the same dgraph.acl.rule concurrently, one of them would fail.

Putting upsert directive on dgraph.rule.predicate will ensure that we can't insert 2 rules concurrently (even to different groups) with the same predicate.

animesh2049

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

ee/acl/acl.go, line 489 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should be removed

Done.

animesh2049

Dismissed @golangcibot from a discussion.
Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @manishrjain and @pawanrawal)

pawanrawal

Reviewed 2 of 11 files at r1, 8 of 10 files at r2, 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)

manishrjain

Reviewed 2 of 11 files at r1, 8 of 10 files at r2, 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

animesh2049 added 5 commits January 31, 2020 17:14

Add new acl predicates

1369ca8

Add test and fix typos

47a6ffe

Use new ACL schema in acl tool

37519c9

Fix tests

734439b

Minor typo fixes

44c40e9

animesh2049 changed the title ~~ACL schema change~~ [BREAKING] ACL schema change Feb 4, 2020

animesh2049 added 2 commits February 4, 2020 17:46

Fix test

9ab121c

Remove debugging statements

5d0e0a6

pawanrawal reviewed Feb 6, 2020

View reviewed changes

Change acl predicate names

37d81a1

animesh2049 commented Feb 6, 2020

View reviewed changes

animesh2049 added 3 commits February 6, 2020 18:59

Delete rule if permission is negative

4d0a39e

Add test for rule deletion

dfd71b1

Delete updateACL function and its tests

aa82224

golangcibot reviewed Feb 6, 2020

View reviewed changes

pawanrawal reviewed Feb 6, 2020

View reviewed changes

animesh2049 added 2 commits February 6, 2020 21:22

Temp

e85e32a

Fix failing tests

6aa8889

animesh2049 marked this pull request as ready for review February 7, 2020 09:20

animesh2049 requested review from manishrjain and a team as code owners February 7, 2020 09:20

animesh2049 commented Feb 7, 2020

View reviewed changes

Return err if setting rule for non existent group

78cfa80

animesh2049 commented Feb 7, 2020

View reviewed changes

Write comments for chMod

3da2f97

pawanrawal approved these changes Feb 7, 2020

View reviewed changes

Parallelize table driven tests

4e4cd09

manishrjain approved these changes Feb 7, 2020

View reviewed changes

animesh2049 merged commit b40aa98 into master Feb 7, 2020

animesh2049 deleted the animesh2049/acl-schema-change branch February 7, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING] ACL schema change #4725

[BREAKING] ACL schema change #4725

animesh2049 commented Feb 4, 2020 •

edited by manishrjain

Loading

pawanrawal left a comment

animesh2049 left a comment

golangcibot Feb 6, 2020

pawanrawal left a comment

animesh2049 left a comment

animesh2049 left a comment

animesh2049 left a comment

pawanrawal left a comment

manishrjain left a comment

[BREAKING] ACL schema change #4725

[BREAKING] ACL schema change #4725

Conversation

animesh2049 commented Feb 4, 2020 • edited by manishrjain Loading

pawanrawal left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

golangcibot Feb 6, 2020

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

animesh2049 commented Feb 4, 2020 •

edited by manishrjain

Loading