comments inline ...
David Jencks wrote:
On Mar 9, 2008, at 10:17 AM, Dave wrote:
On Tue, Feb 19, 2008 at 4:42 AM, David Jencks <[EMAIL PROTECTED]>
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 :-)
I'm with Dave here, this sounds more confusing and I don't understand
the need. I am all for a security model that is relatively basic and
abstract enough that it works with standard security models, but I'm not
in favor of over complicating things to support all security models.
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>
this can be considered further, but ...
<harp on maven>I have *never* had a maven project which easily resolved
all dependencies without causing errors and headache. *Never*. If
there is a way to make the project maven without the chance for these
headaches then I am willing to listen.</harp on maven>
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.
I'm not sure I see how this would work in the current code. We have
absolutely no notion of weblogs belonging to other weblogs, so that
would likely just cause problems.
Its an interesting idea, but I suggest we separate it out from the
initial work of this proposal, which is just to cleanup the UserManager
and make it possible to externalize user management.
- 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.
I commented on this in my previous email. At the end of the day I don't
want to consider the set of actions to be fixed. I want them to be
dynamic so that application plugins can add new actions easily.
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.
I actually do not see the purpose of roles. A role is really the same
thing as an action that implies other actions. If we allow actions to
imply other actions then there is no reason to use the term "roles" or
do any special handling for them.
So from Dave's original proposal you can take his definition of roles
and simply use the word actions instead ...
role.actions.editor=login,comment,createWeblog
... becomes ...
permission.actions.editor=login,comment,createWeblog
so that if a user has the "editor" permission that is equivalent to
having the "login,comment,createWeblog" permission.
there is no real need to store this or refer to this as something other
than an action though, as long as we allow an action to imply other actions.
-- Allen
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
