Hi Ian,
I changed the logic to correct with my misunderstandings as suggested and
refactored all code to use apache commons Base64 for encoding and rest of
the code review suggestions. And it works. Hence I commited the code. And
also upload the code to the JIRA at this milestone (without json parser).
I also spent more time trying to move the existing logics to json parser
since there are several places that I need to change and need to make sure
my working impl not break from this refactor. I am new to json. I used
com.google.gson.*;
{"key1":{"principal":"dishara","grant":"false","permission":"1"},"key2":{"principal":"dishara2","grant":"false","permission":"7"}}
Here when I add a ACE, I have to add a property along with the json child
object. i.e key1. So above is the json file of a ACL from the code what I
tried. But in this case, when try to update a ACL with a new ACE, we have
to provide a unique key always. I believe there will be a better ways of
doing this. Does sling has a JSON parser ? I saw only JSONObject.
On Sat, Sep 21, 2013 at 10:16 AM, Ian Boston <[email protected]> wrote:
> 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
>
--
Thanks
/Dishara
