Can we improve how we guard collections of items in the schema?

[tristan-orourke]

Using the @can directive with resolve: true allows us to run a policy on every item in a collection. This will cause a query to fail if the collection contains an item which fails the policy.

This is nice and easy in some ways - we can think about the policy in terms of individual items, which may be simpler, and blanket-apply it to collections without thinking about it. However, this can lead to inconsistent errors, since a query may fail or not depending on the contents of the collection. (See #6005.)

Some suggested alternatives:

• instead of using policies to fail if the query would return unsafe items, use scopes to simply filter out any items the user is not allowed to see
• try to use @canOnParent instead of @can on collections. Since this will only apply to a single item, not a collection, any errors may be more consistent.

[esizer]

So, this may be news to some (it is for me) but, I was totally misunderstanding the viewAny policy method. Which is making me settle on preferring scopes vs policies for this.

viewAny should almost always be true, for any model. Why?

viewAny works more like viewSome than viewAll

I initially understood viewAny as viewAll but it is not that at all. The idea behind viewAny is that, if you can see just one entity within a model then viewAny should be true.

viewAny shouldn't care what entity you can view as long as you can view at least one of them. What it is not; is picking an item from random and determining if it can be viewed which is what I thought it was at first 😅

So, for queries that return an array of items we should be using viewAny then applying scopes to only return the items a user can view. I think we can come up with a consistent name for this scope like availableToQuery or something and apply it to any of the index queries that have some sort of restriction over who may view specific items in that particular model.

I think this makes sense if we see policies like this:

1. The Query.pools is run
2. Policy checks viewAny to see if the user can view at least one pool
3. Yes, you can view at least one pool
4. The scope now kicks in to tell you what pools you can view

So, the viewAny method doesn't really care which entity you can view it just cares that it is possible to view something from that model. The scope is when we decide what we want to return to a user.

[tristan-orourke]

Okay, so as I think about it more, scoping queries to the items a user is already authorized to see is sounding nicer. It would solve a few recent bugs 😅 .

Devils advocate - I do want to point out that this would be a pretty significant change to our schema patterns, the authorization scopes might be a lot to maintain, and some of them may be difficult to write (while remaining performant queries).

[esizer]

Hmm. I've been interpreting viewAny as viewAll, not viewSome, for years. 🤔 So that's what its implemented as, right now. You really think it would be more beneficial as the alternative?

Yeah, that is how it is meant to be used. Terrible naming on their part for this policy method. Having something like viewAll and viewSome or viewOneOrMore makes way more sense, "any" is far too vague 😛

REF:

• https://www.reddit.com/r/laravel/comments/lmie8a/viewany_policy/
• Policy::view must also pass Policy::viewAny laravel/nova-issues#1762 (comment)

A name suggestion for the scope you're talking about: authorizedToView

I like this name suggestion 👍

Devils advocate - I do want to point out that this would be a pretty significant change to our schema patterns, the authorization scopes might be a lot to maintain, and some of them may be difficult to write (while remaining performant queries).

Agreed, it is quite the divergence from what we are currently doing 😅 As well, it will take quite a bit of time and effort.

[petertgiles]

Using scopes for security seems like a very messy mixing of concerns and I agree that we should have second-thoughts about that. I wish it was possible to have a policy simply prevent delivery of a model instead of erroring out the entire response.

I think that's a REST hangover where a single request probably returned some slice of the data instead of your entire data model one shot. I think our API should do its best to return as much of the requested data as possible, while possibly redacting some of it, and avoid shutting down the whole query.

Agreeed about the name. Should have been some, like collections and js arrays.

[esizer]

Yeah, it seems odd for sure. I have been trying to find other solutions but basically everything I come across recommends using different queries for the index based on what the user can see. Though, I just saw that you can apply scopes in the @can directive... not sure how that works though. 🤔

[tristan-orourke]

I think the scope arg on @can helps determine which model is passed to the policy, if the query arg is true.

[tristan-orourke]

One more point to make about scoping out items the user is unauthorized to view:

This wouldn't really reduce the amount of runtime variability, just move it around. Instead of Policy behaviour depending on what's in the collection (and not just who's asking), we'll now have query behaviour depend on who's asking (and not just what they're asking for).

[petertgiles]

I did a bit of googling over the weekend and found it quite difficult to find much about this.

On https://graphql.org/learn/authorization/ they actually have the exact example of "only owners can see their drafts". Their recommendation is to have the graphql layer pass a fully hydrated user object down and let the business logic layer handle it. The reason is that you might have multiple apis/consumers of the service, not just the graphql api. In our case I guess that means handling it in Eloquent policies and scopes and not in schema directives? This would be a really large change for us since we extensively rely on directives for authorization right now.

This is a flimsy resource but in rmosolgo/graphql-ruby#2169 the issue reporter says they expect the Ruby library to simply remove unauthorized items from the array. The author agrees and mentions that they can use scoping in addition to authorization checks to remove unauthorized items before they get to the checks. For us, this confirms to me that removing unauthorized items from an array with a scope instead of just unauthorizing the entire field array is an accepted pattern and using a scope to this is an accepted approach. The linked page about authorized scoping: https://graphql-ruby.org/authorization/scoping.html .

I wanted to find a big in-production graphql api to compare with and it seems like maybe Github itself is the main one since the Facebook one is not public. Unfortunately, skimming their docs last night, I couldn't find anything about this topic.

[petertgiles]

The writeup on the Ruby library is worth reading on its own. They agree with graphql.org that auth should happen in the business layer, not the graphql layer. They actually return a nil for unauthorized objects, not just remove them. They also have the ability to hide parts of the schema based on auth though they admit that's not part of the graphql spec.

https://graphql-ruby.org/authorization/overview.html

[esizer]

This has been my experience and generally what I was able to dig up during my research as well.

This is the way, it seems 🤷‍♀️

[tristan-orourke]

Thanks Peter! Interesting to see how Ruby thinks about this.

I feel like the closest thing to "delegate to the business logic layer" in our case would mean adding authorizedToView scopes directly to relationships in our Eloquent models. Which might be fine, actually. 🤔 But lighthouse is already pretty invested in tightly coupling with the business logic, so I'm not sure we would gain anything.

If we really wanted our Policy methods to replace items with null instead of aborting the query, we could consider writing a custom version of the @can directive. Though if we're relying on this for any queries which return large collections, using a scope (which can modify the query before it runs) is probably still more efficient.