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

fix(ACL): use acl for export, add GoG admin resolvers #7420

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

NamanJain8
Copy link
Contributor

@NamanJain8 NamanJain8 commented Feb 10, 2021

  • Fix the use of admin resolvers.
  • Fix the way we extract the namespace to export.

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.

Reviewed 6 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @NamanJain8, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/http.go, line 647 at r1 (raw file):

// TODO(Pawan): Are we passing the correct namespace over here?

yes, for admin server this needs to be done ATM.


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

ctx = x.AttachJWTNamespace(ctx)

why do we need to do this if we are anyways using ExtractJWTNamespace later? i.e., we aren't using the namespace directly present in the context metadata, we are only using the one from JWT, so why need to attach the namespace from JWT to metadata?


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

	// guardianOfTheGalaxyQueryMWs are the middlewares which should be applied to queries served by
	// admin server unless some exceptional behaviour is required

comment can be updated
same below for GoGMutationMWs


graphql/resolve/middlewares.go, line 136 at r1 (raw file):

// GuardianOfTheGalaxyAuthMW4Query blocks the resolution of resolverFunc if there is no Guardian
// auth present in context, otherwise it lets the resolverFunc resolve the query.
... of resolverFunc if the context doesn't have Guardian auth for Galaxy namespace, otherwise ...

same for comments on mutation MW snd the helper function resolveGuardianOfTheGalaxyAuth

Copy link
Contributor Author

@NamanJain8 NamanJain8 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: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/http.go, line 647 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// TODO(Pawan): Are we passing the correct namespace over here?

yes, for admin server this needs to be done ATM.

Okay.


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
ctx = x.AttachJWTNamespace(ctx)

why do we need to do this if we are anyways using ExtractJWTNamespace later? i.e., we aren't using the namespace directly present in the context metadata, we are only using the one from JWT, so why need to attach the namespace from JWT to metadata?

Thanks. We don't need it here.


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
	// guardianOfTheGalaxyQueryMWs are the middlewares which should be applied to queries served by
	// admin server unless some exceptional behaviour is required

comment can be updated
same below for GoGMutationMWs

Done.


graphql/resolve/middlewares.go, line 136 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// GuardianOfTheGalaxyAuthMW4Query blocks the resolution of resolverFunc if there is no Guardian
// auth present in context, otherwise it lets the resolverFunc resolve the query.
... of resolverFunc if the context doesn't have Guardian auth for Galaxy namespace, otherwise ...

same for comments on mutation MW snd the helper function resolveGuardianOfTheGalaxyAuth

Done.

@NamanJain8 NamanJain8 merged commit cdd74b9 into ibrahim/multitenancy Feb 11, 2021
@NamanJain8 NamanJain8 deleted the naman/multitenancy/export-acl branch February 11, 2021 09:23
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.

2 participants