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): fix interface query with auth rules. #7401

Merged
merged 6 commits into from
Feb 8, 2021

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Feb 5, 2021

Fixes GRAPHQL-1013.
This PR fixes a bug in query rewriting for interface query with auth rules. For more context please read here.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Feb 5, 2021
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.

The user on the discuss post says that they also say this bug for delete Mutations, can we verify that is fixed as well?

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @minhaj-shakeel)


graphql/resolve/auth_query_test.yaml, line 1952 at r1 (raw file):

  jwtvar:
    ROLE: "ADMIN"
    USER: "user"

What about the other Auth rule about the user, is that not applied here?


graphql/resolve/query_rewriter.go, line 790 at r1 (raw file):

			// then simply we need to Put uid(Todo1) with OR in the main query filter.
			// 2. If rbac evaluates to `Positive` which means RBAC rule is satisfied.
			// Either it is the only auth rule, or it is present with `OR`, which means

Can't it be present with AND as well? Is it necessary that it can only have OR?

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel left a comment

Choose a reason for hiding this comment

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

Yeah, essentially all the operations for the query in which `RBAC were evaluated to positive were giving incorrect results due to error in query rewriting.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pawanrawal)


graphql/resolve/auth_query_test.yaml, line 1952 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What about the other Auth rule about the user, is that not applied here?

Yeah, because RBACRB rules are evaluated to positive so it should return all the nodes.


graphql/resolve/query_rewriter.go, line 790 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can't it be present with AND as well? Is it necessary that it can only have OR?

No, if the RBAC rule is present with AND this means that if the auth rules passed, we still need to attach the auth queries to our DQL query.

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.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @minhaj-shakeel)

@minhaj-shakeel minhaj-shakeel merged commit 8b29efc into master Feb 8, 2021
@minhaj-shakeel minhaj-shakeel deleted the minhaj/fix-interface-auth branch February 8, 2021 10:49
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