Ehouriga/ch9191/implement grant #3

ehourigan · 2018-03-13T20:11:05Z

add the ability to GRANT permissions on a namespace or specific object

Please SQUASH my commits

ehourigan · 2018-03-14T14:51:23Z

lib/accesscontrol.rb

@@ -2,6 +2,10 @@
 require "accesscontrol/query"

 module AccessControl
+
+  unless defined?(CouldNotGrantError) == "constant" && CouldNotGrantError.class == Class


borrowed this structure from acts_as_archival

ehourigan · 2018-03-14T14:54:40Z

test/schema.rb

    t.column :action, :integer
    t.column :actor_id, :integer
    t.column :actor_type, :string
    t.column :object_type, :string
    t.column :object_id, :integer
  end

+  add_index(:access_control_permitted_actions, [:segment_id, :action, :actor_id, :actor_type, :object_type], unique: true, name: "acpm_acpa_uniq_table_idx")
+  add_index(:access_control_permitted_action_on_objects, [:segment_id, :action, :actor_id, :actor_type, :object_type, :object_id], unique: true, name: "acpm_acpaoo_uniq_table_idx")


adding unique index on all columns to force it at the database level (prevents identical GRANTs)

ehourigan · 2018-03-14T14:55:36Z

test/schema.rb

@@ -6,20 +6,23 @@
  end

  create_table :access_control_permitted_actions, force: true, id: :uuid do |t|
-    t.column :segment_id, :integer
+    t.column :segment_id, :integer, default: -1


The table indexes need a sensible default on segment_id for the unique constraint functionality

armilam · 2018-03-14T16:47:19Z

I just added the Clubhouse wekbhook, so:

[ch9191]

ehourigan · 2018-03-14T18:44:29Z

test/grant_records_test.rb

+    assert_nil(AccessControl::Query.new(actor).grant(1, Post, post.id))
+  end
+
+  it 'retuns raises an error the actor has access' do


I fix all these poorly worded 'it' blocks in #4

armilam

This is good! I have a few questions here asking whether we should tweak a couple things. But overall, I like this.

One caveat: I haven't tested it fully yet, but I wanted to get my comments to you early in case there's any discussion. I will be testing shortly.

armilam · 2018-03-15T14:40:35Z

lib/accesscontrol.rb

+
+  unless defined?(CouldNotGrantError) == "constant" && CouldNotGrantError.class == Class
+    CouldNotGrantError = Class.new(StandardError)
+  end
  # AccessControl's tables are prefixed with access_control to


Tiny nitpick, but normally I would expect to see an empty line between the end of one thing and the comment for the next thing.

armilam · 2018-03-15T14:43:15Z

lib/accesscontrol/permitted_action_on_object_query.rb

+    # A grant is universally unique and is enforced at the database level.
+    #
+    # @param action_id [Integer] The action to grant for the object
+    # @param object_type [ActiveRecord::Base] The ActiveRecord model that receives a permission grant.


Technically, we pass in a Class for object_type, right? I know I did in can? exactly what you're doing here by specifying ActiveRecord::Base, but I'm wondering if this should actually be Class. I'm still getting used to yardoc, so I could be wrong. What do you think?

Ya I think you're right - I was talking with @rreinhardt9 about it yesterday and were talking about this scenario:

Consider we have a LessonPolicy and a RestrictedLessonPolicy - Both of these policies define actions with different meaning BUT refer to the Lesson ActiveRecord model/object. This means that the object_type we compare against should be different between the two policies? (in this case modifying what you have suggested to Class)

Based on previous conversations, I expect LessonPolicy and RestrictedLessonPolicy to use the same Lesson permissions via Accessly, but to filter them with business logic. Especially since we use object_type to chain AR calls on in list.

yup! business logic

The only argument I can think of to keep [ActiveRecord::Base] as the object_type is in support of the list operation

armilam · 2018-03-15T14:44:31Z

lib/accesscontrol/permitted_action_on_object_query.rb

+    # @param action_id [Integer] The action to grant for the object
+    # @param object_type [ActiveRecord::Base] The ActiveRecord model that receives a permission grant.
+    # @param object_id [Integer] The id of the ActiveRecord object which receives a permission grant
+    # @return [nil] Returns nil if successful, otherwise will raise an Error (AccessControl::CouldNotGrantError).


You can use yardoc's @raise syntax to describe raised exceptions.

armilam · 2018-03-15T14:45:22Z

lib/accesscontrol/permitted_action_on_object_query.rb

+    #   # Allow the user access to Post 7
+    #   AccessControl::Query.new(user).grant(3, Post, 7)
+    def grant(action_id, object_type, object_id)
+      a = PermittedActionOnObject.create(


Is there a need for the local variable a? If so, can we name it something more clear? If not, remove it?

damn - that shouldn't be there

armilam · 2018-03-15T14:49:39Z

lib/accesscontrol/permitted_action_query.rb

+    #
+    # @param action_id [Integer] The action to grant for the object
+    # @param object_type [String] The namespace of the given action_id.
+    # @return [nil] Returns nil if successful, otherwise will raise an Error (AccessControl::CouldNotGrantError).


Again, you can use yardoc's @raise syntax to describe raised exceptions.

armilam · 2018-03-15T14:52:24Z

lib/accesscontrol/permitted_action_on_object_query.rb

+      )
+      nil
+    rescue
+      raise AccessControl::CouldNotGrantError.new("Could not grant action #{action_id} on object #{object_type} with id #{object_id} for actor #{@actor}")


