security layer restricted to only crud actions

[Hernan Astudillo]

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.

[Kenny Ortmann]

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.

If the line you are suggesting is removed then say suspend is an action that updates the record, the ID column could now be updated in this method.

This might be a far fetched scenario, but this is one of the reasons the security is the way it is.

~Kenny

[Hernan Astudillo]

Activate and suspend i took from restful_authentication and its acts_as_state_machine plugin, which handles records in states as string.
The problem with using update, is that the permission is too generic:
for example:
activate: it's only authorized if user is not deleted nor active.
But you can't put that in a "def authorized_for_update?" since you don't know in the model which is the real action (it isn't passed as parameter and using active_scaffold_session_storage is unpredictable and ugly).

On the example you tell as posible issue, it doesn't affect since "suspend" action updates the record but is not the default "update" provided by AS which accepts parameters in many ways and thus making it vulnerable. "suspend" (or any other action different to crud actions) is an action that must be defined by the developer in the controller, and it's his concern to control the parameters he/she may accept, or use the same convention if wanted:
def id_authorized_for_suspend?
well, doesn't make sense in this example, but if applies, it's a new feature :)

[ser...@entrecables.com]

On Tue, 7 Jul 2009 14:52:52 -0500, Kenny Ortmann <kenny....@gmail.com>
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?

[Hernan Astudillo]

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.

[Kenny Ortmann]

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. 

Hernan's 3rd point is a very strong reason as to why it shouldn't be moved entirely to the model.

~Kenny

[ser...@entrecables.com]

On Tue, 7 Jul 2009 17:36:31 -0400, Hernan Astudillo <naa...@gmail.com>
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?

It doesn't, so you should forbid update crud type and authorize approve
action, authorized_for_#{action}? will have higher priority, like
#{column}_authorized_for_#{crud_type} has higher priority so you can forbid
update (disable the edit link) but authorize update in a column (for
inplace_edit).

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

I think you can check it in the model anyway, admin controller will be use
login and browse controller doesn't, or admin controller will be used by an
admin user. But I have no problem leaving security_method, it's a feature I
don't use. There is a reported bug, which it's not a bug IMO, about
security method not being used for association links, but it's not possible
(or it's bad) use another controller for a security check, and in that case
authorized_for? is called directly in the controller.

[ser...@entrecables.com]

On Tue, 7 Jul 2009 17:00:05 -0500, Kenny Ortmann <kenny....@gmail.com>
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.