Determine pattern for base policy #9

armilam · 2018-03-22T13:00:04Z

DO NOT MERGE

This PR currently contains two options for the base policy class.

I am looking for feedback on the ~~two~~ three options. Which should we go with? Is there another option?

See the tests for examples of how they would be used in practice.

rreinhardt9 · 2018-03-22T16:08:23Z

lib/accessly/policy/base_option_1.rb

@@ -0,0 +1,86 @@
+module Accessly
+  module Policy
+    class BaseOption1


@armilam So in the first case, you would interact with the policy like BaseOptions1.new(actors).edit? or BaseOptions1.new(actors).edit?(object) and in the second case you would use it like BaseOptions2.new(actors).can?(:edit) or BaseOptions2.new(actors).can?(:edit, object)? Would that be correct?

oh nevermind... the tests spell it out 😆

rreinhardt9 · 2018-03-22T16:18:33Z

@armilam One of the goals that we had for the policy classes is that they would be able to customize the individual checks for permission based on business logic. In option1, if you wanted a straight check for "edit" for example, you would just get #edit? for free based on your list of actions. If you wanted to customize the check for edit based on you business logic, then you could define your own #edit? method to override it in the policy and it would use that method when checking for the edit? permission on that policy. How would we do that with the option2 direction?

rreinhardt9 · 2018-03-22T17:11:54Z

lib/accessly/policy/base_option_1.rb

+      end
+
+      def self._define_action_method(action, action_id)
+        unless method_defined?("#{action}?")


One other potential option with this might be to overide "method_missing". It might end up being potentially easier to reason about than metaprogramming them? Something like:

def method_missing(method_name, *args) action = method_name.to_s.sub(/\?$/,"") if args.empty? action_id = actions[action] return super unless action_id accessly_query.can?(actions[action], object_type) else action_id = actions_on_object[action] return super unless action_id object = args.first accessly_query.can?(action_id, object.class, object.id) end end

That's maybe a roughly what I'm picturing... and then if you defined anything more complex than a straight check, it would not be a missing method. If you didn't, then the this would catch that and make a basic check for that action?

on thing that would make that even simpler is if we came up with a convention of naming the checker methods to distinguish an action from an action on... something like #edit? vs #edit_on? then we could just know which type it was from the name?

armilam · 2018-03-22T18:54:02Z

@rreinhardt9: #9 (comment)

If you wanted to customize the check for edit based on you business logic, then you could define your own #edit? method to override it in the policy and it would use that method when checking for the edit? permission on that policy. How would we do that with the option2 direction?

Good point! You are right that we wanted to make customizing super easy to do. I'll put some thought into how we might do that in both scenarios.

armilam · 2018-03-23T00:04:04Z

@rreinhardt9, again regarding #9 (comment)

If you wanted to customize the check for edit based on you business logic, then you could define your own #edit? method to override it in the policy and it would use that method when checking for the edit? permission on that policy. How would we do that with the option2 direction?

I added the ability to customize the permission checks in each of option 1 and 2. I didn't like the customization for option 2, so I made an option 3 with the same API as option 2, but with a better customization pattern.

@rreinhardt9 and @ehourigan I would love to get your take on those.

armilam · 2018-03-23T13:38:08Z

test/base_option_3_policy_test.rb

+
+  class CustomizedPolicyOption3 < UserPolicyOption3
+    # Customize a general action check
+    override_action_check :destroy, ->(actor) { true if actor.name == "Aaron" }


I'm liking the Option3 API, but I'm not married to the override_action_check naming.

armilam · 2018-03-23T14:01:22Z

test/base_option_3_policy_test.rb

+
+  class CustomizedPolicyOption3 < UserPolicyOption3
+    # Customize a general action check
+    override_action_check :destroy, ->(actor) { true if actor.name == "Aaron" }


For each permission check, the logic is like this:

Is there an override for this action?

If yes, run it

Is the result of that overridden check nil?

If so, do the normal Accessly::Query lookup

If not

Do the normal Accessly::Query lookup

I like this API - but is our main use case to override on the given 'actor'? So for any given action, if the actor is an "admin" return true

Also - How do you envision the policies handle the multiple actor scenario? Would a policy accept an actor and then know how to get the actor relationships?

