Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3755549903
##########
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:
Fixed in `552a472`. Rather than patching the two copies separately I pulled
the mapping into a single `AccessControlUtils.getAccessTypeForAction` that both
factories call, so they cannot drift apart later.
`Download` maps to READ now, and `CancelRebalance` is an exact match
returning UPDATE evaluated before the `Cancel` prefix, which stays DELETE so
`CancelQuery` is unaffected.
While auditing the rest of `Actions` against the endpoint annotations I
found `Validate` has the same defect you flagged: `ValidateSchema` and
`ValidateTableConfigs` are READ endpoints but the prefix fell through to the
default UPDATE branch, so that moved to READ as well. Tests cover all of these
for both factories, plus null still mapping to READ.
##########
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 fix, see the reply on the other thread. Both factories now delegate to
one shared `AccessControlUtils.getAccessTypeForAction`, so the two copies
cannot drift.
--
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]
