When authorize_resource is called before loading the instance, it grants access to unauthorized actions

[duarme]

Steps to reproduce

School belongs_to Admin, School gets Inquiries from students. So an admin should be able to see only the inquiries linked to their school:

# ability.rb

def admin_permissions(current_admin)
    # [...]
    can :manage, Inquiry, school_id: current_admin.school.id
    # [...]
  end

# app/controllers/admins/inquiry_forms_controller.rb
module Admins
  class InquiryFromController < Admins::ApplicationController
    authorize_resource
    before_action :load_inquiry_form, only: [:show, :update]

Please note that InquiryForm is actually one type of Inquiry, defined through single-table inheritance.

Expected behavior

It should not authorize get admins_inquiry_form_path(some_other_schools_inquiry) or, at lest, warn the developer by returning one error or exception.

Specifically, the following test should pass:

# test/controllers/admins/auth_inquiry_forms_controller_test.rb
class AuthInquiryFormsControllerTest < ActionDispatch::IntegrationTest
  setup do
    logout(:admins) # from Warden::Test::Helpers
    @admin_school = schools(:my_school)
    @other_school = schools(:another_school)
    @admin = @admin_school.admin
  end

  test 'Admin cannot see inquiry_forms of other admins' do
    sign_in(@admin)
    get admins_inquiry_form_url(id: @other_school.inquiry_forms.last.id)
    assert_response :redirect
  end
  
  # [...]

  def sign_in(admin, admin_password = 'supersecret')
      post admin_session_url("admin[email]" => admin.email, "admin[password]" => admin_password)
      follow_redirect!
    end

Actual behavior

It authorizes the signed_in admin to access other schools -- hence other admin's -- inquiries!

Specifically, the test above fails because the response is :success (200) instead of a :redirect.

Calling authorize_resource after before_action :load_inquiry_form, only: [:show, :update] makes the test pass.

System configuration

Rails version: 5.2.4.3 Ruby version: 2.5.1p57 CanCanCan version 3.3.0

[rocket-turtle]

I think that you call authorize_resource without the Inquiry loaded is not correct.

You can either use load_and_authorize_resource or load the Inquiry before you do the permission check.

Without the resource loaded the authorize_resource will probably check authorize!(:show, Inquiry) or authorize!(:show, InquiryFrom) (I'm not sure) . If a block or hash of conditions exist they will be ignored when checking on a class, and it will return true. See: https://github.com/CanCanCommunity/cancancan/blob/develop/docs/Checking-Abilities.md#checking-with-class

[duarme]

I think that you call authorize_resource without the Inquiry loaded is not correct.

I agree! the point is that in case of a mistake like this one CanCanCan should either not authorize by default or at least warn you (for instance by raising an exception). Because the actual behavior is dangerous, if not caught by some test, it can result in a serious privacy policy and/or business model violation (like it was for us, we were lucky that we found this mistake before it was exploited).

[kreintjes]

Same problem over here. We were performing the authorize_resource after the load, but since we used inheritance in our controllers the authorize was performed on the wrong instance variable. We had something like this:

class VehiclesController < ApplicationController
   load_and_authorize_resource :owner

  before_action :load_vehicle
  authorize_resource

  private

  def load_vehicle
    @vehicle = <magic to load correct vehicle for owner>
  end
end

class CarsController < VehiclesController
  def show
      <special car magic>
      respond_with @vehicle
   end
end

When visiting CarsController#show @vehicle is loaded, but the authorize_resource call in VehiclesController tried to authorize the @car variable which was nil. However this didn't trigger any warning or error whatsoever so we didn't know this was happening. Also due to the load_and_authorize_resource :owner call CanCanCan's check_authorization feature didn't warn us for this.

I think authorize_resource should raise or at least give a warning when trying to authorize a non-existing/nil object. This would have prevented the mistake. I think that also would have prevented @duarme's problem.

Ps. The fix in this case was changing authorize_resource to authorize_resource instance_name: :vehicle.

It might also be wise to have CanCanCan default to authorizing the instance name based on the controller that includes authorize_resource instead of the (lowest sub) controller that is requested. Not sure about that yet though.