roller-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Jencks <david_jen...@yahoo.com>
Subject Re: Proposal for some refactoring around security
Date Sun, 09 Mar 2008 18:56:16 GMT

On Mar 9, 2008, at 10:17 AM, Dave wrote:

> On Tue, Feb 19, 2008 at 4:42 AM, David Jencks  
> <david_jencks@yahoo.com> wrote:
>> i talked with Dave Johnson a bit about some of this at apachecon.
>
> David,
>
> I'm sorry to have taken so long to look at this patch. I really don't
> want to be an impediment to further development and I hope you'll
> continue to work on this.  I do value your input and your work and
> I'll try to respond more quickly to your patches in the future.

No problem...  unfortunately I've gotten involved in some other  
things and may not be able to update the patch for a few days.  I'll  
try to answer some of your questions meanwhile.
>
>
>>  Fundamentally I'm interested in Roller working with javaee security
>>  and a role-based access control framework.  It's quite clear this
>>  will require some additional capabilities in javaee security, but I
>>  think Roller can  be refactored to make this plausible, and that  
>> this
>>  refactoring will also make "stand-alone" roller security easier to
>>  understand and work with.
>>
>>  I've been working on this for a week or so and have some results  
>> that
>>  I think are reasonable and working.  I've opened ROLLER-1680  and
>>  attached a patch.  Working on the security code it looked to me  
>> as if
>>  there were a lot of bugs: I've fixed the ones I've noticed but
>>  haven't tried to track them individually.
>>
>>  I've had two main ideas here:
>>  - From the business layer, make all security decisions by  
>> checking if
>>  the current user has a particular permission
>>  - Abstract what is tracking the current user.
>
> These are all good goals and I agree with them.
>
>
>>  This results in a SecurityService with a method
>>    boolean checkPermission(RollerPermission perm, UserSource
>>  userSource);
>
> I don't understand why we need to pull this out into a separate
> interface. Why not just leave it in UserManager? That way, somebody
> only has to implement one interface to plugin their own
> User/Permissions manager.
>

My thinking, IIRC, is that the UserManager interface makes a number  
of specific assumptions about the relationship between users and  
permissions that may not hold with other security  backends such as a  
hierarchical rbac model.    With a separate SecurityService interface  
we can start to separate permission evaluation, which is pretty  
universal, with user provisioning, which may not be so universal.  I  
don't recall looking into what the actual dependencies between the  
front end and UserManager are so the prospect of using an entirely  
different user manager that does not map well to the UserManager  
interface may be harder than I dream of :-)

>
>>  UserSource is the abstraction of what is tracking the current user.
>>  Basically it attempts to avoid looking up the current User object
>>  unless it's really necessary.  For instance with a JACC based
>>  authorization system the security service would already know the
>>  current user from the container login and would not need to consult
>>  the UserSource.
>
> Sounds good, but you've got UserSource in the front-end and you've got
> back-end classes depending on it. UserSource should be in the
> org.apache.roller.weblogger.business package.

ok.... <harp on sore point> if you just had a reasonably structured  
maven build I would have realized this from the structure </harp on  
sore point>
>
>
>>  I've also separated storage of security information such as which
>>  users have which permissions from the Permission implementation
>>  itself.  The user administration code works with the data objects
>>  WeblogPermission and GlobalPermission which are no longer Permission
>>  objects, whereas the security code as we just saw works with
>>  RollerPermission, which is.
>>
>>  I've combined several bits of functionality into RollerPermission
>>  which is now the only Permission class needed.  Since I'm familiar
>>  with the code I borrowed the JACC 1.1 UserDataPermission class and
>>  simplified it by leaving out some functionality I'm pretty sure  
>> isn't
>>  needed.  It still has some capabilities that may or may not be  
>> useful
>>  and can probably be simplified further.
>
> This all looks good to me.
>
>
>>  Here's a brief description of what it can do now and what might be
>>  simplified:
>>
>>  - name.  This is adapted from the URLPattern handling of
>>  UserDataPermission.  We don't need exclusions so there's only one
>>  pattern, which acts like URL patterns in web security constraints.
>>  Currently global permissions get "/*" and permissions specific to a
>>  particular blog, say "foo", get "/foo".  This could be simplified a
>>  little bit more, but what is there now allows hierarchical
>>  categorization of blogs.  For instance one might organize blogs
>>  under /internal and /external: it would then be possible to give
>>  permissions to categories of blogs, say /internal/*.  I thought it
>>  would be worth asking if this sounded interesting before removing  
>> the
>>  code that lets you do this.
>
> From looking at the code, I cannot understand how "hierarchical
> categorization of blogs" works.

This is just an idea, supported by the current proposed permission  
implementation: it would require some support in the rest of roller  
to actually be usable.  The idea is that blog handles could be  
hierarchical like
/myco/mydept/myblog

Then you could give someone a permission for all myco blogs, or all  
myco/mydept blogs, or just the myco/mydept/myblog itself.

I started with a more-capable-than-needed permission implementation  
and removed stuff: a couple of features seemed like they might  
possibly be interesting so I thought I'd ask before hacking them out.
>
>
>>  - actions.  This is adapted from the HTTPMethod handling of
>>  UserDataPermission.  This is probably significantly more complicated
>>  that necessary, but my questions as to what is needed have so far
>>  gone unanswered.  The actions I've found in the existing code
>>  ("admin", "post", "editdraft", "weblog", "login") are represented in
>>  a bitmask.  Any additional actions are stored as strings.   
>> There's an
>>  "isExcluded" flag that indicates whether the set of actions
>>  explicitly listed (in the mask or as strings) is the set of granted
>>  actions or the set of denied actions.  Thus any finite set of  
>> actions
>>  or the complement of any finite set of actions can be  
>> represented.  I
>>  strongly suspect that there is a known finite set of actions so a
>>  bitmap would be sufficient.  I'm hoping someone can explain whether
>>  or not this is the case.
>
> I do not understand the need for a bitmask here. Why not store all
> actions as simple strings? This seem to overcomplicate things to an
> extent that I prefer the previous code.

I'm waiting for an answer to my question about whether all the  
possible actions are known right now :-)  If they are, I think the  
argument for a bitmap is stronger.  Having both a bitmap and a map  
for extensions is a leftover from the implementation I was  
simplifying, and I was hoping to remove the extensions when I found  
out how many actions there might be.

Reasons to use a bitmap:
- it makes it really easy to do stuff like having admin permission  
imply other permissions -- the bitmap for "admin" just has more than  
one bit set.
- its faster
- it helps with input validation if there is a known finite set of  
actions.

I think the first point is a strong argument for bitmaps, but don't  
really have a problem going to strings.

>
>
>>  Some of the actions are not independent.  For instance, admin  
>> implies
>>  post and editdraft.  Rather than requiring code to check these I've
>>  simply represented these in the masks for these permissions.
>>
>>  Open questions:
>>  - as already mentioned, I'd like to know what actions are possible.
>>  - I don't really understand the thinking behind the ORM for
>>  ObjectPermission.  It doesn't look to me as if GlobalPermissions can
>>  be persisted which I don't understand.  In any case I suspect this
>>  area might be possible to simplify.
>
> GlobalPermissions not persisted directly, they are implied by the
> roles that a user has.

I'll have to think some more about whether I think this is a  
reasonable model.

>
> See the original proposal for details:
> http://cwiki.apache.org/confluence/x/PfM
>
>
>
>>  Next steps
>>  With something like this patch  in place I could start looking at
>>  running roller with javaee security and a role-based access control
>>  system.  The obvious problem with  javaee security is that currently
>>  it doesn't really support security changes while the app is running
>>  very well.  For instance, adding a new users and permissions for  
>> that
>>  user is problematical, especially for content that isn't there until
>>  that new user generates it (their new blog, for instance).  Beyond
>>  this, I think RBAC will provide some interesting capabilities that
>>  are currently lacking.  The basic idea is to, starting with a
>>  directed acyclic graph of roles,  assign permissions to roles rather
>>  than users, and assign users to roles.  For instance you might have
>>  an author role specific to a particular department,
>>  "DevelopmentPoster".  You could have a bunch of blogs with post
>>  permissions assigned to that role.  Then any user assigned to that
>>  role could post to all of these blogs.
>
>>  Any comments are welcome.  Aside from running (and adding to) the
>>  unit tests which I eventually discovered in the ant build despite
>>  their lack of documentation using -p, I've tested this with  the
>>  geronimo roller plugin.  I'm not a roller expert but everything I've
>>  tried seems to have the same behavior as with plain roller.
>
>
> And some general comments:
>
> The mix of bug fixes and architectural changes is a little difficult
> to parse, can we separate those out?

I doubt it -- most of the bug fixes are a consequence of the new  
permission implementation.
>
> There are a fair number of formatting changes which make it difficult
> to evaluate the patch. Can we limit the patch to substantive changes
> only and leave formatting out of the patch?

I'll see what I can come up with.  I already did remove some of the  
formatting changes -- I'm afraid a lot of changes happened when I  
made a bunch of changes and told my IDE to reformat the whole class.
>
>
> For the reasons  above, I don't feel comfortable about committing this
> patch. Generally, the changes here are complex and sweeping enough to
> warrant a full proposal along the lines of
> http://cwiki.apache.org/confluence/x/PfM.

OK, I'll see what I can come up with.  Your thoughts on whether  
hierarchical blog names have any value and on whether the set of  
actions is known and finite would help in simplifying the next  
version of the patch.

Many thanks for the review!
david jencks

>
> - Dave


Mime
View raw message