Copilot commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3717359334
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java:
##########
@@ -87,15 +88,58 @@ public boolean hasAccess(String tableName, AccessType
accessType, HttpHeaders ht
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders,
String endpointUrl) {
- if (getPrincipal(httpHeaders).isEmpty()) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (principal.isEmpty()) {
throw new NotAuthorizedException("Basic");
}
- return true;
+ return hasClusterAccess(principal.get(), Objects.toString(accessType));
}
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE ||
p.hasUnrestrictedTableAccess())
+ .isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType,
String targetId, String action) {
+ Optional<BasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p ->
p.hasTable(TableNameBuilder.extractRawTableName(targetId)) &&
hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(BasicAuthPrincipal principal,
String action) {
+ return principal.hasUnrestrictedTableAccess() &&
hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(BasicAuthPrincipal principal,
String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return
principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") ||
action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") ||
action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") ||
action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
The coarse-permission fallback in getAccessTypeForAction() misclassifies
some existing fine-grained actions. For example, Actions.Table.DOWNLOAD_SEGMENT
is a GET endpoint (PinotSegmentUploadDownloadRestletResource) but is currently
mapped to UPDATE because it starts with "Download"; and
Actions.Table.CANCEL_REBALANCE is protected with AccessType.UPDATE
(PinotTableRestletResource) but is currently mapped to DELETE due to the
"Cancel" prefix. This will cause fine-grained authorization to deny requests
that previously worked with coarse READ/UPDATE configs.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java:
##########
@@ -90,12 +90,54 @@ public boolean hasAccess(String tableName, AccessType
accessType, HttpHeaders ht
@Override
public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders)
+ .filter(p -> targetType == TargetType.TABLE ||
p.hasUnrestrictedTableAccess())
+ .isPresent();
}
@Override
public boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders,
String endpointUrl) {
- return getPrincipal(httpHeaders).isPresent();
+ return getPrincipal(httpHeaders).filter(p -> hasClusterAccess(p,
Objects.toString(accessType))).isPresent();
+ }
+
+ @Override
+ public boolean hasAccess(HttpHeaders httpHeaders, TargetType targetType,
String targetId, String action) {
+ Optional<ZkBasicAuthPrincipal> principal = getPrincipal(httpHeaders);
+ if (targetType == TargetType.TABLE) {
+ return principal
+ .filter(p ->
p.hasTable(TableNameBuilder.extractRawTableName(targetId)) &&
hasActionPermission(p, action))
+ .isPresent();
+ }
+ if (targetType == TargetType.CLUSTER) {
+ return principal.filter(p -> hasClusterAccess(p, action)).isPresent();
+ }
+ return false;
+ }
+
+ private static boolean hasClusterAccess(ZkBasicAuthPrincipal principal,
String action) {
+ return principal.hasUnrestrictedTableAccess() &&
hasActionPermission(principal, action);
+ }
+
+ private static boolean hasActionPermission(ZkBasicAuthPrincipal principal,
String action) {
+ if (action != null && principal.hasPermission(action)) {
+ return true;
+ }
+ return
principal.hasPermission(Objects.toString(getAccessTypeForAction(action)));
+ }
+
+ private static AccessType getAccessTypeForAction(String action) {
+ if (action == null || action.startsWith("Get") ||
action.startsWith("List") || action.startsWith("Query")
+ || action.startsWith("Debug") || action.startsWith("Estimate") ||
action.startsWith("Recommend")) {
+ return AccessType.READ;
+ }
+ if (action.startsWith("Create") || action.startsWith("Ingest") ||
action.startsWith("Commit")
+ || action.startsWith("Upload")) {
+ return AccessType.CREATE;
+ }
+ if (action.startsWith("Delete") || action.startsWith("Cancel")) {
+ return AccessType.DELETE;
+ }
+ return AccessType.UPDATE;
}
Review Comment:
Same issue as the static BasicAuth factory: the coarse-permission fallback
in getAccessTypeForAction() misclassifies some fine-grained actions used by
controller endpoints (e.g. DownloadSegment should behave like READ;
CancelRebalance is protected with AccessType.UPDATE but is mapped to DELETE via
the "Cancel" prefix). This can cause fine-grained auth to block requests even
when the coarse `@Authenticate` check would allow them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
