feat(ACL): Make ACL work with multitenancy

[ahsanbarkati]

Make ACL work with multi-tenancy.

With this change, a user will require to login to use multi-tenancy. On login the user
will get an accessJWT, the claims of the accessJWT will contain the namespace the
user is accessing. The guardians of the galaxy can create/delete namespaces.

---

This change is 

[manishrjain]

Good to go from my side. There're a lot of TODOs here, so deal with those. Get final approval from @jarifibrahim .

Reviewed 30 of 30 files at r1.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @ahsanbarkati, @pawanrawal, and @vvbalaji-dgraph)

---

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

		userId := userData[0]
		user, err = authorizeUser(ctx, userId, "", request.Namespace)

shouldn't ctx have the namespace?

---

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

	// authorize the user using password
	var err error
	user, err = authorizeUser(ctx, request.Userid, request.Password, request.Namespace)

Just pass in request? See if that'd work.

---

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

// authorizeUser queries the user with the given user id, and returns the associated uid,
// acl groups, and whether the password stored in DB matches the supplied password
func authorizeUser(ctx context.Context, userid string, password string, namespace uint64) (

This func is not using namespace.

But, how do you auth, if you don't know which namespace to try? Also, should namespace be present in the context, or is that too early?

---

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

	closer.AddRunning(1)
	go worker.SubscribeForUpdates(aclPrefixes, "3-11", func(kvs *bpb.KVList) {

Put the ignorable byte range (3-11) it in keys.go, where you generate the keys.

---

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

	x.PredicatePrefix(x.NamespaceAttr(x.DefaultNamespace, "dgraph.acl.permission")),
	x.PredicatePrefix(x.NamespaceAttr(x.DefaultNamespace, "dgraph.acl.predicate")),
	x.PredicatePrefix(x.NamespaceAttr(x.DefaultNamespace, "dgraph.acl.rule")),

Rename DefaultNamespace to NamespaceGalaxy.

x.GalaxyAttr(...)

---

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

	}

	guardiansGroupUidUint, err := strconv.ParseUint(guardiansGroupUid, 0, 64)

uid, err := ...

For such a local variable, there's no point in having long names -- particularly which are confusing.

---

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

	guardiansGroupUidUint, err := strconv.ParseUint(guardiansGroupUid, 0, 64)
	if err != nil {
		return errors.Wrapf(err, "Error while parsing Uid: %s of guardians Group", guardiansGroupUid)

100 chars.

---

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

		return errors.Wrapf(err, "Error while parsing Uid: %s of guardians Group", guardiansGroupUid)
	}
	x.GuardiansGroupUid.Store(x.ExtractNamespace(ctx), guardiansGroupUidUint)

x.GuardiansUid

---

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

		return errors.Wrapf(err, "Error while parsing Uid: %s of groot user", grootUserUid)
	}
	x.GrootUserUid.Store(x.ExtractNamespace(ctx), grootUserUidUint)

same here.

---

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

}

