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