capistrant commented on code in PR #19552:
URL: https://github.com/apache/druid/pull/19552#discussion_r3482374497
##########
server/src/main/java/org/apache/druid/server/security/AuthorizationUtils.java:
##########
@@ -90,6 +96,13 @@ public static void verifyUnrestrictedAccessToDatasource(
ResourceAction resourceAction = createDatasourceResourceAction(datasource,
req);
AuthorizationResult authResult = authorizeResourceAction(req,
resourceAction, authorizerMapper);
if (!authResult.allowAccessWithNoRestriction()) {
+ emitAuthMetric(
+ authorizerMapper.getServiceEmitter(),
+ authenticationResultFromRequest(req),
+ resourceAction,
+ METRIC_ACCESS_DENIED,
+ authResult.getErrorMessage()
+ );
throw new ForbiddenException(authResult.getErrorMessage());
}
Review Comment:
I think the comment from Frank is right. Could we just do a quick check on
basic access and emit if basic access was granted but not if it wasn't,
avoiding the double emit in that latter case?
```suggestion
if (!authResult.allowAccessWithNoRestriction()) {
if (authResult.allowBasicAccess()) {
// Basic access was granted, but access was restricted by a policy.
// This is checked to avoid double emitting the access denied metric
if the basic access was denied,
// since the authorizeResourceAction method already emits the metric
in that case.
emitAuthMetric(
authorizerMapper.getServiceEmitter(),
authenticationResultFromRequest(req),
resourceAction,
METRIC_ACCESS_DENIED,
authResult.getErrorMessage()
);
}
throw new ForbiddenException(authResult.getErrorMessage());
}
```
--
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]