func authorizePreds(ns uint64, userId string, groupIds, preds []string,

can't have 5 arguments. Even 4 is stretching it a bit.

---

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

	// as a byproduct, it also sets the userId, groups variables
	doAuthorizeAlter := func() error {
		userData, err := extractUserAndGroups(ctx)

create a struct from ctx? And pass that into authorizePreds.

---

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

}

func AuthorizeGalaxyGuardians(ctx context.Context) error {

AuthGuardiansOfTheGalaxy

---

edgraph/server.go, line 117 at r1 (raw file):

	ctx = x.AttachNamespace(ctx, namespace)
	if err := upsertGuardian(ctx); err != nil {
		return err

errors.Wrapf(...)

---

edgraph/server.go, line 120 at r1 (raw file):

	}
	if err := upsertGroot(ctx); err != nil {
		return err

errors.Wrapf

---

edgraph/server.go, line 146 at r1 (raw file):

	// TODO(Ahsan): Remove this sleep, use a retry loop instead.
	time.Sleep(2 * time.Second)

we shouldn't need to do this if we were not doing indexing in background. There is a run_in_background option in api.Operation -- so see how that is used here.

Just run worker.WaitForIndexing (rename it from WaitForIndexingOrCtxErr).

---

graphql/admin/schema.go, line 71 at r1 (raw file):

	if x.WorkerConfig.AclEnabled == false {

what's happening here? add a TODO or something.

---

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

	guardianGroupUid, ok := x.GuardiansGroupUid.Load(namespace)
	if !ok {
		guardianGroupUid = 0

if you can't find the namespace, then return error.

---

systest/multi-tenancy/basic_test.go, line 1 at r1 (raw file):

package main

license.

---

t/t.go, line 440 at r1 (raw file):

	// slowPkgs := []string{"systest", "ee/acl", "cmd/alpha", "worker", "e2e"}
	slowPkgs := []string{"systest", "cmd/alpha", "worker", "ee/acl"} //, "e2e"}

Does this need to be reverted?

---

worker/groups.go, line 1125 at r1 (raw file):

		// Get Subscriber stream.
		stream, err := client.Subscribe(closer.Ctx(), &pb.SubscriptionRequest{Prefixes: prefixes,

move pb.Subs.. to the next line. Then ignore is part of the same line as the request.

---

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

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

does this need to be a pointer?

---

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

	}

	ns := strconv.FormatUint(namespace, 10)

If user would be shown a hex, then keep it to hex. So, our debugging becomes easier.

[jarifibrahim]

Got some comments.

Reviewed 6 of 44 files at r2.
Reviewable status: 10 of 48 files reviewed, 32 unresolved discussions (waiting on @ahsanbarkati, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

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

Previously, manishrjain (Manish R Jain) wrote…

AuthGuardiansOfTheGalaxy

Guardian

---

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

		if user == nil {
			return nil, errors.Errorf("unable to authenticate: invalid credentials, no user found")

Please undo this change. The error message about login should always be generic. Don't return a specific error from login.

---

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

	if user == nil {
		return nil, errors.Errorf("unable to authenticate: invalid credentials, no user found")

Same. Don't put specific errors.

---

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

	}

	namespace, ok := claims["namespace"].(float64)

why float64? The namespace is saved as uint64 in the claims, isn't it?

---

edgraph/access_ee.go, line 576 at r4 (raw file):

		return nil, err
	}
	userInfo, err := validateToken(accessJwt[0])

return ...

---

edgraph/access_ee.go, line 1011 at r4 (raw file):

		}
		ns := x.ExtractNamespace(ctx)
		blockedPreds, _ := authorizePreds(ns, userData, preds, acl.Read)

You can pass the context and call x.ExtrctNamespace inside authorizePreds.

---

edgraph/access_ee.go, line 1049 at r4 (raw file):

	ns := x.ExtractNamespace(ctx)
	if ns != 0 {
		return errors.New("Only guardians of galaxy is allowed to do this operation")

only a guardian of the galaxy ...

---

edgraph/server.go, line 122 at r5 (raw file):

	num := &pb.Num{Val: 1, Type: pb.Num_NS_ID}
	ids, err := worker.AssignNsIdsOverNetwork(ctx, num)

check error

---

edgraph/server.go, line 132 at r5 (raw file):

	m.Types = schema.InitialTypes(ns)
	_, err = query.ApplyMutations(ctx, m)
	if err != nil {

if _, err := ... ; err !- nil ...

---

edgraph/server.go, line 137 at r5 (raw file):

	if err = worker.WaitForIndexing(ctx, true); err != nil {
		return 0, errors.Wrap(err, "While creating namespace got error")

fix this error message.

---

systest/multi-tenancy/basic_test.go, line 2 at r5 (raw file):

/*
 * Copyright 2017-2021 Dgraph Labs, Inc. and Contributors

2021. Use DCL