Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(multi-tenancy): Add mutation for guardian of galaxy to reset password of users of any namespaces #7418

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

ahsanbarkati
Copy link
Contributor

@ahsanbarkati ahsanbarkati commented Feb 10, 2021

This PR adds a new mutation which can be used by the guardians of the galaxy
to reset passwords of any user belonging to any namespace.


This change is Reviewable

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 5 files reviewed, 8 unresolved discussions (waiting on @ahsanbarkati, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


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

ResetPassword

since this is an enterprise only feature, shouldn't we add it separately in access.go and access_ee.go?
same for CreateNamespace and DeleteNamespace too.


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

x as updateUser(func: type(dgraph.type.User)) @filter(eq(dgraph.xid, "%s"))

we should put eq as root func and type as filter, that is a bit performant query.


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

	ctx = x.AttachNamespace(ctx, ns)
	resp, err := (&Server{}).doQuery(ctx, req)

nice! so this is the way to bypass that namespace addition from JWT, directly use doQuery().
We have two options now :)


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

return errors.Wrapf(err, "while upserting user with id %s", x.GrootId)

should it be

errors.Wrapf(err, "while resetting password for user with id %s", id)

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

		"export":          resolveExport,
		"login":           resolveLogin,
		"resetPassword":   resolveResetPassword,

need to also add the middleware configuration for this, IP and logging middleware should be the required ones, for guardian auth, maybe we will need to create a new middleware, but that we can look later.


graphql/admin/endpoints_ee.go, line 492 at r1 (raw file):

resetPassword(userId: String, password: String, namespace: Int)

should each of these parameters be a required parameter?
also let's continue the convention of having only one parameter for the mutation which should be named input and that parameter should wrap these 3.

login is the only exception to this rule ATM because of legacy issues but we should follow this convention.
From this, now I think we should also change the deleteNamespace signature as well as per the convention.


graphql/admin/reset_password.go, line 37 at r1 (raw file):

glog.Info("Got reset password request")

lets remove this because we will anyways add the logging middleware


graphql/admin/reset_password.go, line 40 at r1 (raw file):

err := (&edgraph.Server{}).ResetPassword(ctx, in.Namespace, in.UserID, in.Password)

should ResetPassword directly accept the ResetPasswordInput struct instead of passing the parameters one by one?

Copy link
Contributor Author

@ahsanbarkati ahsanbarkati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
ResetPassword

since this is an enterprise only feature, shouldn't we add it separately in access.go and access_ee.go?
same for CreateNamespace and DeleteNamespace too.

I think access.go is primarily for ACLs, so I have created a separate file.


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
x as updateUser(func: type(dgraph.type.User)) @filter(eq(dgraph.xid, "%s"))

we should put eq as root func and type as filter, that is a bit performant query.

Nice. Thanks.


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
	ctx = x.AttachNamespace(ctx, ns)
	resp, err := (&Server{}).doQuery(ctx, req)

nice! so this is the way to bypass that namespace addition from JWT, directly use doQuery().
We have two options now :)

Yeah!


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
return errors.Wrapf(err, "while upserting user with id %s", x.GrootId)

should it be

errors.Wrapf(err, "while resetting password for user with id %s", id)

Done


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

need to also add the middleware configuration for this, IP and logging middleware should be the required ones, for guardian auth, maybe we will need to create a new middleware, but that we can look later.

It is already done in a PR by Naman.


graphql/admin/endpoints_ee.go, line 492 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
resetPassword(userId: String, password: String, namespace: Int)

should each of these parameters be a required parameter?
also let's continue the convention of having only one parameter for the mutation which should be named input and that parameter should wrap these 3.

login is the only exception to this rule ATM because of legacy issues but we should follow this convention.
From this, now I think we should also change the deleteNamespace signature as well as per the convention.

Done


graphql/admin/reset_password.go, line 37 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
glog.Info("Got reset password request")

lets remove this because we will anyways add the logging middleware

Done

Copy link
Contributor Author

@ahsanbarkati ahsanbarkati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


graphql/admin/reset_password.go, line 40 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
err := (&edgraph.Server{}).ResetPassword(ctx, in.Namespace, in.UserID, in.Password)

should ResetPassword directly accept the ResetPasswordInput struct instead of passing the parameters one by one?

Done

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

just a minor comment

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @ahsanbarkati, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


edgraph/multi_tenancy_ee.go, line 70 at r2 (raw file):

errors.Wrapf(err, "Reset password for user %s got error", inp.Namespace)

errors.Wrapf(err, "Reset password for user %s in namespace %d, got error:", inp.UserID, inp.Namespace)

@ahsanbarkati ahsanbarkati merged commit 373462d into ibrahim/multitenancy Feb 11, 2021
@ahsanbarkati ahsanbarkati deleted the ahsan/change-passowrd branch February 11, 2021 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants