add allow_with_resource #8

jnwaletzko · 2016-12-05T22:21:26Z

Description and Impact

Adds an allow_with_resource method that will require a resource check when accessing the endpoint. This will act exactly the same as allow except that the CustomRoleObject used will need a method that checks if the resource matches the role. A method role_resource will need to be defined on the controller to allow the passing of the resource object.
Ex.

class SomeCustomRoleObject
  def resource?(resource)
    self.resources.includes?(resource)
  end
end

class ProfilesController < ApplicationController
  allow_with_resource(:admin).to_access(:index)
 
  ...

  private def role_resource
     { resource: params[:resource_id] }
  end
end

Deploy Plan

Does Platform Operations need to know anything special about this deploy? Are migrations present?

Rollback Plan

To roll back this change, revert the merge with: git revert -m 1 MERGE_SHA and perform another deploy.

URLs

Links to bug tickets or user stories.

QA Plan

Fill in scenarios below in checklist format.
Consider Regression scenarios (did we break something else related to this change) in addition to Happy Path (testing the new feature directly).
Evaluate the risk level and label accordingly and ensure the QA Plan matches the risk level!

Clone this repo and switch to this branch.

Switch an existing rails app to point the rolypoly gem to this branch.
Verify your existing endpoints still work for permissions checks.
Verify you can add an allow_by_resource and a role_resource controller and the role check is working.

vivekbisen

All in all, I like this approach. Doesn't intervene with the existing logic and the code changes seem a lot safer. My suggestions are merely subjective and optional. With new tests for allow_with_resource method, I think we should be good. Also, I would consider raising an error and force users to define role_resource than building one for them. In the raised error, it might be helpful to mention that if you don't really care for resource for your roles, consider using allow instead of allow_with_resource.

vivekbisen · 2016-12-06T01:23:44Z

lib/rolypoly/controller_role_dsl.rb

@@ -90,8 +98,8 @@ def try_super(mname)
        end
      end

-      def build_gatekeeper(roles, actions)
-        RoleGatekeeper.new(roles, actions).tap { |gatekeeper|
+      def build_gatekeeper(roles, actions, require_resource)


You could make this false by default and only pass true when you need to. So, def build_gatekeeper(roles, actions, require_resource = false) for definition and have the existing methods the same but call build_gatekeeper roles, nil, true in allow_with_resource method.

Yeah, I couldn't decide if I should default it or not. My thinking it make more sense to explicitly say it's false or true. But I'm not sure if that really helps, so defaulting seems good.

vivekbisen · 2016-12-06T01:27:55Z

lib/rolypoly/role_gatekeeper.rb

@@ -31,22 +32,23 @@ def to_all
      self.all_actions = true
    end

-    def allow?(current_roles, action)
+    def allow?(current_roles, action, role_resource)


You should name this differently if you intend to use the passed in param value in role?. It could get confusing very easily when you have the same name in initializer that sets the self attribute and the same name gets used in other parts of the class as local variable.

vivekbisen · 2016-12-06T01:31:23Z

lib/rolypoly/role_gatekeeper.rb


-    def match_roles(check_roles)
+    def match_roles(check_roles, role_resource)
+      check_roles = filter_roles_by_resource(check_roles, role_resource) if require_resouce


Since you call filter_roles_by_resource only when require_resauce is true, you should move the check in the method itself. You'll save yourself from repeated conditional and since require_resauce is already is an accessor, it's available across the class.

vivekbisen · 2016-12-06T01:32:14Z

spec/lib/rolypoly/role_gatekeeper_spec.rb

+        subject.to(:admin, :scorekeeper)
+      end
+
+      it_should_behave_like "allow should behave correctly"


jnwaletzko · 2016-12-06T17:20:11Z

@vivekbisen could you take another look? I considered your idea of raising an error but we would have to raise it in the role_gatekeeper and I really don't like it there. I also don't want it to break any existing functionality, so if role_resource is not defined on the controller it won't break anything.

vivekbisen · 2016-12-06T19:02:28Z

spec/lib/rolypoly/controller_role_dsl_spec.rb

+        let(:admin_role) do
+          admin = RoleObject.new(:admin)
+          allow(admin).to receive(:resource?).and_return true
+          admin


I wonder if you need the allow here. Did you try moving the allow to your before-block. These let blocks should only execute when called.

vivekbisen · 2016-12-06T19:03:17Z

spec/lib/rolypoly/controller_role_dsl_spec.rb

+        let(:scorekeeper_role) { RoleObject.new(:scorekeeper) }
+        let(:current_user_roles) { [admin_role, scorekeeper_role] }
+        let(:role_resource) { {resource: 123} }
+        let(:check_access) { controller_instance.rolypoly_check_role_access! }


I like to append ! to my method names when they execute code that uses !. So, check_access!?

vivekbisen · 2016-12-06T19:03:47Z

spec/lib/rolypoly/controller_role_dsl_spec.rb

+          subject.publicize(:landing)
+          allow(controller_instance).to receive(:current_user_roles).and_return(current_user_roles)
+          allow(controller_instance).to receive(:action_name).and_return(action_name)
+          allow(controller_instance).to receive(:role_resource).and_return(action_name)


vivekbisen · 2016-12-06T19:04:09Z

spec/lib/rolypoly/controller_role_dsl_spec.rb

+        describe "#index" do
+          let(:action_name) { "index" }
+
+          it { expect(controller_instance).to_not be_public }


Is there a be_private?

You wish. https://github.com/sportngin/rolypoly/blob/master/lib/rolypoly/controller_role_dsl.rb#L41

vivekbisen

Rest of it looks good. Sorry, these things don't pop up all at once.

vivekbisen · 2016-12-06T19:36:22Z

README.md

@@ -93,6 +93,46 @@ class ProfilesController < ApplicationController
 end
 ```

+# Allow roles with a resource
+`allow_with_resource` acts similarly to `allow` but requires a resource check to access the endpoint.


So, as we discussed, it doesn't really require a resource check. It will check resources against an empty role_resource.

vivekbisen · 2016-12-06T19:36:38Z

lib/rolypoly/controller_role_dsl.rb

@@ -17,6 +17,10 @@ def self.included(sub)
      unless sub.method_defined? :current_user_roles
        define_method(:current_user_roles) { [] }
      end
+
+      unless sub.method_defined? :role_resource
+        define_method(:role_resource) { [] }


Also, you wanna change this to an empty hash.

vivekbisen · 2016-12-06T19:38:39Z

spec/lib/rolypoly/role_gatekeeper_spec.rb

@@ -4,102 +4,144 @@ module Rolypoly
  describe RoleGatekeeper do
    let(:roles) { %w[admin scorekeeper] }
    let(:actions) { %w[index show] }
+    let(:resource) { [] }


And again, this resource would be a hash.

vivekbisen

👏 Let's get this moving.

production-status-check · 2016-12-09T19:24:30Z

Has QA approval

vivekbisen

Jake Waletzko added 2 commits December 5, 2016 16:20

add allow_with_resource

d8a274a

add role resource for gatekeeper spec

3a59658

vivekbisen reviewed Dec 6, 2016

View reviewed changes

Jake Waletzko added 8 commits December 6, 2016 09:25

default require resource to false

2e76f3b

rename resource

792dc90

add resource required tests

912949f

add resource required tests

1fb4689

add allow_with_resource spec

b7c027c

add allow_with_resource to readme

267c31b

display as ruby

f8ad77d

Add allow_with_resource summary

87839d3

vivekbisen reviewed Dec 6, 2016

View reviewed changes

move allows

1f1b372

vivekbisen reviewed Dec 6, 2016

View reviewed changes

pass empty hash instead of empty array

b81174c

vivekbisen approved these changes Dec 6, 2016

View reviewed changes

vivekbisen added enhancement qa-approved labels Dec 6, 2016

vivekbisen approved these changes Dec 9, 2016

View reviewed changes

bump version

72713eb

vivekbisen merged commit 8d31502 into master Dec 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add allow_with_resource #8

add allow_with_resource #8

jnwaletzko commented Dec 5, 2016 •

edited

Loading

vivekbisen left a comment

vivekbisen Dec 6, 2016

jnwaletzko Dec 6, 2016

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

jnwaletzko Dec 6, 2016

jnwaletzko commented Dec 6, 2016 •

edited

Loading

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

jnwaletzko Dec 6, 2016

vivekbisen left a comment

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

vivekbisen Dec 6, 2016

vivekbisen left a comment

production-status-check bot commented Dec 9, 2016

vivekbisen left a comment

add allow_with_resource #8

add allow_with_resource #8

Conversation

jnwaletzko commented Dec 5, 2016 • edited Loading

Description and Impact

Deploy Plan

Rollback Plan

URLs

QA Plan

vivekbisen left a 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

jnwaletzko commented Dec 6, 2016 • edited Loading

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

vivekbisen left a 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

vivekbisen left a comment

Choose a reason for hiding this comment

production-status-check bot commented Dec 9, 2016

vivekbisen left a comment

Choose a reason for hiding this comment

jnwaletzko commented Dec 5, 2016 •

edited

Loading

jnwaletzko commented Dec 6, 2016 •

edited

Loading