Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pundit caching isn't sensitive to current user changing #811

Open
danielfone opened this issue Jun 10, 2024 · 3 comments · May be fixed by #830
Open

Pundit caching isn't sensitive to current user changing #811

danielfone opened this issue Jun 10, 2024 · 3 comments · May be fixed by #830
Labels
documentation problem

Comments

@danielfone
Copy link

@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 #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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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!

@Burgestrand Burgestrand changed the title New Pundit::Context caching prevents changing current_user Pundit caching isn't sensitive to current user changing Jul 8, 2024
@furkanural furkanural linked a pull request Sep 13, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation problem
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add hook for clear pundit context furkanural/pundit
2 participants
@danielfone @Burgestrand