inossidabile / protector

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

Access Denied when try to set variables directly on object #17

Closed anazar closed 11 years ago

anazar commented 11 years ago

My protect block looks like this:

    protect do |user, note|
      can :view
        can :update, :comment
      end
    end

When I do the following: n = Note.find params[:id] n.comment = params[:comment] n.updater_id = current_user.id n.restrict!(current_user).save!

It's throwing an ActiveRecord::RecordInvalid with the error: Validation failed: Access denied

If I do... can :update, [:comment, :updater_id] it works... but my understanding was that I should be able to set attributes manually without validation failing.

How can I set and save a variable manually that bypasses restrictions? We don't want to add it to a before save in the model.

inossidabile commented 11 years ago

Protector does not care about the initial source of a new value. You are still modifying it from the outside in both cases and it raises correctly. What you really need to do instead is to limit a value of updater_id like this:

protect do |user, note|
  can :view
  can :update, :comment, updater_id: user.id
end

In this case Protector will check for the end value of the attribute. This is the most exact description of security domain you are going to have.

Note that current stable version does not allow raw values as restrictors (only Range and Proc). To make it work you should include gem from the master branch of this repo. Or you can make it look like this (that I wouldn't recommend):

protect do |user, note|
  can :view
  can :update, :comment, updater_id: lambda {|x| x == user.id}
end
inossidabile commented 11 years ago

In fact you can automate this in the way I'm showing below. This is NOT something I consider a good practice. I'm just trying to explain the logic that stands behind Protector. You are trying to use it as strong_parameters – you should not. You already have strong_parameters around. Data-level restrictions work on data level. No matter how you set an attribute from outside – it has to be validated.

protect do |user, note|
  can :view
  can :update, :comment, updater_id: user.id

  # Force updater_id
  note.updater_id = user.id if note && !note.new_record?
end
cj commented 11 years ago

I'm running into the same issue, I've tried setting it in before_save which happens after active record validation and I'm still getting access denied....

inossidabile commented 11 years ago

@cj does my explanation solve your issue?

cj commented 11 years ago

@inossidabile I understand what you are saying, but I should still be able to set values in before_save inside the model after validation...

inossidabile commented 11 years ago

@cj before_save happens before validation. You can set any values you want in the after_validation filter. AR gives you all the hooks you need so no, you should not be able to set values at before_save.

cj commented 11 years ago

according to http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html before_save happens after_validation ......

inossidabile commented 11 years ago

@cj Oh. Indeed. Then if it still happens you should raise a separate issue with an example of code you are evaluating. This one is not related to filters in any way.

cj commented 11 years ago

I mean a really simple example that fails would be:

controller:

note = Note.new params
note.save!

model:

class Note < ActiveRecord::Base
  protect do |user|
    can :create, :comment
  end

  before_save do
     self.creator_id = ENV['SYSTEM_ID']
  end
end

would give this error: Validation failed: Access denied. It would also be nice to give the field names that they didn't have access too so you know which ones it's failing on.

cj commented 11 years ago

@note.errors => #<ActiveModel::Errors:0xb6fbbc4 @base=#<SigningNote id: nil, order_id: nil, comment: "ccccccccccccc", permission: "public", creator_id: nil, updater_id: nil, created_at: nil, updated_at: nil, signing_id: 56237>, @messages={:base=>["Access denied"]}> it would be nice to change base to the name of the field that is denied

inossidabile commented 11 years ago

In your example, @cj, it's not failing because of before_save. I hope it's clear. However the idea of extended validation message is something I really like. It was implemented in the version 0.5.4 of Protector that was just released. Enjoy.

cj commented 11 years ago

@inossidabile thank you for adding that :)

I'm not sure I understand about why the error is still showing up. If I change it to:

class Note < ActiveRecord::Base
  protect do |user|
    can :create, [:comment, :creator_id]
  end

  before_save do
     self.creator_id = ENV['SYSTEM_ID']
  end
end

It works with no errors. I thought I should be able to set values after the validation......

inossidabile commented 11 years ago

You you should be able. Is it showing you a error stating that it can not set exactly the creator_id field?

cj commented 11 years ago

Yes, I get Validation failed: Access denied if I just have :create, :comment and not :create, [:comment, :creator_id]

inossidabile commented 11 years ago

Update the version of Protector to see which field exactly triggers the error.

cj commented 11 years ago

@inossidabile ah there we go, it was another field :) Quick suggestion, to be able to use it with form validation it might be nicer to do :signing_id=>["Access denied"] instead of :base=>["Access denied to 'signing_id'"]

inossidabile commented 11 years ago

It's not supposed to be used with validation. Can you imagine a form where you'd show a field that user should not edit to mark it red later? I can not. This is "the last defence" trying to save you from showing 500 when something goes wrong. So on production I'd recommend changing I18n (protector.invalid) to something more reasonable and probably NOT containing %field.

cj commented 11 years ago

ah, I understand what you are saying. It was just a thought. Thank you again :)