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

Use pundit policy_namespace in controllers #2379

Conversation

bhtabor
Copy link
Contributor

@bhtabor bhtabor commented May 5, 2023

Add pundit policy_namespace to controllers
Deprecate resolve_admin from pundit policies

@bhtabor
Copy link
Contributor Author

bhtabor commented May 5, 2023

Fixes #1332

To use specific Policy Namespacing for authorization, like admin namespaced policies for example, just set the policy_namespace array like below.

module Admin
  class PostsController < ApplicationController
    include Administrate::Punditize

    def policy_namespace
      [:admin]
    end
  end
end

@pablobm
Copy link
Collaborator

pablobm commented May 12, 2023

Nice, thank you! I don't have much time now, but I'll review it a bit more thoroughly when I can.

One thing is that ideally I'd like to have some sort of back-compatibility hook where we show a warning to people using the old interface. Do you know how we could achieve this? If you don't, it's ok, I'll get to it. At least this PR gets us moving.

@bhtabor bhtabor force-pushed the feature/use_pundit_policy_namespaces_in_controllers branch from 4fb6ee4 to aab1325 Compare May 13, 2023 07:06
@bhtabor
Copy link
Contributor Author

bhtabor commented May 13, 2023

One thing is that ideally I'd like to have some sort of back-compatibility hook where we show a warning to people using the old interface. Do you know how we could achieve this? If you don't, it's ok, I'll get to it. At least this PR gets us moving.

Just added pundit policy scope resolve_admin deprecation. Thank you!

Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

One thing I can't find a good place to put: I think we should remove OrderPolicy#resolve_admin, for two reasons:

  1. It pollutes the specs with warnings.
  2. Our example app should avoid deprecated APIs.

The problem is that then this would break the specs you wrote to check for deprecations. I wonder if we can do this differently, perhaps adding #resolve_admin in a before block with metaprogramming, then remove it in the after block. Thoughts? I'm happy with having a much simpler spec for this, instead of looking

Anyway, this is looking pretty good! I want to give it another review though, so make sure I'm not missing anything as this could have large repercussions for many people.

Also I wonder if @nickcharlton or @littleforest would have some time to have a look too 👀

app/controllers/concerns/administrate/punditize.rb Outdated Show resolved Hide resolved
app/controllers/concerns/administrate/punditize.rb Outdated Show resolved Hide resolved
app/controllers/concerns/administrate/punditize.rb Outdated Show resolved Hide resolved
@bhtabor bhtabor changed the title Use pundit policy_namespaces in controllers Use pundit_policy_namespace in controllers May 20, 2023
@bhtabor bhtabor force-pushed the feature/use_pundit_policy_namespaces_in_controllers branch from 9fb1aab to 98c92e9 Compare May 20, 2023 07:08
@bhtabor
Copy link
Contributor Author

bhtabor commented May 20, 2023

One thing I can't find a good place to put: I think we should remove OrderPolicy#resolve_admin, for two reasons:

  1. It pollutes the specs with warnings.
  2. Our example app should avoid deprecated APIs.

Totally agree. I've addressed those issues. Thank you!

@bhtabor bhtabor force-pushed the feature/use_pundit_policy_namespaces_in_controllers branch from f06c9f3 to c700025 Compare May 21, 2023 08:55
Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

We are getting there! Once we resolve the open threads, I think this can be merged if my colleagues don't have any other comments.

app/controllers/concerns/administrate/punditize.rb Outdated Show resolved Hide resolved
bhtabor added 5 commits June 4, 2023 12:23
Add pundit policy_namespaces to controllers
Remove resolve_admin from pundit policies
Add pundit policy scope resolve_admin method deprecation
Fix hound errors
Replace #policy_namespaces with ::pundit_policy_namespace in controllers
Remove unnecessary namespaced_policy_scope private method from punditize
Remove unnecessary guard in punditize policy_scope!
Remove deprecated resolve_admin from spec example app policy scope
Fix pundit_policy_namespace inheritance
@bhtabor bhtabor force-pushed the feature/use_pundit_policy_namespaces_in_controllers branch from c700025 to 51cc9fd Compare June 4, 2023 09:27
@bhtabor bhtabor changed the title Use pundit_policy_namespace in controllers Use pundit policy_namespace in controllers Jun 4, 2023
@bhtabor
Copy link
Contributor Author

bhtabor commented Jun 4, 2023

We are getting there! Once we resolve the open threads, I think this can be merged if my colleagues don't have any other comments.

I've addressed the open issues and rebased to main. Thank you @pablobm!

Revert pundit_policy_namespace use in controllers
Use pundit policy_namespace in controllers
@bhtabor bhtabor force-pushed the feature/use_pundit_policy_namespaces_in_controllers branch from 51cc9fd to 6ff8e26 Compare June 4, 2023 09:45
@pablobm
Copy link
Collaborator

pablobm commented Jun 9, 2023

Let's go go go! 🚀

@pablobm pablobm merged commit b8bfeab into thoughtbot:main Jun 9, 2023
@pablobm
Copy link
Collaborator

pablobm commented Jun 9, 2023

Thank you @bhtabor 🙂

@bhtabor bhtabor deleted the feature/use_pundit_policy_namespaces_in_controllers branch June 10, 2023 07:34
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