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(GraphQL): Remove auth error from mutation. #6329

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented Aug 31, 2020

Fixes GRAPHQL-595
Discuss Issue

  • Auth errors are only returned in debug mode which is turned off by default.

This change is Reviewable

@arijitAD arijitAD requested a review from pawanrawal August 31, 2020 12:02
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Aug 31, 2020
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

There should be a way to set this flag through an API ideally because otherwise Slash users can't make use of it.

Reviewed 20 of 20 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arijitAD, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 203 at r1 (raw file):

	flag.Bool("graphql_introspection", true, "Set to false for no GraphQL schema introspection")
	flag.Bool("graphql_debug", false, "Enable debug_off mode in GraphQL")

Enable debug mode in GraphQL. This returns auth errors to clients. We do not reccomend turning it on for production.


graphql/e2e/auth/add_mutation_test.go, line 29 at r1 (raw file):

)

func (us *UserSecret) delete(t *testing.T, user, role string) {

Any tests which show the behavior when graphql_debug is false?


graphql/e2e/auth/add_mutation_test.go, line 192 at r1 (raw file):

		if tcase.result == "" {
			require.Equal(t, len(gqlResponse.Errors), 1)
			require.Contains(t, gqlResponse.Errors[0].Message, "authorization failed")

So we still get this error if graphql_debug is set to true?


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

	// GraphqlExtension will be set to see extensions in graphql results
	GraphqlExtension bool
	// GraphqlDebug will enable debug_off mode in GraphQL

whats debug_off mode? Shouldn't it be debug mode?

Copy link
Author

@arijitAD arijitAD 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: 18 of 20 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 203 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Enable debug mode in GraphQL. This returns auth errors to clients. We do not reccomend turning it on for production.

Done.


graphql/e2e/auth/add_mutation_test.go, line 29 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Any tests which show the behavior when graphql_debug is false?

I have added E2E test here with debug mode off.
graphql/e2e/auth/debug_off/debugoff_test.go


graphql/e2e/auth/add_mutation_test.go, line 192 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

So we still get this error if graphql_debug is set to true?

Yes. Ifgraphql_debugis set to true it will throw auth errors.


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

Previously, pawanrawal (Pawan Rawal) wrote…

whats debug_off mode? Shouldn't it be debug mode?

Yes. Changed it. debug was changed to debug_off when I was refactoring code in IDE.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

Copy link
Author

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

There should be a way to set this flag through an API ideally because otherwise Slash users can't make use of it.
I will create a JIRA ticket so that we would be able to change flags using API.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

@arijitAD arijitAD merged commit 991f72f into master Sep 2, 2020
@arijitAD arijitAD deleted the arijitAD/add-mutation-auth-err branch September 2, 2020 12:39
arijitAD pushed a commit that referenced this pull request Sep 21, 2020
* Remove auth error from add mutation.

(cherry picked from commit 991f72f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants