Patch review (hosts_escalation.patch, please upload new revisions of the patch as new attachments):

1. my main concern is that it slightly breaks the implied contract of resource_class by returning an AR::Relation instead of the class. While this works as we can call find_by_* on it, I'm a bit concerned. Maybe we can document the base method to say that either the class or a scope is acceptable, or better, add a second method that explicitly returns a scope and use that instead.

2. tests please :)

what about compute resources, don't we have the same problem there?

Dominic Cleal wrote:

Patch review (hosts_escalation.patch, please upload new revisions of the patch as new attachments):

1. my main concern is that it slightly breaks the implied contract of resource_class by returning an AR::Relation instead of the class. While this works as we can call find_by_* on it, I'm a bit concerned. Maybe we can document the base method to say that either the class or a scope is acceptable, or better, add a second method that explicitly returns a scope and use that instead.

I agree but since AR::Relation is usually used in exactly same way as a model and is some kind of a 'proxy' object I wouldn't see this as a big problem. I like the idea that we could add and use new resource_scope method which would by default run .scoped on resource_class.

2. tests please :)

Correct, will upload with new version.

Try this:

  test "should not allow access to a host out of users hosts scope" do
    users(:three).roles = [Role.find_by_name('Anonymous'), Role.find_by_name('Viewer')]
    as_user :three do
      get :show, { :id => hosts(:one).to_param }, set_session_user.merge(:user => users(:three).id)
    end
    assert_response :not_found
  end

Marek Hulán wrote:

Thanks Dominic. I did it via fixtures. I'm still not sure why it didn't work with fixture names (see user_roles.yml), probably because of fixed ids of roles. Rails generated random ids for them.

Yeah, agreed. We normally assign roles on the fly in tests, but I think this is OK too.

One thing I'd like to ask. Do you think that checking of show action is sufficient or should I cover all controller actions that were affected? They use the same logic but if someone customize any of them in future (update/delete) he could make the same mistake.

I think just the show action is sufficient as find_resource is used consistently, but other tests would be welcome. I can't do the 1.2.1 release until the start of next week due to lack of time, so you have until then if you want to improve it :)

Review comments:
- resource_scope is public in the base controller (correct IMO), but visibility has been changed to private in hosts/compute_resources controllers, it should still be public
- renaming the user to something more descriptive would be useful ("restricted"/"filtered"?), just so people don't reuse it for other purposes in the future and subtly break the tests

Otherwise good, thanks.