Don't re-authorize objects that were part of a scoped list

[rmosolgo]

trafficstars

After a list has been scoped, don't re-run authorization on the objects it permitted.

Fixes #3893

TODO:

• [x] consider the breaking-ness of this change. Make it opt-in instead of default?
• [x] somehow pass this thru connection -> edges -> node and connection -> nodes
• [ ] use .equal? to test same-object, add a test for that behavior

[rmosolgo]

cc @hvenables I've worked up a basic implementation, described here: https://github.com/rmosolgo/graphql-ruby/pull/3994/files#diff-d60a70644c1e9b36288190a468466b7186cd98ab45bb3519e30b922fc9708cac

Does that sound like it would work for what you had in mind?

[fameoflight]

Would love to have this available in next version. We have usecase for this as well. Let me know if I can help in anyway.

[rmosolgo]

Sure thing, I'll try to wrap it up soon. The trick is determining when an application-defined scope_items method was applied. I've been trying to use the return value, but I don't think that's right: sometimes, applying scope_items will return the exact same list that was passed in.

Instead, I think I'll try inspecting .method(:scope_items) to see if it's the default implementation or a custom implementation. The other option is, in the default implementation, checking caller to see if an application-defined method is calling through to it. (Maybe both of those approaches will be required.)

[bessey]

@hvenables left the company (enjoy Shopify Harry :stuck_out_tongue:) so I'll have to fill in on feedback! This definitely works for us. Our GraphQL Pundit authorization architecture is essentially

• All objects are expected to have a policy by default
• All objects are authorized with #view? by default (we felt show? was inappropriate given the non RESTy nature of GraphQL, though in practice view? and show? are aliases for cross compatibility`
• #view? is expected to behave the same as the policy's scope, i.e. it should never return false when for scoped items

So given your API we would probably implement reauthorize_scoped_objects(false) on our BaseObject.

We're still very keen for this API btw! Authorization induced N+1s is one of our biggest perf issues in our GraphQL API.

[rmosolgo]

Thanks for sharing your thoughts about it! I'll make some time to land this PR soon.

[bessey]

I've been thinking about this more lately, in the context of #4087, because we perform the same scoping that GraphQL Pro Pundit does to ActiveRecord::Relations, within all our ActiveRecord::Relation sources.

In this context, I cannot think of a way that you could indicate to the runtime "hey, I know you're returning an instance of MyRecord, but trust me, I loaded that through a dataloader which scoped it, so you do not need to authorize it again". The only option I can think that could work is

1. Set some state on the ActiveRecord::Relation object
2. Modify ActiveRecord's behaviour to persist this state through to all records instantiated from that Relation

e.g. theoretical API

scope = MyRecord.where(visible: true).mark_authorized!(:view) # sets @authorized = :view
results = scope.to_a # ActiveRecord sets @authorized on each instantiated record 
results.first.pre_authorized?(:view) => true # accesses record specific @authorized

I have no idea if this is actually possible without monkeypatching ActiveRecord yet, but I thought I'd put my thoughts somewhere others with more experience might see!

EDIT: Yeah, I looked into the AR source to see how 7.0's strict record loading works and it certainly suggests there's no API flexible enough to do this from the outside.

However, I just realised that you don't need to do pass the state through the relation. The Dataloader is responsible for instantiating the relation, so it is well placed to add this state to the records! We could easily modify a source's #fetch(keys) method to set this state on every loaded record, which is of course visible to the GraphQL authorization framework and therefore our own authorization framework.

[ioquatix]

We appear to be running into the same issue, any chance this will make it into a release soon? Thanks!

[rmosolgo]

I'll address the lint failure in another branch...

[ioquatix]

Thanks for merging this. Any chance we can get an ETA on when it would potentially be released?

[rmosolgo]

I just finally released this in GraphQL-Ruby 2.1.0 😅

[ioquatix]

Thanks, we will try it out and report back!

[bmulholland]

Hi! Thanks for the contributions. Two questions:

1. The release notes for 2.1 list this as a breaking change, but the docs and code suggest that this isn't enabled by default, which means it's not a breaking change. Which is it?
2. I am worried about a very simple footgun: move an existing method to the dataloader pattern and bam: data exposed to other users. I've previously described this in more detail. In brief: scope_items can sometimes be called with an array -- e.g. when they come from a dataloader -- in which case those aren't actually scoped. We rely on the authorized? to be a backstop for what is therefore a potentially complex integration error that almost results in a data leak. Doesn't this PR remove that backstop, leaving us exposed to the scenario I describe?

I could totally be misunderstanding this -- after years of working with these parts, I still haven't wrapped my head around it. Please fill me in if I'm missing anything important, I'd love to be wrong on this.

[rmosolgo]

Hey @bmulholland, thanks for pointing out that issue with the Changelog. I updated it in a3c093f69.

move an existing method to the dataloader pattern and bam

Could you give a before-and-after example of a refactor where this would happen? I would expect lists to go through scope_items regardless of how the data was loaded, but maybe I've missed a spot!

[bmulholland]

I updated it in https://github.com/rmosolgo/graphql-ruby/commit/a3c093f692855976b0192087a14b42afefd88a07.

Thanks for clarifying, appreciate it!

Could you give a before-and-after example of a refactor where this would happen?

Thanks for getting me to check this again. It looks like this is only in effect when using the Pundit integration -- so the footgun is limited to that scenario.

It looks like this is actually a risk for us, but only from our own app's code. That's because of GraphQL-Ruby-related history: We were trying to use the Pundit integration but honestly we could never figure out how the concepts fit together so we dropped the attempt. However, because of that, we still have explicit handling of arrays in our scope_items calls, which leads to this issue.

We'll discuss the best way to handle this internally: either somehow ensure that the "disable authorized?" option is never enabled, or have a policy that Arrays in scope_items are always scoped too. No action needed from GraphQL, though this does feel a bit too close for comfort.

[jderose9]

The documentation for this change would lead me to believe that the default is to reauthorize scoped objects, however after observing the behavior and examining the code it would seem that the default behavior has changed (a breaking change) and that scoped objects are no longer re-authorized. This means that scopes which were written too broadly with the assumption that objects would be further authorized by the policy method will now unintentionally be exposing objects after upgrading.

The check seems to be here, and I don't see anything defaulting this value to true: https://github.com/rmosolgo/graphql-ruby/blob/109c26c5fa9118a9c69ed4c4823048c1ce1aea6d/lib/graphql/schema/field/scope_extension.rb#L14

Additionally, unless I've got something setup incorrectly, I have found that calling reauthorize_scoped_objects(true) in the node type's class (as documented here) doesn't seem to do anything. I had to call it in the connection type's class.

[bmulholland]

Uh, what? That's really alarming: as noted above, this is a big risk for us. It's incredibly critical that we don't expose data to the wrong users, and we need all the checks that we can get. I think that's worth it's own Issue, right?

More broadly, as I've noted a few times, there's several moving parts here that lead to a high risk for disastrous integration-level bugs: Dataloaders, array handling, and scope re-authorization could easily combine together to skip all checks, exposing all data to users. Since there are several options and configuration variations of these, this is probably hard to both test thoroughly and reason about. Even if the specifics here mean there's no/low concern, I've already had a few similar conversations on this project that have ended with that same outcome. What happens when several areas of "no concern" collide?

[jderose9]

More broadly, as I've noted a few times, there's several moving parts here that lead to a high risk for disastrous integration-level bugs: Dataloaders, array handling, and scope re-authorization could easily combine together to skip all checks, exposing all data to users. Since there are several options and configuration variations of these, this is probably hard to both test thoroughly and reason about. Even if the specifics here mean there's no/low concern, I've already had a few similar conversations on this project that have ended with that same outcome. What happens when several areas of "no concern" collide?

I agree. Also, in my situation, while I would like to get as close as possible to loading only the permitted objects for performance reasons (ie. I'm not going to load objects for a completely different tenant), sometimes there is code that runs in the policy method that is difficult or impossible to reproduce in a database scope. So certainly I don't think the default behavior should have changed between versions.

[rmosolgo]

Hey @jderose9, thanks for reporting these problems -- I've opened #4720 address them!

[jderose9]

@rmosolgo Awesome, thank you very much for the fast resolution!

[bessey]

More broadly, as I've noted a few times, there's several moving parts here that lead to a high risk for disastrous integration-level bugs: Dataloaders, array handling, and scope re-authorization could easily combine together to skip all checks, exposing all data to users

I share this concern of yours @bmulholland. We are GraphQL Pro (and Ent) users and make heavy use of Dataloaders and the GraphQL Pro Pundit integration. I tried to adopt this new optimisation in a spike, but ran into a problem like yours: it has no effect for Dataloaders. I believe because they (or at least ours) load an Array, and Pundit can't do anything useful with scoping an already instantiated Array.

That's not a security issue at least, but it does mean these two GraphQL Ruby provided tools are not compatible with one another.

I've been doing a lot of GraphQL Ruby perf optimisation work lately on our codebase, and I keep coming back to one thing: reasoning about this stuff is getting incredibly difficult. I have used GraphQL Ruby for years and still I have a hard time reasoning about how these factors combine:

(I know the answers to these Qs now, or so I hope, but just to illustrate how difficult this is to reason about):

• Dataloaders
  • Should a Source do its own authorization?
• Connection objects (both automatically wrapped and manually returned from resolvers)
  • Is authorization done before or after pagination?
  • What object is passed to scope_items in each case?
  • Pundit Scopes expect an ActiveRecord::Relation, what should I do with a connection object?
• The Pro Pundit integration (and more broadly the authorization framework itself)
  • Object vs field level authz
  • Scope items, and its special case for Arrays and Arrays only
  • This new scope optimisation

We've used Module#prepend to wrap the GraphQL Pro Pundit integration with verbose debug logging of the decisions it makes, and still I regularly bundle open GraphQL + GraphQL Pro just to understand the control flow through these parts.

I'm coming around to the view point that

1. The costs of outsourcing such a rich integration as GraphQL + Pundit to a 3rd party library outweigh the benefits
2. This new scope optimisation does not belong in GraphQL Ruby at all. It is a great idea, but is better implemented by the application developer and richly integrated with their specific authorization strategy, rather than an additional layer of complexity to reason about

Is it constructive to continue this conversation, and if so is GH Discussions the best place to do so? I appreciate my problems can largely be solved by ignoring this features existence and building our own Pundit integration, but since it seems others in the community have similar concerns, perhaps its worth discussing nonetheless?

PS I don't want to come across as ungrateful, GraphQL Ruby is a wonderful piece of software ❤️

[rmosolgo]

Hey @bessey and @bmulholland, thanks for your detailed writeups on the conflict between these features -- and sorry for the trouble you've run into so far on them :S I can see that they need some reconsideration. Over the next couple of days I'm going to re-read your descriptions and suggestions and open a new issue with some proposals. I'll follow up back here when I do :+1:

[rmosolgo]

I went digging on the Pundit Arrays issue and found a previous suggestion which seems like a good candidate for a new default behavior. I worked out the code locally and wrote up a description here: https://github.com/rmosolgo/graphql-ruby/issues/4726

@bmulholland and @bessey , could you let me know on that issue what you think, if you have a minute? Thanks!