Fix pundit namespace detection

[vlad-psh]

Given a following config:

config.authorization_adapter = ActiveAdmin::PunditAdapter
config.pundit_policy_namespace = :admin

ActiveAdmin will search policies with Admin:: namespace.
But if model already contains the configured namespace name (anywhere in the model name), AA won't tell Pundit to search policy with configured namespace.

For example, if we have model called ShopAdmin, we'll look up for ShopAdminPolicy instead of Admin::ShopAdminPolicy

Replacing condition in https://github.com/activeadmin/activeadmin/blob/master/lib/active_admin/pundit_adapter.rb#L61 to something like this, fixes this bug:

- if default_policy_namespace && !object.class.to_s.include?(default_policy_namespace.to_s.camelize)
+ if default_policy_namespace && !object.class.to_s.match?(/^#{default_policy_namespace.to_s.camelize}::/)

[deivid-rodriguez]

Makes sense to me, can you add a spec?

[vlad-psh]

Makes sense to me, can you add a spec?

@deivid-rodriguez Thanks for your comment.

Sure, I'll add specs for this change, but if possible, I'd like to hear your thoughts regarding this PR first.

The thing is, although this PR fixes policies lookup to be working as expected, it probably also will break existing policies for some users.
I believe there are users who has all AA policies in app/policies/admin/*, but policy for ShopAdmin (for example) in app/policies/shop_admin_policy.rb instead of namespaced app/policies/admin/shop_admin_policy.rb just because it worked that way before.

What do you think? How can we make this transition smooth?

[deivid-rodriguez]

I see, nice catch!

So, I guess we should keep both conditions, the old one and the new one. If the old one is met, but the new one is not met, then we should keep current behavior and print a warning, so that users can accomodate their policy to the proper convention. Something like

def namespace(object)
  if default_policy_namespace && !object.class.to_s.include?(default_policy_namespace.to_s.camelize)
    if object.class.to_s.match?(/^#{default_policy_namespace.to_s.camelize}::/)
      [default_policy_namespace.to_sym, object]
    else
      warn "<explain the issue and how to fix it>"
      object
    end
  else
    object
  end
end

Makes sense?

[deivid-rodriguez]

Ping @vlad-psh, still interested?

[vlad-psh]

@deivid-rodriguez Thanks for your suggestion!

I found out that actual logic would be a little bit more complicated

We need first search namespaced policy (eg: Admin::ShopAdminPolicy), if this policy weren't found, we can try to search for a policy without namespace (eg: ShopAdminPolicy) but only if default_namespace is configured and model name contains namespace in the name.

We'll show a warning only if policy were found at second try.

Otherwise, we fallback to default_policy (as we always did)

Please, take a look, what do you think?

If all good, I'll write the specs for it

UPD: specs added

[vlad-psh]

Rebased onto current master to see if tests are still passing

[deivid-rodriguez]

It looks really good, I only left some minor comments.

Also I noticed that our test app has this issue since we have an AdminUser policy at the top level. Can you move the policy to fix the warnings being introduced?

[vlad-psh]

@deivid-rodriguez Thank you for review 👍
Fixed everything except following two things:

Also I noticed that our test app has this issue since we have an AdminUser policy at the top level. Can you move the policy to fix the warnings being introduced?

As I can see, we don't have configured config.pundit_policy_namespace in our test app. Can you point me where you saw new warnings? Those probably are just some special cases and needed to be fixed individually 🤔

- warn "WARN: You have `pundit_policy_namespace` configured, " \
+ Deprecation.warn "You have `pundit_policy_namespace` configured, " \

I tried this, but Deprecation.warn will throw an exception. I thought this shouldn't be a fatal error, but just a simple annoying warning in the console 🤔

UPD: Refactored new compat_policy method. Got rid of two nested if conditions. Now it's a little bit easier to read this method.

[deivid-rodriguez]

Yes, Pundit is not enabled by default (perhaps it should be?) but can be enabled by setting authorization_adapter to ActiveAdmin::PunditAdapter and pundit_default_policy = "ApplicationPolicy" in activeadmin configuration's of the generated test application.

That configuration should live at the tmp/development_apps/rails_70/config/initializers/active_admin.rb file generated by running bin/rake local s. If you make those changes and restart the application, visiting http://localhost:3000/admin/admin_users should trigger the warning (because the admin_users_policy is not scoped to admin).

I tried this, but Deprecation.warn will throw an exception. I thought this shouldn't be a fatal error, but just a simple annoying warning in the console 🤔

Interesting... I tried the above and didn't raise in the development application. I think this depends on the warning configuration of each specific application, you probably have config.activesupport.deprecation set to :raise? See https://guides.rubyonrails.org/configuring.html#config-active-support-deprecation

[vlad-psh]

Interesting... I tried the above and didn't raise in the development application. I think this depends on the warning configuration of each specific application, you probably have config.activesupport.deprecation set to :raise? See https://guides.rubyonrails.org/configuring.html#config-active-support-deprecation

@deivid-rodriguez Thanks for directing me, I found the reason. It's in https://github.com/activeadmin/activeadmin/blob/master/spec/rails_helper.rb#L29
This option configured explicitly to :raise for rspec.

Should I change rspec example to catch an error instead of capturing stderr?
I'm not sure because this will be a little bit different behavior comparing to the scenario with default configuration (which is :stderr)

Yes, Pundit is not enabled by default (perhaps it should be?) but can be enabled by setting authorization_adapter to ActiveAdmin::PunditAdapter and pundit_default_policy = "ApplicationPolicy" in activeadmin configuration's of the generated test application.

I think this is fine as it is now. We don't have to configure pundit_default_policy in our test app.
Or if we do, we should move all the policies (not just a single AdminUserPolicy) into our configured namespace.
I don't think it is necessary for test app. What do you think?

[deivid-rodriguez]

Regarding deprecations, the motivation for having :raise in our test suite is to make sure we get all deprecations fixed right when deprecations are first introduced, so that we can easily add support for future Rails versions later. But I understand how this caused confusion here since it deviates from the standard configuration for test environments. So, not fully sure what's best, what do you think?

Regarding "fixing" deprecations in the test app, you're totally right, it's ok as it is, nevermind.

[vlad-psh]

@deivid-rodriguez I decided to just add ActiveSupport::Deprecation.behavior = :stderr for these few tests (and to restore it back to :raise in the after block). Should be a good compromise for everybody, I guess 🤓

[deivid-rodriguez]

Sounds good! I'll have a final look soon and get this shipped with the next release. Thanks so much!

[deivid-rodriguez]

I just added a very minor style tweak (keep the variable to capture the exception named as e instead of error, for consistency with the other similar code in this file, and to keep the fix more focused) and rebased this.

Thanks so much for the outstanding work! 💪