Support alternate Authorization schemes, example: Action policy

[dhnaranjo]

Description

I want to use https://github.com/palkan/action_policy instead of Pundit because I do. The two are very similar, as the migration guide would show, but I'm not asking you to switch defaults. Rather, I'm hoping to enable the use of other authorization strategies

So I pulled all direct Pundit calls out of the AuthorizationService and threw them in a class for Pundit interacting, then made one for ActionPolicy too. Tests pass. It's neat!

I have not filled out the checklist as I consider this a draft. Dunno if this is something y'all even want. I'd be 100% with the official way being Pundit and the ActionPolicy client maybe listed as a recipe in the documentation

Checklist:

• [x] I have performed a self-review of my own code
• [x] I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [x] I have added tests that prove my fix is effective or that my feature works

Manual review steps

1. Step 1
2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

[qlty-cloud-legacy[bot]]

Code Climate has analyzed commit 77e555cd and detected 0 issues on this pull request.

View more on Code Climate.

[adrianthedev]

Hey @dhnaranjo. Thank you for your contribution. This is a great addition, and it's definitely something we want to support going forward.

Let me have a look this week and see what needs to be done to finish this PR.

[adrianthedev]

Hey @dhnaranjo. Thanks for all the work!

I'd love it if we had a switch somewhere between pundit and AP (maybe configuration.rb). I'm open to auto-detect the gem used, but the switch would help us to test both frameworks.

Regarding how the packages will be installed, we'll probably remove pundit from our gemspec and tell the developers to add it in their Gemfile.

BTW, you need to run appraisal install and update all the rest of the gemfiles to have the tests passing on CI.

This is definitely going in the right direction. I'd love to support AP in Avo, so whenever you have the time, if you finish it, I'll merge it ✌️

[dhnaranjo]

I would actually like to not test Pundit or AP directly at all, I think we can depend on their gem test suites. I think it would be sufficient for unit tests of each of the two clients to validate that they forward calls appropriately for the specific auth provider, and for the creation of a dummy client to serve the needs of Avo's own auth-dependent behavior.

The alternative approach suggests we'd duplicate all auth-relevant tests for each provider, and then further for a third (maybe someone really likes CanCanCan), or that we would have only one provider thoroughly tested.

The main risk of my approach is that any of these providers introduces a breaking change to a method one of our clients uses and we mock our way out of being able to observe it, but I mean...

But I'm down for whatever. Lemme know.

[adrianthedev]

I know what you mean and appreciate you thinking about the best method to test it. Usually, I would use the same strategy, but with Avo, we rely a lot on integration tests that test the whole app on all layers.

How about we try a hybrid approach? Let's keep the integration tests we have now for our authorization needs (which use Pundit) and add new unit tests as you proposed that test the clients that they forward the calls. Does that sound good?

Thank you!

[gee-forr]

This PR is a big deal for us, as we use ActionPolicy exclusively.

Really hope it gets merged :)

❤️

[dhnaranjo]

Awright @adrianthedev this thing is ready to review, tho I haven't updated docs yet.

[dhnaranjo]

Ah man see this is why people say don't be monkeypatching stuff. I'm just not sure if we're gon be able to reliably load both ActionPolicy n Pundit in the same test suite at the same time.

[dhnaranjo]

Both Pundit and ActionPolicy use Model.policy_class (or Model#policy_class). ActionPolicy defines ActiveRecord::Relation.policy_class as nil for some real specific issue, so when both are loaded Pundit is unable to identify the policy_class of a relation.

Man I dunno. Maybe we just ship support for using alternate authorization strategies but don't package ActionPolicy in the main gem?

I am soliciting ideas because I am out of them.

[adrianthedev]

Yes, I'm thinking about it too.

Can't we actively re-apply the monkey patch in the test's before block? Spitballing here.

[dhnaranjo]

Something like

diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 46727af2..9d7c285b 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -138,6 +138,12 @@ RSpec.configure do |config|
     WebMock.allow_net_connect!(net_http_connect_on_start: true)
     WebMock.reset!
     ENV["RUN_WITH_NULL_LICENSE"] = "0"
+
+    config.before(:suite) do
+      ActiveRecord::Relation.class_eval do
+        undef policy_class
+      end
+    end
   end
 
   # You can uncomment this line to turn off ActiveRecord support entirely.
diff --git a/spec/services/authorization_client/action_policy_client_spec.rb b/spec/services/authorization_client/action_policy_client_spec.rb
index 537078cf..79a241ac 100644
--- a/spec/services/authorization_client/action_policy_client_spec.rb
+++ b/spec/services/authorization_client/action_policy_client_spec.rb
@@ -18,10 +18,18 @@ RSpec.describe 'Avo::Services::AuthorizationClient::ActionPolicyClient' do
   let(:record) { Dummy.new }
 
   around do |example|
+    ActiveRecord::Relation.class_eval do
+      def policy_class
+        nil
+      end
+    end
     current_client = Avo.configuration.authorization_client
     Avo.configuration.authorization_client = :action_policy
     example.run
     Avo.configuration.authorization_client = current_client
+    ActiveRecord::Relation.class_eval do
+      undef policy_class
+    end
   end
 
   before do

Ooh that feels super gross. Also I didn't test it, just a terrible idea. Also I just found out the undef keyword exists.

[adrianthedev]

@dhnaranjo how about generating separate gemfiles using Appraisal?

So then we'd add one more level to the test matrix. action_policy and pundit. That will have the tests passing in the CI, and then we document that we manually have to change the gem when testing on local (I'm open to having better testing on local envs too).

Something like this:

jobs:
  Test:
    strategy:
      matrix:
        ruby:
          - '3.0.3'
        rails:
          - '6.0'
          - '6.1'
        authorization_gem:
          - 'pundit'
          - 'action_policy'

[dhnaranjo]

The Policy classes aren't quite identical. We'd have to conditionally switch the way scopes are defined and have ApplicationPolicy inherit from ActionPolicy::Base. But I do think that's an interesting idea. It would best reflect what we expect users to experience.

I just pushed a commit where I got the gem-specific tests to pass without conflicting in a way that is located only in those spec files, which is good, though they're very ugly hacks that look real brittle, which is bad.

I also realized that we gotta handle the Pundit inclusion in ApplicationController. I'm hesitant to add another dimension to the build matrix because, you know, math. Doing that can get out of hand pretty quick, but it does seem to be the best way to catch that kind of business.

[github-actions[bot]]

This PR has been marked as stale because there was no activity for the past 15 days.

[dhnaranjo]

I tucked back into this for a while last night, tried out the Appraisal method. I was wrapping so much code in so many defined? conditionals that it was just ugly and hard to follow, and I can't help but feel that doubling the number of CI jobs forever is just not the move.

What do you think about one or more test-only authorization clients, like say a PermissiveClient that just approves everything, same for a RestrictiveClient that denies, and use them to test the rest of the engines behavior based on authorization responses, then test the proper clients for appropriate message passing to their library.

[adrianthedev]

Hey @dhnaranjo. I'm open to that, as long as we can really test every behavior. Even the more advanced ones like act_on? (actions authorization), create_comments, attach_members? (associations), download_file? (files), etc. So yes, let's do it! Let me know if you want to jump on a pairing session together 💪

[dhnaranjo]

@adrianthedev I'mma take this one in smaller chunks so we can hit all those concerns without making a mess. I'm gonna continue work here: https://github.com/avo-hq/avo/pull/1300