Mailing-List: contact dev-help@roller.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@roller.apache.org
Received-SPF: pass (athena.apache.org: domain of snoopdave@gmail.com
 designates 66.249.82.234 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references;
        b=JxlnXJc0+gDb+jErPUAJOfuc7b9+XthYcObhfw6v1cQlEyDXjM/CkwdI/jyx0mo1qE1VN5xeO2cBvZt+vtwYMyMV8To89Zpq6RoKlEFZjSniwjE4q/1+fW422v4ZFmgUxvVQKoKexL4a8/1wxGsMJru2gi6HSbLXrDjzfAv8n0o=
Message-ID: <8fb9ac720803091017s1e3787f1j657675b9e78c908e@mail.gmail.com>
Date: Sun, 9 Mar 2008 13:17:22 -0400
From: Dave <snoopdave@gmail.com>
To: dev@roller.apache.org
Subject: Re: Proposal for some refactoring around security
In-Reply-To: <95E96BF7-D0DF-43C1-AD57-FC91FD4A8D17@yahoo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <95E96BF7-D0DF-43C1-AD57-FC91FD4A8D17@yahoo.com>

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.


>  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.


>  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.


>  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.


>  - 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.


>  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.

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?

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?


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.

- Dave