activescaffold / active_scaffold

Save time and headaches, and create a more easily maintainable set of pages, with ActiveScaffold. ActiveScaffold handles all your CRUD (create, read, update, delete) user interface needs, leaving you more time to focus on more challenging (and interesting!) problems.
MIT License
1.1k stars 330 forks source link

Security Model Consistency: Nesting Singular Model #438

Open gmontard opened 9 years ago

gmontard commented 9 years ago

Hello,

I'm working with lot of AS models and nested controller and with a layer of security on top of it. I'm not big fan of the model security layer as it spread models with AS code and like to keep it clear inside the controller.

Almost everything is fine doing only controller security but sadly the behavior of nested models when having a singular relationship is not very consistent, let me explain:

When you have a singular nested model, by default it tries to open the nested in the "edit" action, compared the the "index" one when it's a plural model. There is a security check based on the model security layer and if you don't have access to the authorized_for_update? this will fallback to "index".

Sadly if you only use "controller" security and use the "authorized_for_update?" inside the controller, this layer is not checked and then the action of the nested will still be "edit" which will after that throw an exception when you click on it because you in fact don't have the right security clearance.

I tried to find the trace in the AS code, and it seems everything is related to those two pieces of code:

#https://github.com/activescaffold/active_scaffold/blob/bb6f69723b8412141550dfa97a81e8a9a33e6e32/lib/active_scaffold/helpers/view_helpers.rb#L181
def configure_column_link(link, record, associated, actions = nil)
        actions ||= link.column.actions_for_association_links
        if column_empty?(associated) # if association is empty, we only can link to create form
          if actions.include?(:new)
            link.action = 'new'
            link.crud_type = :create
            link.label ||= as_(:create_new)
          end
        elsif actions.include?(:edit)
          link.action = 'edit'
          link.crud_type = :update
        elsif actions.include?(:show)
          link.action = 'show'
          link.crud_type = :read
        elsif actions.include?(:list)
          link.action = 'index'
          link.crud_type = :read
        end

        unless column_link_authorized?(link, link.column, record, associated)
          link.action = nil
          # if action is edit and is not authorized, fallback to show if it's enabled
          if link.crud_type == :update && actions.include?(:show)
            link = configure_column_link(link, record, associated, [:show])
          end
        end
        link
      end

And

  #https://github.com/activescaffold/active_scaffold/blob/bb6f69723b8412141550dfa97a81e8a9a33e6e32/lib/active_scaffold/helpers/view_helpers.rb#L200
def column_link_authorized?(link, column, record, associated)
        if column.association
          associated_for_authorized =
            if column.plural_association? || (associated.respond_to?(:blank?) && associated.blank?)
              column.association.klass
            else
              associated
            end
          authorized = associated_for_authorized.authorized_for?(:crud_type => link.crud_type)
          authorized &&= record.authorized_for?(:crud_type => :update, :column => column.name) if link.crud_type == :create
          authorized
        else
          action_link_authorized?(link, record)
        end
      end

Do you think it will be possible to check the controller security of the nested model to fallback to a different action based on the result of the check ?

Thanks,

Guillaume.

PS : It may be a corner case but it's seems weird that the security is handled differently depending if it's model or controller based.

scambra commented 9 years ago

I don't see how can we use update_authorized?(record = nil) controller method to do somehting like: associated_for_authorized.authorized_for?(:crud_type => link.crud_type)

Or to do this check: record.authorized_for?(:crud_type => :update, :column => column.name) if link.crud_type == :create

gmontard commented 9 years ago

Hi @scambra

I'm not sure to follow, you think it's not possible to do it?

Thanks.

scambra commented 9 years ago

No, I don't see how to do it

This is controller method you want to use:

def update_authorized?(record = nil) (!nested? || !nested.readonly?) && (record || self).authorized_for?(:crud_type => :update) end

And these are calls on helpers: associated_for_authorized.authorized_for?(:crud_type => link.crud_type) record.authorized_for?(:crud_type => :update, :column => column.name) if link.crud_type == :create

If I change them to use update_authorized, they are not the same check:

update_authorized?(associated_for_authorized) will check associated_for_authorized.authorized_for?(:crud_type => :update) instead of link.crud_type

update_authorized?(record) if link.crud_type == :create will check record.authorized_for?(:crud_type => :update), so no column param

gmontard commented 9 years ago

Ok so there is no real solution then.

I guess at some point the security restriction applied in model and controller should be merged inside the different check in AS because today having both and using it differently introduce some inconstancies.

Thanks for your time.

scambra commented 9 years ago

maybe we can add some parameters to update_authorized?(record = nil), so we can pass them, but I'm worried with don't break anything