Hi, You need to publish the comments before anyone else can see them, thats why they say draft on them.
I'll reply here, but if you could publish them it would keep the thread going on code review. On 21 September 2013 02:53, Dishara Wijewardana <[email protected]> wrote: > Hi Ian, > I could see them when I logged in. Did you checked whether you logged in to > the google project? When I was not logged in I was not able to see my > comments also. > > But in case if it is still not viewable, I am copying the 3 comments I > added. > > * > Ian Boston* 2013/09/18 08:18:57 > I think this should be getPrivilegeFromACE. READ, WRITE, DELETE are > privileges > *Reply* *Done* > *(Draft)* 2013/09/18 18:36:19 > As per explanation of yours in the mail, read=0x01,write=0x02 and etc are > categorized under Principals. Can you confirm that? no. read, write, delete are categorised as privileges. ieb, dishara are categorised as principals. On 2013/09/18 08:18:57, > Ian Boston wrote: > Show quoted text > *Edit > > * > *Ian Boston* 2013/09/18 08:18:57 > 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); } > *Reply* *Done* > *(Draft)* 2013/09/18 18:43:48 > According to the mail ACE is something like > <order>_<principal>_<allow|deny> where i.e 0_everyone_allow : 0x01. As per > you, user name is the "principle" ? Initially I did it in that way, but I > thought principle is read, write etc as per last comment of mine, and hence > the values of it can be represent in a integer bitmap. But if READ,WRITE > are priviledges my impl is wrong. Can you please confirm what I think is > correct or not ? If the ACE is 0_everyone_allow: 0x01 then the fields are <order>_<principal>_<grant|deny> : <privileges> On 2013/09/18 08:18:57, Ian Boston wrote: > Show quoted text > *Edit > > * > *Ian Boston* 2013/09/18 08:18:57 > the int userPrinciples should be a Set<String> userPrincipals > *Reply* *Done* > *(Draft)* 2013/09/19 21:29:51 > Can you provide an example set of principals. According to me, it can be > the values of i.e read=0x01, write=0x02 so the set should be <0x01> <0x02> > or a single integer 0x03 which represents above. But as per you I am wrong. Principals identify a user or a group, and every user or group has a principal. eg ieb is the Principal that identifies the user ieb. everyone is the Principal that identifies the group everyone, containing all users. anonymous is the Principal that identifies the anonymous user who has not logged in. The concept of principal is widely used and in java is frequently implemented extending the interface java.lang.Principal. If you go into Eclipse and show Type Hierarchy you will see some examples, including the Jackrabbit User and Group implementations. In the ACEs we just need the name of the principal to identify who the ACE applies to. Finally, in addition to a User and Group being identified by a Principal, they will have Principals by virtue of the fact they are members of other groups. A user will always have the principal "everyone" since they are part of the group "everyone". HTH Best Regards Ian > So please correct me. On 2013/09/18 08:18:57, Ian Boston wrote: > Show quoted text > *Edit > > > ** > > * > > > On Fri, Sep 20, 2013 at 4:44 PM, Ian Boston <[email protected]> wrote: > >> Hi, >> >> Did you publish your replies to my comments at [1]. I cant see >> anything other than what I said. Be sure to click on each comment to >> exand it to full size. >> >> Ian >> >> 1 >> https://codereview.appspot.com/13396052/diff/1/main/cassandra/src/main/java/org/apache/sling/cassandra/resource/provider/security/AccessControlUtil.java >> >> On 19 September 2013 22:38, Dishara Wijewardana <[email protected]> >> wrote: >> > Hi Ian, >> > If you find some time, appreciate if you can add some reply to the >> comments >> > on the code review basically regarding the issues you raised in the >> > algorithm and the conflict of principals and privileges as per my >> > understanding. I will cleanup the code with your clarification in this >> > weekend. >> > >> > >> > On Thu, Sep 19, 2013 at 12:16 AM, Dishara Wijewardana < >> > [email protected]> wrote: >> > >> >> Hi Ian, >> >> Thank you very much for the valuable feedbacks as always. I added some >> >> comments to clarify couple of cases. Appreciate your response, so that I >> >> can proceed with the improvements. >> >> >> >> >> >> On Wed, Sep 18, 2013 at 1:19 PM, Ian Boston <[email protected]> wrote: >> >> >> >>> Hi Dishara, >> >>> Looking good, I have some comments, so I did a code review at [1] >> >>> Ian >> >>> >> >>> 1 https://codereview.appspot.com/13396052/ >> >>> >> >>> >> >>> On 18 September 2013 04:13, Dishara Wijewardana < >> [email protected]> >> >>> wrote: >> >>> > Hi Ian, >> >>> > Sorry for the delay of updating the thread. I had to to some >> >>> > experiment(writing dummy tests iteratively) to figure out what you >> >>> exactly >> >>> > meant. And finally was able to implement what you said. I have >> commited >> >>> the >> >>> > src under a new package called "security". Currently it is a util >> class >> >>> > which is executable and we can test. >> >>> > >> >>> > Now i.e following can be done and returns accurate results. >> >>> > >> >>> > String path = "/content/cassandra/p2/c2"; >> >>> > String policy="0_dishara_allow :0x01 "; >> >>> > >> >>> > accessControlUtil.addACE(path,policy); >> >>> > >> >>> > int results[] = >> accessControlUtil.buildAtLevel(0x04,path); >> >>> > System.out.println("GRANT" +results[0]); >> >>> > System.out.println("DENY" +results[1]); >> >>> > >> >>> > >> >>> > >> >>> > >> >>> > >> >>> > On Mon, Sep 16, 2013 at 2:28 PM, Ian Boston <[email protected]> wrote: >> >>> > >> >>> >> Hi, >> >>> >> >> >>> >> Yes you could store the ACL (ordered list of ACE's) with the >> resource >> >>> >> itself, although you will then have to add additional code to >> protect >> >>> >> access to that property which will complicate the CassandraProvider, >> >>> >> which is why I was sugesting that you do ACL storage in a completely >> >>> >> separate Column Family. >> >>> >> >> >>> >> If you choose to do it that way (storing ACLs with the resource), >> then >> >>> >> you might consider storing the ACL as a JSON string. >> >>> >> >> >>> >> By default, permissions inherit from the permissions on the parent >> >>> >> resource, so there is no need to create an ACL on creation, but the >> >>> >> implementation of the ResourceAccessGate will need compute access >> >>> >> control including parent ACLs. >> >>> >> >> >>> >> How you implement ACL crud is entirely upto you. I suggest you use >> >>> >> Hector directly to do this. >> >>> >> >> >>> >> This will make your CassandraResourceAccessGate completely separate >> >>> >> from you CassandraResourceProvider. >> >>> >> >> >>> >> Best Regards >> >>> >> Ian >> >>> >> >> >>> >> On 13 September 2013 13:21, Dishara Wijewardana < >> >>> [email protected]> >> >>> >> wrote: >> >>> >> > On Thu, Sep 12, 2013 at 8:37 PM, Ian Boston <[email protected]> >> wrote: >> >>> >> > >> >>> >> >> Hi >> >>> >> >> >> >>> >> >> On 12 September 2013 13:24, Dishara Wijewardana < >> >>> >> [email protected]> >> >>> >> >> wrote: >> >>> >> >> > Hi Ian >> >>> >> >> > >> >>> >> >> > >> >>> >> >> > On Thu, Sep 12, 2013 at 1:50 PM, Ian Boston <[email protected]> >> >>> wrote: >> >>> >> >> > >> >>> >> >> >> Hi Dishara, >> >>> >> >> >> To make the Cassandra Resource Provider really useful I think >> we >> >>> need >> >>> >> >> >> to add access control. I think the best way of doing this is >> to >> >>> >> borrow >> >>> >> >> >> some concepts from Jackrabbit access control. >> >>> >> >> >> >> >>> >> >> >> >> >>> >> >> > The following algorithms, and etc does sling already have any >> >>> >> >> > implementation of it. If so I can reuse them. Since sling has >> few >> >>> >> >> providers >> >>> >> >> > I believe they probably have some common interface. >> >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> There is not, or at least, not in a way that is exposed. There >> is an >> >>> >> >> implementation inside Jackrabbit, but it is tightly coupled to >> >>> >> >> Jackrabbit and more complex than the simple system below. >> >>> >> >> >> >>> >> >> Once implemented, this access control system may be exposed as a >> >>> >> >> ResourceAccessGate, but since (IIRC) imposing anything other than >> >>> read >> >>> >> >> access control has not been done, the CassandraResourceProvider >> may >> >>> >> >> use the implementation directly to impose write and delete. >> >>> >> >> >> >>> >> >> > >> >>> >> >> > >> >>> >> >> >> Take a deep breath, and you will see why I left this till >> last. >> >>> >> >> >> >> >>> >> >> >> I think we should provide path base access control, which >> >>> inherits >> >>> >> >> >> from parent resources in the path. At every level there is a >> an >> >>> >> >> >> ordered list of access control entries each access control >> entry >> >>> >> (ACE) >> >>> >> >> >> being either an allow entry or a deny entry. What is allowed >> or >> >>> >> denied >> >>> >> >> >> is defined in a 32bit bitmap with each bit representing 1 >> >>> permission, >> >>> >> >> >> so we can have upto 32 permissions. Each ACE specifies a >> single >> >>> >> >> >> principal. So an ACL consists of a ordered list of ACE's each >> one >> >>> >> >> >> bound to a principal. >> >>> >> >> >> >> >>> >> >> >> A user has a set of principals, so to resolve the ACL at any >> one >> >>> path >> >>> >> >> >> for a user the global ACL is filtered to contain only the >> ACE's >> >>> with >> >>> >> >> >> principals that the user has. >> >>> >> >> >> >> >>> >> >> >> Computing a final access control bitmap for a user at a >> location >> >>> >> >> >> requires ordered processing of all the ACEs relevant to the >> user >> >>> at >> >>> >> >> >> the current path and then all ancestors. >> >>> >> >> >> >> >>> >> >> >> The pseudo algorithm to calculate the a grant bitmap and a >> deny >> >>> >> bitmap >> >>> >> >> >> at any level is: >> >>> >> >> >> >> >>> >> >> >> function getCurrentLevelBitmaps(currentPath): >> >>> >> >> >> int grants = 0; >> >>> >> >> >> int denies = 0; >> >>> >> >> >> for all ACEs in the ACL at the currentPath: >> >>> >> >> >> if the user has the principal of the current ACE: >> >>> >> >> >> int toGrant = 0; >> >>> >> >> >> int toDeny = 0; >> >>> >> >> >> if the ACE is a grant: >> >>> >> >> >> toGrant = the ACE bitmap; >> >>> >> >> >> else: >> >>> >> >> >> toDeny = the ACE bitmap; >> >>> >> >> >> toGrant = toGrant & ~denies; >> >>> >> >> >> toDeny = toDeny & ~grants; >> >>> >> >> >> grants = grants | toGrant; >> >>> >> >> >> denied = denies | toDenies; >> >>> >> >> >> return (grants, denies); >> >>> >> >> >> >> >>> >> >> > >> >>> >> >> > - Can you please tell me how to calculate the ACE bitmap ? >> >>> >> >> >> >>> >> >> That is just the 32bit integer stored in the cassandra column >> >>> >> >> representing the grant or deny for the principal >> >>> >> >> >> >>> >> >> eg >> >>> >> >> Cassandra rowID : base64(sha1(/content/cassandra/foo/bar )) >> >>> >> >> Columns: 0_everyone_allow : 0x01, 1_admin_allow : 0x07 >> >>> >> >> >> >>> >> >> Allows everyone read and admin everything at the path /cassandra >> >>> >> >> >> >>> >> >> > - Also I will be more clear if you can provide a sample value >> for >> >>> the >> >>> >> >> input >> >>> >> >> > and output of this function ? i.e When currentPath= >> >>> >> >> > /content/cassandra/foo/bar it returns grants=? denies=? some >> >>> actual >> >>> >> >> values >> >>> >> >> > just for my understanding. >> >>> >> >> >> >>> >> >> Using the above for a random user (ie has the everyone >> principal): >> >>> >> >> grants = 0x01 >> >>> >> >> denies - 0x0 >> >>> >> >> >> >>> >> >> For the following values >> >>> >> >> Cassandra rowID : base64(sha1(/content/cassandra/foo/bar )) >> >>> >> >> Columns: 0_everyone_allow : 0x01, 1_admin_allow : 0x07, >> 2_ieb_deny : >> >>> >> 0x01 >> >>> >> >> >> >>> >> >> results in >> >>> >> >> dishara : grants = 0x01, denies = 0x00 >> >>> >> >> ieb : grants = 0x00, denies = 0x01 >> >>> >> >> admin: grants = 0x07, denies = 0x00 >> >>> >> >> >> >>> >> >> >> >>> >> >> >> >>> >> >> > >> >>> >> >> > >> >>> >> >> >> To combine what is granted at the child level with what is >> >>> granted at >> >>> >> >> >> a parent level we need to mask the parent level with the deny >> at >> >>> the >> >>> >> >> >> child level. >> >>> >> >> >> >> >>> >> >> >> eg >> >>> >> >> >> toGrant = grantedAtParent & ~denies; >> >>> >> >> >> toDeny = deniedAtParent & ~grants; >> >>> >> >> >> grants = grants | toGrant; >> >>> >> >> >> denied = denies | toDenies; >> >>> >> >> >> >> >>> >> >> >> The simplest way of achieving this is to use recursion again >> in >> >>> >> pseudo >> >>> >> >> >> code: >> >>> >> >> >> >> >>> >> >> >> function buildAtLevel(): >> >>> >> >> >> if not root level: >> >>> >> >> >> (grantedAtParent, deniedAtParent) = >> >>> >> >> >> buildAtLevel(getParentLevel(currentLevel)); >> >>> >> >> >> (grants, denies) = >> >>> getCurrentLevelBitmaps(currentLevel); >> >>> >> >> >> toGrant = grantedAtParent & ~denies; >> >>> >> >> >> toDeny = deniedAtParent & ~grants; >> >>> >> >> >> grants = grants | toGrant; >> >>> >> >> >> denied = denies | toDenies; >> >>> >> >> >> return (grants, denied); >> >>> >> >> >> >> >>> >> >> >> >> >>> >> >> >> There are some optimisations you can apply here, and there are >> >>> plenty >> >>> >> >> >> of opportunities to cache intermediate bitmaps in memory. Just >> >>> >> caching >> >>> >> >> >> the ACL reduces resolution to bitwise operations. >> >>> >> >> >> >> >>> >> >> >> Principals >> >>> >> >> >> ---------------- >> >>> >> >> >> Initially keep it simple. >> >>> >> >> >> >> >>> >> >> >> read = 0x01 >> >>> >> >> >> write = 0x02 >> >>> >> >> >> delete = 0x04 >> >>> >> >> >> >> >>> >> >> >> Storage of ACLs. >> >>> >> >> >> ------------------------- >> >>> >> >> >> I suggest you store ACLs in their own Column Family, where the >> >>> rowID >> >>> >> is >> >>> >> >> >> base64(sha1(path)) or whatever path -> rowid encoding you have >> >>> >> >> currently. >> >>> >> >> >> >> >>> >> >> >> IIRC Cassandra columns come out in the natural order of >> Strings >> >>> >> >> >> <order>_<principal>_<allow|deny> and the value is the bitmap >> of >> >>> >> >> >> permissions. >> >>> >> >> >> >> >>> >> >> >> - If I understand you correctly is >> >>> >> <order>_<principal>_<allow|deny> is >> >>> >> >> > one ACE ? >> >>> >> >> >> >>> >> >> yes >> >>> >> >> >> >>> >> >> If per row there can be a ACL, there should be one additional >> >>> >> >> > column by default called "ACL" and it will have a comma >> separated >> >>> >> string >> >>> >> >> > which are set of ACEs. >> >>> >> >> >> >>> >> >> not necessary as the order of columns is returned in the natural >> >>> >> >> string sort order (IIRC) so 0_* will be the first followed by 1_ >> >>> >> >> >> >>> >> >> However, if thats not the case a column called ACL will be >> required >> >>> to >> >>> >> >> list the ACLs in order. >> >>> >> >> >> >>> >> >> Correct me if I am wrong. >> >>> >> >> > - Who stores these ACEs ? any API? >> >>> >> >> >> >>> >> >> Not determined yet. It would need an API able to modify ACLs >> >>> >> >> >> >>> >> > >> >>> >> > OK, I am trying to understand. Can you please clarify following. >> >>> >> > >> >>> >> > To start with, as I feel I should start storing data to cassandra >> >>> with an >> >>> >> > updated column which stores a string which is a comma separated >> ACE >> >>> list. >> >>> >> > i.e when the provider stores a resource at >> >>> /content/cassandra/foo/bar >> >>> >> the >> >>> >> > ACL column can be empty. Does permissions added upon node >> creation ? >> >>> Or >> >>> >> > there is some interface,and when it get called, (path,ACE-list) I >> >>> will >> >>> >> > update the row with the new value for the ACL column. If that is >> the >> >>> case >> >>> >> > permission should be granted to a path only of the path exists. OR >> >>> upon >> >>> >> > node creation does it suppose to store parent resolved ACE list >> by >> >>> >> > default. If that is so it will be more complex for Provider and >> have >> >>> to >> >>> >> > change that to do so. >> >>> >> > >> >>> >> > >> >>> >> > >> >>> >> >> > - i.e <order> is a auto increment number we have to do a >> >>> additional >> >>> >> read >> >>> >> >> > before storing ACE to check what is the last number for >> <order>. >> >>> >> >> >> >>> >> >> >> >>> >> >> Ok, that indicates having an ACE column might be better, which >> would >> >>> >> >> indicate that no count is required. >> >>> >> >> >> >>> >> >> Best Regards >> >>> >> >> Ian >> >>> >> >> >> >>> >> >> > >> >>> >> >> > Where <order> is 000 to 999 ( I really doubt that a single ACL >> >>> will >> >>> >> >> >> have 1000 ACEs ever) >> >>> >> >> >> >> >>> >> >> >> Once you have this working, we can wire it into the >> >>> ResourceProvider >> >>> >> >> >> or another Sling API. >> >>> >> >> >> >> >>> >> >> >> Does that make sense ? >> >>> >> >> >> Ian >> >>> >> >> >> >> >>> >> >> > >> >>> >> >> > >> >>> >> >> > >> >>> >> >> > -- >> >>> >> >> > Thanks >> >>> >> >> > /Dishara >> >>> >> >> >> >>> >> > >> >>> >> > >> >>> >> > >> >>> >> > -- >> >>> >> > Thanks >> >>> >> > /Dishara >> >>> >> >> >>> > >> >>> > >> >>> > >> >>> > -- >> >>> > Thanks >> >>> > /Dishara >> >>> >> >> >> >> >> >> >> >> -- >> >> Thanks >> >> /Dishara >> >> >> > >> > >> > >> > -- >> > Thanks >> > /Dishara >> > > > > -- > Thanks > /Dishara
