inossidabile / protector

Comfortable (seriously) white-list security restrictions for models on a field level
MIT License
270 stars 31 forks source link

has_and_belongs_to_many associations being restricted #55

Open mwalsher opened 10 years ago

mwalsher commented 10 years ago

I'm upgrading to rails 4.1.5 and seem to keep running into protector-related issues. Have you had a chance to test things out in > 4.1?

The issue I'm running into now is the following error message on save:

Validation failed: Access denied to '[field]`

This is despite having the strong_parameters config setting disabled. Any ideas?

mwalsher commented 10 years ago

In my example,

class Group < ActiveRecord::Base
  has_and_belongs_to_many :accounts_users
end

class AccountsUser < ActiveRecord::Base
  belongs_to :account
  belongs_to :user
end

The code that produces the error is (@group being restricted by protector):

@group.assign_attributes(group_params.merge(account_id: current_account.id))

Request params are:

{"utf8"=>"✓", "_method"=>"patch", "authenticity_token"=>"WRqzvdIH90iXdw2fO8RS+e2bcQHTYmrSDn08V97m7wk=", "group"=>{"name"=>"no user", "description"=>"", "accounts_user_ids"=>["3", ""]}, "commit"=>"Update Group", "action"=>"update", "controller"=>"groups", "id"=>"42"}
mwalsher commented 10 years ago

The fix for #31 appears to be the cause of this. The problem with this approach is that it's restricting calls on HABTM associations where there is no explicitly defined model/protect block. Is that intentional? This makes sense for has_many :through associations, but perhaps not for HABTM.

Let me know your thoughts. I'd be happy to take a stab at a fix for this if you could provide some direction. Perhaps simply checking the reflection for HABTM and not restricting in that case will work?

E.g. inside build_record_with_protector in Protector::Adapters::ActiveRecord::Association, a check for habtm could be added:

if !protector_subject? or (reflection.parent_reflection and reflection.parent_reflection[1].macro == :has_and_belongs_to_many)
  return build_record_without_protector(*args)
else
  build_record_without_protector(*args).restrict!(protector_subject)
end

(Perhaps there is a better way to check for habtm, but there have been some changes in the reflections behaviour in AR per https://github.com/rails/rails/issues/14682)

mwalsher commented 10 years ago

Any thoughts on this?

mwalsher commented 9 years ago

You must be busy :)