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: us...@infra.apache.org With regards, Apache Git Services