[WIP] Proposal: add inaccessible_field feature

[swalkinshaw]

Right now accessibility is done at query analysis time. The only provided hook is inaccessible_fields where you can either allow the query to continue or raise an error.

If you raise an error, query execution does not occur. This prevents any partial responses from happening.

If nil is returned and the query continues, all information from GraphQL::Authorization::Analyzer is lost.

The idea behind this is to provide a 2nd option where the authorization enforcement is done during field execution (but before actually resolving) to allow for partial responses.

Ideally this would be opt-in at the schema level by providing a inaccessible_field (note the difference, no s at the end, name TBD) method which would be called for each field which is not accessible. An ExecutionError could be raised to prevent the field was being resolved.

Things to figure out:

1. how to opt-in to this behaviour. The optional schema method doesn't actually work due to the legacy schema and delegation.
2. this only works for class-based fields. Does this need to be supported for the legacy ones as well? Is there even a way to do this nicely?
3. is checking the irep_node a stable enough way to do this?

[swalkinshaw]

This can be added to the context regardless. The only conditional thing should be skipping inaccessible_fields.

[rmosolgo]

Thanks for the careful explanation and code! It sounds like you want runtime authorization at the field level. How about implementing #authorized?(obj, ctx) on your base field class, for example:

class BaseField < GraphQL::Schema::Field 
  def authorized?(parent_object, context)
    # check parent_object & self for the given context, 
    # return true|false, or raise an error 
  end 
end 

It's just barely mentioned in the the docs but it's checked before resolving. Do you think that might work?

[swalkinshaw]

🤔 I think I sufficiently confused you in our original chat which didn't make this obvious. I'll take a look at that and report back.

[swalkinshaw]

One benefit of using the analyzer is much more efficient access checks. If we used authorized? we'd be doing n accessibility checks for every object.

In fact we already have to do this and manage it with our own cached access checks. But it would be nice to avoid this and have the gem ensure it's efficient which it is when done during the analysis phase.

[rmosolgo]

Hmm, I wonder though, because the other downside of ahead-of-time checks is that it's going to check the whole query, even if the first field returns nil and none of the query is executed.

I know that analysis is a moving target, because of performance issues with irep_node and friends (#1172), so I don't really want to build more stuff on top of it if I can help it.

Besides that, my experience with analysis at GitHub doesn't leave me enamored with it. For example, we have an analyzer that checks if the given oauth token has all scopes required for the query. But, it can't do the whole job because sometimes the required scope depends on the object (some repositories require :public_repo, others require :private_repo), so it can really only run those checks at runtime. So, which should we do? Either run partial checks and incur the overhead of analysis, and provided a so-so ahead of time check; or just leave it to runtime checks, since that's the source of truth anyways. Currently we do run that analyzer, but I'm less and less sure that it's worth it.

Since my own experience with analysis is not that good, I'm not excited about supporting a hybrid ahead-of-time/runtime feature. Instead, I would encourage people to use either visibility or runtime authorization, since those two are usually good enough.

Regarding duplicate work, would it be sufficient to cache the result based on field.owner, for example:

def authorized?(_parent_object, context) 
  # The context here is Query::Context, so I think it's safe to close over in this proc:
  context[:return_type_checks] ||= Hash.new { |h, k| h[k] = k.allowed_return_type?(context) } 
  # Get the cached value, or run the block for `owner`: 
  context[:return_type_checks][owner]
end 

Is that ^^ similar to the existing caching you have?

[swalkinshaw]

because the other downside of ahead-of-time checks is that it's going to check the whole query, even if the first field returns nil and none of the query is executed.

Yeah that's a good point that I've realized as well.

Our caching is more independent from GraphQL. Instead of the cache key being a field/type, it's the required access (such as :public_repo).

def authorized?(_parent_object, context) 
  context[:access].include?(@access)
end

context[:access] is what does the caching. So if two fields require :public_repo, then the first check caches it, and the second uses the cached result.

Considering the unknown future around irep_node I agree this shouldn't go forward. The main thing which would be nice to avoid is everyone having inefficient access checks out of the box and then having to discover they need to implement some sort of caching. So if the gem could provide a default solution, that would be great 👍