Add query integrity check in AccessControlManager

[mayankgarg1990]

Resubmitting #13632

== RELEASE NOTES ==

SPI Changes
* Add API to check if the query is unexpectedly modified using the credentials passed in the identity

[highker]

LGTM given it has already been reviewed #13632. Make sure the tests pass.

[ajaygeorge]

LGTM . Minor comment

[ajaygeorge]

I thought requireNonNull was only for checking constructor args.
See #13705 (comment)

[mayankgarg1990]

While I am not completely sure about the context behind the comment that you linked to, I feel that it is reasonable to validate that the input that the class is getting from an external agent. So I can generalize it to when something out of the class creates an instance by calling the constructor or to validate arguments passed to a public method of a class.

[ajaygeorge]

@mayankgarg1990 , definitely I see the need for null checking params in public methods.
However seems like in presto we usually do not null check method parameters using requireNonNull. We do that only for constructor arguments. cc @mbasmanova .

[mayankgarg1990]

@ajaygeorge - If you take a look AccessControlManager that has been doing this since 2015 - so there are examples already in the codebase. Also, it is better that we focus on what makes sense rather than how things are being done.

[mbasmanova]

@mayankgarg1990

• Add API to check the integrity of a query

Perhaps, clarify what "integrity" means here.

[mayankgarg1990]

@mayankgarg1990

• Add API to check the integrity of a query

Perhaps, clarify what "integrity" means here.

I feel that integrity generally refers to ensuring that the query is what was expected. The exact details are up for the implementation and this PR just adds the API to implement to perform the check. I am unable to think of how I can clarify what "integrity" means. I will be glad if you can share what is the missing context that you will like or if you have some suggestion.

[mbasmanova]

@mayankgarg1990 I'd like to come up with a one-liner for the release notes that would be meaningful to the reader. We can start with Check if the query is unexpectedly modified compared to token provided in Identity., but then I don't see "token" in "Identity" class. There is only user, principal, roles and extraCredentials. What is "token" here?

[mayankgarg1990]

@mbasmanova - I didn't pay attention to this comment. The token is an implementation detail and in general the idea is to use the identity object (where extra information can be passed as extraCredentials). The way Facebook would do it is to have the token passed as a part of extraCredentials field of Identity. But everyone is free to have their implementation of how they want to assess the integrity of the query.

I have changed the comment - Check if the query is unexpectedly modified using the credentials passed in the identity. Let me know if this makes more sense. Then I can change the Release note to - Add API to check if the query is unexpectedly modified using the credentials passed in the identity

[mbasmanova]

@mayankgarg1990 I like these proposals.

[mayankgarg1990]

Thank you for the feedback @mbasmanova . I had already updated the code, I have updated the PR description as well

[mayankgarg1990]

@highker , @mbasmanova - can one of you help with merging this PR?