Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-12-17 Thread via GitHub 


soumitra-st commented on PR #14414:
URL: https://github.com/apache/pinot/pull/14414#issuecomment-2549465789

   @ilamhs , have you looked at 
[FineGrainedAccessControl.hasAccess](https://github.com/apache/pinot/blob/2719c5c9bc695d5257a1b5309c951e4a8b7dac9a/pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAccessControl.java#L39)
 method? This method has access to the HttpHeaders. What information you need 
to access that is not present in HttpHeaders that requires access to the 
org.glassfish.grizzly.http.server.Request? If FineGrainedAccessControl 
interface fits your need, then consider developing using that.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-22 Thread via GitHub 


shounakmk219 commented on code in PR #14414:
URL: https://github.com/apache/pinot/pull/14414#discussion_r1853639186


##
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java:
##
@@ -43,24 +44,25 @@ private AccessControlUtils() {
   /**
* Validate permission for the given access type against the given table
*
-   * @param tableName name of the table to be accessed (post database name 
translation)
-   * @param accessType type of the access
-   * @param httpHeaders HTTP headers containing requester identity required by 
access control object
-   * @param endpointUrl the request url for which this access control is called
+   * @param tableName name of the table to be accessed (post database name 
translation)
+   * @param accessTypetype of the access
+   * @param httpHeaders   HTTP headers containing requester identity required 
by access control object
+   * @param request   the request for which this access controll is called
+   * @param endpointUrl   the request url for which this access control is 
called
* @param accessControl AccessControl object which does the actual validation
*/
   public static void validatePermission(@Nullable String tableName, AccessType 
accessType,
-  @Nullable HttpHeaders httpHeaders, String endpointUrl, AccessControl 
accessControl) {
+  @Nullable HttpHeaders httpHeaders, Request request, String endpointUrl, 
AccessControl accessControl) {

Review Comment:
   That's correct.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-21 Thread via GitHub 


ilamhs commented on code in PR #14414:
URL: https://github.com/apache/pinot/pull/14414#discussion_r1852725021


##
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java:
##
@@ -43,24 +44,25 @@ private AccessControlUtils() {
   /**
* Validate permission for the given access type against the given table
*
-   * @param tableName name of the table to be accessed (post database name 
translation)
-   * @param accessType type of the access
-   * @param httpHeaders HTTP headers containing requester identity required by 
access control object
-   * @param endpointUrl the request url for which this access control is called
+   * @param tableName name of the table to be accessed (post database name 
translation)
+   * @param accessTypetype of the access
+   * @param httpHeaders   HTTP headers containing requester identity required 
by access control object
+   * @param request   the request for which this access controll is called
+   * @param endpointUrl   the request url for which this access control is 
called
* @param accessControl AccessControl object which does the actual validation
*/
   public static void validatePermission(@Nullable String tableName, AccessType 
accessType,
-  @Nullable HttpHeaders httpHeaders, String endpointUrl, AccessControl 
accessControl) {
+  @Nullable HttpHeaders httpHeaders, Request request, String endpointUrl, 
AccessControl accessControl) {

Review Comment:
   I see, just for my understanding, is the concern that `validatePermission` 
function is used outside this codebase and a deprecation path will be better?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-21 Thread via GitHub 


shounakmk219 commented on code in PR #14414:
URL: https://github.com/apache/pinot/pull/14414#discussion_r1852140764


##
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##
@@ -101,10 +102,16 @@ public class PinotQueryResource {
   @Inject
   ControllerConf _controllerConf;
 
+  @Context

Review Comment:
   Just for my info, how is the injection handled across different requests on 
various endpoints?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-21 Thread via GitHub 


shounakmk219 commented on code in PR #14414:
URL: https://github.com/apache/pinot/pull/14414#discussion_r1852061362


##
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java:
##
@@ -43,24 +44,25 @@ private AccessControlUtils() {
   /**
* Validate permission for the given access type against the given table
*
-   * @param tableName name of the table to be accessed (post database name 
translation)
-   * @param accessType type of the access
-   * @param httpHeaders HTTP headers containing requester identity required by 
access control object
-   * @param endpointUrl the request url for which this access control is called
+   * @param tableName name of the table to be accessed (post database name 
translation)
+   * @param accessTypetype of the access
+   * @param httpHeaders   HTTP headers containing requester identity required 
by access control object
+   * @param request   the request for which this access controll is called
+   * @param endpointUrl   the request url for which this access control is 
called
* @param accessControl AccessControl object which does the actual validation
*/
   public static void validatePermission(@Nullable String tableName, AccessType 
accessType,
-  @Nullable HttpHeaders httpHeaders, String endpointUrl, AccessControl 
accessControl) {
+  @Nullable HttpHeaders httpHeaders, Request request, String endpointUrl, 
AccessControl accessControl) {

Review Comment:
   Can you create a new method with the `request` param and deprecate the 
existing one, instead of changing the existing method?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-12 Thread via GitHub 


ilamhs commented on PR #14414:
URL: https://github.com/apache/pinot/pull/14414#issuecomment-2471227616

   The failing Integration Tests seem to be unrelated to the code in the PR 
above.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-07 Thread via GitHub 


codecov-commenter commented on PR #14414:
URL: https://github.com/apache/pinot/pull/14414#issuecomment-2463358607

   ## 
[Codecov](https://app.codecov.io/gh/apache/pinot/pull/14414?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 0.00%. Comparing base 
[(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 to head 
[(`3839bac`)](https://app.codecov.io/gh/apache/pinot/commit/3839bac6ac0d0359dac903264a63df46c744487b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 1302 commits behind head on master.
   
   > :exclamation:  There is a different number of reports uploaded between 
BASE (59551e4) and HEAD (3839bac). Click for more details.
   > 
   > HEAD has 53 uploads less than BASE
   >
   >| Flag | BASE (59551e4) | HEAD (3839bac) |
   >|--|--|--|
   >|integration|7|1|
   >|integration2|3|1|
   >|temurin|12|1|
   >|java-21|7|1|
   >|skip-bytebuffers-true|3|1|
   >|skip-bytebuffers-false|7|0|
   >|unittests|5|0|
   >|unittests1|2|0|
   >|java-11|5|0|
   >|unittests2|3|0|
   >|integration1|2|0|
   >|custom-integration1|2|0|
   >
   
   Additional details and impacted files
   
   
   ```diff
   @@  Coverage Diff  @@
   ## master   #14414   +/-   ##
   =
   - Coverage 61.75%0.00%   -61.76% 
   =
 Files  24363 -2433 
 Lines1332336   -133227 
 Branches  206360-20636 
   =
   - Hits  822740-82274 
   + Misses449116-44905 
   + Partials   60480 -6048 
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[integration](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-0.01%)` | :arrow_down: |
   | 
[integration1](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[integration2](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (ø)` | |
   | 
[java-11](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[java-21](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-61.63%)` | :arrow_down: |
   | 
[skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-27.73%)` | :arrow_down: |
   | 
[temurin](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-61.76%)` | :arrow_down: |
   | 
[unittests](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unittests1](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unittests2](https://app.codecov.io/gh/apache/pinot/pull/14414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   
   Flags with carried for

Re: [PR] Controller AccessControl interface accepts Request parameter [pinot]

2024-11-07 Thread via GitHub 


ilamhs commented on PR #14414:
URL: https://github.com/apache/pinot/pull/14414#issuecomment-2463292682

   @Jackie-Jiang , @mcvsubbu Adding in previous reviewers of the similar draft 
PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org