Add option to disable authorization context memoization about action_policy HOT 12 CLOSED

we also have another failing test that is related to this change that broke:

Oh, that's bad. Fixed (and added a missing test so I won't break it again 🙂).

Adding #build_authorization_context adds a new action that is unreachable, thus failing CI.

Made private.

Both released in 0.6.3.

from action_policy.

I totally understand that assumption. For my app, sometimes it has intermediary authorization contexts that build up to a final authorization context (i.e. when scoping nested resources), so that assumption doesn't always hold true.

I'm fine using the monkey patch for now. No rush. Just wanted to start a discussion.

p.s. I really dig this gem! I was on my way to implement a similar system on top of Pundit when I stumbed upon Action Policy. Rather than continue adding abstractions on top of Pundit (allow!, deny!, authorization contexts, etc.), I decided to rip all that out and migrate to Action Policy. Still lots of work to do on that front, but so far it has helped clean up a lot of technical debt, and has made policies much easier to reason about.

p.p.s. Sponsored the gem on behalf of @keygen-sh. 🙂

from action_policy.

the memoization by authorized_scope is rather unexpected and took awhile to debug. As far as I can tell, this behavior by authorized_scope isn't documented.

All helper methods (authorize!, allowed_to? and authorized_scope) uses policy_for under the hood, which is responsible for memoization/caching of policies and contexts.

I see several possible solutions to the problem:

1. Add an API to reset authorization context: reset_authorization_context!
2. Add an option to use a fresh authorization context without caching/memoization: authorized_scope(Post.all, isolate_context: true)
3. Set context explicitly:

@post = authorized_scope(Post.all).find(params[:post_id])
authorization_context[:post] = @post

I would prefer the latter one. Adding 1) could be also useful. The 2) doesn't look good to me.

from action_policy.

Thanks for the consideration. I feel like a configuration option may be even better.

config.action_policy.memoize_authorization_context = false

from action_policy.

I think, we can proceed with one of the application-level options:

• Updating the context manually: authorization_context[:post] = @post. That, IMO, the best way to deal with it, explicit and performant.
• Overriding the authorization_context method:

class ApplicationController
  def authorization_context
    {
       user: current_user
    }
  end
end

class Posts::CommentsController < ApplicationController
  def authorization_context
    super.merge(post: post)
  end
end

We already have at least two pure Ruby options, hence adding configuration/API complexity to the library is not necessary.

from action_policy.

Those solutions are fine, and seem to work. I think def authorization_context = super.merge(post:) is the best option out of those. But I still think it's confusing that authorized_scope memoizes the authorization context, excluding the later set post. Especially because the controller explicitly calls authorize :post. I expected the post to be in the authorization context after it had been set, without additional work.

That behavior was not expected and took awhile to debug, having to dig into the internals of Active Policy to figure out what was going on. I'd expect any changes to the authorization context and its records be reflected in future authorizations.

Just curious — is there a particular reason the authorization_context method is memoized? I can't see how a handful of, likely memoized themselves, method calls e.g. current_account, current_user, would be noticeably slower than a memoized authorization_context call.

I looked through the method's history and I see it's always been this way, but I'm kind of curious if there's a reasoning here. To me, it seems like the memoization here wouldn't do much for performance.

from action_policy.

I'd expect any changes to the authorization context and its records be reflected in future authorizations.

I'm kind of curious if there's a reasoning here

The library was designed with the assumption that the context is defined once and never changes during the execution of a request (or another unit of work). Which doesn't seem to cover all the use cases. "A great abstraction will be used in ways you never anticipated" 🙂

I think, getting rid of the memoization could be added to the v1 backlog.

For now, I suggest the following: split the current #authorization_context method into two: #build_authorization_context (building a Hash from the configured targets) and #authorization_context (adds memoization). So you can "disable" memoization by adding an alias: alias authorization_context build_authorization_context. WDYT?

from action_policy.

Sounds great!

Please, let me know if you some ideas of what could be great to have in the library itself.

P.S. Thanks for sponsoring ☺️

from action_policy.

@palkan this actually breaks our builds because we use traceroute gem to check for unused routes. Is it possible to just make it these two methods private instead? 🤔 Happy to make the PR for it if needed.

from action_policy.

@corroded Oh, that's interesting. Could you provide more details? Sure, we can make #build_authorization_context private (but not #authorization_context, it was always public).

from action_policy.

@corroded Oh, that's interesting. Could you provide more details? Sure, we can make #build_authorization_context private (but not #authorization_context, it was always public).

Should I provide this here or in another issue? Anyway, we have an ActiveSupport concern that includes ActionPolicy::Controller:

app/controllers/api/concerns/authorization.rb

module API::Concerns::Authorization
  extend ActiveSupport::Concern
  include ActionPolicy::Controller

  ...

this is then included in all controllers via:

class API::SomeController < ApplicationController

  include API::Concerns::Authorization

I think the traceroute gem basically audits all our controllers and checks if there are actions that are unreachable - meaning there are no routes against it. Adding #build_authorization_context adds a new action that is unreachable, thus failing CI.

THAT SAID, I can just add it to our ignore file (which I have done). Just thought it would be a good idea to make the method private since it is not a public API? WDYT?

Additionally, we also have another failing test that is related to this change that broke:

ddb7e63

this one that checks if ActiveRecord is being hit 🤔 interesting though as I don't see the specific change affecting this but maybe you might have a clue.

    # This is a regression test for a performance problem that was fixed
    # upstream in the `action_policy` gem.
    # See: https://github.com/palkan/action_policy/commit/ddb7e635550f96e6a7b4f3a639a68c602a8f0700
    it "does not actually execute the scoped query" do
      allow(ActiveRecord::Base.connection).to receive(:exec_query).and_call_original
      controller_class.action(:index).call(fake_request)
      expect(ActiveRecord::Base.connection).not_to have_received(:exec_query)
        .with(/FROM "webhook_deliveries"/, anything, anything, anything)
    end

from action_policy.

many thanks!

from action_policy.