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

First pass of Pundit::Context #797

Merged
merged 7 commits into from
May 8, 2024
Merged

First pass of Pundit::Context #797

merged 7 commits into from
May 8, 2024

Conversation

Burgestrand
Copy link
Member

@Burgestrand Burgestrand commented Mar 1, 2024

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

See #774 initial draft/rationale.

Caution

  • We're introducing a new method Pundit::Authorization#pundit. This is a namespace that is technically not ours, as this module included in controllers.

Hopefully this allows us to:

  • ... improve/fix existing functionality while staying backwards-compatible, by using dependency injection.
  • ... introduce new functionality without necessarily bloating controllers in Pundit::Authorization.
  • ... introduce an actual API for namespaces rather than overriding authorize/policy.

@Burgestrand Burgestrand force-pushed the kbs/pundit-context branch 4 times, most recently from eaf3765 to 5e8c0d2 Compare March 7, 2024 09:22
@Burgestrand
Copy link
Member Author

Right now this change is backwards-compatible, possibly with the exception of our new pundit method. We could probably try this out in the wild, and then expand on the Pundit::Context API surface to introduce namespacing.

lib/pundit/context.rb Outdated Show resolved Hide resolved
@Burgestrand
Copy link
Member Author

Unless it's far too controversial I'd like to be able to remove the singleton methods on Pundit class in the future, and instead call them through a Pundit::Context. This is only if you're using Pundit outside of Rails.

For those who uses Pundit::Authorization (i.e. Rails), I'm thinking not much would change.

lib/pundit/context.rb Outdated Show resolved Hide resolved
lib/pundit/context.rb Outdated Show resolved Hide resolved
lib/pundit/context.rb Show resolved Hide resolved
@Burgestrand Burgestrand force-pushed the kbs/pundit-context branch 7 times, most recently from a2f28ee to 399fe6e Compare March 27, 2024 15:29
@Burgestrand
Copy link
Member Author

@mattzollinhofer I've added some more commits to kinda hint at where I'm going at.

A change I haven't gotten to is passing more information to policy_cache.fetch, at least the user and maybe the passed-in policy_class. Once that's done it would be up to the cache strategy to turn those three into a cache key.

We would achieve backwards-compatibility by having a LegacyCache as default cache, and that cache simply ignores the user and policy_class passed. It would then work just as before.

By then implementing a new cache strategy that's opt-in we can get caching behaviour when passing policy_class. How that looks then later in the view I'm not sure. Maybe we would require passing the overridden policy from the view too, so that the cache key remains the same - otherwise it would be impossible to mix explicit policy_class with pundit-found policy.

@Burgestrand
Copy link
Member Author

Burgestrand commented Mar 28, 2024

Another idea I had for this is some shortcuts to how one could override the default behavior in specific controllers, e.g:

class AdminControllerBase < ApplicationController
  def pundit
    @pundit ||= super.with_namespace(Admin)
  end
end

@Burgestrand Burgestrand marked this pull request as ready for review May 8, 2024 16:38
@Burgestrand
Copy link
Member Author

Had a few people at RailsConf try this out in their apps, and nothing broke! Thank you all :)

Burgestrand and others added 7 commits May 8, 2024 12:40
It has unwanted behaviour, such as being lenient in initialisation parameters and being loose in equality check.
This is not strictly a feature that was intended, but it's how it works so it needs to be supported in the future.
See #774

An API-change in this commit is that we're introducing a new method named `pundit` in `Pundit::Authorization`. We should not take this change lightly, because this is a namespace that is technically not ours as we're being included in controllers.

The hope is that the context method will help _reduce_ the overall API surface.

Additionally this will hopefully help us introduce an API for a namespaced Pundit.
An `!` means caution, but not much more. These comments help highlight what to be cautious about.

Co-authored-by: mattzollinhofer <matt.zollinhofer@gmail.com>
See #801 (comment)

We'd like to support caching a few more lookups, but that's backwards-incompatible. By swapping out the cache, we could have a legacy cache (that only caches "the old way"), and a new cache that will cache _more_ things. This allows backwards-compatibility with the same API interface.
This should be backwards-compatible because:

* `Pundit.policy` and `Pundit.policy!` are unaffected by this, since they use a non-cache cache.
* `Pundit::Context` is new, so nobody is relying on these methods yet.
* `Pundit::Authorization#policy` already cached the policy anyway.
* `policies` is explicitly discouraged, but even if it is relied upon it works as it always has, by default.

This is an improvement because we can change the cache strategy to introduce new features without breaking backwards-compatibility.
... and possibly by extension also the `policy_class`.
@Burgestrand Burgestrand merged commit 176cabb into main May 8, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants