diff --git a/app/controllers/administrate/application_controller.rb b/app/controllers/administrate/application_controller.rb index 63a5674b33..114060da5e 100644 --- a/app/controllers/administrate/application_controller.rb +++ b/app/controllers/administrate/application_controller.rb @@ -105,10 +105,27 @@ def nav_link_state(resource) resource_name.to_s.pluralize == underscore_resource ? :active : :inactive end - helper_method :valid_action? - def valid_action?(name, resource = resource_class) - routes.include?([resource.to_s.underscore.pluralize, name.to_s]) + # Whether the named action route exists for the resource class. + # + # @param resource [Class, String, Symbol] A class of resources, or the name + # of a class of resources. + # @param action_name [String, Symbol] The name of an action that might be + # possible to perform on a resource or resource class. + # @return [Boolean] `true` if a route exists for the resource class and the + # action. `false` otherwise. + def existing_action?(resource, action_name) + routes.include?([resource.to_s.underscore.pluralize, action_name.to_s]) + end + helper_method :existing_action? + + # @deprecated Use {#existing_action} instead. Note that, in + # {#existing_action}, the order of parameters is reversed and + # there is no default value for the `resource` parameter. + def valid_action?(action_name, resource = resource_class) + Administrate.warn_of_deprecated_authorization_method(__method__) + existing_action?(resource, action_name) end + helper_method :valid_action? def routes @routes ||= Namespace.new(namespace).routes.to_set @@ -226,9 +243,26 @@ def show_search_bar? ).any? { |_name, attribute| attribute.searchable? } end - def show_action?(_action, _resource) + # Whether the current user is authorized to perform the named action on the + # resource. + # + # @param _resource [ActiveRecord::Base, Class, String, Symbol] The + # temptative target of the action, or the name of its class. + # @param _action_name [String, Symbol] The name of an action that might be + # possible to perform on a resource or resource class. + # @return [Boolean] `true` if the current user is authorized to perform the + # action on the resource. `false` otherwise. + def authorized_action?(_resource, _action_name) true end + helper_method :authorized_action? + + # @deprecated Use {#authorized_action} instead. Note that the order of + # parameters is reversed in {#authorized_action}. + def show_action?(action, resource) + Administrate.warn_of_deprecated_authorization_method(__method__) + authorized_action?(resource, action) + end helper_method :show_action? def new_resource @@ -237,7 +271,14 @@ def new_resource helper_method :new_resource def authorize_resource(resource) - resource + if authorized_action?(resource, action_name) + resource + else + raise Administrate::NotAuthorizedError.new( + action: action_name, + resource: resource, + ) + end end def paginate_resources(resources) diff --git a/app/controllers/concerns/administrate/punditize.rb b/app/controllers/concerns/administrate/punditize.rb index d49de39468..f655cc16c0 100644 --- a/app/controllers/concerns/administrate/punditize.rb +++ b/app/controllers/concerns/administrate/punditize.rb @@ -5,6 +5,8 @@ module Punditize include Pundit::Authorization included do + private + def scoped_resource policy_scope_admin super end @@ -13,7 +15,7 @@ def authorize_resource(resource) authorize resource end - def show_action?(action, resource) + def authorized_action?(resource, action) Pundit.policy!(pundit_user, resource).send("#{action}?".to_sym) end end diff --git a/app/helpers/administrate/application_helper.rb b/app/helpers/administrate/application_helper.rb index efc9378c8d..b314abd898 100644 --- a/app/helpers/administrate/application_helper.rb +++ b/app/helpers/administrate/application_helper.rb @@ -25,6 +25,28 @@ def model_from_resource(resource_name) dashboard.try(:model) || resource_name.to_sym end + # Unification of + # {Administrate::ApplicationController#existing_action? existing_action?} + # and + # {Administrate::ApplicationController#authorized_action? + # authorized_action?} + # + # @param target [ActiveRecord::Base, Class, Symbol, String] A resource, + # a class of resources, or the name of a class of resources. + # @param action_name [String, Symbol] The name of an action that might be + # possible to perform on a resource or resource class. + # @return [Boolean] Whether the action both (a) exists for the record class, + # and (b) the current user is authorized to perform it on the record + # instance or class. + def accessible_action?(target, action_name) + target = target.to_sym if target.is_a?(String) + target_class_or_class_name = + target.is_a?(ActiveRecord::Base) ? target.class : target + + existing_action?(target_class_or_class_name, action_name) && + authorized_action?(target, action_name) + end + def display_resource_name(resource_name, opts = {}) dashboard_from_resource(resource_name).resource_name( count: opts[:singular] ? SINGULAR_COUNT : PLURAL_MANY_COUNT, diff --git a/app/views/administrate/application/_collection.html.erb b/app/views/administrate/application/_collection.html.erb index 96b44bdfd8..044a53efdd 100644 --- a/app/views/administrate/application/_collection.html.erb +++ b/app/views/administrate/application/_collection.html.erb @@ -59,13 +59,13 @@ to display a collection of resources in an HTML table. <% resources.each do |resource| %> + <% if accessible_action?(resource, :show) %> <%= %(tabindex=0 role=link data-url=#{polymorphic_path([namespace, resource])}) %> <% end %> > <% collection_presenter.attributes_for(resource).each do |attribute| %> - <% if valid_action?(:show, resource.class) && show_action?(:show, resource) -%> + <% if accessible_action?(resource, :show) -%> +<% [existing_action?(collection_presenter.resource_name, :edit), + existing_action?(collection_presenter.resource_name, :destroy)].count(true).times do %> <% end %> diff --git a/app/views/administrate/application/_collection_item_actions.html.erb b/app/views/administrate/application/_collection_item_actions.html.erb index 381716c4ec..3bddff6c0f 100644 --- a/app/views/administrate/application/_collection_item_actions.html.erb +++ b/app/views/administrate/application/_collection_item_actions.html.erb @@ -1,17 +1,17 @@ -<% if valid_action?(:edit, collection_presenter.resource_name) %> +<% if existing_action?(collection_presenter.resource_name, :edit) %> <%= link_to( t("administrate.actions.edit"), [:edit, namespace, resource], class: "action-edit", - ) if show_action?(:edit, resource) %> + ) if accessible_action?(resource, :edit) %> <% end %> -<% if valid_action?(:destroy, collection_presenter.resource_name) %> +<% if existing_action?(collection_presenter.resource_name, :destroy) %> <%= link_to( t("administrate.actions.destroy"), [namespace, resource], class: "text-color-red", method: :delete, data: { confirm: t("administrate.actions.confirm") } - ) if show_action?(:destroy, resource) %> + ) if accessible_action?(resource, :destroy) %> <% end %> diff --git a/app/views/administrate/application/_index_header.html.erb b/app/views/administrate/application/_index_header.html.erb index 03a2fc9c79..ad069cac3a 100644 --- a/app/views/administrate/application/_index_header.html.erb +++ b/app/views/administrate/application/_index_header.html.erb @@ -23,6 +23,6 @@ ), [:new, namespace, page.resource_path.to_sym], class: "button", - ) if valid_action?(:new) && show_action?(:new, new_resource) %> + ) if accessible_action?(new_resource, :new) %> diff --git a/app/views/administrate/application/_navigation.html.erb b/app/views/administrate/application/_navigation.html.erb index 670aab66eb..a0f26641c1 100644 --- a/app/views/administrate/application/_navigation.html.erb +++ b/app/views/administrate/application/_navigation.html.erb @@ -15,6 +15,6 @@ as defined by the routes in the `admin/` namespace display_resource_name(resource), resource_index_route(resource), class: "navigation__link navigation__link--#{nav_link_state(resource)}" - ) if valid_action?(:index, resource) && show_action?(:index, model_from_resource(resource)) %> + ) if accessible_action?(model_from_resource(resource), :index) %> <% end %> diff --git a/app/views/administrate/application/edit.html.erb b/app/views/administrate/application/edit.html.erb index abacffce42..415de8a5c8 100644 --- a/app/views/administrate/application/edit.html.erb +++ b/app/views/administrate/application/edit.html.erb @@ -27,7 +27,7 @@ It displays a header, and renders the `_form` partial to do the heavy lifting. t("administrate.actions.show_resource", name: page.page_title), [namespace, page.resource], class: "button", - ) if valid_action?(:show) && show_action?(:show, page.resource) %> + ) if accessible_action?(page.resource, :show) %> diff --git a/app/views/administrate/application/show.html.erb b/app/views/administrate/application/show.html.erb index 41e5b787b3..71b4638ff7 100644 --- a/app/views/administrate/application/show.html.erb +++ b/app/views/administrate/application/show.html.erb @@ -28,7 +28,7 @@ as well as a link to its edit page. t("administrate.actions.edit_resource", name: page.page_title), [:edit, namespace, page.resource], class: "button", - ) if valid_action?(:edit) && show_action?(:edit, page.resource) %> + ) if accessible_action?(page.resource, :edit) %> <%= link_to( t("administrate.actions.destroy"), @@ -36,7 +36,7 @@ as well as a link to its edit page. class: "button button--danger", method: :delete, data: { confirm: t("administrate.actions.confirm") } - ) if valid_action?(:destroy) && show_action?(:destroy, page.resource) %> + ) if accessible_action?(page.resource, :destroy) %> diff --git a/app/views/fields/belongs_to/_index.html.erb b/app/views/fields/belongs_to/_index.html.erb index 0dece5cc4f..1e0ab2f4c9 100644 --- a/app/views/fields/belongs_to/_index.html.erb +++ b/app/views/fields/belongs_to/_index.html.erb @@ -16,7 +16,7 @@ By default, the relationship is rendered as a link to the associated object. %> <% if field.data %> - <% if valid_action?(:show, field.associated_class) && show_action?(:show, field.associated_class) %> + <% if accessible_action?(field.data, :show) %> <%= link_to( field.display_associated_resource, [namespace, field.data], diff --git a/app/views/fields/belongs_to/_show.html.erb b/app/views/fields/belongs_to/_show.html.erb index 15e2d36c2c..3bca714d68 100644 --- a/app/views/fields/belongs_to/_show.html.erb +++ b/app/views/fields/belongs_to/_show.html.erb @@ -16,7 +16,7 @@ By default, the relationship is rendered as a link to the associated object. %> <% if field.data %> - <% if valid_action?(:show, field.associated_class) && show_action?(:show, field.associated_class) %> + <% if accessible_action?(field.data, :show) %> <%= link_to( field.display_associated_resource, [namespace, field.data], diff --git a/app/views/fields/has_one/_index.html.erb b/app/views/fields/has_one/_index.html.erb index e6fc01a44f..4ff3ad1747 100644 --- a/app/views/fields/has_one/_index.html.erb +++ b/app/views/fields/has_one/_index.html.erb @@ -16,7 +16,8 @@ By default, the relationship is rendered as a link to the associated object. %> <% if field.linkable? %> - <%= link_to( + <%= link_to_if( + accessible_action?(field.data, :show), field.display_associated_resource, [namespace, field.data], ) %> diff --git a/app/views/fields/has_one/_show.html.erb b/app/views/fields/has_one/_show.html.erb index a4b3dc5fe8..542af5e09d 100644 --- a/app/views/fields/has_one/_show.html.erb +++ b/app/views/fields/has_one/_show.html.erb @@ -18,7 +18,8 @@ All show page attributes of has_one relationship would be rendered <% if field.linkable? %>
- <%= link_to( + <%= link_to_if( + accessible_action?(field.data, :show), field.display_associated_resource, [namespace, field.data], ) %> diff --git a/app/views/fields/polymorphic/_index.html.erb b/app/views/fields/polymorphic/_index.html.erb index 7c27a3b379..e9608f461f 100644 --- a/app/views/fields/polymorphic/_index.html.erb +++ b/app/views/fields/polymorphic/_index.html.erb @@ -17,7 +17,8 @@ By default, the relationship is rendered as a link to the associated object. %> <% if field.data %> - <%= link_to( + <%= link_to_if( + accessible_action?(field.data, :show), field.display_associated_resource, [namespace, field.data] ) %> diff --git a/app/views/fields/polymorphic/_show.html.erb b/app/views/fields/polymorphic/_show.html.erb index 27a5645aee..a656d2925e 100644 --- a/app/views/fields/polymorphic/_show.html.erb +++ b/app/views/fields/polymorphic/_show.html.erb @@ -17,7 +17,7 @@ By default, the relationship is rendered as a link to the associated object. %> <% if field.data %> - <% if valid_action?(:show, field.data.class) %> + <% if accessible_action?(field.data, :show) %> <%= link_to( field.display_associated_resource, [namespace, field.data], diff --git a/docs/authorization.md b/docs/authorization.md index 8e84875f0a..247199800b 100644 --- a/docs/authorization.md +++ b/docs/authorization.md @@ -49,23 +49,36 @@ end ## Authorization without Pundit -If you use a different authorization library, or you want to roll your own, -you just need to override a few methods in your controllers or -`Admin::ApplicationController`. For example: +Pundit is not necessary to implement authorization within Administrate. It is +simply a common solution that many in the community use, and for this reason +Administrate provides a plugin to work with it. However you can use a different +solution or roll out your own. + +To integrate a different authorization solution, you will need to +implement some methods in `Admin::ApplicationController` +or its subclasses. + +These are the methods to override, with examples: ```ruby -# Limit the scope of the given resource +# Used in listings, such as the `index` actions. It +# restricts the scope of records that a user can access. +# Returns an ActiveRecord scope. def scoped_resource super.where(user: current_user) end -# Raise an exception if the user is not permitted to access this resource -def authorize_resource(resource) - raise "Erg!" unless show_action?(params[:action], resource) -end - -# Hide links to actions if the user is not allowed to do them -def show_action?(action, resource) - current_user.can? action, resource +# Return true if the current user can access the given +# resource, false otherwise. +def authorized_action?(resource, action) + current_user.can?(resource, action) end ``` + +Additionally, the method `authorize_resource(resource)` +should throw an exception if the current user is not +allowed to access the given resource. Normally +you wouldn't need to override it, as the default +implementation uses `authorized_action?` to produce the +correct behaviour. However you may still want to override it +if you want to raise a custom error type. diff --git a/docs/customizing_controller_actions.md b/docs/customizing_controller_actions.md index 545bd91ad8..f9fedad26e 100644 --- a/docs/customizing_controller_actions.md +++ b/docs/customizing_controller_actions.md @@ -46,17 +46,22 @@ end ## Customizing Actions -To enable or disable certain actions you could override `valid_action?` method in your dashboard controller like this: +To disable certain actions globally, you can disable their +routes in `config/routes.rb`, using the usual Rails +facilities for this. For example: ```ruby -# disable 'edit' and 'destroy' links -def valid_action?(name, resource = resource_class) - %w[edit destroy].exclude?(name.to_s) && super +Rails.application.routes.draw do + # ... + namespace :admin do + # ... + + # Payments can only be listed or displayed + resources :payments, only: [:index, :show] + end end ``` -Action is one of `new`, `edit`, `show`, `destroy`. - ## Customizing Default Sorting To set the default sorting on the index action you could override `default_sorting_attribute` or `default_sorting_direction` in your dashboard controller like this: diff --git a/lib/administrate.rb b/lib/administrate.rb index 3dff5bd624..0a3c94ebb2 100644 --- a/lib/administrate.rb +++ b/lib/administrate.rb @@ -30,4 +30,12 @@ def self.warn_of_deprecated_method(klass, method) "does not use a deprecated API", ) end + + def self.warn_of_deprecated_authorization_method(method) + ActiveSupport::Deprecation.warn( + "The method `#{method}` is deprecated. " + + "Please use `accessible_action?` instead, " + + "or see the documentation for other options.", + ) + end end diff --git a/lib/administrate/engine.rb b/lib/administrate/engine.rb index 3961cf4f62..55a179758b 100644 --- a/lib/administrate/engine.rb +++ b/lib/administrate/engine.rb @@ -4,6 +4,8 @@ require "selectize-rails" require "sprockets/railtie" +require "administrate/namespace/resource" +require "administrate/not_authorized_error" require "administrate/page/form" require "administrate/page/show" require "administrate/page/collection" diff --git a/lib/administrate/not_authorized_error.rb b/lib/administrate/not_authorized_error.rb new file mode 100644 index 0000000000..bc65ab0705 --- /dev/null +++ b/lib/administrate/not_authorized_error.rb @@ -0,0 +1,18 @@ +module Administrate + class NotAuthorizedError < StandardError + def initialize(action:, resource:) + @action = action + @resource = resource + + case resource + when Module, String, Symbol + super("Not allowed to perform #{action.inspect} on #{resource.inspect}") + else + super( + "Not allowed to perform #{action.inspect} on the given " + + resource.class.name + ) + end + end + end +end diff --git a/spec/administrate/views/fields/belongs_to/_index_spec.rb b/spec/administrate/views/fields/belongs_to/_index_spec.rb index b3ea2cbbfb..5ea69f7c61 100644 --- a/spec/administrate/views/fields/belongs_to/_index_spec.rb +++ b/spec/administrate/views/fields/belongs_to/_index_spec.rb @@ -14,11 +14,25 @@ ) end + context "without an associated record" do + let(:belongs_to) do + instance_double( + "Administrate::Field::BelongsTo", + associated_class: associated_class, + data: nil, + ) + end + + it "displays nothing" do + render_belongs_to_index + expect(rendered.strip).to eq("") + end + end + context "if associated resource has a show route" do context "and the user has permission to access it" do it "displays link" do - allow(view).to receive(:valid_action?).and_return(true) - allow(view).to receive(:show_action?).and_return(true) + allow(view).to receive(:accessible_action?).and_return(true) render_belongs_to_index expect(rendered.strip).to include(link) end @@ -26,8 +40,7 @@ context "and the user does not have permission to access it" do it "hides link" do - allow(view).to receive(:valid_action?).and_return(true) - allow(view).to receive(:show_action?).and_return(false) + allow(view).to receive(:accessible_action?).and_return(false) render_belongs_to_index expect(rendered.strip).to_not include(link) end @@ -36,7 +49,7 @@ context "if associated resource has no show route" do it "hides link" do - allow(view).to receive(:valid_action?).and_return(false) + allow(view).to receive(:accessible_action?).and_return(false) render_belongs_to_index expect(rendered.strip).to_not include(link) end diff --git a/spec/administrate/views/fields/belongs_to/_show_spec.rb b/spec/administrate/views/fields/belongs_to/_show_spec.rb index b4d14bc917..f61a1ae416 100644 --- a/spec/administrate/views/fields/belongs_to/_show_spec.rb +++ b/spec/administrate/views/fields/belongs_to/_show_spec.rb @@ -17,8 +17,7 @@ context "if associated resource has a show route" do context "and the user has permission to access it" do it "displays link" do - allow(view).to receive(:valid_action?).and_return(true) - allow(view).to receive(:show_action?).and_return(true) + allow(view).to receive(:accessible_action?).and_return(true) render_belongs_to_show expect(rendered.strip).to include(link) end @@ -26,8 +25,7 @@ context "and the user does not have permission to access it" do it "hides link" do - allow(view).to receive(:valid_action?).and_return(true) - allow(view).to receive(:show_action?).and_return(false) + allow(view).to receive(:accessible_action?).and_return(false) render_belongs_to_show expect(rendered.strip).to_not include(link) end @@ -36,7 +34,7 @@ context "if associated resource has no show route" do it "hides link" do - allow(view).to receive(:valid_action?).and_return(false) + allow(view).to receive(:accessible_action?).and_return(false) render_belongs_to_show expect(rendered.strip).to_not include(link) end diff --git a/spec/administrate/views/fields/has_one/_index_spec.rb b/spec/administrate/views/fields/has_one/_index_spec.rb index 56a234ee9e..6093d745b2 100644 --- a/spec/administrate/views/fields/has_one/_index_spec.rb +++ b/spec/administrate/views/fields/has_one/_index_spec.rb @@ -1,41 +1,63 @@ require "rails_helper" describe "fields/has_one/_index", type: :view do - context "without an associated record" do - it "displays nothing" do - has_one = Administrate::Field::HasOne.new( - :product_meta_tag, - build(:product_meta_tag), - :index, - ) + let(:product) { create(:product) } + let(:product_path) { polymorphic_path([:admin, product]) } + let(:link) { "#{product.name}" } + let(:has_one) do + instance_double( + "Administrate::Field::HasOne", + data: product, + linkable?: true, + display_associated_resource: product.name, + ) + end - render( - partial: "fields/has_one/index", - locals: { field: has_one }, + context "without an associated record" do + let(:has_one) do + instance_double( + "Administrate::Field::HasOne", + linkable?: false, ) + end + it "displays nothing" do + allow(view).to receive(:accessible_action?).and_return(true) + render_has_one_index expect(rendered.strip).to eq("") end end - context "with an associated record" do - it "renders a link to the record" do - product = create(:product) - product_path = polymorphic_path([:admin, product]) - has_one = instance_double( - "Administrate::Field::HasOne", - data: product, - linkable?: true, - display_associated_resource: product.name, - ) + context "if associated resource has a show route" do + context "and the user has permission to access it" do + it "displays link" do + allow(view).to receive(:accessible_action?).and_return(true) + render_has_one_index + expect(rendered.strip).to include(link) + end + end - render( - partial: "fields/has_one/index", - locals: { field: has_one, namespace: :admin }, - ) + context "and the user does not have permission to access it" do + it "hides link" do + allow(view).to receive(:accessible_action?).and_return(false) + render_has_one_index + expect(rendered.strip).to_not include(link) + end + end + end - expected = "#{product.name}" - expect(rendered.strip).to eq(expected) + context "if associated resource has no show route" do + it "hides link" do + allow(view).to receive(:accessible_action?).and_return(false) + render_has_one_index + expect(rendered.strip).to_not include(link) end end + + def render_has_one_index + render( + partial: "fields/has_one/index", + locals: { field: has_one, namespace: :admin }, + ) + end end diff --git a/spec/administrate/views/fields/has_one/_show_spec.rb b/spec/administrate/views/fields/has_one/_show_spec.rb index 61cdcad0b2..76b7aeb986 100644 --- a/spec/administrate/views/fields/has_one/_show_spec.rb +++ b/spec/administrate/views/fields/has_one/_show_spec.rb @@ -2,6 +2,10 @@ require "administrate/field/has_one" describe "fields/has_one/_show", type: :view do + before do + allow(view).to receive(:accessible_action?).and_return(true) + end + context "without an associated record" do it "displays nothing" do has_one = Administrate::Field::HasOne.new( @@ -20,87 +24,123 @@ end context "with an associated record" do - it "renders a link to the record" do - product = create(:product) - product_path = polymorphic_path([:admin, product]) - nested_show = instance_double( - "Administrate::Page::Show", - resource: double( - class: ProductMetaTag, - ), - attributes: [], - resource_name: "Product Tag", - ) - has_one = instance_double( - "Administrate::Field::HasOne", - display_associated_resource: product.name, - data: product, - linkable?: true, - nested_show: nested_show, - ) + context "when linking the record is allowed" do + it "renders a link to the record" do + product = create(:product) + product_path = polymorphic_path([:admin, product]) + nested_show = instance_double( + "Administrate::Page::Show", + resource: double( + class: ProductMetaTag, + ), + attributes: [], + resource_name: "Product Tag", + ) + has_one = instance_double( + "Administrate::Field::HasOne", + display_associated_resource: product.name, + data: product, + linkable?: true, + nested_show: nested_show, + ) - render( - partial: "fields/has_one/show", - locals: { - field: has_one, - namespace: :admin, - resource_name: "product_meta_tag", - }, - ) + render( + partial: "fields/has_one/show", + locals: { + field: has_one, + namespace: :admin, + resource_name: "product_meta_tag", + }, + ) - link = "#{product.name}" - expect(rendered.strip).to include(link) - end + link = "#{product.name}" + expect(rendered.strip).to include(link) + end - it "renders nested attribute relationships" do - view.extend Administrate::ApplicationHelper - - product = create(:product) - page = create(:page, product: product) - - nested_has_many = instance_double( - "Administrate::Field::HasMany", - associated_collection: [page], - attribute: :page, - data: [page], - resources: [page], - html_class: "has-many", - name: "Page", - to_partial_path: "fields/has_many/index", - order_from_params: {}, - ) + it "renders nested attribute relationships" do + view.extend Administrate::ApplicationHelper - nested_show = instance_double( - "Administrate::Page::Show", - resource: double( - class: ProductMetaTag, - ), - attributes: [nested_has_many], - resource_name: "Product Tag", - ) + product = create(:product) + page = create(:page, product: product) - has_one = instance_double( - "Administrate::Field::HasOne", - display_associated_resource: product.name, - data: product, - linkable?: true, - nested_show: nested_show, - ) + nested_has_many = instance_double( + "Administrate::Field::HasMany", + associated_collection: [page], + attribute: :page, + data: [page], + resources: [page], + html_class: "has-many", + name: "Page", + to_partial_path: "fields/has_many/index", + order_from_params: {}, + ) - page_double = instance_double("Administrate::Page::Show") + nested_show = instance_double( + "Administrate::Page::Show", + resource: double( + class: ProductMetaTag, + ), + attributes: [nested_has_many], + resource_name: "Product Tag", + ) - render( - partial: "fields/has_one/show", - locals: { - field: has_one, - namespace: :admin, - page: page_double, - resource_name: "product_meta_tag", - }, - ) + has_one = instance_double( + "Administrate::Field::HasOne", + display_associated_resource: product.name, + data: product, + linkable?: true, + nested_show: nested_show, + ) + + page_double = instance_double("Administrate::Page::Show") + + render( + partial: "fields/has_one/show", + locals: { + field: has_one, + namespace: :admin, + page: page_double, + resource_name: "product_meta_tag", + }, + ) + + has_many_count = "1 page" + expect(rendered.strip).to include(has_many_count) + end + end + + context "when linking the record is not allowed" do + it "displays the record without a link" do + allow(view).to receive(:accessible_action?).and_return(false) + product = create(:product) + nested_show = instance_double( + "Administrate::Page::Show", + resource: double( + class: ProductMetaTag, + ), + attributes: [], + resource_name: "Product Tag", + ) + has_one = instance_double( + "Administrate::Field::HasOne", + display_associated_resource: product.name, + data: product, + linkable?: true, + nested_show: nested_show, + ) + + render( + partial: "fields/has_one/show", + locals: { + field: has_one, + namespace: :admin, + resource_name: "product_meta_tag", + }, + ) - has_many_count = "1 page" - expect(rendered.strip).to include(has_many_count) + expect(rendered.strip).to include(product.name) + expect(rendered.strip).not_to include("#{product.name}" } + let(:polymorphic) do + instance_double( + "Administrate::Field::Polymorphic", + data: product, + display_associated_resource: product.name, + ) + end - render( - partial: "fields/polymorphic/index", - locals: { field: polymorphic }, + context "without an associated record" do + let(:polymorphic) do + instance_double( + "Administrate::Field::Polymorphic", + data: nil, ) + end + it "displays nothing" do + render_polymorphic_index expect(rendered.strip).to eq("") end end - context "with an associated record" do - it "renders a link to the record" do - product = create(:product) - product_path = polymorphic_path([:admin, product]) - polymorphic = instance_double( - "Administrate::Field::Polymorphic", - data: product, - display_associated_resource: product.name, - ) + context "if associated resource has a show route" do + context "and the user has permission to access it" do + it "displays link" do + allow(view).to receive(:accessible_action?).and_return(true) + render_polymorphic_index + expect(rendered.strip).to include(link) + end + end - render( - partial: "fields/polymorphic/index", - locals: { field: polymorphic, namespace: :admin }, - ) + context "and the user does not have permission to access it" do + it "hides link" do + allow(view).to receive(:accessible_action?).and_return(false) + render_polymorphic_index + expect(rendered.strip).to_not include(link) + end + end + end - expected = "#{product.name}" - expect(rendered.strip).to eq(expected) + context "if associated resource has no show route" do + it "hides link" do + allow(view).to receive(:accessible_action?).and_return(false) + render_polymorphic_index + expect(rendered.strip).to_not include(link) end end + + def render_polymorphic_index + render( + partial: "fields/polymorphic/index", + locals: { field: polymorphic, namespace: :admin }, + ) + end end diff --git a/spec/administrate/views/fields/polymorphic/_show_spec.rb b/spec/administrate/views/fields/polymorphic/_show_spec.rb index 1b143a7c97..9e19f2d480 100644 --- a/spec/administrate/views/fields/polymorphic/_show_spec.rb +++ b/spec/administrate/views/fields/polymorphic/_show_spec.rb @@ -31,7 +31,7 @@ attribute: "product", ) - allow(view).to receive(:valid_action?).and_return(true) + allow(view).to receive(:accessible_action?).and_return(true) render( partial: "fields/polymorphic/show", diff --git a/spec/controllers/admin/application_controller_spec.rb b/spec/controllers/admin/application_controller_spec.rb index 8e6e20c5a1..4e87b88df9 100644 --- a/spec/controllers/admin/application_controller_spec.rb +++ b/spec/controllers/admin/application_controller_spec.rb @@ -1,41 +1,110 @@ require "rails_helper" -RSpec.describe Admin::OrdersController, type: :controller do - controller(Admin::OrdersController) do - def after_resource_destroyed_path(_requested_resource) - { action: :index, controller: :customers } +RSpec.describe Admin::ApplicationController, type: :controller do + describe "redirections after actions" do + controller(Admin::OrdersController) do + def after_resource_destroyed_path(_requested_resource) + { action: :index, controller: :customers } + end + + def after_resource_created_path(requested_resource) + [namespace, requested_resource.customer] + end + + def after_resource_updated_path(requested_resource) + [namespace, requested_resource.customer] + end end - def after_resource_created_path(requested_resource) - [namespace, requested_resource.customer] + it "redirect to custom route after destroy" do + order = create(:order) + + delete :destroy, params: { id: order.to_param } + expect(response).to redirect_to(admin_customers_path) end - def after_resource_updated_path(requested_resource) - [namespace, requested_resource.customer] + it "redirect to custom route after create" do + customer = create(:customer) + order_attributes = build(:order, customer: customer).attributes + params = order_attributes.except( + "id", + "created_at", + "updated_at", + "shipped_at", + ) + + post :create, params: { order: params } + expect(response).to redirect_to(admin_customer_path(customer)) + end + + it "redirect to custom route after update" do + order = create(:order) + order_params = { address_line_one: order.address_line_one } + + put :update, params: { id: order.to_param, order: order_params } + expect(response).to redirect_to(admin_customer_path(order.customer)) end end - it "redirect to custom route after destroy" do - order = create(:order) + describe "authorization" do + controller(Administrate::ApplicationController) do + def resource_class + Order + end - delete :destroy, params: { id: order.to_param } - expect(response).to redirect_to(admin_customers_path) + def dashboard + OrderDashboard + end + + def authorized_action?(resource, _action) + resource.address_zip == "666" + end + end + + it "authorizes allowed actions" do + resource = FactoryBot.create(:order, address_zip: "666") + expect { get :show, params: { id: resource.id } }. + not_to raise_error + end + + it "does not authorize disallowed actions" do + resource = FactoryBot.create(:order, address_zip: "667") + expect { get :show, params: { id: resource.id } }. + to raise_error(Administrate::NotAuthorizedError) + end end - it "redirect to custom route after create" do - customer = create(:customer) - order_attributes = build(:order, customer: customer).attributes - params = order_attributes.except("id", "created_at", "updated_at", "shipped_at") + describe "deprecated methods: show_action" do + controller(Administrate::ApplicationController) do + def index + show_action?(:index, Order) + end + end - post :create, params: { order: params } - expect(response).to redirect_to(admin_customer_path(customer)) + it "triggers a deprecation warning" do + allow(ActiveSupport::Deprecation).to receive(:warn) + get :index + expect(ActiveSupport::Deprecation).to( + have_received(:warn). + with(/`show_action\?` is deprecated/), + ) + end end - it "redirect to custom route after update" do - order = create(:order) - order_params = { address_line_one: order.address_line_one } + describe "deprecated methods: valid_action" do + controller(Administrate::ApplicationController) do + def index + valid_action?(:index, Order) + end + end - put :update, params: { id: order.to_param, order: order_params } - expect(response).to redirect_to(admin_customer_path(order.customer)) + it "triggers a deprecation warning" do + allow(ActiveSupport::Deprecation).to receive(:warn) + get :index + expect(ActiveSupport::Deprecation).to( + have_received(:warn). + with(/`valid_action\?` is deprecated/), + ) + end end end diff --git a/spec/controllers/admin/orders_controller_spec.rb b/spec/controllers/admin/orders_controller_spec.rb index 6bbe2f9588..8b16ba292b 100644 --- a/spec/controllers/admin/orders_controller_spec.rb +++ b/spec/controllers/admin/orders_controller_spec.rb @@ -82,21 +82,21 @@ def send_request(order:) end end - describe "#show_action?" do + describe "#authorized_action?" do it "shows edit actions for records by the user" do o = create(:order, customer: user) - expect(controller.show_action?(:edit, o)).to be true + 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.show_action?(:edit, o)).to be false + 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.show_action?(:destroy, o)).to be false + expect(controller.send(:authorized_action?, o, :destroy)).to be false end end end diff --git a/spec/example_app/app/views/admin/customers/_collection_header_actions.html.erb b/spec/example_app/app/views/admin/customers/_collection_header_actions.html.erb index 05c814078b..134b2dd130 100644 --- a/spec/example_app/app/views/admin/customers/_collection_header_actions.html.erb +++ b/spec/example_app/app/views/admin/customers/_collection_header_actions.html.erb @@ -1,7 +1,7 @@ <% [ - valid_action?(:edit, collection_presenter.resource_name), - valid_action?(:destroy, collection_presenter.resource_name), + existing_action?(collection_presenter.resource_name, :edit), + existing_action?(collection_presenter.resource_name, :destroy), true, # "Become" action ].count(true).times do %> diff --git a/spec/example_app/app/views/admin/customers/_collection_item_actions.html.erb b/spec/example_app/app/views/admin/customers/_collection_item_actions.html.erb index 9554cabdd6..a741a7290c 100644 --- a/spec/example_app/app/views/admin/customers/_collection_item_actions.html.erb +++ b/spec/example_app/app/views/admin/customers/_collection_item_actions.html.erb @@ -1,19 +1,19 @@ -<% if valid_action? :edit, collection_presenter.resource_name %> +<% if existing_action?(collection_presenter.resource_name, :edit) %> <%= link_to( t("administrate.actions.edit"), [:edit, namespace, resource], class: "action-edit", - ) if show_action? :edit, resource%> + ) if authorized_action?(resource, :edit) %> <% end %> -<% if valid_action? :destroy, collection_presenter.resource_name %> +<% if existing_action?(collection_presenter.resource_name, :destroy) %> <%= link_to( t("administrate.actions.destroy"), [namespace, resource], class: "text-color-red", method: :delete, data: { confirm: t("administrate.actions.confirm") } - ) if show_action? :destroy, resource %> + ) if authorized_action?(resource, :destroy) %> <% end %> diff --git a/spec/example_app/app/views/admin/customers/_index_header.html.erb b/spec/example_app/app/views/admin/customers/_index_header.html.erb index 8b54cc84d0..5716ec7908 100644 --- a/spec/example_app/app/views/admin/customers/_index_header.html.erb +++ b/spec/example_app/app/views/admin/customers/_index_header.html.erb @@ -28,7 +28,6 @@ ), [:new, namespace, page.resource_path.to_sym], class: "button", - ) if valid_action?(:new) && show_action?(:new, new_resource) %> + ) if accessible_action?(new_resource, :new) %> - diff --git a/spec/helpers/administrate/application_helper_spec.rb b/spec/helpers/administrate/application_helper_spec.rb index f2bbf42600..1d74e39e28 100644 --- a/spec/helpers/administrate/application_helper_spec.rb +++ b/spec/helpers/administrate/application_helper_spec.rb @@ -101,4 +101,90 @@ expect(sort_order("for anything else")).to eq("none") end end + + describe "#accessible_action?" do + context "when given a string target" do + it "checks the class it names with `existing_action?` and `authorized_action?`" do + class MyResource; end + ctx = double(existing_action?: true, authorized_action?: true) + ctx.extend(Administrate::ApplicationHelper) + + ctx.accessible_action?("my_resource", "foo") + + expect(ctx).to( + have_received(:existing_action?).with(:my_resource, "foo"), + ) + expect(ctx).to( + have_received(:authorized_action?).with(:my_resource, "foo"), + ) + ensure + remove_constants :MyResource + end + end + + context "when given a symbol target" do + it "checks the class it names with `existing_action?` and `authorized_action?`" do + class MyResource; end + ctx = double(existing_action?: true, authorized_action?: true) + ctx.extend(Administrate::ApplicationHelper) + + ctx.accessible_action?(:my_resource, "foo") + + expect(ctx).to( + have_received(:existing_action?).with(:my_resource, "foo"), + ) + expect(ctx).to( + have_received(:authorized_action?).with(:my_resource, "foo"), + ) + ensure + remove_constants :MyResource + end + end + + context "when given a class target" do + it "checks it with `existing_action?` and `authorized_action?`" do + class MyResource; end + ctx = double(existing_action?: true, authorized_action?: true) + ctx.extend(Administrate::ApplicationHelper) + + ctx.accessible_action?(MyResource, "foo") + + expect(ctx).to have_received(:existing_action?).with(MyResource, "foo") + expect(ctx).to( + have_received(:authorized_action?).with(MyResource, "foo"), + ) + ensure + remove_constants :MyResource + end + end + + context "when given an ActiveRecord::Base target" do + it "tests its class with `existing_action?` and the object with `authorized_action?`" do + ctx = double(existing_action?: true, authorized_action?: true) + ctx.extend(Administrate::ApplicationHelper) + + object = Product.new + ctx.accessible_action?(object, "foo") + + expect(ctx).to have_received(:existing_action?).with(Product, "foo") + expect(ctx).to have_received(:authorized_action?).with(object, "foo") + end + end + + context "when given an object target" do + it "checks its class with `existing_action?` and the object with `authorized_action?`" do + class MyResource; end + ctx = double(existing_action?: true, authorized_action?: true) + ctx.extend(Administrate::ApplicationHelper) + + object = MyResource.new + ctx.accessible_action?(object, "foo") + + expect(ctx).to have_received(:existing_action?).with(MyResource, "foo") + expect(ctx).to have_received(:authorized_action?).with(object, "foo") + ensure + remove_constants :MyResource + end + end + end end diff --git a/spec/lib/administrate/not_authorized_error_spec.rb b/spec/lib/administrate/not_authorized_error_spec.rb new file mode 100644 index 0000000000..a5196bf113 --- /dev/null +++ b/spec/lib/administrate/not_authorized_error_spec.rb @@ -0,0 +1,51 @@ +require "rails_helper" + +describe Administrate::NotAuthorizedError do + context "when the resource is a class or module" do + it "produces a message mentioning it directly" do + error = described_class.new( + resource: Administrate, + action: "foo", + ) + expect(error.message).to eq( + %{Not allowed to perform "foo" on Administrate}, + ) + end + end + + context "when the resource is a string" do + it "produces a message mentioning it directly" do + error = described_class.new( + resource: "User", + action: "foo", + ) + expect(error.message).to eq(%{Not allowed to perform "foo" on "User"}) + end + end + + context "when the resource is a symbol" do + it "produces a message mentioning it directly" do + error = described_class.new( + resource: :user, + action: "foo", + ) + expect(error.message).to eq(%{Not allowed to perform "foo" on :user}) + end + end + + context "when the resource is something else" do + it "produces a message that refers to the class of the resource" do + class TestStuff; end + + error = described_class.new( + resource: TestStuff.new, + action: "foo", + ) + expect(error.message).to eq( + %{Not allowed to perform "foo" on the given TestStuff}, + ) + ensure + remove_constants :TestStuff + end + end +end