How to enable/disable actions based on CanCan abilities? #355

Closed
imella opened this Issue Aug 10, 2011 · 17 comments

Comments

Projects
None yet
6 participants

imella commented Aug 10, 2011

Hi, I'm using CanCan for user authorization, so I can have different types of users access the admin interface.

So is there a way to enable/disable actions based on the abilities defined in the ability model?

I tried to pass a condition to the actions method, like with the menu, but no luck.

actions :all, :if => proc { can? :manage, Project }
Contributor

tobyhede commented Aug 12, 2011

This should work:

controller.authorize_resource

menu :if => lambda{|tabs_renderer|
  controller.current_ability.can?(:manage, EventType)
}

Taken from: gregbell#72 (comment)

Contributor

tobyhede commented Aug 12, 2011

You might need to be using the latest master to get this working, though.

In your Gemfile:

gem 'activeadmin', :git => 'https://github.com/gregbell/active_admin.git'

imella commented Aug 12, 2011

Hi @tobyhede, thanks for the reply. Yeah that currently works, and hides the tab menu.

But I would like to disable some actions for users with certain privileges. Like, some group can edit and create Projects for example, but another can only see those (and hiding the buttons for those actions). Or in an index action, see only the projects that the current user has created.

@imella imella closed this Aug 12, 2011

@imella imella reopened this Aug 18, 2011

Contributor

tobyhede commented Aug 23, 2011

I am still working on hiding the buttons, but have managed to scope index and resource views to roles.

    ActiveAdmin.register Event do
      controller.load_and_authorize_resource :except => :index

      controller do
        def scoped_collection
          end_of_association_chain.accessible_by(current_ability)
        end
      end
    end

With the following in my ability.rb:

can [:read, :create, :update, :destroy], Model, :user_id => user.id

Works really well, and in scoped collection you can actually add other conditions to the query (in my case I am using includes to bring down associated models more efficiently)

Contributor

tobyhede commented Aug 23, 2011

Ok, I may have got the actions working as well, although it is not pretty at this stage. I just duck-punch my way through:

    module ActiveAdmin

      class Resource
        module ActionItems

          alias_method :original_add_default_action_items, :add_default_action_items unless method_defined?(:original_add_default_action_items)

          private

          def add_default_action_items
            add_action_item :except => [:new, :show] do
              if controller.current_ability.can?(:create, active_admin_config.resource_name)
                if controller.action_methods.include?('new')
                  link_to(I18n.t('active_admin.new_model', :model => active_admin_config.resource_name), new_resource_path)
                end
              end
            end

            # Edit link on show
            add_action_item :only => :show do
             if controller.current_ability.can?(:update, resource)
                if controller.action_methods.include?('edit')
                  link_to(I18n.t('active_admin.edit_model', :model => active_admin_config.resource_name), edit_resource_path(resource))
                end
              end
            end

            # Destroy link on show
            add_action_item :only => :show do
              if controller.current_ability.can?(:destroy, resource)
                if controller.action_methods.include?("destroy")
                  link_to(I18n.t('active_admin.delete_model', :model => active_admin_config.resource_name),
                    resource_path(resource),
                    :method => :delete, :confirm => I18n.t('active_admin.delete_confirmation'))
                end
              end
            end
          end

        end
      end

    end
Contributor

DMajrekar commented Sep 27, 2011

I've been working on something similar to this and come up with the following:

DMajrekar/active_admin@2b75484

I'm happy to add tests etc but would like some feedback on the implementation before doing so.

dgutov commented Sep 30, 2011

I think a nicer way to hack this is override action_methods in the controller class, it also takes care of the other 2 places this method is called. My monkeypatch:

module ActiveAdmin
  class ResourceController < ::InheritedResources::Base
    class << self
      attr_reader :restricted_actions, :restricted_actions_check

      def restrict_actions(*actions, &block)
        @restricted_actions = actions
        @restricted_actions_check = block
      end
    end

    def action_methods
      methods = self.class.action_methods
      restricted_actions = self.class.restricted_actions
      if restricted_actions
        methods.dup.delete_if do |method|
          sym = method.to_sym
          restricted_actions.include?(sym) and !current_active_admin_user? ||
            !instance_exec(sym, params[:id],
                           &self.class.restricted_actions_check)
        end
      else
        methods
      end
    end
  end

  class DSL
    delegate :restrict_actions, :to => :controller
  end
end

From here, you can call restrict_actions in the ActiveAdmin.register block.

Contributor

DMajrekar commented Oct 3, 2011

@dgutov, that does seem like a nicer way of doing things. I've updated the monkey path to take if / unless in the restrict_actions method. I prefer this as I think it makes the code easier to read.

I've also updated the restrict_actions method to allow multiple calls with different actions / blocks in the ActiveAdmin.register block.

Seeing that instance_exec is being used to evaluate the block, I've removed all args being passed to the evaluated block, the params etc are available as they would be inside any other instance method within the controller.

module ActiveAdmin
  class ResourceController < ::InheritedResources::Base
    class << self
      attr_reader :restricted_actions

      def restrict_actions(*args)
        options = args.extract_options!

        @restricted_actions ||= {}
        args.each do |action_name|
          @restricted_actions[action_name.to_s] = options
        end
      end
    end

    def action_methods
      methods = self.class.action_methods.dup

      if self.class.restricted_actions
        self.class.restricted_actions.each do |method_name, options|
          methods.delete(method_name) if options[:if]     &&  instance_exec(&options[:if])
          methods.delete(method_name) if options[:unless] && !instance_exec(&options[:unless])
        end
      end

      return methods
    end
  end

  class DSL
    delegate :restrict_actions, :to => :controller
  end
end

Usage:

ActiveAdmin.register ChangeRequest do
  restrict_actions :edit, :destroy, :if => lambda { params[:id] && resource.should_be_restricted? }
end

Thoughts?

dgutov commented Oct 3, 2011

Looks good to me.

Two other things you might want to do:

  1. Cache this method's return value for the lifetime of the request. Not sure how to do that best.
  2. Redefine ResourceController#only_render_implemented_actions. It's called before other filters, and the programmer might want to redirect the users to authentication page for restricted actions. In case their session/remember token has timed out, for example.
    Or maybe move it below "before_filter :authenticate_active_admin_user".
Contributor

DMajrekar commented Oct 4, 2011

I think the following should address 1) and 2):

module ActiveAdmin
  class ResourceController < ::InheritedResources::Base
    class << self
      attr_reader :restricted_actions_config

      def restrict_actions(*args)
        options = args.extract_options!

        @restricted_actions_config ||= {}
        args.each do |method_name|
          @restricted_actions_config[method_name.to_s] = options
        end
      end
    end

    def action_methods
      return self.class.action_methods - restricted_actions
    end
    memoize :action_methods

    def restricted_actions
      methods = []
      if self.class.restricted_actions_config
        self.class.restricted_actions_config.each do |method_name, options|
          methods << method_name if options[:if]     &&  instance_exec(&options[:if])
          methods << method_name if options[:unless] && !instance_exec(&options[:unless])
        end
      end

      return methods.uniq
    end
    memoize :restricted_actions

    protected

    def only_render_implemented_actions
      return self.restricted_action_filter(params[:action]) if restrict_actions.include?(params[:action]) &&
                                                               self.respond_to?(:restricted_action_filter)
      raise AbstractController::ActionNotFound unless action_methods.include?(params[:action])
    end

  end

  class DSL
    delegate :restrict_actions, :to => :controller
  end
end

In the above implementation, 2) would be addressed by the programmer defining a restricted_action_filter method. The other alternative, which might be neater, is to raise an exception which can the be handled in a rescue_from call.

dgutov commented Oct 4, 2011

If I'm not mistaken, :memoize will cache the return value longer than the lifetime of request. Have you tried your code in production mode, switching from a more restricted user to a less restricted one?

Regarding 2), I think it'd be better if we could pass the filter name to the restrict_actions call:

restrict_actions :edit, :update, :destroy, :if => { ... }, :filter => :authenticate_user!
Contributor

DMajrekar commented Oct 4, 2011

Memoize works on the existence of an instance variable, hence the return will only get cached per request. I've also checked this in the production mode.

As for 2, I'm still not certain of the use case we're trying to cover. Would we want the specified filter to be run before the if block is run so that current_admin_user is available in the if block?

dgutov commented Oct 4, 2011

The idea is being able to redirect the user to the login page if they are trying to access a restricted action, and they are not authenticated, or don't have sufficient privileges for the action.

I, for one, need to support guest users, that means I can't require current_active_admin_user to be non-nil. In that case I delete the action from array without calling the block.

Contributor

DMajrekar commented Oct 5, 2011

In that case, would you not make a check in the if block for active_admin_user_logged_in?. e.g:

restrict_actions :edit, :if => {
  !!current_active_admin_user && current_active_admin_user.can?(:edit, resource)
}

The issues I see with running the filter are as follows:

Consider the situation where a guest user has access to the show page but not the delete for a resource. When visiting any page for the resource, the authenticate_user! filter would be run and, as the user cannot get access to the delete page, they will be asked to login. This does not seem to be the desired behaviour.

Another thought: the cancan docs (https://github.com/ryanb/cancan/wiki/Defining-Abilities) would suggest that even a nil current_user should work through the ability class.

dgutov commented Oct 5, 2011

You're probably right. If CanCan supports guest users, that's the direction I should be looking in.

Authentication and redirection for the current action's then should be taken care of by CanCan's load_and_authorize_resource, and the only problem that remains is only_render_implemented_actions is still the first filter. Maybe just use the original definition in it?

    def only_render_implemented_actions
      raise AbstractController::ActionNotFound unless
        self.class.action_methods.include?(params[:action])
    end
Contributor

gregbell commented Feb 16, 2012

Closing this issue. Please create another issue if there is a specific problem with Active Admin that is holding you back from using CanCan. There is some great documentation on the wiki.

@gregbell gregbell closed this Feb 16, 2012

Owner

seanlinsley commented Apr 1, 2014

related discussion with @vbalazs in IRC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment