Closed xxx closed 9 months ago
Hey, sorry this came as a nasty surprise!
I see you posted the plain Pundit error, but I would have expected GraphQL-Pro to wrap that message with a couple of suggestions about how to address this. Was there more in the error message, or was that all? Maybe I need to catch Pundit's error somewhere else to make sure it's appropriately wrapped.
To address your questions:
What do I do in the case where we are returning a Hash of data, rather than an object instance?
Since Pundit infers policy classes based on the object's class (Hash
, in the error above), you could either:
HashPolicy
to use for hashes (which seems a bit weird, since Hashes are used for all kinds of things) How do I deal with arrays of union or interface types?
They aren't handled differently than object types. If they implement .scope_items
(which PunditIntegration
does), then that method is called on fields that return lists of that type.
What does
scope: false
actually do?
It disables GraphQL-Ruby's scoping feature for that field (https://graphql-ruby.org/authorization/scoping.html), which would also skip the Pundit lookup of the ...::Scope
class.
"small"
I guess it depends how you judge the size. It sounds like this change caused a lot of failures in your test suite, but how many fields actually need to be updated? In my own experience, it's much more common for list fields to return ActiveRecord relations (or similar list-ish objects), and that's what scoping is for anyways.
In cases where a change is necessary, you can slap a scope: false
on there (which is what was happening by default, anyways) or add a no-op Policy class which has a nested (also no-op) Scope
class. And then the application is in a better state than it was before, since a previous questionable default is now easily discoverable (and correctable, if need be).
But maybe I'm wrong about how that change will go -- please do share more about what this looks like in your application. If necessary, I can revise the patch to make a smoother on-ramp!
heya, thanks for responding.
Here's a complete error example:
GraphQL::Pro::PunditIntegration::PolicyNotFoundError:
No policy found for:
- Type: MetricDataPoint
- Runtime value: `[{:date=>Mon, 18 Dec 2023, :value=>3.0}, {:date=>Mon, 15 Jan 2024, :value=>5.0}]`
Since this value is an **Array**, Pundit can't find a scope for it. To fix this:
- Configure a policy class with `pundit_policy_class` in the definition for `MetricDataPoint`
- Or, skip scoping by adding `scope: false` to the field definition
Pundit error: Pundit::NotDefinedError, unable to find scope `Hash::HashPolicy::Scope` for `[{:date=>Mon, 18 Dec 2023, :value=>3.0}, {:date=>Mon, 15 Jan 2024, :value=>5.0}]`
In this case, we already have a pundit policy for MetricDataPoint (i.e. on Types::MetricDataPointType
), and that is the policy I would really like to be applied here (since that's the policy on that type that the field
is configured as), regardless of the fact that we're passing an array of hashes instead of instances - we have a number of fields that delegate to services that reduce objects to hashes for performance reasons, due to needing to process large amounts of data at times. This looks to be affecting 10-15 fields (out of many hundreds) overall for us.
Ok, thanks for sharing that. It definitely should have looked for the ::Scope
class in that type. (And maybe it did, but the error got handled wrongly...) I'll take a closer look tomorrow morning and follow up back here!
And, if that is what it did, it should have raised a more descriptive error for how to address it.
and to verify, MetricDataPointPolicy::Scope
does exist
I spent a bit of time trying to replicate this error. First, here's a script that shows how this is supposed to work:
```ruby require "bundler/inline" gemfile do gem "graphql", "2.2.5" gem "pundit", "2.3.1" gem "graphql-pro", "1.26.0", source: "https://gems.graphql.pro" end class MySchema < GraphQL::Schema class BaseField < GraphQL::Schema::Field include GraphQL::Pro::PunditIntegration::FieldIntegration pundit_role nil end class BaseObject < GraphQL::Schema::Object include GraphQL::Pro::PunditIntegration::ObjectIntegration field_class(BaseField) pundit_role nil end class DataPointPolicy class Scope def initialize(current_user, items) @current_user = current_user @items = items end def resolve @items # do nothing end end end class DataPoint < BaseObject pundit_policy_class DataPointPolicy field :x, Int field :y, Int end class Query < BaseObject field :data, [DataPoint] def data [ { x: 1, y: 2 }, { x: 2, y: 4 }, { x: 3, y: 6 }, ] end end query(Query) end query_str = "{ data { x y } }" pp MySchema.execute(query_str).to_h # {"data"=>{"data"=>[{"x"=>1, "y"=>2}, {"x"=>2, "y"=>4}, {"x"=>3, "y"=>6}]}} ```
As written, it works:
DataPointPolicy
class; ::Scope
module; But a few changes can create errors. If I remove the explicit link to DataPointPolicy
like this:
class DataPoint < BaseObject
+ # pundit_policy_class DataPointPolicy
- pundit_policy_class DataPointPolicy
Then it raises a very similar error to the one you reported here:
``` No policy found for: (GraphQL::Pro::PunditIntegration::PolicyNotFoundError) - Type: DataPoint - Runtime value: `[{:x=>1, :y=>2}, {:x=>2, :y=>4}, {:x=>3, :y=>6}]` Since this value is an **Array**, Pundit can't find a scope for it. To fix this: - Configure a policy class with `pundit_policy_class` in the definition for `DataPoint` - Or, skip scoping by adding `scope: false` to the field definition Pundit error: Pundit::NotDefinedError, unable to find scope `Hash::Hash::HashPolicy::Scope` for `[{:x=>1, :y=>2}, {:x=>2, :y=>4}, {:x=>3, :y=>6}]` ```
That makes me think that somehow, GraphQL-Pro isn't finding MetricDataPoint
's policy class. In a Rails console, can you confirm that the expected policy class is returned for:
# $ rails console
Types::MetricDataPointType.pundit_policy_class
# Does this return the expected class?
Another error I found is that, if the pundit policy doesn't have a ...::Scope
class, it raises almost the same error:
- class Scope
+ # class Scope
# ... etc
``` No policy found for: (GraphQL::Pro::PunditIntegration::PolicyNotFoundError) - Type: DataPoint - Runtime value: `[{:x=>1, :y=>2}, {:x=>2, :y=>4}, {:x=>3, :y=>6}]` Since this value is an **Array**, Pundit can't find a scope for it. To fix this: - Configure a policy class with `pundit_policy_class` in the definition for `DataPoint` - Or, skip scoping by adding `scope: false` to the field definition Pundit error: NameError, uninitialized constant MySchema::DataPointPolicy::Scope ```
That error is just plain wrong -- it's not properly recognizing that, in this case, the only thing missing is the Scope
class. I'm going to improve that to detect the error properly and recommend the right fix.
Let me know what you find about Types::MetricDataPoint.pundit_policy_class
-- if that's returning the right policy class, but scoping isn't working, we'll have to keep digging...!
(I just released graphql-pro v1.26.1 which raises a better error when the nested Scope
class is missing. In 33a154b40, I updated CHANGELOG-pro
to include some notes about addressing this error.)
Sorry for the late response - Types::MetricDataPoint.pundit_policy_class
returns nil (we don't explicitly set pundit_policy_class
in that type). It looks like you ran into a similar situation without the explicit set
No worries, that's great. Then the fix is to manually attach the policy class with pundit_policy_class(...)
in the Types::MetricDataPoint
class definition.
The reason it's necessary is because:
MetricDataPointPolicy
. (Pundit looks for policy classes by class name, but neither Array
nor Hash
will lead it there.) PunditIntegration
includes a pundit_policy_class(...)
configuration for just cases like theses -- but it hasn't been set yet. If you add that configuration, does it work as expected for you?
If you want to add some auto-lookup logic to pundit_policy_class
, you could do it via def self.pundit_policy_class_for
as described here: https://graphql-ruby.org/authorization/pundit_integration.html#custom-policy-methods
When implementing self.pundit_policy_class_for
, can I access the field definition? It doesn't seem like this method receives enough information to do anything more than a type sniff on the data, unless you want to stuff a lot of things into context. I want the data to be interpreted as the type that is set on the original definition, rather than trying to infer the correct policy based on the actual data - I already configured the field
, and my query is for that same field. I think the field
should be the single source of the truth in the default case, not the type of the data itself. The error message shows that enough information is available to make this possible.
How do you opt out of checks at the type level? In the past we've used pundit_policy_class nil
in the gql type, but that no longer works (pundit_role nil
instead also fails the same way) -
491) Mutations::WorkstreamDelete#resolve when successful returns successful response
GraphQL::Pro::PunditIntegration::PolicyNotFoundError:
No policy found for:
- Type: Error
- Runtime value: `[]`
Since this value is an **Array**, Pundit can't find a scope for it. To fix this:
- Configure a policy class with `pundit_policy_class` in the definition for `Error`
- Or, skip scoping by adding `scope: false` to the field definition
Pundit error: Pundit::NotDefinedError, unable to find scope `::NilClassPolicy::Scope` for `[]`
I'm still not understanding this new behavior. In the past, if I had returned an array of hashes instead of an array of AR objects, would it just silently fail to check the fields in those hashes?
Hmm, thanks for sharing that -- I think this is actually a bug in Scope
detection ... it should have recognized that it was configured to return nil
. I'll release a fix for this soon.
an array of hashes instead of an array of AR objects
In that case, previously, the would both have skipped scoping (their objects would still be authorized one-by-one, and any unauthorized ones would be replaced by nil). Now, both of them are scoped -- but require a manual configuration of the policy class. (An ActiveRecord::Relation
would have been scoped, since Pundit knows how to find a scope class for that kind of object.)
In that case, previously, the would both have skipped scoping (their objects would still be authorized one-by-one, and any unauthorized ones would be replaced by nil).
This is what I want. Actual Array
objects are always assumed as pre-filtered for us. Any scoping would have already been done. How do I configure this in one place? I didn't see it documented in the changelog.
You could do it in def self.scope_items(items, context)
, for example:
def self.scope_items(items, context)
if items.is_a?(Array)
items # This was already filtered at a lower level
else
super
end
end
That would return all Arrays as-is. (If it was me, I'd put it in a module and extend SkipScopingOnArrays
in any GraphQL type that currently needs it, just so that the behavior was discoverable by the next developer, including my future self.)
I took a look at handing pundit_role nil
for scoping, but I decided not to change the behavior here. It might breake scoping for people who already configured pundit_role nil
but expect scoping to applied to lists of that type. I really don't want to stop authorizing things that are currently authorized!
So I'd suggest re-implementing the previous default for Arrays, instead. I updated the changelog in 3233e94d7 to include two approaches to that. Besides the one mentioned above, you could also implement the policy class's Scope#resolve
method to no-op when it receives an array.
Does one of those approaches work for you?
Sorry for all the trouble and thanks for helping me work through these options!
Thanks for that. I ended up implementing scope_items
and prepending it to our base object, union, and interface classes, which fixes all the failures.
I took a look at handing pundit_role nil for scoping, but I decided not to change the behavior here
is pundit_policy_class nil
going to be changed to the previous? We'd still be hitting that if I wasn't skipping array scopes.
the previous
I'd expect pundit_policy_class nil
to continue working as it always has: it overrides any custom policy class in the class's inheritance chain, delegating to Pundit.policy!
instead, which infers a policy class based on the object being authorized. Did you notice something else change here?
I didn't expect an explicit configuration of nil
to then try to find and apply a policy. I would expect that it instead does not try to check at all.
That actually gets back to a question in https://github.com/rmosolgo/graphql-ruby/issues/4801#issuecomment-1908511178 - what's the way to opt-out at the type level? The changelog mentions setting pundit_policy_class
, which leads me to assume that setting it to nil
means don't use a policy. That's what we've always assumed it meant.
I think some of my confusion here is my conflating object types with mutation types. I'm going to close this since my issue is resolved. Thanks again for all the fast responses
To disable object-level checking, you can use pundit_role nil
; There's not an equivalent type-level opt-out for scoping (yet). But you can implement the various scoping methods as no-ops to skip filtering. Would you find a pundit_scoping(false)
-type configuration handy? I could certainly add it.
Describe the bug
Hi,
Per the versioning policy, the changelog should include an explanation on how to upgrade for any breaking changes.
pro-1.26.0 included a breaking change around scoping for arrays in the Pundit integration, which has led to several hundred failures. The instructions in the error do not explain how to resolve the error the 'right way' very well, imo.
Versions
graphql
version: 2.2.5 / pro 1.26.0rails
(or other framework): 7.1.3Steps to reproduce
This error occurs when querying any field returning an array.
Expected behavior
A clear and concise description of what should do in various cases.
What do I do in the case where we are returning a Hash of data, rather than an object instance? How do I deal with arrays of union or interface types? What does
scope: false
actually do?All I want is to skip checks on the Array itself, and instead check with the already-configured policies on the items contained within the Array. How do I do that without having to repeat that configuration all over the place?
Actual behavior
We get errors of the sort
Additional context
I'd like to voice a disagreement about this being a "small" breaking change.