thoughtbot / administrate

A Rails engine that helps you put together a super-flexible admin dashboard.
http://administrate-demo.herokuapp.com
MIT License
5.9k stars 1.12k forks source link

Restricting specific fields visible in show/edit #1862

Open sedubois opened 3 years ago

sedubois commented 3 years ago

The authorization/pundit system allows to restrict which resources an admin can view/edit, but how to authorize them to only view/edit specific fields within specific records? The authorization doc does not seem to cover this.

I did not find a way to do this.

pablobm commented 3 years ago

Have a look at the documentation for Pundit. A policy object typically includes the current user and the resource to be authorized. In Administrate, we have an ApplicationPolicy superclass, which stores both as @user and @record:

https://github.com/thoughtbot/administrate/blob/c40c317678ca3e0b140d09deb81842948e498443/spec/example_app/app/policies/application_policy.rb#L4-L7

Then these can be used in the policy methods. Here's an example from the example app:

https://github.com/thoughtbot/administrate/blob/c40c317678ca3e0b140d09deb81842948e498443/spec/example_app/app/policies/order_policy.rb#L16-L18

Does this answer your question? Perhaps we could improve the documentation on this point.

pablobm commented 3 years ago

Oh, wait. You say fields, not records! Apologies, I see now...

I don't think there's a way to do it at the moment, no :thinking:

pablobm commented 3 years ago

Thinking a bit about this, perhaps a way to achieve it would be to use different dashboards for different users. Say that you have a User model, and only admins can access some information. The UserDashboard could have limited fields, whereas another ActiveRecord model, AdministrableUser (or something like that) would inherit from User and have its own dashboard, while only accessible by admins.

I haven't tried this though, and it would quickly get out of hand if there are many fields to authorize across many models. However, I think we should try out these solutions first before complicating Administrate's internal APIs in order to add more options that will rarely be used. Ultimately we may implement new APIs, but we'll benefit from trying other things first.

Would this solution work for you?

sedubois commented 3 years ago

Thanks @pablobm for sharing your thoughts on this. In practice I probably would not be willing to make several copies of the same dashboard as it likely would quickly get out of hand and lead to more issues than it solves. It would also not cover all scenarios, such as the second and third points below.

Here are some use cases:

NB: we already had a discussion in another issue about making dashboards inheritable/extensible (don't know where to find it however). It might not be an ideal solution but might be an improvement on the idea of just copying the dashboards. NB2: I use Pundit and it's not clear what part of this problem should be handled at the policy level and what part should be dealt with through dashboard configuration.

pablobm commented 3 years ago

Good thinking. To add to the use cases, here's a generalisation of your second one:

The third one is probably the trickiest, due to the reasons I explained at #1877. However there may be a way to tackle the other two... here's a little hack I just tried.

  1. Copy Administrate template for app/views/administrate/application/_form.html.erb into your project, so that Rails picks your version instead of the original.
  2. In the template, find the block where it loops through the page.attributes, and add a condition.

For example, I just tried the following to hide the field "name" in the form for "customer":

  <% page.attributes.each do |attribute| -%>
    <% unless page.resource.is_a?(Customer) && attribute.attribute == :name %>
      <div class="field-unit field-unit--<%= attribute.html_class %> field-unit--<%= requireness(attribute) %>">
        <%= render_field attribute, f: f %>
      </div>
    <% end %>
  <% end -%>

If you want security, you'll also want to add checks at the update and create actions to ensure that nobody tries to post info that they are not allowed to post.

If you are using an authentication mechanism, you should be able to access current_user from there and defer to Pundit to make authorisation decisions.

So that's a first step! Now this could be improved...

  1. We could split Administrate's templates into more partials than we currently do. This way people could customise smaller parts of them with less risk of the templates changing in a new version and having to change the customisations too.
  2. We could add a hook to dashboards, which would be used in a condition like the one I propose above. Then users wouldn't need to customise their templates. The question here is: what information (parameters) should this hook have?
sedubois commented 3 years ago

Thank you @pablobm, that is helpful. About point 2, I guess there needs to be access to the field, which itself knows about the attribute name, data, page, and resource, to which could be added the action. So we could define a method show_field?(field, action) in Administrate::ApplicationController similar to the existing show_action? which would always return true but could be overwritten with custom logic which could delegate to field-level Pundit policies if needed (e.g. to differentiate between users):

  <% page.attributes.select { |field| show_field?(field, action) }.each do |field| -%>
    <div class="field-unit field-unit--<%= field.html_class %> field-unit--<%= requireness(field) %>">
      <%= render_field field, f: f %>
    </div>
  <% end -%>

NB: in the code above I've used field instead of attribute to be consistent with what seemed to be their type (Administrate::Field::Base). Ideally page.attributes should be renamed page.fields? NB2: in Administrate there is valid_action? and show_action?, not sure what's the difference....

pablobm commented 3 years ago

Sorry for the delay. This is a difficult problem that requires some dedicated thinking...

Starting with your notate bene:

  1. I agree that page.fields would be more descriptive. I have been confused at that before.
  2. For the difference between valid_action? and show_action?, see my ongoing work to clarify them: https://github.com/thoughtbot/administrate/pull/1941

As for where that hypothetical show_field? method should live. I'm not sure that it should be in the controller. Perhaps it should be in the dashboard? This is: Admininistrate::BaseDashboard#show_field?. One reason is that it would avoid mixing code affecting several different dashboards in the same method. Additionally, it may be necessary (I can't recall with certainty) when dealing with nested forms, like the ones provided by Field::HasOne

pablobm commented 3 years ago

See https://github.com/thoughtbot/administrate/issues/1949 for another use case: loading fields depending on the application domain name.

getaaron commented 2 years ago

A nice API for this might be to provide skip: or only: options in the dashboard file, e.g.

customer: Field::HasOne.with_options(skip: [:name]),

or

customer: Field::HasOne.with_options(only: [:id, :favorite_color]),

In addition to an array, these could take a callback to return fields to show/hide, for example if you wanted to call Pundit or whatever:

customer: Field::HasOne.with_options(skip: ->(field) { Pundit.whatever ? [:name] : [] }),
customer: Field::HasOne.with_options(skip: ->(field) { Pundit.whatever ? [] : [:id, :favorite_color] }),
DavidGeismarLtd commented 1 year ago

Has there been any evolution on this ?

Nitr commented 1 year ago

I did it like this.

module AttributesFilter
  extend ActiveSupport::Concern

  included do
    def form_attributes
      apply_attributes_filters(super, :form)
    end

    def show_page_attributes
      apply_attributes_filters(super, :show)
    end

    def collection_attributes
      apply_attributes_filters(super, :collection)
    end

    private
    def apply_attributes_filters(attributes, action)
      self.class.filters.each do |key, filters|
        if key.nil? || key == action
          filters.each do |filter|
            attributes = filter.call(attributes)
          end
        end
      end
      attributes
    end
  end

  class_methods do
    def filters
      @filters ||= {}
      @filters.dup
    end

    protected
    def filter_attributes(action = nil, &block)
      @filters ||= {}
      @filters[action] ||= []
      @filters[action] << block
    end
  end
end

class MyDashboard < ApplicationDashboard
  include AttributesFilter

  filter_attributes do |attributes|
     attributes -= %i[some_attribute] unless Current.user.admin?
     attributes
  end

  filter_attributes :collection do |attributes|
     # ....
  end

  filter_attributes :show do |attributes|
     # ....
  end

  filter_attributes :form do |attributes|
     # ....
  end
end
sedubois commented 3 months ago

Thanks @Nitr, this workaround is helpful for filtering administrate dashboard attributes based on some runtime condition. It can be used to show the localized version of some attributes which can't be localized at the model level (we use Mobility for translations but there is no integration with ActiveStorage, and some attachments need to change per language).

class ContentDashboard < BaseDashboard
  include Admin::AttributesFilter

  ATTRIBUTE_TYPES = {
    # ...
    media_en: Field::ActiveStorage.with_options(...),
    media_fr: Field::ActiveStorage.with_options(...),
  }

  # ...

  SHOW_PAGE_ATTRIBUTES = %i[
    ...
    media_en
    media_fr
  ]

  # ...

  filter_attributes do |attributes|
    attributes -= %i[media_fr] unless I18n.locale == :fr
    attributes -= %i[media_en] unless I18n.locale == :en
    attributes
  end
end

NB: I had to update form_attributes as follows to avoid an error wrong number of arguments (given 1, expected 0) when editing a record.

module Admin::AttributesFilter
  extend ActiveSupport::Concern

  included do
    def form_attributes(action = nil)
      apply_attributes_filters(super, :form)
    end

    def show_page_attributes
      apply_attributes_filters(super, :show)
    end

    def collection_attributes
      apply_attributes_filters(super, :collection)
    end

    private
    def apply_attributes_filters(attributes, action)
      self.class.filters.each do |key, filters|
        if key.nil? || key == action
          filters.each do |filter|
            attributes = filter.call(attributes)
          end
        end
      end
      attributes
    end
  end

  class_methods do
    def filters
      @filters ||= {}
      @filters.dup
    end

    protected
    def filter_attributes(action = nil, &block)
      @filters ||= {}
      @filters[action] ||= []
      @filters[action] << block
    end
  end
end