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

Integrated Authorization #1494

Merged
merged 45 commits into from
Jun 21, 2018
Merged

Integrated Authorization #1494

merged 45 commits into from
Jun 21, 2018

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented May 13, 2018

I'd like to take some lessons learned from graphql-pro auth and from GitHub's auth and roll them into an integrated auth system.

The goal is:

  • Have hooks for authorization on type classes & field/argument instances:
    • .visible?(context) for hiding parts of the schema altogether
    • .accessible?(context) for preventing queries that touch those parts
    • .authorized?(value, context) for runtime authorization of application values
  • A schema-level method for invoking auth logic (for testing, or for applying auth in application-specific edge cases)
  • Eventually, use the schema methods for automatic authorization in mutations
  • Compatibility with current graphql-pro API (no changes required)

Using the class-based API will make it much easier to share code between hooks and easier to create default logic. Open-sourcing this logic will help more people adopt GraphQL because we'll have a clear, shared auth system, and it will improve the system, since intensive users can contribute with fixes and features.

TODO

  • Ping people who have expressed interest in more auth
  • Add a schema-level filter method
  • Add visible? / accessible? / authorized? hooks to the new class-based type, field, and argument classes
  • Integrate those class methods with validation and the runtime
  • Support first-class auth APIs on schemas
  • Write docs
  • Support a lazy object returned from auth hooks
  • Document migration path for graphql-pro (use DSL code in place, but use this new framework)
  • Support auth levels on enum values
  • Use GraphQL::UnauthorizedError for debugging Will add later

@rmosolgo rmosolgo self-assigned this May 13, 2018
@bgentry
Copy link
Contributor

bgentry commented May 14, 2018

I rolled my own pundit-based auth that attempted to mimic what I'd get from graphql-pro: https://gist.github.com/bgentry/da703027ed80002b9686f2f90ab3ab2c

I'm interested in trying a built-in solution for this, lmk when there's something ready to try or if you need other feedback on ideas! ✌️

@phaedryx
Copy link
Contributor

This is awesome, thank you

@phaedryx
Copy link
Contributor

We dynamically generate our enums. For example, we allow ordering on any database field:

{
  SomeTypes(orderAsc: someFieldAsEnum) {
    someField
  }
}

It would be great if we could hide the enum values that correspond to our fields that aren't visible.

@rmosolgo
Copy link
Owner Author

That's a great point about enum values, I don't think they can be authorized specifically on this branch yet, but I will add it!

@rmosolgo
Copy link
Owner Author

I added a conceptual overview of graphql auth here:

https://github.com/rmosolgo/graphql-ruby/blob/integrated-auth/guides/authorization/overview.md

I'd love a review if anyone has a few minutes.

end
```

The downside of this is that, when `Types::Post` is queried in other contexts, the same authorization check may not be applied. Additionally, since the authorization code is couple with the GraphQL API, the only way to test it is via GraphQL queries, which adds some complexity to tests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/is couple with/is coupled with/

@rmosolgo
Copy link
Owner Author

I pushed some WIP docs for each step. Still some error customization work to do, also the enum value bit as described above.

@theorygeek
Copy link
Contributor

One concept that might be worth thinking about is the idea of trimming a list or a connection.

def posts
  # Fetch the posts this user can see:
  Post.posts_for(context[:current_user])
end

Would it be possible to move the posts_for call into the framework so that you get it by default?

The authorization instrumentation that we use does two checks:

  • One before executing the field. It uses the identity of the parent object and metadata on the field to decide if you're allowed to query it.
  • One after executing the field:
    • If it returned an array, we remove inaccessible objects from the array
    • If it returned an ActiveRecord::Relation, we do the equivalent of calling .posts_for on it

It seems like a common pattern.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Jun 5, 2018

That's a great point, I think we can also see CanCan's accessible_by(user) scope and Pundit's Scope classes for inspiration.

@rmosolgo rmosolgo force-pushed the integrated-auth branch 3 times, most recently from 083fb68 to 990cf02 Compare June 6, 2018 18:50
@phaedryx
Copy link
Contributor

phaedryx commented Jun 8, 2018

Thanks for all of your work, btw

@rmosolgo
Copy link
Owner Author

Just pushed one nice bonus, Schema::Object.new is now protected, so the only way to initialize an instance is through authorized_new. That guarantees that every instance will hit the auth hook. (Now we'll see if the tests pass, and see LICENSE.md for all ... "guarantees" 😅 )

@rmosolgo
Copy link
Owner Author

Ok, I think enough has been written here for one PR. It turned out tough!

On monday I'll give the code another look and see if there are any hacks to clean up.

This leaves out several features which I'd like to investigate, probably in this order:

  • Mutation authorization
  • Support for existing cancan / pundit integrations
  • Scoping / filtering lists -- This would be nice, but I think it's tangential to the also-needed object-by-object auth used here. So I will put it off for while. (I wonder if we could have a .filter_list(items) method on types for this?)

@rmosolgo
Copy link
Owner Author

rmosolgo commented Jun 15, 2018

Oh, that's not quite right, 2 more things to do:

  • Make it opt-in? That's probably not possible, since it's so baked in. And probably not worth it. :S
  • Try to get GitHub passing on it. (We probably won't migrate right away though.)

argument :after, "String", "Returns the elements in the list that come after the specified cursor.", required: false
argument :before, "String", "Returns the elements in the list that come before the specified cursor.", required: false
argument :after, "String", "Returns the elements in the list that come after the specified global ID.", required: false
argument :before, "String", "Returns the elements in the list that come before the specified global ID.", required: false
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, bad merge

@benkimball
Copy link
Contributor

One of your TODOs is to ping interested people. I’m interested :)

@rmosolgo
Copy link
Owner Author

GitHub isn't quite passing yet, but it's because of how some code that used to not be in a Promise, is now in a Promise, because our authorized? checks return promises. But I think the system basically works.

@rmosolgo rmosolgo added this to the 1.8.4 milestone Jun 21, 2018
@rmosolgo rmosolgo merged commit da1030a into master Jun 21, 2018
@rmosolgo rmosolgo deleted the integrated-auth branch June 21, 2018 14:00
@rmosolgo
Copy link
Owner Author

Starting to build on this for mutations here: #1609

@rmosolgo rmosolgo mentioned this pull request Jul 15, 2018
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.

None yet

6 participants