>-----Original Message----- >From: Chris Geer [mailto:[email protected]] >Sent: Thursday, October 04, 2012 11:32 AM >To: [email protected] >Subject: Re: [Proposal] Spring Permissions Change > >On Thu, Oct 4, 2012 at 8:22 AM, Carlucci, Tony <[email protected]> wrote: > >> >-----Original Message----- >> >From: Chris Geer [mailto:[email protected]] >> >Sent: Thursday, October 04, 2012 11:08 AM >> >To: [email protected] >> >Subject: Re: [Proposal] Spring Permissions Change >> > >> >Tony, >> > >> >On Thu, Oct 4, 2012 at 6:58 AM, Carlucci, Tony <[email protected]> >> 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 >> >attribute >> >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 >> >functions) >> > - 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. >> > >> >Chris >> >> 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. >> > >The only place the string would show up would be in the hasPermission >annotation (which is already a string) and in the PermissionEvaluator. In >the permission evaluators we could still define/use enums to define the >valid strings, the interface just wouldn't be bound to a single enum. Sound >reasonable? >
Yup, +1 Tony >> >> 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 >> >> Tony >> >> > >> >> >> >> Am I understanding your use case or completely off the mark? :) >> >> >> >> Tony >> >> >> >> >-----Original Message----- >> >> >From: Chris Geer [mailto:[email protected]] >> >> >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 >> >> >>
