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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
