ctubbsii commented on code in PR #5480:
URL: https://github.com/apache/accumulo/pull/5480#discussion_r2045687287
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java:
##########
@@ -401,14 +401,9 @@ public InitialMultiScan startMultiScan(TInfo tinfo,
TCredentials credentials,
}
}
- try {
- if (!security.authenticatedUserHasAuthorizations(credentials,
authorizations)) {
- throw new ThriftSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.BAD_AUTHORIZATIONS);
- }
- } catch (ThriftSecurityException tse) {
- log.error("{} is not authorized", credentials.getPrincipal(), tse);
- throw tse;
+ if (!security.authenticatedUserHasAuthorizations(credentials,
authorizations)) {
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.BAD_AUTHORIZATIONS);
}
Review Comment:
This code appears to do nothing but intercept the exception that it threw
itself, in order to log it. Even if we kept the log message, this try-catch
isn't needed.
In some places in the code this pattern can be seen because the ability to
request information from the security module itself can throw a
ThriftSecurityException. (See `SecurityOperation.canAskAboutOtherUsers` when
`authenticate` fails, for example). This is a bit convoluted and these need
reworked to be more clear, but that's out of scope here.
For this case, there's no reason to have this wrapping and the extra log
message, so this part is fine to remove.
In fact, there's another case where this happens that can also be removed:
in `ThriftScanClientHandler.getActiveScans` when calling
`TabletClientHandler.checkPermission`, the same thing is done.
Both logging and throwing are not necessary. In this case, we are throwing
the exception back to the client side, where it should be handled. So, there's
no reason to log extra things here, beyond what is logged in the
AuditedSecurityOperation already (and any internal errors that aren't the
user's fault).
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java:
##########
@@ -152,6 +152,9 @@ public boolean isValidAuthorizations(String user,
List<ByteBuffer> auths) {
for (ByteBuffer auth : auths) {
if (!userauths.contains(ByteBufferUtil.toBytes(auth))) {
+ log.info(
+ "User {} attempted to use Authorization {} which they do not have
assigned to them.",
+ user, new String(auth.array(), UTF_8));
Review Comment:
We're already throwing the exception back to the user, so there's no need to
also log it. Also, this is redundant, since the user should already have a copy
of their authorizations for the request via existing mechanisms (either from
their own knowledge of the client side application/configuration, or available
from the server-side audit logs), and can double-check them against the public
API to list the current user's authorizations.
Additionally, this extra log message adds risk that routine client requests
can fill the server-side logs by merely submitting an invalid request.
Given that the appropriate place to log this would be in the audit logs, and
the authorizations are in fact logged there already, I'm going to revert this
new log message. I think we can consider improving the audit logging with a
redesign of the security module internals, but we should not do that in a 2.1
bugfix release.
--
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]
