Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security Model Consistency: Nesting Singular Model #438

Open
gmontard opened this issue Aug 10, 2015 · 5 comments
Open

Security Model Consistency: Nesting Singular Model #438

gmontard opened this issue Aug 10, 2015 · 5 comments

Comments

@gmontard
Copy link

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.

@gmontard gmontard changed the title Security Model Consistency Security Model Consistency: Nesting Singular Model Aug 10, 2015
@scambra
Copy link
Member

scambra commented Aug 10, 2015

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
Copy link
Author

Hi @scambra

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

Thanks.

@scambra
Copy link
Member

scambra commented Aug 12, 2015

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
Copy link
Author

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
Copy link
Member

scambra commented Aug 12, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants