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

Support for Pundit Policy Namespacing? #101

Open
drewnichols opened this issue Jul 19, 2018 · 8 comments
Open

Support for Pundit Policy Namespacing? #101

drewnichols opened this issue Jul 19, 2018 · 8 comments

Comments

@drewnichols
Copy link

Perhaps I have overlooked it but it seems that the way this gem is designed we are unable to take advantage of Pundit's Policy Namespacing feature.

We have a motivation similar to #8 in which users can access our apis under different contexts, such as an admin or a user, and thus should receive different policies and different scopes. The namespacing seemed a good fit as we could pass the PolicyFinder an array containing the context and the object instead just the object and it will resolve policy classes as follows:

PolicyFinder.new([:admin, post]) # => Admin::PostPolicy 
PolicyFinder.new([:user, post])  # => User::PostPolicy

Was this intentional? Am I missing something?

@Subtletree
Copy link

I would also be hugely in favor of this.

Namespaced policies would fit perfectly with namespaced resources/controllers imo.

@valscion Would this just require a processor change?

@valscion
Copy link
Member

Hi, thanks for opening an issue! Sorry for taking so long to reply — I've been on two weeks vacation and just came back to work today.

It seems like jsonapi-authorization hasn't been designed for use with policy namespacing the way Pundit does it. Policy namespacing seems to have been documented for v2.x of Pundit varvet/pundit@ef5cc85 and this library was written with Pundit v1, so this feature hasn't been considered when this gem was written.

I see @Subtletree has created a proof of concept for automatic namespacing based on resource class names in #102. I think it would be better if we didn't look at the resource class names and then use the namespace from that.

It would be nice to see some design around an explicit policy class definition, for example by using a special resource class method as originally proposed in #8 if that would fit your use case.

@Subtletree
Copy link

Hope you had a good break @valscion 😄

I think that's a good idea to step back to a design stage and look at actual use cases for namespaced resources/policies.

If there are use cases for specifying a policy override per resource then that sounds like the way to go.

If it ends up being that namespaced policies are always wanted for namespaced resources then I think automatic namespacing should be the way to go. This is how jsonapi-resources handles matching controllers to resources. Namespaced controllers are automatically matched to namespaced resources via the controller class name.

Another option would be to have a separate NamespacedAuthorizingProcessor for those who want that functionality.

@valscion
Copy link
Member

valscion commented Aug 1, 2018

Hmm. Yeah. My use case will break if namespacing is applied automatically, as our resources and controllers are namespaced under module API but we don't want to namespace policies with the API as well.

So this will need to be somehow configurable. And the implementation in this gem should be simple, or it won't be worth the maintenance cost. What is simple, is of course subjective — that's why throwing out some ideas and designs here can be useful

@nadnoslen
Copy link

With Pundit 2 supported in #104, could we not author a NamespaceAuthorizingProcessor that will take the resource's namespace into consideration when requesting the appropriate policy(ies)?

Enabling this behaviour is as simple as specifying this new config.authorizer in the config/initializers/jsonapi_authorization.rb initializer file.

I'm thinking ... drying up some of the existing JSONAPI::Authorization::AuthorizingProcessor could allow this to be achieved with pretty much the same code base. Several new tests will be required.

I'm very interested in this use case. Maybe I'm the only one! 😄

@valscion
Copy link
Member

As long as it's configurable and as straightforward as possible to do this, why not 😊. I don't want the policy finding for namespaced policies to be only possible via class name namespacing.

I'm open to see some code examples on how JA could support this :) a gist of an example use case would be nice to lead the conversation forward

@nadnoslen
Copy link

nadnoslen commented Oct 26, 2018

Interesting, that may add more complexity. Lazy me will argue that in following with Rails' convention over configuration, a NamespaceAuthorizingProcessor would only use the class path to derive the corresponding Policy. That is to say, a resource Api::V1::FooResource would only match to Api::V1::FooPolicy (using my proposed processor). I'm probably missing a bunch of extra complexity out here; especially concerning relationships!

Would you ever mix this class path resolution I describe with this configurable variation you yearn for? Or would only one of these techniques be activated at a time?

EDIT

Apologies -- I just noticed you mentioning in a comment above that you'd need to subtract the Api part from your classpath resolution. That flies right in the face of my convention statement earlier.

I'll fork and tinker.

@valscion
Copy link
Member

valscion commented Oct 26, 2018

One approach could be to have some sort of class method to define the namespace in one place, and then let the policy finding work as it did before. The class method would in that case have to be on the resource level, if I've understood your use case correctly.

module Api
  class MyResource
    some_special_method_from_jsonapi_authorization "ApiPolicyNamespace"
  end
end

Another option would be to apply some configuration on app-level wide, say some sort of mapping from a module namespace to a policy namespace, but I think this might be confusing for users.

# config/initializers/jsonapi_authorization.rb
JSONAPI::Authorization.configure do |config|
  config.foobar_policy_stuff_mapping_of_some_sort = {
    "Api::V1::*" => "Api::V1::*Policy"
  }
end

☝️ this seems like it could easily get confusing. And if all of the resources would have to be defined like this:

# config/initializers/jsonapi_authorization.rb
JSONAPI::Authorization.configure do |config|
  config.foobar_policy_stuff_mapping_of_some_sort = {
    "Api::V1::FooResource" => "Api::V1::FooPolicy"
  }
end

...it would be easy to forget to do this. Making something "easy to forget" would not match the core design principle of JSONAPI::Authorization: "Prefer being overly restrictive rather than too permissive by accident."

So the proposed solution would have to be something that would protect users from accidentally forgetting to do something. Take my contrived second example above: The implementation could yell out loud when a mapping was configured yet some resource didn't have an explicit mapping configured — this way, users would no longer accidentally forget to define the policy class for a resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants