Add guardians group with full authorization

[animesh2049]

Added a new group called guardians. This group will have access to all operations except mutation of acl predicates.
Earlier we had only one super user groot. Now all the users which are in guardians group will have complete access.

This change is 

[martinmr]

Just a minor comment. This PR also needs a more descriptive commit message before it's merged into master.

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @animesh2049 and @manishrjain)

---

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

                                guid as var(func: eq(dgraph.xid, "%s"))
			}

minor: fix indenting for readability

[animesh2049]

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @manishrjain and @martinmr)

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

                                guid as var(func: eq(dgraph.xid, "%s"))
			}

minor: fix indenting for readability

Done

[manishrjain]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @animesh2049)

---

x/x.go, line 105 at r2 (raw file):

	GrootId = "groot"
	// adminGId is the ID of the admin group for ACLs.
	AdminGId = "guardians"

GuardiansId

[mangalaman93]

Superuser word us confusing. As far as I understood, it points to the guardian group. I initially thought that Groot is also in some sense has a superset of permissions and hence, also a superuser.

There is also a use of various terminology admin, superuser, guardian for pointing to the same group. I think that is unnecessary, we should use a uniform terminology across the code so that it is easier to understand.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @animesh2049)

---

edgraph/access_ee.go, line 364 at r2 (raw file):

		query := fmt.Sprintf(`
			{
				guid as var(func: eq(dgraph.xid, "%s"))

Could use query variables here instead.

---

edgraph/access_ee.go, line 379 at r2 (raw file):

		}

		_, err := (&Server{}).doQuery(ctx, req, NoAuthorize)

can move inside the if

---

edgraph/access_ee.go, line 388 at r2 (raw file):

	}

	upsertGroot := func(ctx context.Context) error {

same here

---

edgraph/access_ee.go, line 413 at r2 (raw file):

		}

		_, err := (&Server{}).doQuery(ctx, req, NoAuthorize)

move inside

---

edgraph/access_ee.go, line 426 at r2 (raw file):

		defer cancel()
		if err := upsertGuardians(ctx); err != nil {
			glog.Infof("Unable to upsert the guardian group. Error: %v", err)

why are we retrying here? Please add a comment here
and this seems like an infinite loop.

---

edgraph/access_ee.go, line 636 at r2 (raw file):

					return errors.Errorf("the permission of ACL predicates can not be changed")
				} else if isAclPredMutation(gmu.Del) {
					// Even members of gurardian group can't delete ACL predicates

duplicate comment

---

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

	require.NoError(t, err)

	op := api.Operation{

could be in just one line

[animesh2049]

A superuser is any user which has all the permissions. Given that any member of guardians group can be called superuser. Groot is also a superuser

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mangalaman93 and @manishrjain)

---

edgraph/access_ee.go, line 364 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Could use query variables here instead.

Any specific reason for that? I thought this makes it simpler.

---

edgraph/access_ee.go, line 379 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

can move inside the if

Done.

---

edgraph/access_ee.go, line 388 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

same here

Do you mean I should use query variable?

---

edgraph/access_ee.go, line 413 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

move inside

Done.

---

edgraph/access_ee.go, line 426 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

why are we retrying here? Please add a comment here
and this seems like an infinite loop.

Yes this is an infinite loop. We can't start alpha unless we are done setting up default ACL rules.

---

edgraph/access_ee.go, line 636 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

duplicate comment

Done.

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

could be in just one line

Done.

---

x/x.go, line 105 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

GuardiansId

Done.

[mangalaman93]

Docs are still missing. More comments around test cases, the code looks good.

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

---

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Done.

Not done

---

ee/acl/acl_test.go, line 478 at r3 (raw file):

}

func TestGuardianAccess(t *testing.T) {

Does it make sense to add tests for removing a user from guardian group? What about tests for adding a normal existing user to guardian group and then removing the user from the group, will it have the original permission that we started with?

---

ee/acl/acl_test.go, line 480 at r3 (raw file):

func TestGuardianAccess(t *testing.T) {
	ctx, _ := context.WithTimeout(context.Background(), 100*time.Second)
	unAuthPred := "unauthorizedPredicate"

This doesn't seem very useful, could just be hardcoded with a shorter name everywhere. Avoids all the Sprintf statement.

---

ee/acl/run_ee.go, line 42 at r3 (raw file):

	flag := CmdAcl.Cmd.PersistentFlags()
	flag.StringP("alpha", "a", "127.0.0.1:9080", "Dgraph Alpha gRPC server address")
	flag.StringP(gName, "w", x.GrootId, "User performing this operation")

This could be Guardian username to authorize this operation

---

ee/acl/run_ee.go, line 43 at r3 (raw file):

	flag.StringP("alpha", "a", "127.0.0.1:9080", "Dgraph Alpha gRPC server address")
	flag.StringP(gName, "w", x.GrootId, "User performing this operation")
	flag.StringP(gPassword, "x", "", "Password to authorize this operation")

Guardian password

[animesh2049]

Will raise another PR for docs

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @mangalaman93, @manishrjain, and @martinmr)

---

ee/acl/acl_test.go, line 478 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Does it make sense to add tests for removing a user from guardian group? What about tests for adding a normal existing user to guardian group and then removing the user from the group, will it have the original permission that we started with?

Done.

---

ee/acl/acl_test.go, line 480 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This doesn't seem very useful, could just be hardcoded with a shorter name everywhere. Avoids all the Sprintf statement.

Done.

---

ee/acl/run_ee.go, line 42 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This could be Guardian username to authorize this operation

Done.

---

ee/acl/run_ee.go, line 43 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Guardian password

Done.