add @ManualAuthorization annotation for non-standard endpoints #9252
Conversation
apucher
requested review from
xiangfu0,
walterddr,
sajjad-moradi,
Jackie-Jiang and
npawar
Codecov Report
@@ Coverage Diff @@
## master #9252 +/- ##
============================================
+ Coverage 68.44% 69.97% +1.52%
- Complexity 5008 5012 +4
============================================
Files 1861 1861
Lines 99218 99231 +13
Branches 15092 15096 +4
============================================
+ Hits 67911 69432 +1521
+ Misses 26460 24890 -1570
- Partials 4847 4909 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
apucher
force-pushed
the
manual-authorization-annotation
branch
2 times, most recently
from
a569240 to
c43fdad
Compare
| @@ -51,7 +52,8 @@ public interface AccessControl { | |||
| * @param endpointUrl the request url for which this access control is called | |||
| * @return whether the client has permission | |||
| */ | |||
| default boolean hasAccess(String tableName, AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) { | |||
| default boolean hasAccess(@Nullable String tableName, AccessType accessType, HttpHeaders httpHeaders, | |||
There was a problem hiding this comment.
I believe we shouldn't make these nullable. The existing implementations might assume these are not null.
There was a problem hiding this comment.
We already have scenarios where null is passed in here "by accident" if tableName (schemaName) happens to come out to null. This merely acknowledges that reality. Do you know of an existing implementation that has issues with this?
There was a problem hiding this comment.
+1. table name can already be null. also the access control doesn't distinguish between table or schema name
| @Authenticate(AccessType.READ) | ||
| public Response downloadSegment( | ||
| @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName, | ||
| @ApiParam(value = "Name of the segment", required = true) @PathParam("segmentName") @Encoded String segmentName, | ||
| @Context HttpHeaders httpHeaders) | ||
| throws Exception { | ||
| // Validate data access | ||
| boolean hasDataAccess; | ||
| try { | ||
| AccessControl accessControl = _accessControlFactory.create(); | ||
| hasDataAccess = accessControl.hasDataAccess(httpHeaders, tableName); | ||
| } catch (Exception e) { | ||
| throw new ControllerApplicationException(LOGGER, | ||
| "Caught exception while validating access to table: " + tableName, Response.Status.INTERNAL_SERVER_ERROR, e); | ||
| } | ||
| if (!hasDataAccess) { | ||
| throw new ControllerApplicationException(LOGGER, "No data access to table: " + tableName, | ||
| Response.Status.FORBIDDEN); | ||
| } | ||
|
|
There was a problem hiding this comment.
We shouldn't delete the manual authentication here. Although the method is GET, but we need to only allow ppl who are authorized to be able to download the data.
There was a problem hiding this comment.
This is the same strictness as before. The endpoint provides a table name, therefore, we'll perform auth based on headers and table name. The only change is removing the call to a deprecated method.
| if (!accessControl.hasDataAccess(httpHeaders, rawTableName)) { | ||
| if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders, endpointUrl)) { |
There was a problem hiding this comment.
Let's not change this. Although we're reading data and AccessType.READ looks appropriate, we need to only allow ppl who are authorized to be able to query the data that might contain sensitive info like member information. It's different than reading table config or some other cluster configs that might be ok with weaker authentication level like AccessType.READ.
There was a problem hiding this comment.
Similar to the above, this has similar strictness to the original implementation. The difference here is removing the use of a deprecated method.
On a tangent, this resource shouldn't be in the controller in the first place, rather, it should be controlled by the broker's access control.
apucher
force-pushed
the
manual-authorization-annotation
branch
from
c43fdad to
22d0609
Compare
apucher
force-pushed
the
manual-authorization-annotation
branch
from
7feb69b to
890aa3c
Compare
Several endpoints in pinot aren't served well by the AuthFilter's heuristics for extracting table names. In particular, this includes
/tableConfigsand/schemaendpoints that accept opaque payloads without a table or schema name in the parameters. This causes problems with fine-grained access control, such as non-admin users creating new tables via self-serve.This PR adds a new annotation
@ManualAuthorizationfor REST endpoints, which allows developers to skip the default authorization and deserialize payloads before manually invoking authorization, e.g. viaAccessControlUtils.validatePermissions(). This annotation comes with obvious risks and should be used sparingly, as it enables requests to bypass most of the AuthFilter.