Jackie-Jiang commented on code in PR #11016:
URL: https://github.com/apache/pinot/pull/11016#discussion_r1273028597
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotBrokerDebug.java:
##########
@@ -129,6 +134,7 @@ public Map<String, Map<ServerInstance, List<String>>>
getRoutingTable(
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/debug/routingTable/sql")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_ROUTING)
Review Comment:
This should be a manual authorize request, where table name can be extracted
from the broker request
##########
pinot-integration-tests/src/main/java/org/apache/pinot/broker/api/resources/BrokerEchoWithAutoDiscovery.java:
##########
@@ -28,6 +28,7 @@
import javax.ws.rs.core.MediaType;
import org.apache.pinot.core.api.AutoLoadedServiceForTest;
+
Review Comment:
(minor) Revert
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -288,6 +293,7 @@ private void
persistInstancePartitionsHelper(InstancePartitions instancePartitio
@PUT
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables/{tableName}/instancePartitions")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.ASSIGN_INSTANCE)
Review Comment:
UPDATE_INSTANCE_PARTITIONS
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -342,6 +348,7 @@ public Map<String, InstancePartitions>
setInstancePartitions(
@DELETE
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables/{tableName}/instancePartitions")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.DELETE_PARTITION)
Review Comment:
DELETE_INSTANCE_PARTITIONS
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java:
##########
@@ -370,6 +379,7 @@ public SuccessResponse updateInstance(
@PUT
@Path("/instances/{instanceName}/updateTags")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.UPDATE_TAG)
Review Comment:
UPDATE_INSTANCE
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -339,6 +363,7 @@ public String validateSchema(String schemaJsonString,
@Context HttpHeaders httpH
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/schemas/fieldSpec")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_FIELD_SPEC)
Review Comment:
This API is kind of special because it does not access any data in the
cluster. Is there a way to always allow access?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -149,6 +153,7 @@ public Map<String, InstancePartitions>
getInstancePartitions(
@POST
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables/{tableName}/assignInstances")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.ASSIGN_INSTANCE)
Review Comment:
CREATE_INSTANCE_PARTITIONS
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java:
##########
@@ -63,6 +66,7 @@ public class PinotControllerHealthCheck {
@GET
@Path("pinot-controller/admin")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_INFO)
Review Comment:
This should be `GET_HEALTH`. Remove `GET_INFO` because it is very ambiguous
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java:
##########
@@ -407,6 +417,7 @@ public SuccessResponse updateInstanceTags(
@POST
@Path("/instances/{instanceName}/updateBrokerResource")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.UPDATE_RESOURCE)
Review Comment:
UPDATE_BROKER_RESOURCE
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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.core.auth;
+
+import java.lang.reflect.Method;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+
+
+/**
+ * Utility methods to share in Broker and Controller request filters related
to fine grain authorization.
+ */
+public class FineGrainedAuthUtils {
+
+ private FineGrainedAuthUtils() {
+ }
+
+ /**
+ * Returns the parameter from the path or query params.
+ * @param paramName to look for
+ * @param pathParams path params
+ * @param queryParams query params
+ * @return the value of the parameter
+ */
+ private static String findParam(String paramName, MultivaluedMap<String,
String> pathParams,
+ MultivaluedMap<String, String> queryParams) {
+ String name = pathParams.getFirst(paramName);
+ if (name == null) {
+ name = queryParams.getFirst(paramName);
+ }
+ return name;
+ }
+
+ private static void logAndThrow(Logger logger, String msg, Response.Status
status) {
Review Comment:
Suggest removing this util. I feel it doesn't help with readability
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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.core.auth;
+
+import java.lang.reflect.Method;
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.core.UriInfo;
+import org.apache.commons.lang.StringUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+
+
+/**
+ * Utility methods to share in Broker and Controller request filters related
to fine grain authorization.
+ */
+public class FineGrainedAuthUtils {
+
+ private FineGrainedAuthUtils() {
+ }
+
+ /**
+ * Returns the parameter from the path or query params.
+ * @param paramName to look for
+ * @param pathParams path params
+ * @param queryParams query params
+ * @return the value of the parameter
+ */
+ private static String findParam(String paramName, MultivaluedMap<String,
String> pathParams,
+ MultivaluedMap<String, String> queryParams) {
+ String name = pathParams.getFirst(paramName);
+ if (name == null) {
+ name = queryParams.getFirst(paramName);
+ }
+ return name;
+ }
+
+ private static void logAndThrow(Logger logger, String msg, Response.Status
status) {
+ logger.error(msg);
Review Comment:
We probably don't want to log at error level. For invalid access, currently
we don't log anything because it is pure user error
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -82,6 +85,7 @@ public class PinotInstanceAssignmentRestletResource {
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables/{tableName}/instancePartitions")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_PARTITION)
Review Comment:
GET_INSTANCE_PARTITIONS
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -395,6 +402,7 @@ private void removeInstancePartitionsHelper(String
instancePartitionsName) {
@POST
@Produces(MediaType.APPLICATION_JSON)
@Path("/tables/{tableName}/replaceInstance")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.REPLACE_INSTANCE)
Review Comment:
UPDATE_INSTANCE_PARTITIONS
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/Actions.java:
##########
@@ -0,0 +1,153 @@
+/**
+ * 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.core.auth;
+
+/**
+ * Different action types used in finer grain access control of the rest
endpoints
+ * Action names are in <verb><noun> format, e.g. GetSchema, ListTables, etc.
+ */
+public class Actions {
+ // Action names for cluster
+ public static class Cluster {
+ public static final String CANCEL_QUERY = "CancelQuery";
+ public static final String CLEANUP_TASK = "CleanupTask";
+ public static final String COMMIT_SEGMENT = "CommitSegment";
+ public static final String CREATE_INSTANCE = "CreateInstance";
+ public static final String CREATE_TASK = "CreateTask";
+ public static final String CREATE_TENANT = "CreateTenant";
+ public static final String CREATE_USER = "CreateUser";
+ public static final String DEBUG_TASK = "DebugTask";
+ public static final String DELETE_CLUSTER_CONFIG = "DeleteClusterConfig";
+ public static final String DELETE_INSTANCE = "DeleteInstance";
+ public static final String DELETE_TASK = "DeleteTask";
+ public static final String DELETE_TENANT = "DeleteTenant";
+ public static final String DELETE_USER = "DeleteUser";
+ public static final String DELETE_ZNODE = "DeleteZnode";
+ public static final String ESTIMATE_UPSERT_MEMORY = "EstimateUpsertMemory";
+ public static final String EXECUTE_TASK = "ExecuteTask";
+ public static final String GET_ADMIN_INFO = "GetAdminInfo";
+ public static final String GET_APP_CONFIG = "GetAppConfig";
+ public static final String GET_AUTH = "GetAuth";
+ public static final String GET_BROKER = "GetBroker";
+ public static final String GET_CLUSTER_CONFIG = "GetClusterConfig";
+ public static final String GET_FIELD_SPEC = "GetFieldSpec";
+ public static final String GET_FORCE_COMMIT_STATUS =
"GetForceCommitStatus";
+ public static final String GET_HEALTH = "GetHealth";
+ public static final String GET_INFO = "GetInfo";
+ public static final String GET_INSTANCE = "GetInstance";
+ public static final String GET_LOGGER = "GetLogger";
+ public static final String GET_LOG_FILE = "GetLogFile";
+ public static final String GET_REBALANCE_STATUS = "GetRebalanceStatus";
+ public static final String GET_ROUTING = "GetRouting";
+ public static final String GET_RUNNING_QUERY = "GetRunningQuery";
+ public static final String GET_SCHEDULER_INFO = "GetSchedulerInfo";
+ public static final String GET_SCHEMA = "GetSchema";
+ public static final String GET_SEGMENT = "GetSegment";
+ public static final String GET_SEGMENT_RELOAD_STATUS =
"GetSegmentReloadStatus";
+ public static final String GET_SERVER_ROUTING_STATS =
"GetServerRoutingStats";
+ public static final String GET_TABLE = "GetTable";
+ public static final String GET_TABLE_CONFIG = "GetTableConfig";
+ public static final String GET_TABLE_LEADER = "GetTableLeader";
+ public static final String GET_TASK = "GetTask";
+ public static final String GET_TENANT = "GetTenant";
+ public static final String GET_USER = "GetUser";
+ public static final String GET_VERSION = "GetVersion";
+ public static final String GET_ZNODE = "GetZnode";
+ public static final String INGEST_FILE = "IngestFile";
+ public static final String RECOMMEND_CONFIG = "RecommendConfig";
+ public static final String RESET_SEGMENT = "ResetSegment";
+ public static final String RESUME_TASK = "ResumeTask";
+ public static final String STOP_TASK = "StopTask";
+ public static final String UPDATE_CLUSTER_CONFIG = "UpdateClusterConfig";
+ public static final String UPDATE_INSTANCE = "UpdateInstance";
+ public static final String UPDATE_LOGGER = "UpdateLogger";
+ public static final String UPDATE_QPS = "UpdateQPS";
+ public static final String UPDATE_RESOURCE = "UpdateResource";
+ public static final String UPDATE_TAG = "UpdateTag";
+ public static final String UPDATE_TASK_QUEUE = "UpdateTaskQueue";
+ public static final String UPDATE_TENANT = "UpdateTenant";
+ public static final String UPDATE_TENANT_METADATA = "UpdateTenantMetadata";
+ public static final String UPDATE_TIME_INTERVAL = "UpdateTimeInterval";
+ public static final String UPDATE_USER = "UpdateUser";
+ public static final String UPDATE_ZNODE = "UpdateZnode";
+ public static final String UPLOAD_SEGMENT = "UploadSegment";
+ }
+
+ // Action names for table
+ public static class Table {
+ public static final String ASSIGN_INSTANCE = "AssignInstance";
+ public static final String BUILD_ROUTING = "BuildRouting";
+ public static final String CREATE_SCHEMA = "CreateSchema";
+ public static final String CREATE_TABLE = "CreateTable";
+ public static final String DELETE_PARTITION = "DeletePartition";
+ public static final String DELETE_ROUTING = "DeleteRouting";
+ public static final String DELETE_SCHEMA = "DeleteSchema";
+ public static final String DELETE_SEGMENT = "DeleteSegment";
+ public static final String DELETE_TABLE = "DeleteTable";
+ public static final String DELETE_TIME_BOUNDARY = "DeleteTimeBoundary";
+ // Used in /tables/{tableName} API with state as one of the parameters
+ public static final String DISABLE_TABLE = "DisableTable";
+ public static final String DOWNLOAD_SEGMENT = "DownloadSegment";
+ // Used in /tables/{tableName} API with state as one of the parameters
+ public static final String DROP_TABLE = "DropTable";
+ // Used in /tables/{tableName} API with state as one of the parameters
+ public static final String ENABLE_TABLE = "EnableTable";
+ public static final String FORCE_COMMIT = "ForceCommit";
+ public static final String GET_BROKER = "GetBroker";
+ public static final String GET_CONFIG = "GetConfig";
+ public static final String GET_CONSUMING_SEGMENTS =
"GetConsumingSegments"; // SK:
+ public static final String GET_CONTROLLER_JOBS = "GetControllerJobs"; //
SK:
+ public static final String GET_DEBUG_INFO = "GetDebugInfo";
+ public static final String GET_EXTERNAL_VIEW = "GetExternalView";
+ public static final String GET_IDEAL_STATE = "GetIdealState";
+ public static final String GET_INSTANCE = "GetInstance";
+ public static final String GET_METADATA = "GetMetadata";
+ public static final String GET_PARTITION = "GetPartition";
+ public static final String GET_PAUSE_STATUS = "GetPauseStatus";
+ public static final String GET_ROUTING_TABLE = "GetRoutingTable";
+ public static final String GET_SCHEMA = "GetSchema";
+ public static final String GET_SEGMENT = "GetSegment";
+ public static final String GET_SEGMENT_LINEAGE = "GetSegmentLineage";
+ public static final String GET_SEGMENT_MAP = "GetSegmentMap";
+ public static final String GET_SERVER_MAP = "GetServerMap";
+ public static final String GET_SIZE = "GetSize";
+ public static final String GET_STATE = "GetState";
+ public static final String GET_STORAGE_TIER = "GetStorageTier";
+ public static final String GET_TABLE_CONFIG = "GetTableConfig";
+ public static final String GET_TABLE_LEADER = "GetTableLeader";
+ public static final String GET_TIME_BOUNDARY = "GetTimeBoundary";
+ public static final String PAUSE_CONSUMPTION = "PauseConsumption";
+ public static final String QUERY_TABLE = "QueryTable";
+ public static final String REBALANCE_TABLE = "RebalanceTable";
+ public static final String REBUILD_BROKER_RESOURCE =
"RebuildBrokerResource";
+ public static final String REFRESH_ROUTING = "RefreshRouting";
+ public static final String RELOAD_SEGMENT = "ReloadSegment";
+ public static final String REPLACE_INSTANCE = "ReplaceInstance";
+ public static final String REPLACE_SEGMENT = "ReplaceSegment";
+ public static final String RESUME_CONSUMPTION = "ResumeConsumption";
+ public static final String UPDATE_CONFIG = "UpdateConfig";
+ public static final String UPDATE_SCHEMA = "UpdateSchema";
+ public static final String UPDATE_TABLE = "UpdateTable";
Review Comment:
When managing a table, we have 3 scenarios:
1. TableConfig
2. Schema
3. TableConfigs (TableConfig & schema)
We want to name them more explicitly, e.g.
`<verb><TableConfig|Schema|TableConfigs>`
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
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
