Adding the acl subcommand to support acl features

[gitlw]

• dgraph acl useradd -u alice -p password
• dgraph acl userdel -u alice
• dgraph acl groupadd -g dev
• dgraph acl groupadd -g sre
• dgraph acl groupdel -g dev
• dgraph acl usermod -u alice -G dev,sre
• dgraph acl chmod --group dev --predicate friend --acl 4

---

This change is 

[srfrog]

Reviewed 1 of 18 files at r1.
Reviewable status: 1 of 18 files reviewed, 15 unresolved discussions (waiting on @golangcibot and @gitlw)

---

dgraph/cmd/alpha/run.go, line 404 at r3 (raw file):

	}

	secretFile := Alpha.Conf.GetString("hmac_secret_file")

I think the secret should come from env not a file. It's simple to write a script (using kms or vault) or configure docker/k8s to set that.

---

dgraph/cmd/alpha/run.go, line 432 at r3 (raw file):

	}

	x.LoadTLSConfig(&tlsConf, Alpha.Conf, tlsNodeCert, tlsNodeKey)

How about setting Alpha.Conf with these values instead of changing the func. x.LoadTLSConf just uses the values in tlsConf

---

dgraph/cmd/live/run.go, line 297 at r3 (raw file):

		authToken:           Live.Conf.GetString("auth_token"),
	}
	x.LoadTLSConfig(&tlsConf, Live.Conf, x.TlsClientCert, x.TlsClientKey)

I dont think you need this either, you can set the values in tlsConf

---

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

	out, err := cmd.CombinedOutput()
	if (!shouldFail && err != nil) || (shouldFail && err == nil) {
		t.Errorf("Error output from command:%v", string(out))

how about t.Fatalf instead ?

---

ee/acl/jwt.go, line 2 at r3 (raw file):

// +build !oss

I dont think we need to build JWT by hand, there are good pkgs that give us all the features and stay up to date with security.
Check: github.com/dgrijalva/jwt-go (very popular) or https://github.com/SermoDigital/jose (most complete but newer)

---

ee/acl/cmd/groups.go, line 109 at r3 (raw file):

func queryGroup(txn *dgo.Txn, ctx context.Context, groupid string,
	fields []string) (group *acl.Group, err error) {
	var queryBuilder bytes.Buffer

use strings.Builder for strings - https://golang.org/pkg/strings/#Builder
it's a safer version of bytes.Buffer

---

ee/acl/cmd/run.go, line 44 at r3 (raw file):

var UserMod x.SubCommand
var ChMod x.SubCommand

I dont think we need all these global subcommands, I'd suggest putting them in a slice and initializing them from init(). I dont think we want to export all of them and use global space twice (they will be referenced from the rootCmd).

---

ee/acl/cmd/users.go, line 185 at r3 (raw file):

	targetGroupsMap := make(map[string]struct{})
	var exists = struct{}{}

I dont think you need a var for this, just use the values directly

[manishrjain]

Got a few comments. Address those and feel free to submit after. If you can't address them, then add a TODO. Good work with the refresh token logic.

Reviewable status: 1 of 18 files reviewed, 29 unresolved discussions (waiting on @golangcibot, @srfrog, @gitlw, and @manishrjain)

---

dgraph/cmd/alpha/run.go, line 404 at r3 (raw file):

Previously, srfrog (Gus) wrote…

I think the secret should come from env not a file. It's simple to write a script (using kms or vault) or configure docker/k8s to set that.

Yeah, sounds like a good idea. Also, disallow if enterprise flag is not set.

Update: AFter further discussion, we decided to keep it as a file for now. Until we get further feedback.

---

edgraph/access_ee.go, line 33 at r9 (raw file):

func (s *Server) Login(ctx context.Context,
	request *api.LogInRequest) (*api.Response, error) {
	ctx, span := otrace.StartSpan(ctx, "server.LogIn")

Login, small i.

---

edgraph/access_ee.go, line 74 at r9 (raw file):

		// set the jwt exp according to the ttl
		"exp": json.Number(
			strconv.FormatInt(time.Now().Add(Config.JwtTtl).Unix(), 10)),

Use UTC.

---

edgraph/access_ee.go, line 139 at r10 (raw file):

	claims, ok := token.Claims.(jwt.MapClaims)
	if (!ok) || (!token.Valid) {

Don't need the brackets here.

---

edgraph/access_ee.go, line 145 at r10 (raw file):

	// by default, the MapClaims.Valid will return true if the exp field is not set
	// here we enforce the checking to make sure that the refresh token has not expired
	now := time.Now().Unix()

All of these should be in UTC.

---

edgraph/access_ee.go, line 146 at r10 (raw file):

	// here we enforce the checking to make sure that the refresh token has not expired
	now := time.Now().Unix()
	if claims.VerifyExpiresAt(now, true) == false {

if !claims.Verify...

---

ee/acl/cmd/groups.go, line 197 at r6 (raw file):

// returns whether the existing acls slice is changed
func updateAcl(acls []Acl, newAcl *Acl) ([]Acl, bool) {

Add tests for this.

---

x/x.go, line 448 at r10 (raw file):

func Diff(targetMap map[string]struct{}, existingMap map[string]struct{}) ([]string,
	[]string) {

Could have fit in 100 chars.

---

ee/acl/cmd/run_ee.go, line 210 at r10 (raw file):

	tlsConf.ServerName = CmdAcl.Conf.GetString("tls_server_name")

	ds := strings.Split(opt.dgraph, ",")

Can just take a single IP address. Most users might be using a load balancer in front if they need to divide up the load across multiple Alphas anyway.

---

ee/acl/cmd/run_ee.go, line 220 at r10 (raw file):

	}

	return dgo.NewDgraphClient(clients...)

Can also return a close func here to close connection, which the caller can defer close() on. context.WithTimeout works the same way.

---

ee/acl/cmd/run_ee.go, line 243 at r10 (raw file):

		}

		var userSB strings.Builder

userBuf, or just buf.

---

ee/acl/cmd/run.go, line 37 at r6 (raw file):

var CmdAcl x.SubCommand
var UserAdd x.SubCommand

Dont' declare these here.

---

ee/acl/cmd/run.go, line 138 at r6 (raw file):

		Short: "Run Dgraph acl tool to change a user's groups",
		Run: func(cmd *cobra.Command, args []string) {
			runTxn(UserMod.Conf, userMod)

userMod(cmd.Conf, ...) and calls getClient(..) internally.