accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] ctubbsii commented on a change in pull request #386: ACCUMULO-4799 removed redundant auth check
Date Thu, 01 Jan 1970 00:00:00 GMT
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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message