valid_actions? doesn't work for has_many relations

[getaaron]

• What were you trying to do?

I have a handful of Roles in my db like Administrator, Author, etc. A User has_many Roles.

I don't want users of the admin page to be able to destroy or edit roles, just view them. So I followed the instructions on https://github.com/thoughtbot/administrate/blob/main/docs/customizing_controller_actions.md#customizing-actions and added this to the roles controller:

  class RolesController < Admin::ApplicationController
    def valid_action?(name, resource = resource_class)
      %w[edit destroy].exclude?(name.to_s) && super
    end

I expected this to remove the edit and destroy buttons.

• What did you end up with (logs, or, even better, example apps are great!)?

Indeed, on the Roles page, the edit button is now gone.

However, on the Users dashboard page, I can still see Edit and Destroy buttons:

And they work, which unfortunately I think means that both:

1. the view code needs to be updated for the has_many table
2. the controller is not validating valid_action? — it's a frontend check only, which is a security concern

• What versions are you running?
  • Rails 5.0.7.2
  • administrate 9e462f7 (latest on main, also reproduces with 0.16.0 release)

[pablobm]

Working on this at #1941. Progress has been slow as personal issues have kept me quite busy. I'm looking into it again.

[DucMirack]

Hello !

I allow myself to reopen the subject as the problem still exists here.

We are in a similar case as @getaaron , we have a Field::HasMany in a show view which allow users to do action they are not supposed to be allowed to.

The possibility for an object to realize an action is conditioned by #authorized_action? which is a defined in a controller. But we need to handle 2 different resource types in the #authorized_action? of 1 controller.

The second resource (the one of the Field::HasMany) should have it's authorized actions handled in it's controller and not on it's parent one.

To put this in code, we have :

class UserDashboard < Administrate::DashboardBase
  ATTRIBUTES_TYPES = {
     ...
     roles: Field::HasMany,
     ...
  }
end

class UsersController
  ...
  def authorized_action?(resource, action)
    if resource.is_a?(User)
      ...
    elsif resource.is_a?(Role)
      ...
    end
  end
end

Is there any workaround for this behavior ?

Let me know if this isn't clear enough, have a nice day :)

[pablobm]

Thank you for your thorough descriptions, and apologies for not coming back to this one after I merged #1941.

This issue should be solvable by delegating the question to a separate class (or classes) that decide on the permissions based on the resource type. It's similar to the list of ifs presented by @DucMirack, but at a global level. For example, adding it to Admin::ApplicationController instead of the specific controller.

This is what the Punditize mixin does. Instead of answering the question on the controller, it delegates the question to a policy based on the resource type. In the example app, for an Order it asks OrderPolicy about the permissions. This will be used regardless of whether we are on the order controller, the customer controller, or any other.

If you can use Pundit, that would be the simplest way to do this. If you cannot for any reason, you can implement this similarly yourselves. Does that make sense?

I'm going to close this, as I think it's solved, but please do let us know if this doesn't work for you for any reason, and we can look into it again.

[DucMirack]

Hey @pablobm , thanks for the answer, i'll write a ticket on our backlog to implement the solution you propose and we'll come back to you if we face any issue :)

[pablobm]

Actually, I'm reopening this.

What I described is true for the interface: when to show the "Edit" and "Destroy" controls. However it's not really true when sending requests that actually modify data. An attacker could send a forged POST request that modifies data, and that wouldn't be stopped by these protections.

From what I see, this will affect any models with an accepts_nested_attributes_for declaration. You could also be affected if you have customisations in our admin controllers that accept data to modify resources other than the "main" resource of a given dashboard (but this can't be helped).

Out of the box, the most affected resources would be those that appear as a Field::HasOne of another one, as that is the only situation in which Administrate renders a nested form. For example, in the demo app. ProductMetaTag could be modified through the Product dashboard, with a manually forged request.

Additionally, the popular plugin https://github.com/nickcharlton/administrate-field-nested_has_many will also be affected, as it leans on accepts_nested_attributes_for to work.

A solution would be to modify the create and update actions of Administrate::Applicationcontroller to inspect the model for uses of accepts_nested_attributes_for, and run the authorization checks there.

[DucMirack]

Hey !
I come back to propose the solution we implemented on our side :
We've created a Authorizer class which is responsible for calling the corresponding method on the corresponding authorizer class depending on the resource
For example, if we ask an authorization for an Account record on the create action, it would call AccountAuthorizer.create
Each model which is eligible to be asked for the action authorization would get it's own authorizer class(see example above)

The top class Authorizer is to be called on app/controllers/admin/application_controller.rb and the authorized_action? which is the method called by default for each authorization. Here we override it with our own behaviour

To put this into code :

app/controllers/admin/application_controller.rb

    def authorized_action?(resource, action)
      Authorizer.new(resource).authorize?(action)
    end

app/authorizers/authorizer.rb

class Authorizer
  def initialize(resource)
    @resource = resource
  end

  def authorize?(action)
    return true unless authorizer_class
    return true unless authorizer_class.method_defined?(action.to_sym)
    authorizer_class.new(@resource).send(action.to_sym)
  end

  private

  def authorizer_class
    class_name = if @resource.is_a?(Class)
      "#{@resource.name.pluralize}Authorizer"
    else
      "#{@resource.class.name.pluralize}Authorizer"
    end
    class_name.safe_constantize
  end
end

app/authorizers/accounts_authorizer.rb

class AccountsAuthorizer
  def initialize(resource)
    @resource = resource
  end

  def edit
    @resource.editable?
  end
end

[pablobm]

Thank you for your comment @DucMirack. Your solutions looks very similar to how Pundit works, which is officially supported by Administrate. You may want to check that out.

[pablobm]

I've been looking at the issue I mentioned above on 29 Sep, regarding accepts_nested_attributes_for. This is a tricky one.

The problem

Say that Product has_one ProductMetaTag. It's reasonable to think that some users, while having access to a given Product, may or may not have access to its associated ProductMetaTag.

As a reminder, Administrate displays has_one relationships in a nested fashion, so in this example you can see and edit all details of a ProductMetaTag while seeing/editing its Product.

Say that, for a given user, they can see a specific Product but not its associated ProductMetaTag. Administrate currently has no way to hide the ProductMetaTag on one Product while showing it on another. This is because authorization works at the resource level, not the attribute level. When accessing a ProductMetaTag from a Product page, the ProductMetaTag is handled as an attribute of Product. Therefore we cannot stop anyone from seeing it, even if in theory the user is not authorised to see the specific ProductMetaTag resource.

The same applies in form pages. If you are editing a Product but you shouldn't have access to its ProductMetaTag, we have no way to stop Administrate from displaying this association as a nested form.

What could be possible more easily

A different thing would be modifying Administrate to block actual updates to unauthorized associated resources. Say that the user submits the form with unauthorized changes to the ProductMetaTag. At the update action we can examine the input data, read the product_meta_tag_attributes, and decide whether they are being made to an allowed resource or not.

This is still a pretty bad UX. The user is shown a form, but we block any changes. We shouldn't have shown a UI suggesting that changes could be made in the first place.

Practical issues

But although this in theory is ok, there's a practical matter: checking the authorization of the associated ProductMetaTag would require reading it from the DB and instantiating it. Is that a cost that would be ok to incur?

There's also the case of the plugin administrate-field-nested_has_many:

1. It requires checking permissions not for one, but for many associated resources.
2. Does it even make sense to hide some associated records but not some others?
   • I'm sure there's a use case somewhere for this, but to which extent should Administrate go out of its way in order to support it?

My current feeling is that, in Rails and in general, the moment that you use accepts_nested_attributes_for you are accepting that the user has access to the nested resources. The conventional line of defence is strong_parameters, and at that point you can't (conventionally) stop to check the individual input parameters, but rather you accept them in bulk. Any finer-grained checks would have to be done at a later stage, and I feel that they would smell a bit funny.

An alternative (currently supported)

I think that a more common use case here would be "some admins can access all ProductMetaTags, while the rest can't access any". In other words: all or nothing. This can be implemented with Administrate already, leaning on Rails conventions. It goes like this:

1. Create an additional AdminProduct model, with its own controller and dashboard.
2. The AdminProductDashboard will have ProductMetaTag among its attributes, while ProductDashboard will not.
3. Set up authorization so that some users can access the AdminProductDashboard while others can't.

Example of the above, based on the demo app

diff --git a/spec/example_app/app/controllers/admin/admin_products_controller.rb b/spec/example_app/app/controllers/admin/admin_products_controller.rb
new file mode 100644
index 00000000..ea7bfa6b
--- /dev/null
+++ b/spec/example_app/app/controllers/admin/admin_products_controller.rb
@@ -0,0 +1,4 @@
+module Admin
+  class AdminProductsController < Admin::ProductsController
+  end
+end
diff --git a/spec/example_app/app/controllers/admin/products_controller.rb b/spec/example_app/app/controllers/admin/products_controller.rb
index c0ad6ed5..ac7078b3 100644
--- a/spec/example_app/app/controllers/admin/products_controller.rb
+++ b/spec/example_app/app/controllers/admin/products_controller.rb
@@ -3,7 +3,7 @@ module Admin
     private
 
     def find_resource(param)
-      Product.find_by!(slug: param)
+      resource_class.find_by!(slug: param)
     end
   end
 end
diff --git a/spec/example_app/app/dashboards/product_dashboard.rb b/spec/example_app/app/dashboards/product_dashboard.rb
index 9287b65a..53d68fd0 100644
--- a/spec/example_app/app/dashboards/product_dashboard.rb
+++ b/spec/example_app/app/dashboards/product_dashboard.rb
@@ -7,7 +7,6 @@ class ProductDashboard < Administrate::BaseDashboard
     :price,
     :description,
     :image_url,
-    :product_meta_tag,
     :release_year,
   ]
 
@@ -19,7 +18,6 @@ class ProductDashboard < Administrate::BaseDashboard
     name: Field::String,
     pages: Field::HasMany,
     price: Field::Number.with_options(prefix: "$", decimals: 2),
