Remove deprecated hasDataAccess method. #13574

abhioncbr · 2024-07-10T01:42:39Z

Labels

cleanup

Description
In the year 2021, We deprecated the hasDataAccess method of class AccessControl.java ( for reference: PR ). This PR is just removing the deprecated method from the code.

codecov-commenter · 2024-07-10T02:21:00Z

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.98%. Comparing base (59551e4) to head (41114d0).
Report is 750 commits behind head on master.

Files	Patch %	Lines
...t/controller/api/resources/PinotQueryResource.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13574      +/-   ##
============================================
+ Coverage     61.75%   61.98%   +0.23%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2555     +119     
  Lines        133233   140614    +7381     
  Branches      20636    21819    +1183     
============================================
+ Hits          82274    87163    +4889     
- Misses        44911    46839    +1928     
- Partials       6048     6612     +564

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`61.91% <50.00%> (+0.20%)`	⬆️
java-21	`61.85% <50.00%> (+0.23%)`	⬆️
skip-bytebuffers-false	`61.95% <50.00%> (+0.21%)`	⬆️
skip-bytebuffers-true	`61.81% <50.00%> (+34.08%)`	⬆️
temurin	`61.98% <50.00%> (+0.23%)`	⬆️
unittests	`61.98% <50.00%> (+0.23%)`	⬆️
unittests1	`46.44% <ø> (-0.45%)`	⬇️
unittests2	`27.75% <50.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jackie-Jiang · 2024-07-11T00:15:09Z

cc @soumitra-st @zhtaoxiang Can you help take a look?

siddharthteotia · 2024-07-11T00:52:44Z

@vvivekiyer - Can you help review this please

vvivekiyer · 2024-07-11T01:45:40Z

...t-controller/src/main/java/org/apache/pinot/controller/api/access/AllowAllAccessFactory.java

@@ -24,7 +24,7 @@
 public class AllowAllAccessFactory implements AccessControlFactory {
  private static final AccessControl ALLOW_ALL_ACCESS = new AccessControl() {
    @Override
-    public boolean hasDataAccess(HttpHeaders httpHeaders, String tableName) {
+    public boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {


Perhaps we can remove this function as the default implementation returns true?

vvivekiyer · 2024-07-12T00:19:14Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

@@ -158,7 +158,7 @@ public Response downloadSegment(
    boolean hasDataAccess;
    try {
      AccessControl accessControl = _accessControlFactory.create();
-      hasDataAccess = accessControl.hasDataAccess(httpHeaders, tableName);
+      hasDataAccess = accessControl.hasAccess(tableName, AccessType.READ, httpHeaders, null);


@abhioncbr Can we please pass the endpointUrl Actions.Table.DOWNLOAD_SEGMENT here and in PinotQueryResource (ideally we should pass it in all places where hasAccess is called if not done already). This will allow callers to have different READ restrictions for each endpoint.

Internally at Linkedin, we have different restrictions on READ access for downloadSegment and query endpoints.

Sure, I'll make the required changes. Thanks

vvivekiyer

Requested changes to pass endpointUrl when calling hasAccess method.

soumitra-st · 2024-07-12T02:24:37Z

...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java

@@ -292,7 +293,7 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String

    // Validate data access
    AccessControl accessControl = _accessControlFactory.create();
-    if (!accessControl.hasDataAccess(httpHeaders, rawTableName)) {
+    if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders, Actions.Table.DOWNLOAD_SEGMENT)) {


Is Actions.Table.DOWNLOAD_SEGMENT is not suited for endpointUrl? If endpointUrl is not accessible, we can use null.

@vvivekiyer / @soumitra-st We need some consensus on whether to use Actions.Table.DOWNLOAD_SEGMENT or pass null.

@soumitra-st Passing endpointUrl is critical to allow implementations to provide different access restrictions to different read endpoints. Hence I had suggested that we add the relevant action.

All over our codebase (eg: PinotTableResstletResource), we pass the action as endpointUrl. If we feel that a better identifier is needed to identify endpointUrl, we can discuss.

@abhioncbr as this is the query endpoint, the action here should be Actions.Table.QUERY ?

All over our codebase (eg: PinotTableResstletResource), we pass the action as endpointUrl. If we feel that a better identifier is needed to identify endpointUrl, we can discuss.

@vvivekiyer , if we pass action as endpointUrl across the board, then I am ok with that.

Thanks for the feedback. I will make the change here as suggested by @vvivekiyer and for adding endpointUrl across board, I will raise another PR. Sounds good?

soumitra-st · 2024-07-12T02:25:28Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

@@ -158,7 +158,7 @@ public Response downloadSegment(
    boolean hasDataAccess;
    try {
      AccessControl accessControl = _accessControlFactory.create();
-      hasDataAccess = accessControl.hasDataAccess(httpHeaders, tableName);
+      hasDataAccess = accessControl.hasAccess(tableName, AccessType.READ, httpHeaders, Actions.Table.DOWNLOAD_SEGMENT);


Same here, Actions.Table.DOWNLOAD_SEGMENT action used as endpointUrl. Shall we pass null?

abhioncbr · 2024-07-15T20:34:37Z

Can we merge it?

* Remove deprecated lang3 RandomUtils usage. * Changes as per the PR comment. * Remove deprecated method hasDataAccess. * Changes as per the PR Comments. * Updated the endpointUrl to QUERY. * Fix linting issue.

jadami10 · 2024-11-03T02:20:51Z

pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java

-      @Nullable String endpointUrl) {
-    return true;
-  }
+  boolean hasAccess(@Nullable String tableName, AccessType accessType, HttpHeaders httpHeaders,


Why does this no longer default true? This is backwards incompatible for users of this interface

Good catch. Adding it back in #14382

abhioncbr added 3 commits July 9, 2024 21:03

Remove deprecated lang3 RandomUtils usage.

e6ca292

Changes as per the PR comment.

3d5a722

Remove deprecated method hasDataAccess.

42c7b74

abhioncbr added the cleanup label Jul 10, 2024

abhioncbr requested a review from Jackie-Jiang July 10, 2024 18:06

abhioncbr marked this pull request as ready for review July 10, 2024 18:06

Jackie-Jiang added the security label Jul 11, 2024

abhioncbr requested a review from vvivekiyer July 11, 2024 02:06

vvivekiyer approved these changes Jul 11, 2024

View reviewed changes

vvivekiyer reviewed Jul 12, 2024

View reviewed changes

vvivekiyer requested changes Jul 12, 2024

View reviewed changes

Changes as per the PR Comments.

d0f2007

soumitra-st reviewed Jul 12, 2024

View reviewed changes

abhioncbr added 2 commits July 12, 2024 17:13

Updated the endpointUrl to QUERY.

e1720e7

Fix linting issue.

41114d0

vvivekiyer approved these changes Jul 13, 2024

View reviewed changes

vvivekiyer merged commit 1cc3cef into apache:master Jul 15, 2024
20 checks passed

abhioncbr mentioned this pull request Jul 17, 2024

Cleanup of the method hasAccess. #13633

Merged

jadami10 reviewed Nov 3, 2024

View reviewed changes

Jackie-Jiang mentioned this pull request Nov 5, 2024

Add back default AccessControl.hasAccess() #14382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated hasDataAccess method. #13574

Remove deprecated hasDataAccess method. #13574

abhioncbr commented Jul 10, 2024 •

edited

Loading

codecov-commenter commented Jul 10, 2024 •

edited

Loading

Jackie-Jiang commented Jul 11, 2024

siddharthteotia commented Jul 11, 2024

vvivekiyer Jul 11, 2024

vvivekiyer Jul 12, 2024 •

edited

Loading

abhioncbr Jul 12, 2024

vvivekiyer left a comment

soumitra-st Jul 12, 2024

abhioncbr Jul 12, 2024 •

edited

Loading

vvivekiyer Jul 12, 2024

soumitra-st Jul 12, 2024 •

edited

Loading

abhioncbr Jul 12, 2024

soumitra-st Jul 12, 2024

abhioncbr commented Jul 15, 2024

jadami10 Nov 3, 2024

Jackie-Jiang Nov 5, 2024

Remove deprecated hasDataAccess method. #13574

Remove deprecated hasDataAccess method. #13574

Conversation

abhioncbr commented Jul 10, 2024 • edited Loading

codecov-commenter commented Jul 10, 2024 • edited Loading

Codecov Report

Jackie-Jiang commented Jul 11, 2024

siddharthteotia commented Jul 11, 2024

Choose a reason for hiding this comment

vvivekiyer Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvivekiyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhioncbr Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumitra-st Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhioncbr commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhioncbr commented Jul 10, 2024 •

edited

Loading

codecov-commenter commented Jul 10, 2024 •

edited

Loading

vvivekiyer Jul 12, 2024 •

edited

Loading

abhioncbr Jul 12, 2024 •

edited

Loading

soumitra-st Jul 12, 2024 •

edited

Loading