Ways to abstract JSON Api Authorization

[aaronbhansen]

Mostly putting this out there as a discussion on possible ways to abstract the code to make it easier to extend. A few things I think would be useful:

1. Separate out any Pundit requirements. While I think Pundit is great, is also has some limitations. Be nice to have it as an optional dependency.
2. Possibly create a scoped resource authorization abstraction. Mostly taking whats already in PunditScopedResource and abstracting the calls out so a custom scoped resource could be set in config. The Scoped resource might have three methods:
   1. records
   2. to_one_records
   3. to_many_records
3. Having a single responsibility for each authorization call in the default Authorizer. For example create today has two responsibilities. Just separating these two concerns out into two methods.
   1. authorize the creation of a new record (create_resource)
   2. authorize the creation of of each related record. (create_related_resource)

Here are my thoughts and you can lend me yours, if Pundit is no longer a requirement, it be fairly easy for people to create a custom and even very simple Authorizer. I know that this is possible today, but in my case, we want to use a modify version of Pundit, this is where we start to run into conflicts.

If the scoping authorization is separated out, some of that logic for determining the relationships can be moved into a separate class and create a standard interface for others to use.

Lastly, if the Authorizers only have a single responsibility, it would be easier to have the Processor handle getting the classes, relationships, etc, and then calling the correct methods on the authorizers. Then if others wanted to even have a modified Pundit Authorizer, they could inherit from the base and in the case of ticket 30's discussion, they could implement the authorizer relationship however they wanted and leave the remaining methods the same.

This is mostly quick thoughts to see if its something you're interested in doing or if you've had any thoughts along the same line.

[aaronbhansen]

@valscion any feedback on this? We are at the point where we need to either use jsonapi authorization or fork and create a new authorization library. I would rather combine efforts then create a separate project.

[lime]

Thanks for the input!

It seems to me like you are suggesting many things at once, which makes it a little overwhelming. I'll do my best to respond to each part. :)

[lime]

Separate out any Pundit requirements. While I think Pundit is great, is also has some limitations. Be nice to have it as an optional dependency.

This shouldn't require any large scale changes. It's been discussed before, and would very likely be useful to others as well. A PR is more than welcome.

As far as I can see, Pundit is only used inside default_pundit_authorizer.rb and pundit_scoped_resource.rb, which are both optional to use. (There's also a require inside authorizing_processor.rb, but that seems to be obsolete.)

[lime]

Possibly create a scoped resource authorization abstraction. Mostly taking whats already in PunditScopedResource and abstracting the calls out so a custom scoped resource could be set in config.

PunditScopedResource currently provides .records and #records_for. Both methods are part of the jsonapi-resources API. We're not adding anything of our own here, which I consider to be a good thing.

Likewise, the user has to explicitly include PunditScopedResource to use it. Since it's not automatically included, there's little need for it to be a configuration option. The user is free to implement their own record fetching mechanism if they want to.

The only thing PunditScopedResource does is to provide some convenient glue between the way jsonapi-resources fetches records, and Pundit policy scopes. Maybe this could be documented better?

The Scoped resource might have three methods: i. records ii. to_one_records iii. to_many_records

Do you think these are methods that could / should be added upstream to jsonapi-resources?

I consider it a strength of jsonapi-authorization that we stick to what's provided by jsonapi-resources as much as possible, and avoid adding more API surface than what's needed.

[lime]

Having a single responsibility for each authorization call in the default Authorizer. For example create today has two responsibilities. Just separating these two concerns out into two methods. i. authorize the creation of a new record (create_resource) ii. authorize the creation of of each related record. (create_related_resource)

This is one proposal on how to solve #30, right? Having a single responsibility for each authorization call sounds very good in principle. It's difficult for me to say whether that is a practical way of doing things in this case.

Maybe a PR would be the best way to shine a light on how you'd want to improve the Authorizer interface. I can't guarantee that a specific proposal would get merged, but it would definitely take us closer to a solution.

As for your example, I wonder if there's a mistake in part ii? Currently, create_resource authorizes the new association with, not the creation of each related record. Something like associate_related_resource or create_relationship, if you will.

[lime]

We are at the point where we need to either use jsonapi authorization or fork and create a new authorization library. I would rather combine efforts then create a separate project.

I'm really happy to see that you want to contribute to this project!

I want to encourage you to bring your suggested changes here, even when there's not a 100% agreement on them. If a PR gets rejected, it might just be an opportunity for a different approach, or a new, complementary library. If the approaches differ considerably, then sometimes a fork is the right thing to do. :)

Right now, I'd be eager to the solutions you've come up with in your project! Hopefully I managed to answer at least some of your questions.

[aaronbhansen]

@lime Thanks for the responses. It was a lot in one ticket, but I wanted to get a discussion together before I did any pull requests so that I didn't take the time to do several pull requests only to find out you weren't interested in these ideas anyway.

Mainly I want to create class contracts / interfaces so that authorization classes could be created with a known pattern.

I'll start break these ticket ideas into into several smaller pull requests, then we can discuss each one.

1. I'll look at making pundit optional and create another authorization class for projects without pundit
2. Create a simple pull request for PunditScopedResource. I know all it does today is a little bit of glue, but it might be nice to see what a possible abstraction looks like and if that code should be moved to json api resources or not
3. I'll create a pull request that breaks down some of the authorization methods into finer grained calls. This won't fix ticket 30 in and of itself, just provide the right methods to plug into for others to customize it to their needs. Then at that point we could look at how to better handle ticket 30 on these new interfaces.

[valscion]

Hi there @aaronbhansen! How do you feel about this issue now that time has passed? If you have done some work regarding the abstraction layers, it might affect other additions coming along, such as the work on #40. It would be amazing to get the least amount of merge conflicts as possible :smile:

[aaronbhansen]

@valscion sorry for the delay in movement on this. We planned to implement our json api implementation sooner, but are actually just getting to it now. We are about half way through the conversion and @nathanpalmer is actually working on the permissions aspect of the conversion now. Previous json api work had been done for clients, but we are finally getting to our own implementation. We should have examples and our implementation to show in a few weeks. I'll post an update once thats done.

[valscion]

Ok, great! Thanks for the information. Keep us posted, as we might get some merge conflicts with #40.

[valscion]

Any movement regarding this abstraction? Or can we close this issue, as it seems like it didn't get any traction?