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.

Sign up for GitHub

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

Jump to bottom

Add query integrity check in AccessControlManager #13632

Closed
wants to merge 1 commit into from
Closed

Add query integrity check in AccessControlManager #13632

wants to merge 1 commit into from

Conversation

hellium01
Copy link
Contributor

@hellium01 hellium01 commented Oct 31, 2019 •
edited
Loading

This PR adds support to validate query integrity before we execute it. Client will provide certain token in Identity (most likely in extraCredentials).

== RELEASE NOTES ==

General Changes
*  Added support in `AccessControlManager` to check query integrity.

@hellium01 hellium01 force-pushed the AddQueryIntegratyCheck branch 2 times, most recently from f065d91 to 2c229ab Compare October 31, 2019 18:29
highker
highker reviewed Oct 31, 2019
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

some comments

@hellium01 hellium01 force-pushed the AddQueryIntegratyCheck branch 2 times, most recently from 5ad1dcd to afdc6fa Compare October 31, 2019 22:28
@hellium01 hellium01 force-pushed the AddQueryIntegratyCheck branch from afdc6fa to 93d35e3 Compare November 14, 2019 23:23
@mayankgarg1990
Copy link
Contributor

The change overall looks good to me. My only concern is with the naming. checkQueryIntegrity does not seem to belong to AccessControl class, since it is not really gating the access, rather ensuring the integrity of the query.

Lets get some other opinion on this - @highker , @rschlussel - what are your thoughts on this?

@hellium01
Copy link
Contributor Author

Right, checkQueryIntegrity is technically not part of authorization or AccessControl ( similar to checkCanSetUser) but moving it out to a separate class seems like an overkill.

@hellium01 hellium01 force-pushed the AddQueryIntegratyCheck branch from 93d35e3 to 47a1daf Compare November 18, 2019 23:22
@hellium01 hellium01 requested a review from rschlussel November 18, 2019 23:31
@hellium01 hellium01 force-pushed the AddQueryIntegratyCheck branch from 47a1daf to ce459ec Compare November 26, 2019 06:30
@rschlussel
Copy link
Contributor

yeah, it's not a perfect fit, but I think it's okay. We are basically saying we won't run your query because you don't have a valid token that we're expecting, so it's a form of an access check.

@mayankgarg1990
Copy link
Contributor

@highker - can you accept or request changes. I will take over this PR and get it landed

@mayankgarg1990 mayankgarg1990 mentioned this pull request Dec 18, 2019
Merged
@mayankgarg1990
Copy link
Contributor

Rebased and opened #13878 so that I can work with the committers to land this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@highker highker highker left review comments

@rschlussel rschlussel rschlussel approved these changes

@mayankgarg1990 mayankgarg1990 Awaiting requested review from mayankgarg1990

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hellium01 @mayankgarg1990 @rschlussel @highker @facebook-github-bot