Activeadmin: Should we have a DSL method for Strong Parameters?

[seanlinsley]

This is a bit tedious:

controller do
  def permitted_params
    params.permit foo: [:bar, :baz]
  end
end

Do we want a shortcut method for this, and if so, what should it look like?

[zorab47]

A core idea of AA is to manage a resource without any manual configuration. With that in mind, should the system whitelist known parameters without manual configuration? I'd assume any fields AA would generate in the default new/edit forms would be whitelisted. Once a user begins to manually configure his forms, then he'd need to manually override the default permitted params. That is where the DSL would come into play.

[seanlinsley]

This isn't really the ticket to discuss that. Maybe it'd be better to chime in on #2582. The point of this ticket is to decide 1) if we want a DSL method and 2) what it should be. For example:

ActiveAdmin.register User do

  # the static version
  params :name, :email, :company_id

  # and the dynamic version
  params do
    defaults = [:name, :email]
    if current_user.admin?
      defaults + [:company_id]
    else
      defaults
    end
  end

end

[shekibobo]

If we were to go with your above example DSL, I'd suggest we keep the method called permitted_params. I do like the idea that it would automatically use the correct resource param key.

[seanlinsley]

Maybe permit_params would be a better option, as it verbally makes more sense:

permit_params :name, :email, :company_id

permit_params do
  # ...
end

[shekibobo]

Should we allow :all to be passed?

[seanlinsley]

I'm hesitant to allow blanket-whitelisting, because that disables security protections. If I understand mass assignment correctly, that means that for a User model of mine, any of these can be set without my approval (all methods ending in "="):

password_confirmation=
version_association_name=
version=
version_class_name=
paper_trail_options=
paper_trail_enabled_for_model=
versions_association_name=
paper_trail_event=
before_add_for_versions=
after_add_for_versions=
before_remove_for_versions=
after_remove_for_versions=
remember_me=
extend_remember_period=
password=
company=
versions=
version_ids=
id=
email=
encrypted_password=
reset_password_token=
reset_password_sent_at=
remember_created_at=
sign_in_count=
current_sign_in_at=
last_sign_in_at=
current_sign_in_ip=
last_sign_in_ip=
created_at=
updated_at=
company_id=
role=
validation_context=
_validate_callbacks=
_validators=
_validation_callbacks=
_initialize_callbacks=
_find_callbacks=
_touch_callbacks=
_save_callbacks=
_create_callbacks=
_update_callbacks=
_destroy_callbacks=
record_timestamps=
_commit_callbacks=
_rollback_callbacks=
reflections=
include_root_in_json=
_ransackers=
destroyed_by_association=
attributes=

Depending on how the given library deals with those values being replaced, this could represent a very real security hole.

[shekibobo]

Yeah, I agree. Whitelisting needs to be very intentional.

[shekibobo]

Just found this: CanCanStrongParameters. Might be a good reference if nothing else.

[ball-hayden]

Do you mind if I have a go at getting a PR together to implement @seanlinsley 's ideas above?

[seanlinsley]

Feel free to :cat:

[seanlinsley]

@shekibobo & @zorab47, what do you think of #2635?

[shekibobo]

Looks pretty solid to me. Do we want to have it display some kind of warning if we're in Rails 4 (and/or StrongParameters is defined) that informs the developer to use this method? Maybe have a default definition of the method on a register that spits out a warning when it's loaded if the method isn't overridden?

[shekibobo]

In resource_controller/data_access.rb I'm thinking we add something like:

def permitted_params
    raise StrongParametersRequiredError
end

Since presumably this will never be called in Rails 3 or any time a form is not used, this would throw an error as soon as someone tried to submit a form without including the permit_params declaration. I don't think this would throw on load, though. Not sure where to put it if we did it that way.

[seanlinsley]

Sounds reasonable. Though we should only raise that error if strong parameters is indeed being used:

def permitted_params
  unless Rails::VERSION::MAJOR > 3 && !defined? ProtectedAttributes
    raise StrongParametersRequired
  end
end

[ball-hayden]

Does Rails not do this already?

I get a ActiveModel::ForbiddenAttributesError if I don't define the permitted_params method in the controller (from sanitize_for_mass_assignment in lib/active_model/forbidden_attributes_protection.rb:21)

[zorab47]

2635 looks good to me. The only thought I had on it was supporting both Rails 3 and 4, but that has been discussed.

Should we try to detect conflicting attribute protection methods? Kinda like the issue in #2606.

[seanlinsley]

@ball-hayden IIRC, Inherited Resources prevents that error from being called.

@zorab47 strictly speaking, #2635 _does_ support Strong Parameters with Rails 3; we just aren't testing that configuration. Can you go into more detail on your #2606 point? Wouldn't it be simple enough to raise an error if both StrongParameters and ProtectedAttributes are loaded?

[ball-hayden]

@seanlinsley - I just tried submitting a new post on #2635 and it raised it correctly?

[zorab47]

Can you go into more detail on your #2606 point? Wouldn't it be simple enough to raise an error if both StrongParameters and ProtectedAttributes are loaded?

@seanlinsley yeah, essentially detect the conflicts and raise an error. But now I wonder if that should be a responsibility of AA or not.

[shekibobo]

# extract attributes from params
def build_resource_params
  parameters = respond_to?(:permitted_params, true) ? permitted_params : params
  rparams = [parameters[resource_request_name] || parameters[resource_instance_name] || {}]
  if without_protection_given?
    rparams << without_protection
  else
    rparams << as_role if role_given?
  end

  rparams
end

According to inherited_resources it only calls permitted_params if it's defined on our controllers. I think we should define our method to simply return params if on Rails 3, and to raise an error by default on Rails 4 or defined?(StrongParameters). That way, when we override it with permit_params, it will override the base definition, and it won't hurt anything otherwise. If it gets called in Rails 4 without us overriding it, it will raise our error and let the user know about how to enable them.

Beyond that, it's on the user to make sure they pass all the correct parameters.

def permitted_params
  if Rails::Version::MAJOR > 3 || defined?(StrongParameters)
    raise "You tried to submit a form without whitelisting any attributes. Use `permit_params` to whitelist attributes in your resource configuration."
  else
    params
  end
end

[seanlinsley]

Okay #2635 has been merged so we now have a DSL method. What all still needs to be discussed?

[shekibobo]

I think we're probably good on this one. We have the helper, we have the generator, we have the docs... What else do we need?

[seanlinsley]

I was initially under the impression that Rails 4 doesn't raise an error if no params are defined, but I just confirmed that was the case, so we're good there.

But IMO it's not a useful error message. One way or another, I think we should have an AA-specific error. Whether that's on app boot or on form submission, I'm not sure.

[seanlinsley]

If we raise on form submission, maybe it'd be better to rescue from ActiveModel::ForbiddenAttributesError and then raise our own exception?

[shekibobo]

I still think we should just define our own method by default of permitted_params that raises an exception if it hasn't been overridden, and returns params if we aren't using strong parameters.

def permitted_params
  if #Rails 4 or strong parameters
    raise "You must define permitted params"
  else
    params
  end
end

That is, _if_ we decide we need to raise our own error.

[timoschilling]

I think this can be closed or what do you mean @seanlinsley?

[kmayer]

I'm not sure if this is a bug, but if I have a resource with a belongs_to relationship, the new action fails because the "parent" id is not whitelisted:

class AdminUser < ActiveRecord::Base
  belongs_to :employer
end

class Employer < ActiveRecord::Base
  has_many :administrators
end
...
ActiveAdmin.register Administrator do
  belongs_to :employer

  permit_params :employer_id
end
ActiveAdmin.register Employer do
end

visiting /admin/employer/1/users/new raises an error:

ActionController::UnpermittedParameters in Admin::AdministratorsController#new
found unpermitted parameters: employer_id

And with some digging, it turns out that employer_id isn't wrapped in the param_key (https://github.com/activeadmin/activeadmin/blob/master/lib/active_admin/resource_dsl.rb#L50), so it isn't permitted.

once the generated permitted_params method is called by InheritedResource, the resulting list of keys are:

[:utf8, :_method, :authenticity_token, :commit, :id, {:administrator=>[:employer_id]}]

but the submitted parameters (to the new action) are:

{"action"=>"new", "controller"=>"admin/administrators", "employer_id"=>"1"}

[kmayer]

As a followup and perhaps fix / workaround, I just overrode the permitted_params method in the Administrator resource:

  controller do
    define_method :permitted_params do
      params.permit *active_admin_namespace.permitted_params, :employer_id,
                    :administrator => [:email, :role, :employer_id, :password, :password_confirmation]
    end
  end

[seanlinsley]

@kmayer that indeed looks like a bug. Could you create a separate ticket / PR for it?

[kmayer]

Opened #3556 @seanlinsley

[timoschilling]

I'm going to close this, while the active admin has now a api for permitted_params