Add authorization features and a Pundit mixin #971

pedantic-git · 2017-09-13T12:44:07Z

This PR adds basic authorization features to Administrate, allowing different admins access to different data/actions.

Users are free to roll their own authorization, or use any authz gems they like, but since I personally think Pundit is the best of the bunch, I've implemented a controller concern that allows users to get Pundit support in one line.

Pundit isn't added as a dependency, except for the tests.

I've added an authorization.md file to the docs/ with full instructions... let me know what you think and if you'd like any changes!

houndci-bot · 2017-09-13T12:44:18Z

spec/example_app/app/policies/order_policy.rb

+  def destroy?
+    false
+  end
+


Trailing whitespace detected.

houndci-bot · 2017-09-13T12:44:18Z

spec/example_app/app/policies/order_policy.rb

+  def update?
+    record.address_state == "AZ"
+  end
+


Trailing whitespace detected.

houndci-bot · 2017-09-13T12:44:18Z

spec/example_app/app/policies/order_policy.rb

+  def create?
+    false
+  end
+


Trailing whitespace detected.

houndci-bot · 2017-09-13T12:44:18Z

spec/example_app/app/policies/order_policy.rb

+      scope.where(customer: user)
+    end
+  end
+


Trailing whitespace detected.

houndci-bot · 2017-09-13T12:44:18Z

spec/example_app/app/policies/order_policy.rb

+    def resolve
+      scope.all
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-09-13T12:44:19Z

spec/controllers/admin/orders_controller_spec.rb

+      end
+      it 'does not show edit actions for records elsewhere' do
+        o = create :order, customer: user, address_state: 'GA'
+        expect(controller.show_action? :edit, o).to be false


Add parentheses to nested method call controller.show_action? :edit, o.

houndci-bot · 2017-09-13T12:44:19Z

spec/controllers/admin/orders_controller_spec.rb

+        expect(controller.show_action? :edit, o).to be true
+      end
+      it 'does not show edit actions for records elsewhere' do
+        o = create :order, customer: user, address_state: 'GA'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-13T12:44:19Z

spec/controllers/admin/orders_controller_spec.rb

+        o = create :order, customer: user, address_state: 'AZ'
+        expect(controller.show_action? :edit, o).to be true
+      end
+      it 'does not show edit actions for records elsewhere' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-09-13T12:44:19Z

spec/controllers/admin/orders_controller_spec.rb

+    describe '#show_action?' do
+      it 'shows edit actions for records in AZ' do
+        o = create :order, customer: user, address_state: 'AZ'
+        expect(controller.show_action? :edit, o).to be true


Add parentheses to nested method call controller.show_action? :edit, o.

houndci-bot · 2017-09-13T12:44:19Z

spec/controllers/admin/orders_controller_spec.rb

+    end
+    describe '#show_action?' do
+      it 'shows edit actions for records in AZ' do
+        o = create :order, customer: user, address_state: 'AZ'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

pablobm

This looks pretty good to me. I like the feature and the agnostic approach.

I would say:

Please address Hound's warnings. I'm surprised there aren't more, I wonder if it failed?
Remove the comments, as I don't think they are necessary and there aren't similar ones (I think!) in the codebase.

pablobm · 2017-09-27T13:48:48Z

app/controllers/administrate/application_controller.rb

@@ -104,6 +107,8 @@ def dashboard

    def requested_resource
      @_requested_resource ||= find_resource(params[:id])
+      authorize_resource(@_requested_resource)
+      @_requested_resource
    end


This breaks the existing memoization. Here's an alternative, saving us from (potentially) checking the authorization multiple times:

def requested_resource @_requested_resource ||= find_resource(params[:id]).tap do |r| authorize_resource(r) end end

But then I think... we could go further. If the spec for authorize_resource required it to return the resource on success, we could do this:

def requested_resource @_requested_resource ||= authorize_resource(find_resource(params[:id])) end

In fact that's what Pundit's own authorize does, which in turn means that the implementation provided by Punditize does this, although not the default, blank implementation.

But I'm probably complicating this too much for too little gain, so I'll be happy with something along the lines of my first alternative.

pablobm · 2017-09-27T13:49:32Z

spec/controllers/admin/orders_controller_spec.rb

+        locals = capture_view_locals { get :index }
+        expect(locals[:resources].count).to eq(9) # only my orders
+      end
+    end


Please separate these with blank lines. I'm surprised that Hound is not complaining about this and the single quotes?

nickcharlton · 2017-09-27T14:51:08Z

Thanks @pedantic-git! I know @pablobm left some comments, but I'm thinking it'd be good to get this into 0.9, which should be in a few weeks time (i.e.: the next release).

pedantic-git · 2017-09-27T14:54:42Z

Sure! I'll try to get onto these tomorrow. Just working on another Administrate PR right now (trying to get editing working for Field::Polymorphic... nearly there too!)

houndci-bot · 2017-09-28T09:47:56Z

spec/example_app/app/policies/order_policy.rb

+  def destroy?
+    false
+  end
+


Extra empty line detected at class body end.

houndci-bot · 2017-09-28T09:47:56Z

spec/example_app/app/policies/order_policy.rb

@@ -0,0 +1,25 @@
+class OrderPolicy < ApplicationPolicy
+


Extra empty line detected at class body beginning.

houndci-bot · 2017-09-28T09:47:56Z

spec/controllers/admin/orders_controller_spec.rb

+        expect(controller.show_action?(:destroy, o)).to be false
+      end
+    end
+


Extra empty line detected at block body end.

houndci-bot · 2017-09-28T09:47:56Z

spec/controllers/admin/orders_controller_spec.rb

+    describe "DELETE destroy" do
+      it "never allows me to delete a record" do
+        o = create :order, customer: user, address_state: "AZ"
+        expect{delete :destroy, id: o.id}.to raise_error(Pundit::NotAuthorizedError)


Space missing to the left of {.
Space missing inside {.
Space missing inside }.
Line is too long. [84/80]

houndci-bot · 2017-09-28T09:47:56Z

spec/controllers/admin/orders_controller_spec.rb

+
+      it "does not allow me to edit other records" do
+        ga = create :order, customer: user, address_state: "GA"
+        expect{get :edit, id: ga.id}.to raise_error(Pundit::NotAuthorizedError)


Space missing to the left of {.
Space missing inside {.
Space missing inside }.

houndci-bot · 2017-09-28T09:47:57Z

app/controllers/concerns/administrate/punditize.rb

+      end
+
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:47:57Z

app/controllers/concerns/administrate/punditize.rb

+      def show_action?(action, resource)
+        Pundit.policy!(pundit_user, resource).send("#{action}?".to_sym)
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:47:57Z

app/controllers/concerns/administrate/punditize.rb

+      def authorize_resource(resource)
+        authorize resource
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:47:57Z

app/controllers/concerns/administrate/punditize.rb

+      def scoped_resource
+        policy_scope_admin super
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:47:57Z

app/controllers/concerns/administrate/punditize.rb

+  module Punditize
+    extend ActiveSupport::Concern
+    include Pundit
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:47:58Z

spec/example_app/app/policies/order_policy.rb

+  def destroy?
+    false
+  end
+


Extra empty line detected at class body end.

houndci-bot · 2017-09-28T09:47:58Z

spec/example_app/app/policies/order_policy.rb

@@ -0,0 +1,25 @@
+class OrderPolicy < ApplicationPolicy
+


Extra empty line detected at class body beginning.

houndci-bot · 2017-09-28T09:47:58Z

spec/controllers/admin/orders_controller_spec.rb

+        expect(controller.show_action?(:destroy, o)).to be false
+      end
+    end
+


Extra empty line detected at block body end.

houndci-bot · 2017-09-28T09:47:58Z

spec/controllers/admin/orders_controller_spec.rb

+    describe "DELETE destroy" do
+      it "never allows me to delete a record" do
+        o = create :order, customer: user, address_state: "AZ"
+        expect{delete :destroy, id: o.id}.to raise_error(Pundit::NotAuthorizedError)


Space missing to the left of {.
Space missing inside {.
Space missing inside }.
Line is too long. [84/80]

houndci-bot · 2017-09-28T09:47:59Z

spec/controllers/admin/orders_controller_spec.rb

+
+      it "does not allow me to edit other records" do
+        ga = create :order, customer: user, address_state: "GA"
+        expect{get :edit, id: ga.id}.to raise_error(Pundit::NotAuthorizedError)


Space missing to the left of {.
Space missing inside {.
Space missing inside }.

houndci-bot · 2017-09-28T09:48:00Z

app/controllers/concerns/administrate/punditize.rb

+      end
+
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:48:00Z

app/controllers/concerns/administrate/punditize.rb

+      def show_action?(action, resource)
+        Pundit.policy!(pundit_user, resource).send("#{action}?".to_sym)
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:48:00Z

app/controllers/concerns/administrate/punditize.rb

+      def authorize_resource(resource)
+        authorize resource
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:48:00Z

app/controllers/concerns/administrate/punditize.rb

+      def scoped_resource
+        policy_scope_admin super
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:48:00Z

app/controllers/concerns/administrate/punditize.rb

+  module Punditize
+    extend ActiveSupport::Concern
+    include Pundit
+


Trailing whitespace detected.

houndci-bot · 2017-09-28T09:52:17Z

app/controllers/concerns/administrate/punditize.rb

+      def show_action?(action, resource)
+        Pundit.policy!(pundit_user, resource).send("#{action}?".to_sym)
+      end
+


Extra empty line detected at block body end.

houndci-bot · 2017-09-28T09:52:17Z

app/controllers/administrate/application_controller.rb

+    def authorize_resource(resource)
+      resource
+    end
+


Extra empty line detected at class body end.

houndci-bot · 2017-09-28T09:52:17Z

app/controllers/administrate/application_controller.rb

@@ -145,5 +148,20 @@ def show_search_bar?
        dashboard.collection_attributes
      ).any? { |_name, attribute| attribute.searchable? }
    end
+
+    def show_action?(action, resource)


Unused method argument - action. If it's necessary, use _ or _action as an argument name to indicate that it won't be used. You can also write as show_action?() if you want the method to accept any arguments but don't care about them.
Unused method argument - resource. If it's necessary, use _ or _resource as an argument name to indicate that it won't be used. You can also write as show_action?() if you want the method to accept any arguments but don't care about them.

Not addressing this - it's a method signature that is meant to be overridden in subclasses so the arguments and their names are important.

houndci-bot · 2017-09-28T09:52:17Z

app/controllers/administrate/application_controller.rb

@@ -71,7 +74,7 @@ def destroy
      flash[:notice] = translate_with_resource("destroy.success")
      redirect_to action: :index
    end
-
+ 


Trailing whitespace detected.

pedantic-git · 2017-09-28T09:58:46Z

OK! I've addressed all but two of the Hound violations (see above for my reasoning), I've changed the way authorize_resource works (thanks for spotting this, @pablobm!) and I've removed the comments.

[Removing comments is still something I struggle with - I can understand other elements of house style that I disagree with but this one always seems very counterintuitive to me.]

Let me know if I can do anything else!

pedantic-git · 2017-09-28T11:09:37Z

Just realized that Pundit 1.1.0 (the latest gem) doesn't return the object from authorize - it returns true. Shocking there hasn't been a gem release in over a year, but in the meantime I'll change the requested_resource to use tap instead.

…with Pundit 1.1.0

pustomytnyk · 2017-11-04T13:36:12Z

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")} #{page.page_title}",
      [namespace, page.resource],
      class: "button",
-    ) if valid_action? :show %>
+    ) if valid_action?(:show) && show_action?(:show, page.resource) %>


Is it possible to use only show_action? in all cases?
Like

def show_action?(action, resource) valid_action?(action, resource) && authorized?(action, resource) end

Hi @pustomytnyk - #show_action? is a method that users of administrate can (and should) override in their controllers. If we implement it in terms of #valid_action? in the base controller and again in the Pundit mixin, there's still opportunity for administrate users to accidentally break the expected behaviour by overriding it to something that's not.

nickcharlton · 2017-12-04T16:51:58Z

Thanks, @pedantic-git! I'm (finally!) going to merge this now.

pedantic-git · 2017-12-04T16:52:51Z

Yay! Thanks @nickcharlton !!

houndci-bot reviewed Sep 13, 2017

View reviewed changes

pablobm reviewed Sep 27, 2017

View reviewed changes

pedantic-git force-pushed the pundit branch from 9cf018a to 6661003 Compare September 28, 2017 09:47

Add authorization features and a Pundit mixin

078477d

houndci-bot reviewed Sep 28, 2017

View reviewed changes

pedantic-git force-pushed the pundit branch from 6661003 to 1daee39 Compare September 28, 2017 09:52

houndci-bot reviewed Sep 28, 2017

View reviewed changes

pedantic-git force-pushed the pundit branch 3 times, most recently from 86e29a6 to 5b12481 Compare September 28, 2017 09:56

Address comments by @houndci-bot and @pablobm

5b12481

Change the way authorize_resource works back so it's more compatible …

26db8f9

…with Pundit 1.1.0

pustomytnyk reviewed Nov 4, 2017

View reviewed changes

nickcharlton merged commit 055ef76 into thoughtbot:master Dec 4, 2017

nickcharlton mentioned this pull request Dec 19, 2017

Eager load: uninitialized constant Administrate::Punditize::Pundit (NameError) #1048

Closed

Add authorization features and a Pundit mixin #971

Add authorization features and a Pundit mixin #971

Conversation

pedantic-git commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablobm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickcharlton commented Sep 27, 2017

pedantic-git commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedantic-git commented Sep 28, 2017

pedantic-git commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickcharlton commented Dec 4, 2017

pedantic-git commented Dec 4, 2017