Use Pundit for authorization + let there always be a model

[jensljungblad]

Fixes #229

This PR does two things:

1. Replaces our own authorization logic with Pundit. This is more or less backwards compatible.
2. Modifies the Resource::Service so that it by default expects the model to live under the same namespace. This is not backwards compatible.

Why the second change? A couple of reasons:

1. This is what Pundit expects, so we can basically just replace our own stuff with Pundit.
2. It feels more consistent to me. A standalone installation of Godmin would have models in the models directory. With this change, an engine installation would also have models in the models directory, even if those models only inherit from the main app's model. The downside of this is more files.

Things that remain if we want to do this:

• Modify the resource generator so that it also generates a model if running within an engine
• Update the readme

(This PR also fixes #181, a long outstanding issue that has to do with generating resources for namespaced models)

[jensljungblad]

This is the backwards incompatible change basically

[jensljungblad]

Rebase once #216 has been merged to shrink this one a bit.

[dgmstuart]

@jensljungblad I've been wrestling with this a bit: it makes sense for standalone godmin, but with an engine (as we have on Lärarförbundet, which I'm trying to update to use this) Godmin isn't able to find the policy: it seems to be looking for a policy for the main-app model in the main app, when it should be looking for a policy for the engine-app model in the engine.

I initially tried doing this:

# app/models/mymodel.rb

class MyModel < ApplicationRecord
  def self.policy_class
    Admin::MyModelPolicy
  end
end

...and it works, up to a point, but there are some cases where we have separate policies for the main app and the admin engine (also it's obviously bad to have the main app referencing the engine).

Explicitly specifying the policy in the engine model class doesn't work (and presumably isn't supposed to be necessary?)

I appreciate I'm asking you to dig back into the depths of your memory, but do you remember at all how this is supposed to work? Other than that, this PR message is all I've got to go on 😬 .

[dgmstuart]

More info:

I thought it might be because there are lots of references in view code to the models from the main app, and I thought maybe those need to be namespaced so they're using the model in the engine (eg. replacing every reference to User with Admin::User). But the issue there is that when we get objects via relations, they're of course going to be the model from the main app, not the model from the engine.

Here's an example of a place which causes a problem:

<%= navbar_item admin_user.department.title, edit_department_path(admin_user.department),
   show: -> { policy(admin_user.department).edit? } %>

It's fetching a Department record from the main app, not an instance of Admin::Department, and it's not looking for the Admin::DepartmentPolicy like we'd expect:

unable to find policy `DepartmentPolicy` for `#<Department id: 1, title: "Centralavdelning", 
...

Not sure how this is supposed to work.

[dgmstuart]

With a fresh head this morning, I see something new:

In the old code, the policy finder took a namespace and specifically looked for policies in the namespace:

          if namespace
            "#{namespace}::#{klass}Policy"
          else
            "#{klass}Policy"
          end.constantize

This feels pretty important. I wonder if this is actually a regression that I need to make a fix for, or if there's some intended work around.

[jensljungblad]

Hey @dgmstuart . If I remember correctly, this is roughly what happened:

• Godmin (v1) was not using Pundit, but rather its own policy implementation, which was almost identical to Pundit, but supported namespace lookup. So you could put your models in app/models/article.rb and have that use a policy located in admin/policies/article_policy.rb. So you could have one policy for your main app and one policy for your admin engine.
• At some point, Pundit added support for namespaces
• At some other point, either before or after, we were thinking about some big changes, basically Godmin 2.0. After some deliberation, we decided we could use the master branch for working on Godmin 2.0, and keep the 1.x branch alive and supported while we were working on 2.0
• The first thing we did was this PR, which was intended to replace Godmin's own policy implementation with Pundit. I don't remember if Pundit had support for namespaces by this point or not. But for some reason we had the idea that it might be a good idea to default to using the same namespace as the model for lookups. We had used basically what's mentioned in the PR description, as reason 2, as a technique in some projects, and thought maybe that was promising. Basically having an empty admin/models/article.rb inheriting from app/models/article.rb, which would help with namespace resolution, and also provide a place to put model specific logic that was only relevant to the admin context. This did lead to a lot of files though, and might not have been the best idea :)
• Both me and Linus left, and the state of master was probably left unclear.
• The version on master was released as 2.0, even though this PR was pretty experimental.

In hindsight, we should probably have created a 2.x branch and merged this PR there instead. To sum up: we never really had time to play around with this change, and there might be issues with it.

We did also discuss just ditching authorization altogether from the library and just keep some hooks in the controllers, leaving it up to the app to implement. If I would do it again, that's probably what I would do. There's a separate repo I believe, might be under my account, with more experiments. Not sure how interesting those are, or how much you're gonna work on this. I.e. is it life-support or do you wanna compete with administrate/activeadmin :)

[dgmstuart]

Thanks for the great explanation 🙏

The version on master was released as 2.0, even though this PR was pretty experimental.

Yeah I thought that might be the case!

At the moment it's just life support: I need to get Rails upgraded on a site, godmin has an incompatibility, and I couldn't get the 1.5 branch to build on recent rubies.

I've kind of concluded that the approach of creating models in the engine app doesn't work in general, because of relations: when you reference an Admin::Article.find(1), sure, it gets Admin::ArticlePolicy, but if you reference Admin::Article.find(1).author then that's going to be an instance of ::Author and will look for an ::AuthorPolicy in the main app.

Maybe I'm missing something, but I don't feel like there's an easy way round that.

I'm working on the assumption that we only ever ever want to refer to policies within the engine, and never in the main app. I have a solution which involves always adding the engine namespace when looking for policies (unless it's there already).

[jensljungblad]

I've kind of concluded that the approach of creating models in the engine app doesn't work in general, because of relations: when you reference an Admin::Article.find(1), sure, it gets Admin::ArticlePolicy, but if you reference Admin::Article.find(1).author then that's going to be an instance of ::Author and will look for an ::AuthorPolicy in the main app.

That's a good point!

My recommendation would probably be, for life-support, to either revert this change or change the implementation to use the Pundit namespace feature, passing in the Godmin::EngineWrapper#namespace as the Pundit namespace.

For a long term solution, I would probably remove authorization altogether, just leaving hooks around. In the code base I'm working on currently, for instance, our admin panel (activeadmin btw, haha) is using the same policies as the main app. So for some apps it might not make sense to split policies between main app and admin contexts. As in, probably better to leave it up to the application to decide :) (It's also what the administrate gem is doing)

[dgmstuart]

@jensljungblad thanks for the help. I went with the namespacing route: #259

It required a bit of fiddling to handle cases where the model or policy is already namespaced, but I'm not unhappy with the result.

My feeling is that it does make sense to have different policies between the admin app and the main app: I have the perspective that the admin app can be thought of as a completely separate application with a potentially completely different set of users, which just happens to share the same model layer as the main app.

...but my perspective is extremely coloured by the one app that I'm using Godmin on!

[jensljungblad]

@jensljungblad thanks for the help. I went with the namespacing route: #259

Cool!

My feeling is that it does make sense to have different policies between the admin app and the main app: I have the perspective that the admin app can be thought of as a completely separate application with a potentially completely different set of users, which just happens to share the same model layer as the main app.

...but my perspective is extremely coloured by the one app that I'm using Godmin on!

Yeah in the current app I'm working on there is a completely separate set of users, but they still share the same policy classes! I tend to agree though that mostly it would make sense to split them up. My point is mostly that there are so many use cases, and the "glue code" provided by Godmin would be so trivial to implement for each app, that I'm thinking it would make more sense to just leave it out altogether, and just keep some hooks that apps can leverage. Would also mean no dependency on Pundit.