AccessControlUtil Review for Sling GSOC2013

AccessControlUtil Review for Sling GSOC2013

[Ian Boston, 2013-09-18 08:18:57]

Here is a review of the Security implementation, a great start but there are
some concepts that need to be switched.

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
File
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java
(right):

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:3:
import com.sun.org.apache.xerces.internal.impl.dv.util.Base64;
Avoid using com.sun classes as they are only available in the Sun JDK. Instead
use the Base64 classes from Apache commons codec.

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:51:
String policy="0_dishara_allow : " + READ;
I think you could save a lot of processing by putting the ACEs in JSON. eg
{
   "0_dishara_allow" : 1,
   "1_ieb_deny" : 7,
}

or as an ordered list.
[
   { 
     "principal" : "dishara",
     "grant" : true,
     "permission" : 1
   },     
   { 
     "principal" : "ieb",
     "grant" : false,
     "permission" : 7
   },     
]

or to detach from the bitmap and make it more readable

[
   { 
     "principal" : "dishara",
     "grant" : true,
     "permission" : [ "read" ]
   },     
   { 
     "principal" : "ieb",
     "grant" : false,
     "permissions" : [ "read", "write", "delete" ]
   },     
]

or if you are concerned about space, and dont mind some cyptic keys.

[
   { 
     "pr" : "dishara",
     "g" : true,
     "pe" : [ "r" ]
   },     
   { 
     "pr" : "ieb",
     "g" : false,
     "pe" : [ "r", "w", "d" ]
   },     
]


There are lots of ways of doing this in json and using json means you dont have
to invent a parser.

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:63:
private int getPrincipleFromACE(String ace) {
I think this should be getPrivilegeFromACE.  READ, WRITE, DELETE are privileges

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:71:
private boolean isUserHasPrinciple(int principle,int usePrinciples){
I think this impl might be wrong ?

Principals are things like ieb, dishara, I cant see how you could represent
those in an int.

I think it should be

boolean isUserHasPrincipal(String principal, Set<String> userPrincipals) {
   return userPrincipals.contains(principal);
}

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:82:
private int[] getCurrentLevelBitmaps(int userPrinciples, String currentPath)
throws Exception {
the int userPrinciples should be a Set<String> userPrincipals

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:90:
for (String ACE : aclArray) {
should be ace not ACE, capitals means constant.

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:91:
if (isUserHasPrinciple(getPrincipleFromACE(ACE), userPrinciples)){
This should be getPrincipalFromAce, but it should return the principal. eg ieb
or dishara

https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/o...
main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java:95:
toGrant = userPrinciples;
toGrant should be set to the privileges specified in the ACE.

toGrant = getPrivilegesFromAce(ACE)