Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorize mutation inputs #1609

Merged
merged 12 commits into from
Jul 5, 2018
Merged

Authorize mutation inputs #1609

merged 12 commits into from
Jul 5, 2018

Conversation

rmosolgo
Copy link
Owner

Building on the integrated authorization framework from #1494, I want to add first-class support for a workflow where:

  • Object IDs are accepted as inputs
  • IDs are used to load records from the DB, maybe with batching
  • Those loaded records should be authorized before running the field body

This is the kind of auth we really want for mutations. (Usually, authorization is run after a field returns a value, but for mutations, that's too late!)

@rmosolgo rmosolgo added this to the 1.8.5 milestone Jun 21, 2018
@rmosolgo rmosolgo self-assigned this Jun 21, 2018
@rmosolgo
Copy link
Owner Author

I added a rough doc of my goal here: https://github.com/rmosolgo/graphql-ruby/blob/mutations-auth/guides/mutations/mutation_authorization.md

I'd love some opinions on what you see there if you get a chance!

@rmosolgo rmosolgo mentioned this pull request Jun 21, 2018
9 tasks
@rmosolgo
Copy link
Owner Author

I'm not completely clear on what methods / names need to exist, that's one tricky part

@swalkinshaw
Copy link
Collaborator

I'm wondering if "Loading objects" is too implicit/magical. The only clue that it's not a normal argument is that a non-input object type is used? I believe this is the only instance in the API right now where a schema member definition isn't what will end up in the schema.

I know it's opt-in, but it's not really visible or obvious as to what's different about just by looking at the code.

We have a similar feature for dealing with ID arguments and doing GlobalID parsing:

argument :id, :id, required: true, gid_type: Types::Employee

I'm thinking the same thing could be achieved with an optional kwarg:

argument :id, :id, required: true, load_as: Types::Employee

I think every feature mentioned in the doc could still be done in the same way, with being slightly more explicit.

@rmosolgo
Copy link
Owner Author

too implicit/magical

Yep, I'm on the fence about the same thing. Adding another keyword arg (or two) is another implementation we were kicking around over here.

One question is, does the argument change names? For example, when you have:

argument :id, :id, required: true, load_as: Types::Employee

Does the resolve method accept id: or employee:? (id: seems less magical, but employee: makes more sense in terms of key/values matching)

@swalkinshaw
Copy link
Collaborator

Good point! By making the argument name less implicit, the resolver arguments become more implicit 🙈

Actually is having both id: and employee: bad? We do have a couple of use cases where we parse out the model id but need the original GID too. Not sure how common that is though.

@rmosolgo
Copy link
Owner Author

Woah, interesting, so you might take { id: "abcd" }, load the id, then pass in {id: "abcd", employee: #<Employee ...>}? Interesting idea! I can imagine the benefit. 🤔 I don't think I've ever seen a case in GitHub where we use the ID for anything but loading. But I wonder how/if that could be supported.

@rmosolgo
Copy link
Owner Author

Pushed a change to use the loads: option instead of inferring from the type. It makes it a bit less magical, but since it "just" uses the preexisting as: option, it doesn't inject the ID and the object, only the object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants