Pundit caching isn't sensitive to current user changing

[danielfone]

@Burgestrand Hi there! Firstly, thanks for all the hard work maintaining pundit, it's a brilliant gem and I'm a long time fan.

Re https://github.com/varvet/pundit/pull/797

This should be backwards-compatible, so let me know if this breaks anything.

Unfortunately, 2.3.2 has broken a key piece of authorization functionality in one of my apps. I'm not sure if it's because I was doing it wrong in the first place, or if this is an unintentional regression. I have a controller concern that allows admins to impersonate/become another user, to view the app from another user's perspective. The way we implement this is approximately:

module AdminOverride
  extend ActiveSupport::Concern

  included do
    before_action :override_with_admin_user, if: -> { session[:admin_user_as] }
  end

  def override_with_admin_user
    return unless current_user

    authorize :user, :become? # First, ensure that the current user has permissions to 'become' another user
    @original_user = current_user
    @current_user = User.find session[:admin_user_as] # If so, override the current user
  end

end

Unfortunately, since Pundit::Context is initialised on the first policy look up (when the current user is the admin) and memoizes the current user at that point, all subsequent policies are checked against the admin user, and not the impersonated user.

I can work around it in a variety of ways, but wanted to let you know and see if this is something you're concerned about. It was unfortunate that it (a) happened in a patch release, and (b) my integration testing wasn't sufficient to catch this change, since it slipped into production and I had a very panicked phone call from an admin who thought users could now see everything. 😂 You live and learn!

Thanks again.

[Burgestrand]

Hi @danielfone this is amazing feedback!

I'll ponder what Pundit should do here. The context cache might benefit from being current user-sensitive, to avoid accidents. It also might be worth having a recommended approach documented in the README.

Not sure how much Pundit itself should help with the multi-user case, mostly because it's a relatively rare use-case.

Also, one very important thing to consider, is that previous releases of Pundit did also have issues with caching and changing the current user. We've been caching policy lookups for years, and that cache is also not sensitive to the current user changing.

Effectively this won't be good:

def show?
  post = Post.find(...)
  # current_user = post.author here
  authorize post

  @current_user = User::Guest.new
  authorize post # => WARNING! cached post policy from earlier
end

So if you do switch users like this, you need to be mindful of all the caches: pundit, policies, and policy_scopes.

[danielfone]

Thanks @Burgestrand. Yes, I don't blame you if you don't want to support this use-case, I should really implement it as a high-order concern. In my specific case, the other caches never affected things since the impersonation was very early on in the request.

I suppose the more salient issue is that, as naive user of the gem, there's a bit of hidden 'magic' happening between the authorize call in the controller and the nice clean policy POROs. Maybe calling out those caching details in the README would be sufficient to avoid future edge-case foot guns.

Appreciate your time!

[Burgestrand]

I suppose the more salient issue is that, as naive user of the gem, there's a bit of hidden 'magic' happening between the authorize call in the controller and the nice clean policy POROs.

Very much agree. Pundit should strive to minimise surprise 🙂

If nothing else, documenting how to safetly deal with user-switching mid-request is something we could do!