LakshSingla commented on code in PR #12959:
URL: https://github.com/apache/druid/pull/12959#discussion_r954116699
##########
extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/BasicRoleBasedAuthorizer.java:
##########
@@ -106,8 +114,16 @@ public Access authorize(AuthenticationResult
authenticationResult, Resource reso
}
}
}
-
- return new Access(false);
+ LOG.info("User [%s] is not allowed to [%s] the resource [%s:%s]",
Review Comment:
```suggestion
LOG.info("User [%s] does not have [%s] permission for the resource
[%s:%s]",
```
nit: Making the log consistent with the error message
##########
extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/BasicRoleBasedAuthorizer.java:
##########
@@ -90,7 +94,11 @@ public Access authorize(AuthenticationResult
authenticationResult, Resource reso
Map<String, BasicAuthorizerRole> roleMap = roleProvider.getRoleMap(name);
if (roleNames.isEmpty()) {
- return new Access(false);
+ LOG.info("No roles found for authorizer [%s] and user [%s]", name,
authenticationResult.getIdentity());
+ return new Access(false, StringUtils.format("You don't have '%s'
permission for Resource [%s:%s]",
+ action.name(),
+ resource.getType(),
+ resource.getName()));
Review Comment:
Is it safe to expose the name of the resource to the end user?
If the error message when a resource doesn't exist versus when a resource
exists but the user is not authorized to access it is not the same, might lead
to the user knowing which resources are present in the cluster.
--
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]
