Permission blacklist + field-level auth example #122

sweir27 · 2017-11-15T22:26:28Z

This PR lays out a few concepts-- I'll explain and then clean up before it's mergeable.

First thing, it removes the blanket authorization check on the /graphql endpoint, which used to require that an app and user were present. Instead, it allows the base introspection query to be public and adds more fine-grained authorization on a field-level.

PermissionBlacklist (which I will remove before merging) is an example of how we can use schema masking to blacklist (or whitelist) fields from being seen. This can be done on a role basis, and effectively hides certain fields from a given app/user. Right now I think convection's schema can be totally public, but it's good to keep in mind as we start using this pattern for more sensitive apps/fields.
graphql-ruby has a nice way of extending the DSL on the Field, Type, and Argument classes. I added an example here (which I'll explain inline). This part I do intend to merge in, as it provides a nice way for us to define which roles can access various fields. Again, if something is intended to be hidden completely, we can blacklist it at the root level.

…ization

sweir27 · 2017-11-15T22:27:44Z

app/controllers/api/graphql_controller.rb

+          current_user: current_user,
+          current_user_roles: current_user_roles
+        },
+        except: Util::PermissionBlacklist


This line will whitelist/blacklist fields on a schema-level (so they can be hidden even from the introspection query).

sweir27 · 2017-11-15T22:30:35Z

app/graph/util/permission_blacklist.rb

+module Util
+  class PermissionBlacklist
+    def self.call(schema_member, context)
+      return context[:current_user].blank? if schema_member.name == 'user_id'


Just an example-- this will hide the user_id field if there is no user in the jwt.

I realise this is simply meant as a contrived example of the possibilities, in the same contrived spirit I’d use it as an example of a case where I don’t believe the field needs to be hidden from a privileged schema, the service should simply not return any data for it. (Maybe the service should include a 401-esque error in the response, I think that’s what @ashkan18 and @joeyAghion settled on).

sweir27 · 2017-11-15T22:35:38Z

app/graph/util/authorization_instrumentation.rb

@@ -0,0 +1,37 @@
+module Util
+  class AuthorizationInstrumentation


This also feels like the kind of thing that could be shared across all of our services? Seeing as we are getting all of the same roles from gravity...

ashkan18

Looks great! one question i have is probably in general about how things in GraphQL works and my lack of knowledge. Serving schema is mainly helpful during development and possibly schema stitching, right? From a normal service to service call the actual schema interpolation is not that useful, right?

ashkan18 · 2017-11-16T17:54:51Z

app/controllers/api/base_controller.rb

@@ -51,5 +55,9 @@ def current_app
    def current_user
      @current_user ||= jwt_payload&.fetch('sub', nil)
    end
+
+    def current_user_roles
+      @current_user_roles ||= jwt_payload&.fetch('roles', [])&.split(',')


I think you need '' instead of [], because otherwise split might be called on an array.

joeyAghion

Interesting illustration.

To confirm, allowing users to access a schema property is essentially making it public (because anyone can sign up to be a user), right?

joeyAghion · 2017-11-16T18:12:42Z

app/controllers/api/base_controller.rb

@@ -25,6 +25,10 @@ def set_submission
      @submission = Submission.find(submission_id)
    end

+    def set_current_user_roles
+      current_user_roles
+    end


Unused?

It doesn't appear to do anything beyond the existing method.

joeyAghion · 2017-11-16T18:13:51Z

app/controllers/api/base_controller.rb

@@ -51,5 +55,9 @@ def current_app
    def current_user
      @current_user ||= jwt_payload&.fetch('sub', nil)
    end
+
+    def current_user_roles
+      @current_user_roles ||= jwt_payload&.fetch('roles', [])&.split(',')


I think you need '' instead of [], because otherwise split might be called on an array.

sweir27 · 2017-11-16T19:42:02Z

@joeyAghion yeah, it probably should verify that you also have an app token (not sure what the best wording on that would be).

Maybe?

require_app true
permit ['admin']

I was struggling a bit on naming and whether or not this should be a single or multiple concepts (app-level and user-level authorization).

orta · 2017-11-16T21:43:07Z

require_app true could be verify_app_token true ?

alloy · 2017-11-16T22:02:07Z

This can be done on a role basis, and effectively hides certain fields from a given app/user.

How many roles could there be? Is there a set amount of roles that are used throughout all our systems and have the same semantics? Stitching together many permutations (or per request on-demand) would probably not be great for performance.

And does it actually matter that our internal systems can know about the full schema? (Because you do need an app token, so only our internal system could query those services, right?) My naive thought was that it doesn’t really matter, as long as the service won’t return values when querying for those fields without proper authorization.

joeyAghion · 2017-11-17T15:29:42Z

It's hard to really imagine this because we agree that this particular case doesn't actually demand protecting the schema, and we probably should avoid that overhead wherever possible.

Generally speaking, fields that are relevant to administering content could be fine to expose, especially as that administration might come from partners and not exclusively from Artsy.

That said, I can imagine wanting to keep certain areas internal to Artsy. In that case, I think we'd probably expose them to selective clients (rather than users). Every token identifies its associated client (and user, when authenticated). Both have roles, but currently we only return the roles of the associated client when it's an unauthenticated token. This might work as-is for this purpose, or we might need to extend it to embed client roles alongside users' roles to expose schemas in authenticated cases as well.

sweir27 · 2017-11-17T16:12:56Z

Thank you all for bearing with this confusing prototype. The PR is now in a (mergeable, pending comments) state.

This makes convection's schema public (as there is currently no need for masking, although it's possible), and adds authorization on a field-level. I anticipate this logic being added to/changing with @orta 's coming PRs, the main point of this is to nail down how we want the permit (or otherwise) behavior to look.

sweir27 added 2 commits November 15, 2017 13:44

example of how to blacklist fields from introspection based on author…

9ea7eb6

…ization

adds example of authorizing on a field-level

aafd13d

sweir27 self-assigned this Nov 15, 2017

sweir27 requested review from joeyAghion, orta and ashkan18 November 15, 2017 22:26

sweir27 commented Nov 15, 2017

View reviewed changes

ashkan18 reviewed Nov 16, 2017

View reviewed changes

joeyAghion reviewed Nov 16, 2017

View reviewed changes

orta mentioned this pull request Nov 16, 2017

Neo metaphysics artsy/metaphysics#809

Merged

1 task

sweir27 changed the title ~~[Don't Merge] Permission blacklist example~~ Make convection's schema public + add auth to specific fields Nov 17, 2017

sweir27 assigned joeyAghion and unassigned sweir27 Nov 17, 2017

sweir27 force-pushed the permission-blacklist-example branch from d665a5b to aafd13d Compare November 17, 2017 16:15

sweir27 changed the title ~~Make convection's schema public + add auth to specific fields~~ Permission blacklist + field-level auth example Nov 17, 2017

sweir27 closed this Nov 17, 2017

orta deleted the permission-blacklist-example branch November 17, 2017 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permission blacklist + field-level auth example #122

Permission blacklist + field-level auth example #122

sweir27 commented Nov 15, 2017

sweir27 Nov 15, 2017

sweir27 Nov 15, 2017

alloy Nov 17, 2017 •

edited

Loading

sweir27 Nov 15, 2017

ashkan18 left a comment

ashkan18 Nov 16, 2017

joeyAghion Nov 16, 2017

joeyAghion left a comment

joeyAghion Nov 16, 2017

joeyAghion Nov 16, 2017

sweir27 commented Nov 16, 2017

orta commented Nov 16, 2017

alloy commented Nov 16, 2017

joeyAghion commented Nov 17, 2017

sweir27 commented Nov 17, 2017

		@@ -0,0 +1,37 @@
		module Util
		class AuthorizationInstrumentation

Permission blacklist + field-level auth example #122

Permission blacklist + field-level auth example #122

Conversation

sweir27 commented Nov 15, 2017

sweir27 Nov 15, 2017

Choose a reason for hiding this comment

sweir27 Nov 15, 2017

Choose a reason for hiding this comment

alloy Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

sweir27 Nov 15, 2017

Choose a reason for hiding this comment

ashkan18 left a comment

Choose a reason for hiding this comment

ashkan18 Nov 16, 2017

Choose a reason for hiding this comment

joeyAghion Nov 16, 2017

Choose a reason for hiding this comment

joeyAghion left a comment

Choose a reason for hiding this comment

joeyAghion Nov 16, 2017

Choose a reason for hiding this comment

joeyAghion Nov 16, 2017

Choose a reason for hiding this comment

sweir27 commented Nov 16, 2017

orta commented Nov 16, 2017

alloy commented Nov 16, 2017

joeyAghion commented Nov 17, 2017

sweir27 commented Nov 17, 2017

alloy Nov 17, 2017 •

edited

Loading