ctubbsii commented on a change in pull request #386: ACCUMULO-4799 removed
redundant auth check
URL: https://github.com/apache/accumulo/pull/386#discussion_r168306511
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java
##########
@@ -270,7 +270,7 @@ public Authorizations getUserAuthorizations(TCredentials
credentials) throws Thr
}
public boolean userHasAuthorizations(TCredentials credentials,
List<ByteBuffer> list) throws ThriftSecurityException {
- authenticate(credentials);
+ // Authentication check not done here because this method is always called
in conjunction with canScan that does auth check.
Review comment:
I think this is a bit risky... somebody could add some code internally which
uses this method without an authentication check (they might overlook the
comment). Maybe a method rename, like
`alreadyAuthenticatedUserHasAuthorizations` might make it more clear. It's a
bit wordy, but my concern is that this method will be misused. Maybe there's
another, simpler refactoring which would also avoid risky future misuse?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
