You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/01/29 23:55:31 UTC
[GitHub] [incubator-pinot] sajjad-moradi opened a new pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
sajjad-moradi opened a new pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507
## Description
- This PR adds access control capability for REST endpoints of Controller.
- If an endpoint requires authentication, it can be simply annotated with `@Authenticate` annotation with `AccessType` parameter. This will trigger automatic authentication.
- Authentication happens in a container filter - `AuthFilter` - which automatically gets called before execution of each endpoint.
- `AuthFilter` checks if `@Authenticate` annotation is available on the requested endpoint. If available, then it calls `AccessControl` object to perform actual authentication.
- The described approach works just fine for the endpoints that are not table level. In other words, they don't require table name for authentication.
- For table level endpoints which require table name as an input to authentication, there are two ways:
1. _Table name can be provided as a path (or query) parameter on the endpoint._ In this case, `AuthFilter` can extract it and pass it to AccessControl object. For backward compatibility, `AuthFilter` looks for `tableName`, `tableNameWithType`, or `schemaName` in path (or query) parameters.
2. _Table name cannot be provided as a path (or query) param._ For example in case of uploading a table or schema, tableName is deep inside the json body of the post request and extracting table name needs to happen within the endpoint. In this case, automatic authentication via AuthFilter is not possible. Therefore, `@Authenticate` annotation will not be placed on these endpoints and authentication needs to be explicitly invoked within the endpoint.
## Testing Done
Deployed locally and verified that the authentication gets called automatically for annotated endpoints.
Also verified the expected behavior on the endpoints with no annotation and the explicit (manual) authentication, for example POST method of `/schemas` and `/tables`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r571318931
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
##########
@@ -121,11 +132,19 @@
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables")
@ApiOperation(value = "Adds a table", notes = "Adds a table")
- public SuccessResponse addTable(String tableConfigStr) {
+ public SuccessResponse addTable(String tableConfigStr, @Context HttpHeaders httpHeaders, @Context Request request) {
// TODO introduce a table config ctor with json string.
TableConfig tableConfig;
+ String tableName;
try {
tableConfig = JsonUtils.stringToObject(tableConfigStr, TableConfig.class);
+
+ // validate permission
Review comment:
Got it, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r568197551
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,38 @@
/**
* Return whether the client has data access to the given table.
*
- * @param httpHeaders Http headers
+ * Note: This method is only used fore read access. It's being deprecated and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
+ * @param httpHeaders HTTP headers containing requester identity
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
+ @Deprecated
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param tableName name of the table to be accessed
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers containing requester identity
+ * @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) {
Review comment:
I wonder if we should stay away from a default implementation for access control. Then again, there's an argument to be made for facilitating a smooth transition. @Jackie-Jiang thoughts?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
##########
@@ -124,4 +141,64 @@ public void filter(ContainerRequestContext containerRequestContext,
containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*");
}
}
+
+ @javax.ws.rs.ext.Provider
+ public static class AuthFilter implements ContainerRequestFilter {
Review comment:
Would you mind extracting this to a separate class file?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+import java.lang.reflect.Method;
+import java.util.Optional;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.controller.api.resources.ControllerApplicationException;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class to simplify access control validation. This class is simple wrapper around AccessControl class.
+ */
+public class AccessControlUtils {
Review comment:
imo this could be a static singleton with static methods - though I might be missing some subtle details here.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessType.java
##########
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+/**
+ * Different access types used in access control of the rest endpoints
+ */
+public enum AccessType {
+ CREATE, READ, UPDATE, DELETE
Review comment:
I wonder if there's a way of combining CREATE and UPDATE into one. I can see a case for update without create, but create without update seems tricky (e.g. create vs update with instance partitions)
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java
##########
@@ -289,6 +291,7 @@ public StringResultResponse getTaskStateDeprecated(
@POST
@Path("/tasks/schedule")
+ @Authenticate(AccessType.UPDATE)
Review comment:
create? another one of these create/update cases
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-773724255
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=h1) Report
> Merging [#6507](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=desc) (9f5f20d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.56%`.
> The diff coverage is `42.11%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6507 +/- ##
===========================================
- Coverage 66.44% 43.88% -22.57%
===========================================
Files 1075 1339 +264
Lines 54773 65865 +11092
Branches 8168 9611 +1443
===========================================
- Hits 36396 28904 -7492
- Misses 15700 34531 +18831
+ Partials 2677 2430 -247
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.88% <42.11%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1346 more](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=footer). Last update [1ae53fe...9f5f20d](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r569628074
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
##########
@@ -124,4 +141,64 @@ public void filter(ContainerRequestContext containerRequestContext,
containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*");
}
}
+
+ @javax.ws.rs.ext.Provider
+ public static class AuthFilter implements ContainerRequestFilter {
+
+ @Inject
+ Provider<Request> _requestProvider;
+
+ @Inject
+ AccessControlFactory _accessControlFactory;
+
+ @Context
+ ResourceInfo _resourceInfo;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @Override
+ public void filter(ContainerRequestContext requestContext)
+ throws IOException {
+ // check if authentication is required
+ Method endpointMethod = _resourceInfo.getResourceMethod();
+ if (endpointMethod.isAnnotationPresent(Authenticate.class)) {
+ // Perform authentication:
+ // Note that table name is extracted from "path parameters" or "query parameters" if it's defined as one of the
Review comment:
If someone adds a table that is called "tableName" or "schemaName" will that still work?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,38 @@
/**
* Return whether the client has data access to the given table.
*
- * @param httpHeaders Http headers
+ * Note: This method is only used fore read access. It's being deprecated and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
+ * @param httpHeaders HTTP headers containing requester identity
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
+ @Deprecated
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param tableName name of the table to be accessed
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers containing requester identity
+ * @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) {
+ return true;
+ }
+
+ /**
+ * Return whether the client has permission to access the epdpoints with are not table level
+ *
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers
+ * @param endpointUrl the request url for which this access control is called
+ * @return whether the client has permission
+ */
+ default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Review comment:
If this API is present, do we need the previous one? Can we just provide some util methods in a base class to infer table name, schema name, cluster name, etc. and just leave it to the auth methods to either use them or not as they see fit?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
##########
@@ -121,11 +132,19 @@
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables")
@ApiOperation(value = "Adds a table", notes = "Adds a table")
- public SuccessResponse addTable(String tableConfigStr) {
+ public SuccessResponse addTable(String tableConfigStr, @Context HttpHeaders httpHeaders, @Context Request request) {
// TODO introduce a table config ctor with json string.
TableConfig tableConfig;
+ String tableName;
try {
tableConfig = JsonUtils.stringToObject(tableConfigStr, TableConfig.class);
+
+ // validate permission
Review comment:
I don't think we need to authenticate whether a certain table can be added. Pinot does not support namespace for tables. If there was one, I would agree that we can auth for a certain namespace.
We just need to verify whether someone is allowed to create a table or not.
So, CREATE annotation should suffice.
For example , during creation, an auth system may store the user/group credentials of the creator, and may decide that only the user is allowed to update the table. Anyone may be allowed to create the table, but only owners may be allowed to update their own table (or change their schema)
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,38 @@
/**
* Return whether the client has data access to the given table.
*
- * @param httpHeaders Http headers
+ * Note: This method is only used fore read access. It's being deprecated and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
+ * @param httpHeaders HTTP headers containing requester identity
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
+ @Deprecated
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param tableName name of the table to be accessed
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers containing requester identity
+ * @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) {
Review comment:
We can do one of:
(1) Have a default method here that returns "true"
(2) Have a default implementation of the interface that always return true. Actual implementations need to override the method to return the value they want.
Earlier, we followed the pattern (1). Since we are deprecating the first API, we can eventually remove the default implementations and follow pattern (2) moving forward. I prefer that.
Either way, for installations that are not interested in authenticating, we must come up by default instead of requiring them to implement an empty auth class
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-770123263
Ok great. I wonder if it's worthwhile to expand this to the existing authorization in broker and server as well. Then again, they shouldn't allow changes / be read-only anyways.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-770121598
> Hi Sajjad, great timing! I was drawing up ways to introduce generic request authentication and expand on the existing request authorization in pinot. Looks like you literally just built part of this - would you mind sharing a bit more about the specific use-case you're looking to address?
Basically the changes introduced in this PR prevents a malicious user to change the state of the system. It also prevents mistakes from normal users by constraining them to only modify the tables they own.
Implementation of `AccessControl` interface can define ACL, access control list, for each table. Basically owners (and also admins) will be added to the ACL of each table. During authentication, `AccessControll` checks if the requester's is indeed listed as owner of that table/resource.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r568197551
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,38 @@
/**
* Return whether the client has data access to the given table.
*
- * @param httpHeaders Http headers
+ * Note: This method is only used fore read access. It's being deprecated and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
+ * @param httpHeaders HTTP headers containing requester identity
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
+ @Deprecated
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param tableName name of the table to be accessed
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers containing requester identity
+ * @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) {
Review comment:
I wonder if we should stay away from a default implementation for access control. Then again, there's an argument to be made for facilitating a smooth transition. @Jackie-Jiang thoughts?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
##########
@@ -124,4 +141,64 @@ public void filter(ContainerRequestContext containerRequestContext,
containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*");
}
}
+
+ @javax.ws.rs.ext.Provider
+ public static class AuthFilter implements ContainerRequestFilter {
Review comment:
Would you mind extracting this to a separate class file?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+import java.lang.reflect.Method;
+import java.util.Optional;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.controller.api.resources.ControllerApplicationException;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class to simplify access control validation. This class is simple wrapper around AccessControl class.
+ */
+public class AccessControlUtils {
Review comment:
imo this could be a static singleton with static methods - though I might be missing some subtle details here.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessType.java
##########
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+/**
+ * Different access types used in access control of the rest endpoints
+ */
+public enum AccessType {
+ CREATE, READ, UPDATE, DELETE
Review comment:
I wonder if there's a way of combining CREATE and UPDATE into one. I can see a case for update without create, but create without update seems tricky (e.g. create vs update with instance partitions)
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java
##########
@@ -289,6 +291,7 @@ public StringResultResponse getTaskStateDeprecated(
@POST
@Path("/tasks/schedule")
+ @Authenticate(AccessType.UPDATE)
Review comment:
create? another one of these create/update cases
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r569686621
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
##########
@@ -121,11 +132,19 @@
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables")
@ApiOperation(value = "Adds a table", notes = "Adds a table")
- public SuccessResponse addTable(String tableConfigStr) {
+ public SuccessResponse addTable(String tableConfigStr, @Context HttpHeaders httpHeaders, @Context Request request) {
// TODO introduce a table config ctor with json string.
TableConfig tableConfig;
+ String tableName;
try {
tableConfig = JsonUtils.stringToObject(tableConfigStr, TableConfig.class);
+
+ // validate permission
Review comment:
When there's any `@Authentication` annotation available on a method, authFilter check if tableName parameter is provided. If it is not, the endpoint is considered an admin level endpoint (because it's not related to any specific table). Here if we just put CREATE annotation, it means only admins can create tables and that's not what we want.
I believe what you're referring to about authentication on creation and then updating is correct, but it will be handled by implementation of `AccessControl` interface.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTaskRestletResource.java
##########
@@ -289,6 +291,7 @@ public StringResultResponse getTaskStateDeprecated(
@POST
@Path("/tasks/schedule")
+ @Authenticate(AccessType.UPDATE)
Review comment:
Yeah, as you and I mentioned earlier, some of these endpoints are tricky in terms of annotating with create or update. Here I thought this method just updates the state of the system by triggering tasks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu merged pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-773724255
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=h1) Report
> Merging [#6507](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=desc) (89bfb7e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.52%`.
> The diff coverage is `42.11%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6507 +/- ##
===========================================
- Coverage 66.44% 43.92% -22.53%
===========================================
Files 1075 1339 +264
Lines 54773 65876 +11103
Branches 8168 9614 +1446
===========================================
- Hits 36396 28935 -7461
- Misses 15700 34505 +18805
+ Partials 2677 2436 -241
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.92% <42.11%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1346 more](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=footer). Last update [1ae53fe...9f5f20d](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-773724255
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=h1) Report
> Merging [#6507](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=desc) (9f5f20d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.56%`.
> The diff coverage is `42.11%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6507 +/- ##
===========================================
- Coverage 66.44% 43.88% -22.57%
===========================================
Files 1075 1339 +264
Lines 54773 65865 +11092
Branches 8168 9611 +1443
===========================================
- Hits 36396 28904 -7492
- Misses 15700 34531 +18831
+ Partials 2677 2430 -247
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.88% <42.11%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1346 more](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=footer). Last update [1ae53fe...9f5f20d](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r569686211
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
##########
@@ -124,4 +141,64 @@ public void filter(ContainerRequestContext containerRequestContext,
containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*");
}
}
+
+ @javax.ws.rs.ext.Provider
+ public static class AuthFilter implements ContainerRequestFilter {
Review comment:
Will do.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/ControllerAdminApiApplication.java
##########
@@ -124,4 +141,64 @@ public void filter(ContainerRequestContext containerRequestContext,
containerResponseContext.getHeaders().add("Access-Control-Allow-Origin", "*");
}
}
+
+ @javax.ws.rs.ext.Provider
+ public static class AuthFilter implements ContainerRequestFilter {
+
+ @Inject
+ Provider<Request> _requestProvider;
+
+ @Inject
+ AccessControlFactory _accessControlFactory;
+
+ @Context
+ ResourceInfo _resourceInfo;
+
+ @Context
+ HttpHeaders _httpHeaders;
+
+ @Override
+ public void filter(ContainerRequestContext requestContext)
+ throws IOException {
+ // check if authentication is required
+ Method endpointMethod = _resourceInfo.getResourceMethod();
+ if (endpointMethod.isAnnotationPresent(Authenticate.class)) {
+ // Perform authentication:
+ // Note that table name is extracted from "path parameters" or "query parameters" if it's defined as one of the
Review comment:
Yes. For extracting table name, it looks for "tableName" or "schemaName" as the key in the MultivaluedMap of path/query params. The value for those keys could be anything, even "tableName" or "schemaName"
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,38 @@
/**
* Return whether the client has data access to the given table.
*
- * @param httpHeaders Http headers
+ * Note: This method is only used fore read access. It's being deprecated and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
+ * @param httpHeaders HTTP headers containing requester identity
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
+ @Deprecated
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param tableName name of the table to be accessed
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers containing requester identity
+ * @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) {
Review comment:
You both are right, the default values in the interface is for smooth transition. When the first api gets removed, the default values in the interface will be removed as well and the default implementation of the interface will return true for both methods.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,38 @@
/**
* Return whether the client has data access to the given table.
*
- * @param httpHeaders Http headers
+ * Note: This method is only used fore read access. It's being deprecated and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
+ * @param httpHeaders HTTP headers containing requester identity
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
+ @Deprecated
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param tableName name of the table to be accessed
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers containing requester identity
+ * @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) {
+ return true;
+ }
+
+ /**
+ * Return whether the client has permission to access the epdpoints with are not table level
+ *
+ * @param accessType type of the access
+ * @param httpHeaders HTTP headers
+ * @param endpointUrl the request url for which this access control is called
+ * @return whether the client has permission
+ */
+ default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String endpointUrl) {
Review comment:
Yes, we need two methods here. Table name is a mandatory input for authentication of table-level endpoints. Having two methods in the interface emphasizes that there are two types of endpoints from authentication point of view: table level and non-table level endpoints.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -0,0 +1,98 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+import java.lang.reflect.Method;
+import java.util.Optional;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.controller.api.resources.ControllerApplicationException;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Utility class to simplify access control validation. This class is simple wrapper around AccessControl class.
+ */
+public class AccessControlUtils {
Review comment:
Not having static methods makes it easier for unit testing of the endpoints where authentication is called explicitly.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessType.java
##########
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+/**
+ * Different access types used in access control of the rest endpoints
+ */
+public enum AccessType {
+ CREATE, READ, UPDATE, DELETE
Review comment:
I see your point. It's a bit tricky. I went with 4 basic CRUD operations and to annotate endpoints, I tried to make a call to see if the endpoint actually creates something or it just update the state of the system.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] apucher commented on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
apucher commented on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-770116899
Hi Sajjad, great timing! I was drawing up ways to introduce generic request authentication and expand on the existing request authorization in pinot. Looks like you literally just built part of this - would you mind sharing a bit more about the specific use-case you're looking to address?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#discussion_r569686490
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessType.java
##########
@@ -0,0 +1,27 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+/**
+ * Different access types used in access control of the rest endpoints
+ */
+public enum AccessType {
+ CREATE, READ, UPDATE, DELETE
Review comment:
I see your point. It's a bit tricky. I went with 4 basic CRUD operations and to annotate endpoints, I tried to make a call to see if the endpoint actually creates something or it just updates the state of the system.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6507: Add Access Control for REST endpoints of Controller - Declarative Approach
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6507:
URL: https://github.com/apache/incubator-pinot/pull/6507#issuecomment-773724255
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=h1) Report
> Merging [#6507](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=desc) (89bfb7e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.52%`.
> The diff coverage is `42.11%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6507 +/- ##
===========================================
- Coverage 66.44% 43.92% -22.53%
===========================================
Files 1075 1339 +264
Lines 54773 65876 +11103
Branches 8168 9614 +1446
===========================================
- Hits 36396 28935 -7461
- Misses 15700 34505 +18805
+ Partials 2677 2436 -241
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.92% <42.11%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1346 more](https://codecov.io/gh/apache/incubator-pinot/pull/6507/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=footer). Last update [1ae53fe...9f5f20d](https://codecov.io/gh/apache/incubator-pinot/pull/6507?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org