Unify Action Checks (#1941)

When referring to a route in the code, we run two checks:

* `valid_action?` is `true` if the route is defined, `false` otherwise.
* `show_action?` is expected to be overridden by developers in order to
  implement authorization. For example, it's implemented by
  `Administrate::Punditize` in order to integrate Administrate with Pundit.
  It should return `true` if the current user can access a given route,
  `false` otherwise.

These two check should (almost) always happen together. For this reason,
our code is peppered with `if` statements where both are checked... and a
few others where we forget one or the other.

These checks should be unified into a single method call, in order to avoid
issues like the one described at #1861. This introduces a new method, called
`accessible_action?`.

The original methods should still exist, as they do have their uses
individually. The new method will delegate to the existing ones.

We also rename the two existing methods to something that will make their
intent clear:

* `valid_action?` becomes `existing_action?`
* `show_action?` becomes `authorized_action?`

In order to provide a clear upgrade path, the old names still exist and
work, but they show a deprecation warning when used. They can be removed
properly at a later version of Administrate.

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)

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

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,

app/views/administrate/application/_collection.html.erb

@@ -59,13 +59,13 @@ to display a collection of resources in an HTML table.
   <tbody>
     <% resources.each do |resource| %>
       <tr class="js-table-row"
-          <% if valid_action?(:show, resource.class) && show_action?(:show, 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| %>
           <td class="cell-data cell-data--<%= attribute.html_class %>">
-            <% if valid_action?(:show, resource.class) && show_action?(:show, resource) -%>
+            <% if accessible_action?(resource, :show) -%>
               <a href="<%= polymorphic_path([namespace, resource]) -%>"
                  tabindex="-1"
                  class="action-show"

app/views/administrate/application/_collection_header_actions.html.erb

@@ -1,4 +1,4 @@
-<% [valid_action?(:edit, collection_presenter.resource_name),
-    valid_action?(:destroy, collection_presenter.resource_name)].count(true).times do %>
+<% [existing_action?(collection_presenter.resource_name, :edit),
+    existing_action?(collection_presenter.resource_name, :destroy)].count(true).times do %>
   <th scope="col"></th>
 <% end %>

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) %>
   <td><%= link_to(
     t("administrate.actions.edit"),
     [:edit, namespace, resource],
     class: "action-edit",
-  ) if show_action?(:edit, resource) %></td>
+  ) if accessible_action?(resource, :edit) %></td>
 <% end %>
 
-<% if valid_action?(:destroy, collection_presenter.resource_name) %>
+<% if existing_action?(collection_presenter.resource_name, :destroy) %>
   <td><%= 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) %></td>
+  ) if accessible_action?(resource, :destroy) %></td>
 <% end %>

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) %>
   </div>
 </header>

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 %>
 </nav>

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) %>
   </div>
 </header>
 

app/views/administrate/application/show.html.erb

@@ -28,15 +28,15 @@ 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"),
       [namespace, page.resource],
       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) %>
   </div>
 </header>
 

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],

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],

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],
   ) %>

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? %>
   <fieldset class="attribute--nested">
     <legend>
-      <%= link_to(
+      <%= link_to_if(
+        accessible_action?(field.data, :show),
         field.display_associated_resource,
         [namespace, field.data],
       ) %>

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]
   ) %>

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],

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.

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:

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

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"

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