diff --git a/app/controllers/concerns/administrate/punditize.rb b/app/controllers/concerns/administrate/punditize.rb index 6b4c67aba1..41c81ef03c 100644 --- a/app/controllers/concerns/administrate/punditize.rb +++ b/app/controllers/concerns/administrate/punditize.rb @@ -12,32 +12,53 @@ module Punditize included do private + def policy_namespace + [] + end + def scoped_resource - policy_scope_admin super + namespaced_scope = policy_namespace + [super] + policy_scope!(pundit_user, namespaced_scope) end def authorize_resource(resource) - authorize resource + namespaced_resource = policy_namespace + [resource] + authorize namespaced_resource end def authorized_action?(resource, action) - Pundit.policy!(pundit_user, resource).send("#{action}?".to_sym) + namespaced_resource = policy_namespace + [resource] + policy = Pundit.policy!(pundit_user, namespaced_resource) + policy.send("#{action}?".to_sym) end end private - # Like the policy_scope method in stock Pundit, but allows the 'resolve' - # to be overridden by 'resolve_admin' for a different index scope in Admin - # controllers. - def policy_scope_admin(scope) - ps = Pundit::PolicyFinder.new(scope).scope!.new(pundit_user, scope) - if ps.respond_to? :resolve_admin - ps.resolve_admin + def policy_scope!(user, scope) + policy_scope_class = Pundit::PolicyFinder.new(scope).scope! + + begin + policy_scope = policy_scope_class.new(user, pundit_model(scope)) + rescue ArgumentError + raise(Pundit::InvalidConstructorError, + "Invalid #<#{policy_scope_class}> constructor is called") + end + + if policy_scope.respond_to? :resolve_admin + ActiveSupport::Deprecation.warn( + "Pundit policy scope `resolve_admin` method is deprecated. " + + "Please use a namespaced pundit policy instead.", + ) + policy_scope.resolve_admin else - ps.resolve + policy_scope.resolve end end + + def pundit_model(record) + record.is_a?(Array) ? record.last : record + end end end end diff --git a/docs/authorization.md b/docs/authorization.md index 247199800b..5c8ceafc2f 100644 --- a/docs/authorization.md +++ b/docs/authorization.md @@ -28,20 +28,30 @@ technically have access to see in the main app. For example, a user may have all public records in their scope, but you want to only show *their* records in the admin interface to reduce confusion. -In this case, you can add an additional `resolve_admin` to your policy's -scope and Administrate will use this instead of the `resolve` method. +In this case, you can add additional pundit `policy_namespace` in your controller +and Administrate will use the namespaced pundit policy instead. For example: ```ruby -class PostPolicy < ApplicationPolicy - class Scope < Scope - def resolve - scope.all +# app/controllers/admin/posts_controller.rb +module Admin + class PostsController < ApplicationController + include Administrate::Punditize + + def policy_namespace + [:admin] end + end +end - def resolve_admin - scope.where(owner: user) +# app/policies/admin/post_policy.rb +module Admin + class PostPolicy < ApplicationPolicy + class Scope < Scope + def resolve + scope.where(owner: user) + end end end end diff --git a/spec/controllers/admin/orders_controller_spec.rb b/spec/controllers/admin/orders_controller_spec.rb index 8b16ba292b..423b5d6872 100644 --- a/spec/controllers/admin/orders_controller_spec.rb +++ b/spec/controllers/admin/orders_controller_spec.rb @@ -4,9 +4,132 @@ # which will test all the authorization functionality. describe Admin::OrdersController, type: :controller do - context "with Punditize concern" do + context "with namespaced Punditize concern" do controller(Admin::OrdersController) do include Administrate::Punditize + + def policy_namespace + [:own] + end + + def pundit_user + Customer.find_by(name: "Current User") + end + end + + let!(:user) { create(:customer, name: "Current User") } + + describe "GET index" do + it "shows only your own orders" do + someone = create(:customer) + order1 = create(:order, customer: user) + _order2 = create(:order, customer: someone) + order3 = create(:order, customer: user) + + locals = capture_view_locals { get :index } + + expect(locals[:resources]).to contain_exactly(order1, order3) + end + end + + describe "GET new" do + it "raises a Pundit error" do + expect { get :new }.to raise_error(Pundit::NotAuthorizedError) + end + end + + describe "GET edit" do + it "allows me to edit my records" do + order = create :order, customer: user + expect { get :edit, params: { id: order.id } }.not_to raise_error + end + + it "does not allow me to see other users' records" do + other_user = create(:customer) + order = create(:order, customer: other_user) + expect { get :show, params: { id: order.id } }. + to raise_error(ActiveRecord::RecordNotFound) + end + end + + describe "PUT update" do + def send_request(order:) + put( + :update, + params: { + id: order.id, + order: { address_line_one: "22 Acacia Avenue" }, + }, + ) + end + + it "allows me to update my records" do + order = create(:order, customer: user) + send_request(order: order) + expect(response).to redirect_to([:admin, order]) + expect(order.reload.address_line_one).to eq("22 Acacia Avenue") + end + + it "does not allow me to update other users' records" do + other_user = create(:customer) + order = create(:order, customer: other_user) + expect do + send_request(order: order) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end + + describe "DELETE destroy" do + it "never allows me to delete a record" do + o = create :order, customer: user, address_state: "AZ" + expect { delete :destroy, params: { id: o.id } }. + to raise_error(Pundit::NotAuthorizedError) + end + end + + describe "#authorized_action?" do + it "shows edit actions for records by the user" do + o = create(:order, customer: user) + expect(controller.send(:authorized_action?, o, :edit)).to be true + end + + it "does not show edit actions for records from other users" do + someone = create(:customer) + o = create(:order, customer: someone) + expect(controller.send(:authorized_action?, o, :edit)).to be false + end + + it "never shows destroy actions" do + o = create :order, customer: user, address_state: "AZ" + expect(controller.send(:authorized_action?, o, :destroy)).to be false + end + end + end + + context "with deprecated Punditize concern" do + before do + allow(ActiveSupport::Deprecation).to receive(:warn) + + class OrderPolicy + class Scope + def resolve_admin + scope.where(customer: user) + end + end + end + end + + after do + class OrderPolicy + class Scope + undef resolve_admin + end + end + end + + controller(Admin::OrdersController) do + include Administrate::Punditize + def pundit_user Customer.find_by(name: "Current User") end @@ -24,6 +147,8 @@ def pundit_user locals = capture_view_locals { get :index } expect(locals[:resources]).to contain_exactly(order1, order3) + expect(ActiveSupport::Deprecation).to have_received(:warn). + with(/`resolve_admin` method is deprecated/) end end @@ -37,6 +162,8 @@ def pundit_user it "allows me to edit my records" do order = create :order, customer: user expect { get :edit, params: { id: order.id } }.not_to raise_error + expect(ActiveSupport::Deprecation).to have_received(:warn). + with(/`resolve_admin` method is deprecated/) end it "does not allow me to see other users' records" do @@ -44,6 +171,8 @@ def pundit_user order = create(:order, customer: other_user) expect { get :show, params: { id: order.id } }. to raise_error(ActiveRecord::RecordNotFound) + expect(ActiveSupport::Deprecation).to have_received(:warn). + with(/`resolve_admin` method is deprecated/) end end @@ -63,6 +192,8 @@ def send_request(order:) send_request(order: order) expect(response).to redirect_to([:admin, order]) expect(order.reload.address_line_one).to eq("22 Acacia Avenue") + expect(ActiveSupport::Deprecation).to have_received(:warn). + with(/`resolve_admin` method is deprecated/) end it "does not allow me to update other users' records" do @@ -71,6 +202,8 @@ def send_request(order:) expect do send_request(order: order) end.to raise_error(ActiveRecord::RecordNotFound) + expect(ActiveSupport::Deprecation).to have_received(:warn). + with(/`resolve_admin` method is deprecated/) end end diff --git a/spec/example_app/app/policies/order_policy.rb b/spec/example_app/app/policies/order_policy.rb index 8f434c116f..2d0dbeb2cf 100644 --- a/spec/example_app/app/policies/order_policy.rb +++ b/spec/example_app/app/policies/order_policy.rb @@ -3,14 +3,6 @@ class Scope < Scope def resolve scope.all end - - def resolve_admin - if user.admin? - scope - else - scope.where(customer: user) - end - end end def create? diff --git a/spec/example_app/app/policies/own/order_policy.rb b/spec/example_app/app/policies/own/order_policy.rb new file mode 100644 index 0000000000..6f7044ea18 --- /dev/null +++ b/spec/example_app/app/policies/own/order_policy.rb @@ -0,0 +1,9 @@ +module Own + class OrderPolicy < ::OrderPolicy + class Scope < Scope + def resolve + scope.where(customer: user) + end + end + end +end