rave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Carlucci, Tony" <acarlu...@mitre.org>
Subject RE: [Proposal] Spring Permissions Change
Date Thu, 04 Oct 2012 15:22:40 GMT
>-----Original Message-----
>From: Chris Geer [mailto:chris@cxtsoftware.com]
>Sent: Thursday, October 04, 2012 11:08 AM
>To: dev@rave.apache.org
>Subject: Re: [Proposal] Spring Permissions Change
>On Thu, Oct 4, 2012 at 6:58 AM, Carlucci, Tony <acarlucci@mitre.org> wrote:
>> Hi Chris, could you go a little more into your use case (I think what
>> you've hinted at it with your Widget->add_comment block)?  I believe the
>> spirit of that Permission enum was to define the context of the security
>> check to keep in line with CRUD actions.  The detailed business logic of
>> the Model/Permission Context combination can then be customized as needed
>> in the various Default<Model>PagePermissionEvaluator.hasPermission
>> functions.  So if there is some specific security logic related to adding a
>> comment to a widget, I believe you can put it in the appropriate
>> PermissionEvaluator class.
>I understand the current model and I think it works great for top level
>objects but it doesn't work all so well for subordinate objects, or for
>business logic checks that are beyond CRUD. Right now everything is a top
>level object (everything has it's own repository for example) but as part
>of the object model restructure we have proposed to change that slightly.
>If you view WidgetComment as a subordinate object to a Widget, the security
>checks are different. Instead of checking WidgetComment => "Create" as a
>standalone check, you really want to check Widget => "can_add_comment"
>which is at the Widget level since the Comment doesn't exist yet. This
>check would check to make sure the Widget is published, that the user has
>access to the Widget, etc. Once the WidgetComment exists, the current
>checks in place make sense (mostly).
>I know currently we could just check the widget in the
>WidgetCommentPermissionEvaluator because the WidgetComment has an
>of "widget_id" but that is another thing we are proposing to change in the
>object model restructure. As we try and restructure things so that we can
>support backends other than JPA we need to tweak the object model at the
>interface level. For example, WidgetComment would no longer have an
>attribute of widget_id, it is just associated with whatever widget it's
>part of. This cleans up a few things like being able to create a
>WidgetComment with a widget_id of 3 but adding it to the WidgetComment
>collection of Widget id 2.
>Does that make sense?
>Beyond the WidgetComment example I still think there is a need for more
>fine grained permission checks. For example:
> - can_publish_widget
> - can_reset_other_users_password (low level admin who can't do some other
> - can_delete_other_users_comment (like a moderator)
> - ...
>I know those functions can be covered by "admin" but I know our product
>needs a finer level of control than just "admin". This will become much
>more important as we start talking multi-tenancy which I'll bring up again
>soon where you need multiple levels of admins.

Ok yes, this definitely makes sense to me given the context of the model refactoring changes.
 I think changing the enum to strings should be fine, so long as we don't see hard-coded "can_add_comment"
strings in lots of spots, which could make maintenance a little difficult.

Also we should keep in mind the current ability to override the default security behaviors[1]
as part of this change, and make sure only the most common of changes should go into the Rave
code base.

[1] http://rave.apache.org/documentation/model-permission-override.html


>> Am I understanding your use case or completely off the mark? :)
>> Tony
>> >-----Original Message-----
>> >From: Chris Geer [mailto:chris@cxtsoftware.com]
>> >Sent: Tuesday, October 02, 2012 7:50 PM
>> >To: dev
>> >Subject: [Proposal] Spring Permissions Change
>> >
>> >I would like to propose we change how the spring permission checks work
>> >slightly. Right now the "Permission" value (i.e. Create, Update...) is
>> >defined as part of a enum named Permission defined in
>> >the org.apache.rave.portal.security.ModelPermissionEvaluator interface.
>> The
>> >various hasPermissions methods take an instance of that Permission enum
>> >(created from a string on the check permission annotation). Having the
>> >permissions defined in an enumeration limits what we are able to check
>> >permissions for in my opinion. Right now we have two choices, 1) limit our
>> >permission checks to the list there is now, 2) add new permissions to the
>> >generic Permissions enum which could lead to a bunch of permissions stored
>> >on a generic enum that aren't really reusable (i.e. Widget -> add_comment
>> >permission). I would like to propose we change the way we define
>> >permissions to remove the enum and just pass along the string defined in
>> >the annotations. The only real downside of that is that we can't use a
>> >switch/case statement during permissions checks unless we use Java7.
>> >
>> >Thoughts/concerns?
>> >
>> >Thanks,
>> >Chris

View raw message