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.

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

add hook for clear pundit context #830

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

furkanural
Copy link

@furkanural furkanural commented Sep 13, 2024

Fix #811

As far as I can see, the caches are not being cleared after a UserContext change. Therefore, policies are being performed using the old UserContext. I would like to implement a simple solution to this issue. With this solution, caches can be cleared when deemed necessary.

class ApplicationController
  include Pundit::Authorization
  before_action :switch_user, if: :should_switch_user?

  def switch_user
    current_user = User.find(params[:user_id])
    pundit_reset!  # Ensure that the Pundit context is reset for the new user
  end
end

To do

  • I have read the contributing guidelines.
  • I have added relevant tests.
  • I have adjusted relevant documentation.
  • I have made sure the individual commits are meaningful.
  • I have added relevant lines to the CHANGELOG.

PS: Thank you for contributing to Pundit ❤️

@Burgestrand Burgestrand added the simmering undecided but generally optimistic label Oct 8, 2024
@Burgestrand
Copy link
Member

Thank you for the PR! I like the implementation. Clear, well-written, and tests. Nice!

I have some hesitations that I need to ponder:

  • Against: I don't believe this is a common need for most applications.
  • Against: It expands public API of Pundit, which needs to be carefully considered and maintained.
  • For: Having an official reset allows people using Pundit to trust this to be future-proof. That has value.

Before this would be merged, assuming it will be merged, I think some changes should be made:

  • Documentation change mentions "accounts", but we're dealing with user in Pundit. The hypothetical method should probably be switch_user to keep things simple.
  • I believe the method name should be pundit_reset, to follow the convention.
  • I'm not certain of this, but we should probably clear the flags too.
    • @_pundit_policy_authorized
    • @_pundit_policy_scoped

@Burgestrand Burgestrand self-requested a review October 8, 2024 07:59
Copy link
Member

@Burgestrand Burgestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@furkanural
Copy link
Author

@Burgestrand Thank you for your thoughtful feedback! I appreciate your comments and am glad the implementation resonates well with you.

Regarding the concerns:
Against: Common need for most applications: I understand that this might not be a widely requested feature. However, I believe there are certain cases where having a reliable reset mechanism would greatly benefit applications with complex authorization flows. That said, I see your point and think it’s important to weigh the demand versus maintenance cost.

Against: Expands public API: You’re absolutely right :)

For: Future-proofing: Agreed! One of the key advantages of adding this feature is precisely the long-term value it provides for those who rely on Pundit. It gives them confidence that their systems will remain stable and maintainable as the library evolves.

Now, for the specific changes:

  • Documentation and naming: Excellent catch! I agree that the documentation should refer to "users" rather than "accounts" to maintain consistency within Pundit’s terminology. I'll update the method name to switch_user as well—it keeps things intuitive.
  • Method naming convention: I’ve renamed the method to pundit_reset! to follow Ruby conventions for methods that perform a significant change or could have side effects. Since the reset operation clears important state, I think the bang method makes sense here.
  • Flag clearing: I see your point about potentially clearing the flags. I’ll investigate this further to ensure we aren’t leaving any unnecessary state behind after a reset. I'll review both @_pundit_policy_authorized and @_pundit_policy_scoped and make the necessary adjustments if needed.

Thanks again for the detailed review! I’ll proceed with the suggested changes and address the concerns you’ve raised.

Copy link
Member

@Burgestrand Burgestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the requested changes. I have some more 😁!

I've thought about this some more, and while I haven't yet finally decided I'm leaning even more into merging this.

lib/pundit/authorization.rb Outdated Show resolved Hide resolved
spec/authorization_spec.rb Outdated Show resolved Hide resolved
spec/authorization_spec.rb Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
simmering undecided but generally optimistic waiting for response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pundit caching isn't sensitive to current user changing
2 participants