On Tue, 7 Jul 2009 17:00:05 -0500, Kenny Ortmann <[email protected]> wrote: > I also think the validations should be on the model. I was simply trying > to > state that any action can be narrowed down to a CRUD type. >
You are right, but sometimes you want to forbid an action and authorize another action, both with the same crud type (I will have that issue in my current project later). Adding another authorized_for method we can forbid all actions with a crud type (all update actions), and after authorize an action with that crud type too. > Hernan's 3rd point is a very strong reason as to why it shouldn't be moved > entirely to the model. > > ~Kenny > > On Tue, Jul 7, 2009 at 4:36 PM, Hernan Astudillo <[email protected]> wrote: > >> I think your proposal is great. My thoughts: >> >> 1.- the "restore defaults" method it's to avoid backward compatibility >> issues? If true, great idea, but i'll logger.warn that to the developer. >> 2.- you read my mind (or i read your post :D ). However, making custom >> actions to require base actions pass the check (update in order to >> approve), >> could make a little conflict: >> supose that you want to approve but not make the record editable. Of >> course >> you can exclude the "edit" action, but does it disables the update action >> from passing querystring params? >> 3.- I agree, too. But "most" of the times is posible to put the security >> checks on the model. But sometimes you need to rely not only on the >> user(who) and the model(what), but in the controller (where). You don't >> want >> to mix the same options (security checks) on Admin controllers and Browse >> controllers, for example. And sometimes is not good idea to use the same >> controller for both, and the admin don't want to see edit or destroy >> buttons >> when browsing. So, i would make security methods default to >> (column)_authorized_for_(action) as you said, but still allow to override >> that with a :security_method in the controller. That way you also solve a >> backward compatibility issue. >> >> On Tue, Jul 7, 2009 at 4:26 PM, <[email protected]> wrote: >> >>> >>> On Tue, 7 Jul 2009 14:52:52 -0500, Kenny Ortmann >>> <[email protected] >>> > >>> wrote: >>> > The way ActiveScaffold works I don't see any reason for it to check >>> > any >>> > other type of action. >>> > All of the things you just listed can be classified as a crud type, >>> > activate - update, >>> > suspend - update, >>> > pdf - read, >>> > review - read, >>> > comment - create, >>> > diggit. - probably create >>> > >>> > You can set the crud type on your action_links to be one of these >>> values. >>> > >>> > One thing that I could see being an issue. >>> > Say on my model I have: >>> > >>> > def id_authorized_for_update? >>> > return false >>> > end >>> > >>> > Which means any action with the crud type of update will not be >>> > allowed >>> to >>> > change my id column. >>> >>> I agree ActiveScaffold security is not enough for some cases. I wrote a >>> proposal, but I only get a reply, it was a positive reply. This is my >>> proposal: >>> >>> I'm working currently in a project which requires finer grained >>> permissions >>> >>> than other projects I made before. Also bugs hard to find and fix were >>> reported >>> recently related to security code (like issue 21 in github). So I think >>> currently is confusing and it's not enough in some cases. >>> >>> I have some ideas to improve it: >>> >>> 1. authorized_for? called in a model should look for security methods at >>> class, instead of looking for instance methos in a new instance. This >>> change >>> can be backwards incompatible. Code which returns true for new records >>> won't >>> be affected, but code which hide links returning false for new records >>> would >>> need to add class methods. Maybe I could add a method to restore current >>> behaviour globally. >>> >>> Problems in issue 21 were caused by ActiveScaffold calling a security >>> method >>> twice, first to show or hide an action link in a new instance, second to >>> enable >>> or disable the action link with a instance loaded from DB. It wouldn't >>> be >>> a >>> >>> problem separating methods to show or hide an action link (using class >>> methods) from methods to enable or disable an action link (using >>> instance >>> methods, like now). >>> >>> Another problem was links_for_association, which sets links in columns >>> with >>> associations, calls each in columns object, and then >>> authorized_for_read? >>> is checked, and it was causing some problems, first requestfailed to add >>> links because session was not started and user was not logged, second >>> request added the links (in development mode). >>> >>> 2. Using crud type to enable or disable an action link is not enough for >>> custom action links. I would look for security methods with action name >>> and >>> >>> crud_type. >>> For example it would look for authorized_for_update? and >>> authorized_for_edit? >>> for edit action link. >>> >>> If I want to add an action link to approve an order, I would set crud >>> type >>> to >>> update. Currently it would check permissions with >>> authorized_for_update?. >>> I >>> >>> can't disable editing but enable approving with current schema. Adding >>> this >>> I >>> could enable approving with authorized_for_approve? and disable editing >>> with >>> authorized_for_update? >>> >>> 3. Remove the security method from action links. IMHO security checks >>> should >>> be at the model. So we can change security method calls in views or >>> controllers to direct calls to authorized_for? method in the model >>> (class >>> or >>> instance). >>> >>> Security checks for nested forms don't use security method because we >>> doesn't >>> want to create a new controller for associated model (and I think it's >>> not >>> easy and it would have high cost). People who relies on overridden >>> security >>> >>> methods to implement security checks find it doesn't work for nested >>> forms. >>> If >>> we remove security methods and force to set security checks in the model >>> it >>> >>> would work for normal scaffolds and nested forms. >>> >>> What do you think about this changes? >>> >>> >>> > >>> > On Tue, Jul 7, 2009 at 1:03 PM, Hernan Astudillo <[email protected]> >>> wrote: >>> > >>> >> Hi all, >>> >> Is there a reason to block authorized_for? to only the 4 crud >>> >> actions? >>> >> i mean this line in active_scaffold_permissions.rb: >>> >> def authorized_for?(options = {}) >>> >> raise ArgumentError, "unknown action #{options[:action]}" if >>> >> options[:action] and ![:create, :read, :update, >>> >> :destroy].include?(options[:action]) >>> >> >>> >> It would be nice to allow any action to pass through this security >>> >> structure. For example, use security for many posible actions that >>> apply >>> >> like activate, suspend, pdf, review, comment, diggit... >>> >> >>> >> I checked the code and i don't see anything that could be affected by >>> >> passing any other actions as parameter. However, in the view >>> >> (_list_actions.html.erb) it doesn't look so good to be using >>> "crud_type" >>> >> as >>> >> the action to be checked against to, instead of the real :action >>> >> parameter, >>> >> but changing that might need double checking for backward >>> >> compatibility >>> >> issues. I just say commenting out that raise ArgumentError which does >>> >> nothing. >>> >> >>> >> >>> >> > >>> >> >>> > >>> > >>> >>> >>> >> >> > >> > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "ActiveScaffold : Ruby on Rails plugin" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/activescaffold?hl=en -~----------~----~----~----~------~----~------~--~---
