[controller] Manual store deletion only can be done by specific user group. #1199
Conversation
…er/server/AbstractRoute.java
| * @param method the operation (GET, POST, ...) to perform against the resource | ||
| * @return true if the client is admin | ||
| */ | ||
| boolean isAllowlistUsersForStoreDeletion(X509Certificate clientCert, String resource, String method); |
There was a problem hiding this comment.
Can we rename to hasAccessToDelete?
There was a problem hiding this comment.
Sure, we could do that, but wondering why this naming is better?
There was a problem hiding this comment.
It feels more concise to me, while conveying the same information
| /** | ||
| * Grant access if it's not required to check ACL. | ||
| * {@link accessController} will be empty if ACL is not enabled. | ||
| */ | ||
| return true; |
There was a problem hiding this comment.
We want to do the opposite thing, right? If ACL is not enabled, do not allow the store deletion at all.
Besides, it's cleaner to check whether ACL is enabled in AdminSparkServer - do not register this API if ACL is not enabled:
if (sslEnabled) { httpService.post(DELETE_STORE.getPath(), new VeniceParentControllerRegionStateHandler(admin, storesRoutes.deleteStore(admin))); }
The spike will be fixing the test cases; let's see how difficult that is. WDYT?
| // This is to limit the manual store deletion from admin tool without https to a specific allow list users. | ||
| if (!isSslEnabled() | ||
| && !checkIsAllowListUser(request, veniceResponse, () -> isAllowListUserForStoreDeletion(request))) { | ||
| return; |
There was a problem hiding this comment.
We should fail the request and send bad request error code to user.
There was a problem hiding this comment.
The checkedIsAllowListUser will mark the request as Bad Request:
protected boolean checkIsAllowListUser(
Request request,
ControllerResponse veniceResponse,
BooleanSupplier isAllowListUser) {
if (!isAllowListUser.getAsBoolean()) {
veniceResponse.setError(ACL_CHECK_FAILURE_WARN_MESSAGE_PREFIX + request.url());
veniceResponse.setErrorType(ErrorType.BAD_REQUEST);
return false;
}
return true;
}
Summary, imperative, start upper case, don't end with a period
This Code change adds a feature allowing manual store deletion from admin tool to a specific user group only. The newly added interface of
isAllowlistUsersForStoreDeletionforAccessControllerwill be used to check if user belong to that specific user group.How was this PR tested?
Does this PR introduce any user-facing changes?