Add method of creating actions in the policy. This uses define_method to create methods based on those actions. Those methods respond appropriately when given or not given an object based on whether the action was defined for OnObject contexts or otherwise.

And update Option 1 (the one we're using) to use namespace instead of object_type where possible

…t file

…erride action checks

Also, move grant and revoke policy tests to their own test file

This auto-defines one method per object action that lists the records the actor has the given permission on. For example, if there exists an action `:view`, then the policy defines a method `#view` which returns an ActiveRecord::Relation of the records the actor has `:view` permission granted on.

If the actor is an admin, the base policy will skip the `list` query and return the whole `model_scope`.

armilam · 2018-03-27T03:37:56Z

Closing this PR so I can open one for the proposed solution.

armilam force-pushed the armilam/ch9229/create-a-base-policy branch 2 times, most recently from 78f898e to bd695cb Compare March 22, 2018 13:05

rreinhardt9 reviewed Mar 22, 2018

View reviewed changes

armilam commented Mar 23, 2018

View reviewed changes

armilam added 10 commits March 26, 2018 11:29

Add base policy

7131315

Add method of creating actions in the policy. This uses define_method to create methods based on those actions. Those methods respond appropriately when given or not given an object based on whether the action was defined for OnObject contexts or otherwise.

Add second option for base policy API

0d9c171

Test customizing permission checks for base policy option 1

743afb8

Test customizing permission checks for base policy option 2

8164e3d

Test customizing permission checks for base policy option 3

96940fe

Remove Policy option 3

6c0f46e

And update Option 1 (the one we're using) to use namespace instead of object_type where possible

Allow is_admin? to override permission checks in base policy

34ee462

Test overriding the namespace in the policy

f494516

Break tests for overriding actions in the policy out into its own tes…

2ac1070

…t file

Refactor the base policy so that child policy classes can properly ov…

f040687

…erride action checks

armilam force-pushed the armilam/ch9229/create-a-base-policy branch from fc85ed6 to f040687 Compare March 26, 2018 16:33

armilam added 2 commits March 26, 2018 14:27

Add ability to grant permission via the base policy

d702131

Add support for revoking permission in base policy

b2e886b

Also, move grant and revoke policy tests to their own test file

armilam force-pushed the armilam/ch9229/create-a-base-policy branch from 1398649 to b2e886b Compare March 26, 2018 18:34

armilam added 5 commits March 26, 2018 16:04

Make it so that admins get the whole list in the base policy

ddb6416

If the actor is an admin, the base policy will skip the `list` query and return the whole `model_scope`.

Test the ability to override action list methods in the base policy

957aea1

Support segment_id in base policy

31c8d9d

Remove base policy option 2

156c41c

armilam closed this Mar 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Determine pattern for base policy #9

Determine pattern for base policy #9

armilam commented Mar 22, 2018 •

edited

Loading

rreinhardt9 Mar 22, 2018

rreinhardt9 Mar 22, 2018

rreinhardt9 commented Mar 22, 2018

rreinhardt9 Mar 22, 2018

rreinhardt9 Mar 22, 2018

armilam commented Mar 22, 2018

armilam commented Mar 23, 2018

armilam Mar 23, 2018

armilam Mar 23, 2018

ehourigan Mar 23, 2018 •

edited

Loading

armilam commented Mar 27, 2018

Determine pattern for base policy #9

Determine pattern for base policy #9

Conversation

armilam commented Mar 22, 2018 • edited Loading

DO NOT MERGE

rreinhardt9 Mar 22, 2018

Choose a reason for hiding this comment

rreinhardt9 Mar 22, 2018

Choose a reason for hiding this comment

rreinhardt9 commented Mar 22, 2018

rreinhardt9 Mar 22, 2018

Choose a reason for hiding this comment

rreinhardt9 Mar 22, 2018

Choose a reason for hiding this comment

armilam commented Mar 22, 2018

armilam commented Mar 23, 2018

armilam Mar 23, 2018

Choose a reason for hiding this comment

armilam Mar 23, 2018

Choose a reason for hiding this comment

ehourigan Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

armilam commented Mar 27, 2018

armilam commented Mar 22, 2018 •

edited

Loading

ehourigan Mar 23, 2018 •

edited

Loading