Do you think we should silently rescue from PG::UniqueViolation (or the equivalent on other DBMSs if we end up supporting them)? Technically, if that error is raised, then the permission has already been granted and nothing really wrong has happened, right?

Of course, if other errors happened, then the caller definitely needs to know about it. In those cases, should we bubble up or include the error message so it's clear what the problem is?

Should we just return nil on the PG::UniqueViolation?

Also, if we start using Postgres specific errors we will need to update our test suite to run against a Postgres database, right?

Should we just return nil on the PG::UniqueViolation?

Makes sense to me!

Also, if we start using Postgres specific errors we will need to update our test suite to run against a Postgres database, right?

Bah, you're right! Is there a more generic way we can do it?

~~I don't know there's an easier way - but since we're leveraging where_tuples we may just want to support Postgres to start with?~~

rescue ActiveRecord::RecordNotUnique

armilam · 2018-03-15T14:52:44Z

lib/accesscontrol/permitted_action_query.rb

+      )
+      nil
+    rescue => e
+      raise AccessControl::CouldNotGrantError.new("Could not grant action #{action_id} on object #{object_type} for actor #{@actor}")


Like I asked above, should we silently rescue from PG::UniqueViolation?

see question above

armilam · 2018-03-15T14:53:48Z

lib/accesscontrol/permitted_action_on_object_query.rb

+    #   # Allow the user access to Post 7
+    #   AccessControl::Query.new(user).grant(3, Post, 7)
+    def grant(action_id, object_type, object_id)
+      a = PermittedActionOnObject.create(


I think this should be create! with the ! so that errors get raised (which we rescue below) instead of it just returning false and failing relatively silently. Thoughts?

👍- I had it there initially and then changed it

armilam · 2018-03-15T17:52:43Z

test/grant_general_action_test.rb

+  it 'returns nil after a successful grant' do
+    actor = User.create!
+
+    assert_nil(AccessControl::Query.new(actor).grant(1, Post))


I missed this before, but we're using must_equal in other tests and assert_stuff here. It feels like we should standardize one way or the other.

Preferences? I think it's fine either way. I'm just looking for consistency.

I had must_equal nil -- but there were a bunch of deprecation warnings about that and the suggested remedy was to use assert

armilam

This looks fantastic! I like that we're swallowing duplicate records and bubbling up anything else. And the tests look like they cover the cases I thought of.

Onward!

armilam · 2018-03-16T19:45:54Z

test/schema.rb

+    t.column :action, :integer, null: false
+    t.column :actor_id, :integer, null: false
+    t.column :actor_type, :string, null: false
+    t.column :object_type, :string, null: false


ehourigan added 2 commits March 13, 2018 16:09

implement grant API and tests

9c6274b

add test files

199a7d8

ehourigan added the wip label Mar 13, 2018

Merge branch 'master' into ehouriga/ch9191/implement-grant

6fcbad1

ehourigan changed the base branch from armilam/ch9270/implement-caching-on-can to master March 14, 2018 14:40

remove puts

2ff13da

ehourigan commented Mar 14, 2018

View reviewed changes

remove null constraint from schema rb

e92a1dd

ehourigan commented Mar 14, 2018

View reviewed changes

ehourigan removed the wip label Mar 14, 2018

ehourigan mentioned this pull request Mar 14, 2018

defines segment_id for permission queries #4

Closed

ehourigan commented Mar 14, 2018

View reviewed changes

armilam reviewed Mar 15, 2018

View reviewed changes

ehourigan added 2 commits March 15, 2018 14:21

force values on all columns, rescue on ActiveRecord::RecordNotUnique

2532df5

update the overload docs on Query class

a30f4e7

armilam approved these changes Mar 16, 2018

View reviewed changes

armilam merged commit dd61d65 into master Mar 16, 2018

rreinhardt9 deleted the ehouriga/ch9191/implement-grant branch February 19, 2021 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ehouriga/ch9191/implement grant #3

Ehouriga/ch9191/implement grant #3

ehourigan commented Mar 13, 2018 •

edited

Loading

ehourigan Mar 14, 2018

ehourigan Mar 14, 2018

ehourigan Mar 14, 2018 •

edited

Loading

armilam commented Mar 14, 2018

ehourigan Mar 14, 2018 •

edited

Loading

armilam left a comment

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018 •

edited

Loading

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018 •

edited

Loading

armilam Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam Mar 15, 2018

ehourigan Mar 15, 2018 •

edited

Loading

armilam Mar 15, 2018

ehourigan Mar 15, 2018

armilam left a comment

armilam Mar 16, 2018

Ehouriga/ch9191/implement grant #3

Ehouriga/ch9191/implement grant #3

Conversation

ehourigan commented Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehourigan Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

armilam commented Mar 14, 2018

ehourigan Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

armilam 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

ehourigan Mar 15, 2018 • 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

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

ehourigan Mar 15, 2018 • 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

ehourigan Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armilam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehourigan commented Mar 13, 2018 •

edited

Loading

ehourigan Mar 14, 2018 •

edited

Loading

ehourigan Mar 14, 2018 •

edited

Loading

ehourigan Mar 15, 2018 •

edited

Loading

ehourigan Mar 15, 2018 •

edited

Loading

ehourigan Mar 15, 2018 •

edited

Loading