Support ACL operations using the admin GraphQL API

[pawanrawal]

This PR supports all the operations currently supported by the ACL tool using the admin API available at /admin. The current tests in acl_test.go have also been modified to use the new API. The dgraph acl tool would be removed as part of another PR.

Some more testing is required for the API to test the behavior when users who are not part of the guardian group use it. I'll do that as part of a separate change.

---

This change is 

Docs Preview:

[martinmr]

Changes in the test

Just a couple of comments regarding commented and removed code.

Reviewed 5 of 7 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)

---

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

	// updateRule := exec.Command("dgraph", "acl", "mod", "-a", dgraphEndpoint,
	// 	"-g", devGroup, "-p", "name", "-m", "-1", "-x", "password")
	// require.NoError(t, updateRule.Run())

should this be removed?

---

graphql/admin/admin.go, line 99 at r2 (raw file):

Quoted 8 lines of code…

	input BackupInput {
		destination: String!
		accessKey: String
		secretKey: String
		sessionToken: String
		anonymous: Boolean
		forceFull: Boolean
	}

why are all the backup changes removed in this diff?

[MichaelJCompton]

This is awesome! So little code (other than the tests) to convert all of this to GraphQL :-)

I suspect that the non groot case will make us think harder about the extra functions we can hook into query processing. But for now, this is cool.

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

---

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

	delUser := `mutation deleteUser($name: String!) {
		deleteUser(filter: {name: {eq: $name}}) {
			msg

we should remember when the more data from mutations PR goes in, that there's spots like this where we can use it to ensure that only one thing got deleted

---

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

	client := &http.Client{}
	resp, err := client.Do(req)
	require.NoError(t, err)

this looks like repeated boiler plate - probably worth making a function to make the test logic cleaner ?

---

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

	resetUser(t)
	// TODO - We should be adding this data using the GraphQL API.

yeah, a single mutation should do it

---

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

func TestNonExistentGroup(t *testing.T) {
	t.Skip()

keep it skiped for now. When the more data in mutations PR goes through, this should change to checking for 0 nodes updated

---

graphql/admin/endpoints_ee.go, line 37 at r2 (raw file):

	type User {
		name: String! @id @dgraph(pred: "dgraph.xid")
		# TODO - Update this to actual secret after password PR is merged here.

add a card to make sure this happens

---

graphql/admin/endpoints_ee.go, line 51 at r2 (raw file):

		id: ID!
		predicate: String! @dgraph(pred: "dgraph.rule.predicate")
		# TODO - Change permission to enum type once we figure out how to map enum strings to Int

what's going to happen here?

Should we leave this as an int? We can just validate the input in the mutation rewriting.

It's a bit unfriendly as an int, but closer to how it worked before.

On the other hand, because we don't use this to generate any schema, I suspect that it would work to have an enum as the type listed in the GraphQL schema - that would force input to be of the right type. But if the actual dgraph edge is an int, then we can just map the input enums to int in the rewriting before executing the mutation. I think that'll still work because we don't even check the actual Dgraph types during execution. In query, we can just reverse that mapping after actual query execution.

---

graphql/admin/endpoints_ee.go, line 71 at r2 (raw file):

	}

	input AddUserInput {

this set of user and group input types looks like it lets you do just the right things

---

graphql/admin/endpoints_ee.go, line 189 at r2 (raw file):

	getGroup(name: String!): Group

	# TODO - This needs a custom handler. Implement this later.

this looks like it should be easy - it should just forward to getUser and inject the user that we get from jwt info

[pawanrawal]

Reviewed 3 of 7 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @harshil-goel, @manishrjain, @martinmr, and @MichaelJCompton)

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

we should remember when the more data from mutations PR goes in, that there's spots like this where we can use it to ensure that only one thing got deleted

Added a TODO for now.

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

this looks like repeated boiler plate - probably worth making a function to make the test logic cleaner ?

good point, done

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

	// updateRule := exec.Command("dgraph", "acl", "mod", "-a", dgraphEndpoint,
	// 	"-g", devGroup, "-p", "name", "-m", "-1", "-x", "password")
	// require.NoError(t, updateRule.Run())

should this be removed?

Done.

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

yeah, a single mutation should do it

Will do this later, keeping as it is for now. Need the uid of the rule to delete it.

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

keep it skiped for now. When the more data in mutations PR goes through, this should change to checking for 0 nodes updated

Ok

---

graphql/admin/admin.go, line 99 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	input BackupInput {
		destination: String!
		accessKey: String
		secretKey: String
		sessionToken: String
		anonymous: Boolean
		forceFull: Boolean
	}

why are all the backup changes removed in this diff?

They have been moved to endpoints_ee.go so that they are only shown as part of the schema for enterprise build and not for the open source build.

---

graphql/admin/endpoints_ee.go, line 37 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

add a card to make sure this happens

Done. Assigned to Harshil.

---

graphql/admin/endpoints_ee.go, line 51 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

what's going to happen here?

Should we leave this as an int? We can just validate the input in the mutation rewriting.

It's a bit unfriendly as an int, but closer to how it worked before.

On the other hand, because we don't use this to generate any schema, I suspect that it would work to have an enum as the type listed in the GraphQL schema - that would force input to be of the right type. But if the actual dgraph edge is an int, then we can just map the input enums to int in the rewriting before executing the mutation. I think that'll still work because we don't even check the actual Dgraph types during execution. In query, we can just reverse that mapping after actual query execution.

Leaving it as an INT for now but I will add a card to maybe convert it to an enum later. In that case it would work as you said. It just didn't feel very good to hard code logic for a specific predicate i.e. dgraph.rule.permission. Also the validation for the int being between [0,7] should be added at the Dgraph server level as this data can also be added through the normal mutate API. That will be part of a separate PR.

https://dgraph.atlassian.net/browse/GRAPHQL-255

---

graphql/admin/endpoints_ee.go, line 71 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

this set of user and group input types looks like it lets you do just the right things

Yeah, works beautifully.

---

graphql/admin/endpoints_ee.go, line 189 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

this looks like it should be easy - it should just forward to getUser and inject the user that we get from jwt info

Leaving this as a TODO for this PR and also added a card for it. Will ask @harshil-goel to pick this up and have a separate PR for it.