You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/07/19 17:48:09 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11136: WIP: Returning 403 status code in case of authorization failures
Jackie-Jiang commented on code in PR #11136:
URL: https://github.com/apache/pinot/pull/11136#discussion_r1268436162
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -390,7 +396,9 @@ public String sendPostRaw(String urlStr, String requestStr, Map<String, String>
/*if (LOG.isInfoEnabled()){
LOGGER.info("The http response code is " + responseCode);
}*/
- if (responseCode != HttpURLConnection.HTTP_OK) {
+ if (responseCode == HttpURLConnection.HTTP_FORBIDDEN) {
Review Comment:
Should we first check response code before getting the output stream?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -385,8 +387,7 @@ private BrokerResponseNative handleRequest(long requestId, String query,
if (!hasTableAccess) {
_brokerMetrics.addMeteredTableValue(tableName, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
LOGGER.info("Access denied for request {}: {}, table: {}", requestId, query, tableName);
- requestContext.setErrorCode(QueryException.ACCESS_DENIED_ERROR_CODE);
Review Comment:
Please check all usage of `hasAccess()`, seems there are other places also need to be handled
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -121,6 +121,8 @@ public void processSqlQueryGet(@ApiParam(value = "Query", required = true) @Quer
}
BrokerResponse brokerResponse = executeSqlQuery(requestJson, makeHttpIdentity(requestContext), true);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (WebApplicationException wae) {
Review Comment:
Put some error log? Also do we have a metric tracking the auth failure?
Same for other places
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -385,8 +387,7 @@ private BrokerResponseNative handleRequest(long requestId, String query,
if (!hasTableAccess) {
_brokerMetrics.addMeteredTableValue(tableName, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
LOGGER.info("Access denied for request {}: {}, table: {}", requestId, query, tableName);
- requestContext.setErrorCode(QueryException.ACCESS_DENIED_ERROR_CODE);
Review Comment:
We should still set the request context
--
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-unsubscribe@pinot.apache.org
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