Fix(ACL) : Disallow deleting of groot user and guardians group #6580

vmrajas · 2020-09-28T12:11:24Z

Motivation
Currently, it is possible to delete guardians user group and groot user using a simple graphql query.
This behaviour causes bad user experience as guardians is the admin group and groot user is the primary admin user.
Once, the group has been deleted, to get back access to groot user, the user has to restart dgraph alpha again.
This PR adds fixes this. This PR makes it impossible to delete groot user and guardians group.
Related discuss issue: https://discuss.dgraph.io/t/customers-can-delete-guardians-group-and-lose-access/9614

Components affected by this PR:
Dgraph ACL

Does this PR break backwards compatibility?:
No

Fixes DGRAPH-2454

This change is

Docs Preview:

animesh2049

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

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

}

type groupNode struct {

They are being used only once. I don't think we should create global type for them. Maybe just define them inside the function.

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

}

func TestDeleteGuardiansGroupShouldFail(t *testing.T) {

Please add tests for mutations with *

query/mutation.go, line 51 at r1 (raw file):

	for _, edge := range m.Edges {
		// Disallow deleting of guardians group
		if edge.Entity == guardianGroupUid && edge.Attr == "dgraph.xid" && edge.Op == pb.DirectedEdge_DEL {

I think we might miss deletion with * operator.

<guardians_id> * * .

The above query would still delete the guardians group.
According to me, better approach will be to allow only password change operation on groot and no change for guardians. Block any other operation involving them.

x/x.go, line 175 at r1 (raw file):

	AcceptedOrigins = atomic.Value{}
	// GuardiansGroupUid is Uid of guardians group node.
	GuardiansGroupUid = atomic.Value{}

Any specific reason for using atomic.Value ? Can't GuardiansGroupUid and GrootUserUid simply be 2 uint64 and we can use atomic.LoadUint64 and atomic.StoreUint64 on them?

animesh2049

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

query/mutation.go, line 51 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

I think we might miss deletion with * operator.
<guardians_id> * * .
The above query would still delete the guardians group.
According to me, better approach will be to allow only password change operation on groot and no change for guardians. Block any other operation involving them.

I forgot the fact that start will be expanded before coming here. But I feel later part is still valid.

vmrajas

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

They are being used only once. I don't think we should create global type for them. Maybe just define them inside the function.

Done.

query/mutation.go, line 51 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

I forgot the fact that start will be expanded before coming here. But I feel later part is still valid.

Done.

x/x.go, line 175 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Any specific reason for using atomic.Value ? Can't GuardiansGroupUid and GrootUserUid simply be 2 uint64 and we can use atomic.LoadUint64 and atomic.StoreUint64 on them?

Done.

animesh2049

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

query/mutation.go, line 259 at r2 (raw file):

		// Disallow deleting of guardians group
		if edge.Entity == guardianGroupUid && edge.Op == pb.DirectedEdge_DEL {
			glog.Info("Trying to delete guardians group. Operation not allowed.")

Minor: trying to delete guardians group might not be appropriate every time. Sometimes it might just be trying to delete some edge of guardian. Same for groot and the error message being returned.

pawanrawal

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Please add tests for mutations with *

Might be hard to test this automatically as we won't have the uid for groot or guardians. Though worth testing manually.

query/mutation.go, line 245 at r2 (raw file):

func checkIfDeletingAclOperation(edges []*pb.DirectedEdge) error {

Can we verify that drop_data and drop_all should still work?

Currently after drop all/drop data the groot and guardian group are re-added. Are there uids refreshed currently or only after a restart?

query/mutation.go, line 265 at r2 (raw file):

		// Disallow deleting of groot user
		if edge.Entity == grootUserUid && edge.Op == pb.DirectedEdge_DEL {
			glog.Info("Trying to delete groot user. Operation not allowed.")

I don't think we need these info logs because we are returning an error here.

query/mutation.go, line 271 at r2 (raw file):

	}
	if isDeleteAclOperation {
		return errors.Errorf("guardians group and groot user cannot be deleted.")

Properties of guardians group and groot user...

vmrajas

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

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

Previously, pawanrawal (Pawan Rawal) wrote…

Might be hard to test this automatically as we won't have the uid for groot or guardians. Though worth testing manually.

Done.

query/mutation.go, line 245 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we verify that drop_data and drop_all should still work?

Currently after drop all/drop data the groot and guardian group are re-added. Are there uids refreshed currently or only after a restart?

Done.

query/mutation.go, line 265 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I don't think we need these info logs because we are returning an error here.

Done.

query/mutation.go, line 271 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Properties of guardians group and groot user...

Done.

pawanrawal

Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

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

		atomic.StoreUint64(&x.GuardiansGroupUid, guardiansGroupUidUint)

		glog.Infof("guardians group uid: %d", guardiansGroupUidUint)

Don't print this.

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

		}
		atomic.StoreUint64(&x.GrootUserUid, grootUserUidUint)
		glog.Infof("groot user uid: %d", grootUserUidUint)

We don't need to really print this id here.

vmrajas

Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @animesh2049, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

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

Previously, pawanrawal (Pawan Rawal) wrote…

Don't print this.

Done.

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

Previously, pawanrawal (Pawan Rawal) wrote…

We don't need to really print this id here.

Done.

codelingo

5 issues found. 11 rules errored during the review.

codelingo · 2020-10-16T20:23:17Z

edgraph/access_ee.go

@@ -381,7 +383,7 @@ var aclPrefixes = [][]byte{
 	x.PredicatePrefix("dgraph.xid"),
 }

-// ResetAcl clears the aclCachePtr and upserts the Groot account.
+// clears the aclCachePtr and upserts the Groot account.


Every exported function in a program should have a doc comment. The first sentence should be a summary that starts with the name (ResetAcl) being declared.
From effective go.

View Rule

codelingo · 2020-10-16T20:23:17Z

x/x.go

@@ -176,10 +176,16 @@ var (
 	Nilbyte []byte
 	// AcceptedOrigins is allowed list of origins to make request to the graphql endpoint.
 	AcceptedOrigins = atomic.Value{}
+	// GuardiansGroupUid is Uid of guardians group node.
+	GuardiansGroupUid uint64


Avoid global variables to improve readability and reduce complexity

View Rule

codelingo · 2020-10-16T20:23:17Z

x/x.go

+	// GuardiansGroupUid is Uid of guardians group node.
+	GuardiansGroupUid uint64
+	// GrootUser Uid is Uid of groot user node.
+	GrootUserUid uint64


Avoid global variables to improve readability and reduce complexity

View Rule

codelingo · 2020-10-16T20:23:17Z

ee/acl/acl_test.go

@@ -83,7 +83,7 @@ func checkUserCount(t *testing.T, resp []byte, expected int) {
 	require.Equal(t, expected, len(r.AddUser.User))
 }

-func deleteUser(t *testing.T, accessToken, username string, confirmDeletion bool) {
+func deleteUser(t *testing.T, accessToken, username string, confirmDeletion bool) *testutil.GraphQLResponse {


Boolean arguments can indicate low cohesion. Consider refactoring deleteUser by using a separate function for each case and helper functions for repeated code. This will make each function clearer and more modular, leading to easier maintainability.

View Rule

codelingo · 2020-10-16T20:23:18Z

ee/acl/acl_test.go

 }

-func deleteGroup(t *testing.T, accessToken, name string) {
+func deleteGroup(t *testing.T, accessToken, name string, confirmDeletion bool) *testutil.GraphQLResponse {


Boolean arguments can indicate low cohesion. Consider refactoring deleteGroup by using a separate function for each case and helper functions for repeated code. This will make each function clearer and more modular, leading to easier maintainability.

View Rule

codelingo

3 issues found. 16 rules errored during the review.

codelingo · 2020-10-16T20:23:35Z

edgraph/access_ee.go

@@ -381,7 +383,7 @@ var aclPrefixes = [][]byte{
 	x.PredicatePrefix("dgraph.xid"),
 }

-// ResetAcl clears the aclCachePtr and upserts the Groot account.
+// clears the aclCachePtr and upserts the Groot account.


Every exported function in a program should have a doc comment. The first sentence should be a summary that starts with the name (ResetAcl) being declared.
From effective go.

View Rule

codelingo · 2020-10-16T20:23:36Z

x/x.go

@@ -176,10 +176,16 @@ var (
 	Nilbyte []byte
 	// AcceptedOrigins is allowed list of origins to make request to the graphql endpoint.
 	AcceptedOrigins = atomic.Value{}
+	// GuardiansGroupUid is Uid of guardians group node.
+	GuardiansGroupUid uint64


Avoid global variables to improve readability and reduce complexity

View Rule

codelingo · 2020-10-16T20:23:36Z

x/x.go

+	// GuardiansGroupUid is Uid of guardians group node.
+	GuardiansGroupUid uint64
+	// GrootUser Uid is Uid of groot user node.
+	GrootUserUid uint64


Avoid global variables to improve readability and reduce complexity

View Rule

vmrajas requested review from pawanrawal, abhimanyusinghgaur and animesh2049 September 28, 2020 12:11

vmrajas requested review from manishrjain and vvbalaji-dgraph as code owners September 28, 2020 12:11

github-actions bot added the area/enterprise Related to proprietary features label Sep 28, 2020

animesh2049 suggested changes Sep 29, 2020

View reviewed changes

animesh2049 suggested changes Sep 30, 2020

View reviewed changes

vmrajas force-pushed the rajas/fix_guardians_deletion branch from b28f567 to 7724581 Compare October 12, 2020 11:16

vmrajas commented Oct 12, 2020

View reviewed changes

animesh2049 approved these changes Oct 12, 2020

View reviewed changes

pawanrawal suggested changes Oct 14, 2020

View reviewed changes

vmrajas commented Oct 14, 2020

View reviewed changes

vmrajas requested a review from pawanrawal October 14, 2020 11:53

pawanrawal approved these changes Oct 15, 2020

View reviewed changes

vmrajas added 6 commits October 16, 2020 17:42

Disallow deleting groot user and guardians group

6bed792

Cosmetic changes

ee896f7

Requested changes

c965bf5

Fix Test

f69654f

Changes to acl_test

84ba44e

Final changes

29f9ffe

vmrajas force-pushed the rajas/fix_guardians_deletion branch from 940a922 to 29f9ffe Compare October 16, 2020 12:14

vmrajas commented Oct 16, 2020

View reviewed changes

Fix deepsource and countIndex test

b288c47

parasssh approved these changes Oct 16, 2020

View reviewed changes

Try fixing failing test

7a75b15

vmrajas merged commit 645c0bb into master Oct 16, 2020

vmrajas deleted the rajas/fix_guardians_deletion branch October 16, 2020 16:36

codelingo bot reviewed Oct 16, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(ACL) : Disallow deleting of groot user and guardians group #6580

Fix(ACL) : Disallow deleting of groot user and guardians group #6580

vmrajas commented Sep 28, 2020 •

edited by dgraph-bot

Loading

animesh2049 left a comment

animesh2049 left a comment

vmrajas left a comment

animesh2049 left a comment

pawanrawal left a comment

vmrajas left a comment

pawanrawal left a comment

vmrajas left a comment

codelingo bot left a comment

codelingo bot Oct 16, 2020

codelingo bot Oct 16, 2020

codelingo bot Oct 16, 2020

codelingo bot Oct 16, 2020

codelingo bot Oct 16, 2020

codelingo bot left a comment

codelingo bot Oct 16, 2020

codelingo bot Oct 16, 2020

codelingo bot Oct 16, 2020

Fix(ACL) : Disallow deleting of groot user and guardians group #6580

Fix(ACL) : Disallow deleting of groot user and guardians group #6580

Conversation

vmrajas commented Sep 28, 2020 • edited by dgraph-bot Loading

animesh2049 left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

vmrajas 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

vmrajas left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

vmrajas left a comment

Choose a reason for hiding this comment

codelingo bot left a comment

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot left a comment

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

codelingo bot Oct 16, 2020

Choose a reason for hiding this comment

vmrajas commented Sep 28, 2020 •

edited by dgraph-bot

Loading