Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3725705141
##########
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:
Good catch, both misclassifications are real. downloadSegment is a GET
protected with @Authenticate(AccessType.READ), but DownloadSegment falls
through to the default UPDATE branch here, and cancelRebalance is protected
with @Authenticate(AccessType.UPDATE) while the Cancel prefix maps it to
DELETE. I will fix it in both factories by adding a Download prefix to the READ
branch and adding an explicit case mapping CancelRebalance to UPDATE. The
generic Cancel prefix will keep mapping to DELETE because the CancelQuery
endpoints in PinotRunningQueryResource are annotated with
@Authenticate(AccessType.DELETE). I will also add test cases for
DownloadSegment and CancelRebalance in BasicAuthAccessControlFactoryTest.
##########
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:
Confirmed, this factory has the identical mapping and the identical problem.
I will apply the same fix here as described in my reply on the
BasicAuthAccessControlFactory comment: add a Download prefix to the READ branch
and an explicit CancelRebalance to UPDATE case, keeping the Cancel prefix
mapped to DELETE for CancelQuery.
--
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]
