Unable to get restrictions working for create/update/destroy...

[Altonymous]

Rails 4.0.0 Ruby 2.0.0-p247 Protector 0.5.5 & 0.6.0.beta.1

protect do |user, offer|
  # Admins can retrieve anything
  if user.has_role? :administrator
    scope { all }

    # ... and view, create, update, or destroy anything
    can :view
    can :create
    can :update
    can :destroy
  elsif user.present?
    # Allow to read any field
    can :view
    can :create

    # Checks seller_id keeping possible nil in mind
    # Allow sellers to modify/delete their own offers
    if offer.try(:seller_id) == user.id
      can :update
      can :destroy
    end
  else
    # Guests can't read anything
    cannot :view
  end
end

I am using the above configuration for my model and no matter which offer the user attempts to edit or destroy it works for them.

I have also tried using the option that makes sure certain attributes are set correct and that doesn't seem to be working either...

    # ... and should correctly fill
    # ... the `seller_id` association
    can :create, seller_id: lambda{ |id|
      id == user.id
    }
    # ... the `last_changed_by_id` association
    can :create, last_changed_by_id: lambda{ |id|
      id == user.id
    }

[inossidabile]

How do you use the model?

[Altonymous]

def update
  respond_to do |format|
    if @offer.update(offer_params)
      format.html { redirect_to @offer, notice: 'Offer was successfully updated.' }
      format.json { head :no_content }
    else
      format.html { render action: 'edit' }
      format.json { render json: @offer.errors, status: :unprocessable_entity }
    end
  end
end

def setup_resource
  @offer = Offer.find(params[:id]).restrict!(current_user)
end

def offer_params
  params.require(:offer).permit(:name, :commit_rate_wac_hours, :price_per_wac, :average_price_per_wac, :duration_in_hours, :terms, :offer_type_id, :infrastructure_id, :compute_type_id)
end

[inossidabile]

This is the creation that is marked as allowed according to your protection meta. Can you show me the part that is not working properly?

[inossidabile]

Also can you try doing that in console without strong_parameters? We have at first to ensure it's related to Protector. Make something like Offer.find(x).restrict!(nil).destroy (this btw will make your protection block fail cause it starts from role checking and only secondary checks if user is empty so you should start from fixing that). This should raise. Then try to repeat it with modification. This should invalidate. Then report back :)

[Altonymous]

I just discovered that the delete is working as intended. It is only the update that is a problem.

[inossidabile]

Ehm. But create is allowed.

[Altonymous]

Sorry I've been up all night. I updated the comment above with the update call.

[Altonymous]

I should also note that both the destroy and the update use the same method to get the record... "setup_resource".

[inossidabile]

I don't get it :). What's working and what's not? :) Is that update that is not working? Or create?

[Altonymous]

Just forget everything I've said.. I'm going to close this issue out and go get some sleep. If it's still an issue after some sleep I'll reopen. :P

[inossidabile]

lol, ok :)

[Altonymous]

I got my sleep and my stuff working. I'm still not quite sure on the best way to go about getting a list of items that the current_user has access to. But I've got something working. It just seems wrong.

# in my controller...
@contracts = Contract.all.user_is_party_to(current_user.id).includes(:offer, :buyer).restrict!(current_user)

# in my model...
if user.present?
  can :create

  # Checks seller_id keeping possible nil in mind
  # Allow sellers to view/modify/delete their own contracts
  if contract.try(:offer).try(:seller_id) == user.id || contract.try(:buyer_id) == user.id
    can :view

    if contract.try(:offer).try(:seller_id) == user.id && contract.buyer_id.blank?
      can :update
      can :destroy
    end
  end
end

[inossidabile]

I'm still unsure I understand you right. But aren't you looking for scopes? https://github.com/inossidabile/protector#scopes

if user.present?
  can :create
  scope { where(offer: {seller_id: user.id}) }
end

Then as long as you restrict your model, the selection will be automatically scoped to the current user using offer relation. And you won't even get that models at result set.

[inossidabile]

Scope always appends to the end of query. It overlaps all previous scopes you set manually.

According to the dump you show, they both are NOT owned by user. What is the database you use? I'm a little bit scared of that IDs.

[Altonymous]

Haha I figured out my problem. I got it working. thanks so much for your help.

I use postgresql. In the most recent versions you can create primary keys as uuid. Which is stored as a binary instead of a string to address performance issues with joins.

[inossidabile]

Ah, nice :). Alright. Make sure you use 0.6.0 cause there's huge difference it introduces. Since 0.6.0 Protector will merge all scope calls you actually do inside your protection block.

[Altonymous]

I'm not sure I want it to merge all scope calls... but maybe you can straighten me out again. :+1:

I want the ability to be able to do things like...

    scope { where(sold_by_id: user.id || bought_by_id: user.id) }
      can :view

    scope { where(sold_by_id: user.id && bought_by_id: nil) }
      can :update
      can :destroy

In other words, a contract seller or buyer can view the contract. And a seller of the contract can update/destroy it as long as there is no buyer.

Hopefully that makes sense.

[inossidabile]

This makes logic consistent between can and scope. They both merge. And it's more secure cause you can't accidentally end up with open scope. The worst thing can happen – scope will become even more strict. The protection meta is ruby, remember about it. Just use conditions, cycles any constructions to control logic. You are free to build conditions the way scope will only be called once, right?

[inossidabile]

I think I can see the thing you didn't get so far. Scope works BEFORE we query database. Can works after. They don't interfere. So in the case you posted – the first scope includes second one. Everything else can be done with if.

scope { where(sold_by_id: user.id || bought_by_id: user.id) }
can :view

if user && instance && user.id == instance.sold_by_id && user.id == instance.bought_by_id
  can :update
  can :destroy
end

[Altonymous]

Feature request!

  can :view, scope { where(sold_by_id: user.id || bought_by_id: user.id) }
  can [:update, :destroy], scope { where(sold_by_id: user.id && bought_by_id: nil) }

Assuming it only runs the scope for the specific action requested, that would limit it.

I get what you're saying on your above example. When doing a "manage" action you have a specific instance, so you can check against it rather than worrying about scope. So when it applies the above scope it will return the instance anyway and then stop it from managing it if it doesn't meet the conditions.

So as long as those aren't at odds.. such as a user being allowed to edit something they can't view, which doesn't make sense anyway, it's good to go.

The only downside to that is the index hit of applying the scope to query for the explicit instance instead of just hitting the primary key index. It means tuning the database to always expect that scope regardless what else it is querying on.

This does leave the question of how you go about getting a list of elements a user can manage.. but I guess that gets pushed into the controller as further restriction....

Contract.restrict!(current_user).where(sold_by_id: current_user.id && bought_by_id: nil)

correct?

[inossidabile]

You understand ActiveRecord merges all scopes you apply to a Relation at the end don't you? Then it maps it to SQL. And queries. Protector doesn't influence that and it can't make "multiple scopes". There's always one scope.

Scope defines what you can get from database. And it's up to you to decide based on user. If you don't need that stuff - don't query for it.

Remember, Protector is a security scope. It will never replace named scopes for you. Named scopes are for logic. They let you only query things you actually need. And Protector will additionally strip all stuff that user can't read at all. Is that clear? In your case user can read something that was either bought or sold on his behalf. Protector is NOT a thing that should decide what of that you need in a particular query. Instead it will ensure you nobody will ever select ANYTHING that's beside the scope you applied.

So you restrict relation. Then append another scopes that you need for your logic. And magic happens. Protector is your line of defence. Don't try to inject application logic into security layer. You have your controllers for that.

[Altonymous]

Yes I understand that, but perhaps not as concretely as I might think. Let me rephrase.. the way I understand what you've done above...

With your above example.. if I were to make my controller thus..

@contract = Contract.find(params[:id]).restrict!(current_user)

Instead of constructing a query that is thus...

select * from contracts where id = ?

It would after applying the security scope be...

select * from contracts where id = ? and (sold_by_id = ? || bought_by_id = user.id)

If that's the case what I was trying to point out is that if there is an index on the table for id and another for [sold_by_id, bought_by_id], then there will need to be a third of [id, sold_by_id, bought_by_id] and in actuality the first one for just the primary key will never be used.

Or perhaps ActiveRecord does some magic that figures out the protector scope isn't needed because it's already limited beyond the scope and that in the subsequent lines of code protection will be checked via conditionals. (I'm guessing that's not the case)

[Altonymous]

To that end thinking about it more I would change the above that I marked as a feature request as such...

can :view, scope { where(sold_by_id: user.id || bought_by_id: user.id) }

if user && instance && user.id == instance.sold_by_id && instance.bought_by_id == nil
  can :update
  can :destroy
end

In this case the expectation would be that the scope would only be applied to the view action and the other actions would be protected via conditionals.

[Altonymous]

Great discussion btw. I feel like in the little time you've given to answering my questions my knowledge has increased exponentially about the gem you've created. :+1: I'll have to think on that last bit a little more. For now you've unblocked all my current issues.

Rock on man! Thanks again!

[inossidabile]

First of all – no, you don't need another index. PostgreSQL will use primary key as a primary selector and then simply check conditionals from WHERE. So there's no performance hit here. It's just more about logic.

In this case the expectation would be that the scope would only be applied to the view action and the other actions would be protected via conditionals.

scope can't be applied to can. can happens after scope. I think you should just stick to the firs paragraph – there's no performance issue in that. So there's nothing to worry about.

[inossidabile]

You are very welcome :) Feel free to ask more. The only thing I'd like to ask - could you please use stackoverflow instead? With that we could spread the knowledge further :). Feel free to ping me on twitter or here if you post something.

[Altonymous]

I was actually thinking if I can find some free time to add some of these details to a pull request for your README or a wiki here. But if you want the stackoverflow points I'll be happy to oblige.

[inossidabile]

PRs are even more welcome. StackOverflow is great in that it's automatic. But if you want to spend some time sorting that stuff – I'd really appreciate that :bow:

It's hard for me to write reasonable docs thinking from the position of core internals. I think in a little bit another terms. So the README we have now might be confusing indeed. I understand that but currently can't do a lot on my own :(