Using an instance of Pundit per request…?

[Burgestrand]

Hi!

I wanted to let you weight in on a very rough idea that's been simmering in my mind the past few months.

Today we are:

• delegating calls directly to Pundit.authorize
• caching policies lookup to retain the policy instance, and caching policy scope lookups
• ^ we are not storing lookups when passing an explicit policy_class, similar to what's discussed in policy_class and policy usage #740
• authorize and policy no longer share a connection, making it necessary to override both in certain situations
• Frankly I'm also not much of a fan of the current solution we have to namespacing either, where we're passing an array.

I'm wondering if we can move past the above points by having an contextual Pundit object that tie these things together. The following example is not complete, but I'm hoping it will give a good sense of what I'm after.

class SomeArbitraryController
  # Here's the main entrypoint! Everything Pundit uses this in some way or another.
  def pundit
    @pundit ||= Pundit::Context.new(
      user: pundit_user,
      # finder: Pundit::CachedFinder.new(...) # we could use a cached finder that stores lookups
      # finder: Pundit::Finder.new(namespace: Admin) # maybe this is a way to do namespacing?
      # namespace: Admin # this is another idea of how namespacing could be done?
    )
  end

  # All of the methods we're used to are delegated to the Pundit configuration we're using.
  def authorize(...) = pundit.authorize(...)
  def scope(...) = pundit.scope(...)
  def policy(...) = pundit.policy(...)

  # Maybe store the "authorization called" flag in the context too?
  def verify_authorized = raise unless pundit.authorize_called?
end

The context wouldn't be doing much. Mostly it would be aware of itself and the collaborators we have going on, delegating most of the work to them.

module Pundit
  class Context
    def initialize(user:, finder:)
      @user = user
      @finder = finder
    end

    def authorize(record, query:, policy_class:)
      policy = @finder.find_policy(policy_class).new(@user, record)
      # … rest of the owl 🙂
    end
  end
end

There are some key points to take away:

• there would be a new context for each new request, they're not shared
• the context is configured with a namespace somehow (perhaps through the finder, perhaps in the context itself)
• the context (optionally) caches policy lookups somehow
• this "context" makes some of the Pundit::Authorization things more concrete

I'm not sure this will actually fix all issues. It's a response to me feeling that when we refactored authorize to Pundit.authorize we lost some contextual information, which we tried to make up for by making them parameteric on Pundit.authorize.

[Burgestrand]

This would probably affect #746 too.

[mattzollinhofer]

Quick Take

This approach makes a lot of sense to me. I appreciate the way you're tying together the contextual information for use through the lifecycle of the request while still providing the ability to override as needed.

The Biggest Question:

• How will (or won't) this impact existing systems?
  • Maybe: Are there cases where user's have unintentionally relied on the un-integrated current state of authorize and policy (and maybe other methods)?

Confirm My Understanding

Let's see if I understand how this would affect a straightforward case and my previous override case.

Straightforward Case

There's really no simple right! 😄

In the typical case, I imagine your approach could be implemented with no change to user's code/experience right? Pundit would mixin a default def pundit for all controllers and everything would continue work as it has.

class PostsController < ApplicationController
  def show
    @post = Post.find(params[:id])
    authorize @post
  end
end

Override Case

In the more complex case, user's could implement their own def pundit method to specify/override as needed, but their customizations would affect all uses throughout the request lifecycle.

class ComplexPostsController < ApplicationController
  def pundit
    @pundit ||= Pundit::Context.new(
      user: pundit_user,
      policy_class: SpecialPostPolicy  # <--- continue supporting user's providing a specific `policy_class`?
    )
  end

  def show
    @post = Post.find(params[:id])
    authorize @post
  end
end

<%# Now this WILL USE `SpecialPostPolicy` because `policy` is driven by the overriden `def pundit` %>
<% if policy(@post).update? %>
  <%= link_to "Edit post", edit_post_path(@post) %>
<% end %>

Final Thoughts

@Burgestrand: I think this approach has promise. For me, this has enough merit to warrant working through a rough PR to see if real code would show cracks in this theory. The devil is in the details right 🦉 😆 ?

I've been out of Pundit details for a couple years now. I wish I could commit to working that up a PR for you, but I don't want to offer that and not follow through. If you think this concept is the direction the project should go I could probably get excited to work with you or someone on it though.

[Burgestrand]

@mattzollinhofer that's a great writeup! I'll read it through in detail some time later. I just wanted to let you know I've got a draft up: #797

[mattzollinhofer]

Oh, interesting ... maybe my delay actually ended up being decent timing 😆 . I hope to take a look at your draft later today/tonight.