-    product_meta_tag: Field::HasOne,
     release_year: Field::Select.with_options(
       collection: -> { (Time.current.year - 10)..Time.current.year },
     ),
diff --git a/spec/example_app/app/models/admin_product.rb b/spec/example_app/app/models/admin_product.rb
new file mode 100644
index 00000000..f61c0c20
--- /dev/null
+++ b/spec/example_app/app/models/admin_product.rb
@@ -0,0 +1,11 @@
+class AdminProduct < Product
+  accepts_nested_attributes_for :product_meta_tag
+
+  def self.policy_class=(policy)
+    @admin_product_policy_class = policy
+  end
+
+  def self.policy_class
+    @admin_product_policy_class ||= AdminProductPolicy
+  end
+end
diff --git a/spec/example_app/app/models/admin_product_dashboard.rb b/spec/example_app/app/models/admin_product_dashboard.rb
new file mode 100644
index 00000000..a1793cf0
--- /dev/null
+++ b/spec/example_app/app/models/admin_product_dashboard.rb
@@ -0,0 +1,35 @@
+require "administrate/base_dashboard"
+
+class AdminProductDashboard < Administrate::BaseDashboard
+  ATTRIBUTES = [
+    :name,
+    :pages,
+    :price,
+    :description,
+    :image_url,
+    :product_meta_tag,
+    :release_year,
+  ]
+
+  ATTRIBUTE_TYPES = {
+    created_at: Field::DateTime,
+    updated_at: Field::DateTime,
+    description: Field::Text,
+    image_url: Field::Url,
+    name: Field::String,
+    pages: Field::HasMany,
+    price: Field::Number.with_options(prefix: "$", decimals: 2),
+    product_meta_tag: Field::HasOne,
+    release_year: Field::Select.with_options(
+      collection: -> { (Time.current.year - 10)..Time.current.year },
+    ),
+  }
+
+  COLLECTION_ATTRIBUTES = ATTRIBUTES
+  FORM_ATTRIBUTES = ATTRIBUTES
+  SHOW_PAGE_ATTRIBUTES = ATTRIBUTES
+
+  def display_resource(resource)
+    resource.name
+  end
+end
diff --git a/spec/example_app/app/models/product.rb b/spec/example_app/app/models/product.rb
index 7dc56761..9b3d6a4b 100644
--- a/spec/example_app/app/models/product.rb
+++ b/spec/example_app/app/models/product.rb
@@ -1,10 +1,10 @@
 class Product < ApplicationRecord
   def self.policy_class=(policy)
-    @policy_class = policy
+    @product_policy_class = policy
   end
 
   def self.policy_class
-    @policy_class ||= ProductPolicy
+    @product_policy_class ||= ProductPolicy
   end
 
   has_many :line_items, dependent: :destroy
@@ -26,8 +26,6 @@ class Product < ApplicationRecord
   validates :product_meta_tag, presence: true, on: :some_unclear_situation
   validate :valid_slug
 
-  accepts_nested_attributes_for :product_meta_tag
-
   def name=(value)
     self.slug = value.to_s.parameterize
     super(value)
diff --git a/spec/example_app/app/policies/admin_product_policy.rb b/spec/example_app/app/policies/admin_product_policy.rb
new file mode 100644
index 00000000..79a7629b
--- /dev/null
+++ b/spec/example_app/app/policies/admin_product_policy.rb
@@ -0,0 +1,40 @@
+class AdminProductPolicy < ApplicationPolicy
+  def index?
+    user.admin?
+  end
+
+  def show?
+    user.admin?
+  end
+
+  def create?
+    user.admin?
+  end
+
+  def update?
+    user.admin?
+  end
+
+  def destroy?
+    user.admin?
+  end
+
+  def scope
+    Pundit.policy_scope!(user, record.class)
+  end
+
+  # I'm not sure that the scope is necessary in this case,
+  # but adding it here just in case
+  class Scope < Scope
+    attr_reader :user, :scope
+
+    def initialize(user, scope)
+      @user = user
+      @scope = scope
+    end
+
+    def resolve_admin
+      user.admin? ? scope : scope.none
+    end
+  end
+end
diff --git a/spec/example_app/config/routes.rb b/spec/example_app/config/routes.rb
index f15e62d7..86599bb0 100644
--- a/spec/example_app/config/routes.rb
+++ b/spec/example_app/config/routes.rb
@@ -11,6 +11,7 @@ Rails.application.routes.draw do
     resources :pages
     resources :products
     resources :product_meta_tags
+    resources :admin_products
     resources :payments, only: [:index, :show]
     resources :series
 

Conclusion

So with all the above in mind, there's the question: should we be implementing something to allow the more fine-grained use cases, or are we ok as we are?

In any case, we will have to implement something in the future so that individual fields receive more context than currently do. Several issues have been filed already about things like showing some fields to some users and not some others, which would require this additional context. I think those make sense, so we may get here eventually anyway. I guess it's more of a matter of priority.