Add option to disable authorization context memoization

[ezekg]

Tell us about your environment

Ruby Version: 3.1.2p20

Framework Version (Rails, whatever): 7.0.3.1

Action Policy Version: 0.6.1

Reproduction Script: https://gist.github.com/ezekg/fa8eb565cf9867a288c8f8c699507baf

What did you do?

Calling authorized_scope before authorize! results in a memoized authorization context, so even if the context changes, the changes are not picked up. This is problematic when different policies are used for scoping and authorizing nested resources. A more in-depth example is linked above, but as a quick example:

class Post::CommentPolicy < ApplicationPolicy
  # Needs additional authorization context
  authorize :post

  def show? = user.admin? || post.author == user
end

module Posts
  class CommentsController < ApplicationController
    before_action :set_post

    # We want to assert the post is accessible to the current
    # user, in addition to any comment records.
    authorize :post

    def show
      comment = post.comments.find(params[:id])
      authorize! comment, with: Post::CommentPolicy

      render json: comment
    end

    private

    attr_reader :post

    def set_post
      @post = authorized_scope(Post.all).find(params[:post_id])
    end
  end
end

Unintuitively, authorized_scope memoizes the authorization context, so the call to authorize! comments fails because it doesn't pick up the new :post authorization context without explicitly providing it via context: { post: }.

Having to explicitly provide contexts is cumbersome, and 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.

The idea here being that we want to respond with a 404 if the current user is not allowed to read the parent record (in this case, a Post), rather than a 403. Responding with a 403 leaks information. For example, GitHub responds with a 404, not a 403, when an unauthorized user attempts to access a private repository, even if it exists.

What did you expect to happen?

The authorize! call to succeed and the :post authorization context picked up.

What actually happened?

The authorize! call fails with the following error:

ActionPolicy::AuthorizationContextMissing (Missing policy authorization context: post)

What workarounds have you tried?

The following monkey patch resolves the problem:

module ActionPolicy::Behaviour
  ##
  # Remove memoization of authorization context. This allows us to use
  # authorized_scope() before authorize!() in controllers for nested
  # resources, e.g. /posts/:post_id/comments.
  def authorization_context = self.class.authorization_targets.each_with_object({}) { |(k, m), o| o[k] = send(m) }
end

What do you suggest?

Add an option to disable authorization context memoization.

[palkan]

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.

[ezekg]

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

config.action_policy.memoize_authorization_context = false

[palkan]

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.

[ezekg]

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.

[palkan]

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?

[ezekg]

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. 🙂

[palkan]

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 ☺️

[corroded]

@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.

[palkan]

@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).

[corroded]

@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

[palkan]

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.

[friendlyantz]

many thanks!