From aafd13d3c199610ffff044815ead6581e56e9dd1 Mon Sep 17 00:00:00 2001 From: Sarah Weir Date: Wed, 15 Nov 2017 17:11:32 -0500 Subject: [PATCH] adds example of authorizing on a field-level --- Gemfile | 1 - Gemfile.lock | 3 -- app/controllers/api/base_controller.rb | 12 ++++-- app/controllers/api/graphql_controller.rb | 5 ++- app/graph/mutations.rb | 4 ++ app/graph/permission_blacklist.rb | 7 ---- app/graph/root_schema.rb | 4 +- app/graph/types/query_type.rb | 1 + .../util/authorization_instrumentation.rb | 37 +++++++++++++++++++ app/graph/util/permission_blacklist.rb | 7 ++++ spec/requests/api/graphql/create_spec.rb | 7 +++- spec/requests/api/graphql/query_spec.rb | 19 ++++++++-- spec/requests/api/graphql/update_spec.rb | 7 +++- 13 files changed, 88 insertions(+), 26 deletions(-) delete mode 100644 app/graph/permission_blacklist.rb create mode 100644 app/graph/util/authorization_instrumentation.rb create mode 100644 app/graph/util/permission_blacklist.rb diff --git a/Gemfile b/Gemfile index 08cdd7d8..02cc8702 100644 --- a/Gemfile +++ b/Gemfile @@ -29,7 +29,6 @@ gem 'gemini_upload-rails', gemini_gem_spec # for admins to upload images gem 'graphql' # A lovely API gem 'graphiql-rails' # A lovely interface to the API -gem 'graphql-guard' # Authorization helpers for graphQL watt_gem_spec = { git: 'https://github.com/artsy/watt.git', branch: 'master' } gem 'watt', watt_gem_spec # artsy bootstrap diff --git a/Gemfile.lock b/Gemfile.lock index 898f7482..6888388b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -124,8 +124,6 @@ GEM graphiql-rails (1.4.7) rails graphql (1.6.7) - graphql-guard (1.0.0) - graphql (>= 1.6.0, < 2) haml (5.0.1) temple (>= 0.8.0) tilt @@ -358,7 +356,6 @@ DEPENDENCIES gemini_upload-rails! graphiql-rails graphql - graphql-guard haml-rails hyperclient jquery-rails diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 48fab6ed..354e0494 100644 --- a/app/controllers/api/base_controller.rb +++ b/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 + def require_authentication raise ApplicationController::NotAuthorized unless current_app && current_user end @@ -33,10 +37,6 @@ def require_authorized_submission raise ApplicationController::NotAuthorized unless current_user && current_user == @submission.user_id end - def require_app - raise ApplicationController::NotAuthorized unless current_app - end - private # For now, require that signature is valid by verifying payload w/ secret. @@ -55,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(',') + end end end diff --git a/app/controllers/api/graphql_controller.rb b/app/controllers/api/graphql_controller.rb index 7508557e..f481c53e 100644 --- a/app/controllers/api/graphql_controller.rb +++ b/app/controllers/api/graphql_controller.rb @@ -6,9 +6,10 @@ def execute variables: params[:variables], context: { current_application: current_app, - current_user: current_user + current_user: current_user, + current_user_roles: current_user_roles }, - except: PermissionBlacklist + except: Util::PermissionBlacklist ) render json: result, status: 200 end diff --git a/app/graph/mutations.rb b/app/graph/mutations.rb index f5de70b4..10e728b4 100644 --- a/app/graph/mutations.rb +++ b/app/graph/mutations.rb @@ -5,6 +5,8 @@ module Mutations field :createSubmission, Types::SubmissionType do description 'Create a submission' argument :submission, Inputs::SubmissionInput::Create + permit ['user'] + resolve ->(_obj, args, context) { Submission.create!(args[:submission].to_h.merge(user_id: context[:current_user])) } @@ -13,6 +15,8 @@ module Mutations field :updateSubmission, Types::SubmissionType do description 'Create a submission' argument :submission, Inputs::SubmissionInput::Update + permit ['user'] + resolve ->(_obj, args, _context) { submission = Submission.find_by(id: args[:submission][:id]) || raise(GraphQL::ExecutionError, 'Submission Not Found') diff --git a/app/graph/permission_blacklist.rb b/app/graph/permission_blacklist.rb deleted file mode 100644 index ba28758e..00000000 --- a/app/graph/permission_blacklist.rb +++ /dev/null @@ -1,7 +0,0 @@ -class PermissionBlacklist - def self.call(schema_member, context) - if schema_member.name == 'user_id' - return context[:current_user].blank? - end - end -end diff --git a/app/graph/root_schema.rb b/app/graph/root_schema.rb index 5f48d80e..a185c7e2 100644 --- a/app/graph/root_schema.rb +++ b/app/graph/root_schema.rb @@ -1,6 +1,8 @@ +GraphQL::Field.accepts_definitions(permit: GraphQL::Define.assign_metadata_key(:permit)) + RootSchema = GraphQL::Schema.define do query Types::QueryType mutation Mutations::Root - + instrument(:field, Util::AuthorizationInstrumentation.new) max_depth 5 end diff --git a/app/graph/types/query_type.rb b/app/graph/types/query_type.rb index 609fc593..c3bcbc99 100644 --- a/app/graph/types/query_type.rb +++ b/app/graph/types/query_type.rb @@ -6,6 +6,7 @@ module Types description 'Find Submissions' argument :ids, types[types.ID] argument :id, types.ID + permit ['admin'] resolve ->(_object, args, _context) { args[:ids] ? Submission.where(id: args[:ids]) : [Submission.find(args[:id])] diff --git a/app/graph/util/authorization_instrumentation.rb b/app/graph/util/authorization_instrumentation.rb new file mode 100644 index 00000000..8430cbcf --- /dev/null +++ b/app/graph/util/authorization_instrumentation.rb @@ -0,0 +1,37 @@ +module Util + class AuthorizationInstrumentation + def instrument(_type, field) + if requires_authorization?(field) + old_resolve_proc = field.resolve_proc + new_resolve_proc = ->(obj, args, ctx) do + if can_access?(field, ctx) + resolved = old_resolve_proc.call(obj, args, ctx) + resolved + else + err = GraphQL::ExecutionError.new("Can't access #{field.name}") + ctx.add_error(err) + end + end + + field.redefine do + resolve(new_resolve_proc) + end + else + field + end + end + + def requires_authorization?(field) + field.metadata[:permit].present? + end + + def can_access?(field, ctx) + if field.metadata[:permit] + return false unless ctx[:current_user_roles] + !(ctx[:current_user_roles] & field.metadata[:permit]).empty? + else + field + end + end + end +end diff --git a/app/graph/util/permission_blacklist.rb b/app/graph/util/permission_blacklist.rb new file mode 100644 index 00000000..f02f9010 --- /dev/null +++ b/app/graph/util/permission_blacklist.rb @@ -0,0 +1,7 @@ +module Util + class PermissionBlacklist + def self.call(schema_member, context) + return context[:current_user].blank? if schema_member.name == 'user_id' + end + end +end diff --git a/spec/requests/api/graphql/create_spec.rb b/spec/requests/api/graphql/create_spec.rb index a7890fe8..17120374 100644 --- a/spec/requests/api/graphql/create_spec.rb +++ b/spec/requests/api/graphql/create_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' describe 'Create Submission With Graphql' do - let(:jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid' }, Convection.config.jwt_secret) } + let(:jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid', roles: 'user' }, Convection.config.jwt_secret) } let(:headers) { { 'Authorization' => "Bearer #{jwt_token}" } } let(:create_mutation) do @@ -31,7 +31,10 @@ post '/api/graphql', params: { query: create_mutation }, headers: { 'Authorization' => 'Bearer foo.bar.baz' } - expect(response.status).to eq 401 + expect(response.status).to eq 200 + body = JSON.parse(response.body) + expect(body['data']['createSubmission']).to eq nil + expect(body['errors'][0]['message']).to eq "Can't access createSubmission" end it 'rejects when missing artist_id' do diff --git a/spec/requests/api/graphql/query_spec.rb b/spec/requests/api/graphql/query_spec.rb index 4ea24be8..905530d1 100644 --- a/spec/requests/api/graphql/query_spec.rb +++ b/spec/requests/api/graphql/query_spec.rb @@ -1,8 +1,9 @@ require 'rails_helper' describe 'Query Submissions With Graphql' do - let(:jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid' }, Convection.config.jwt_secret) } - let(:headers) { { 'Authorization' => "Bearer #{jwt_token}" } } + let(:admin_jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid', roles: 'admin' }, Convection.config.jwt_secret) } + let(:user_jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid', roles: 'user' }, Convection.config.jwt_secret) } + let(:headers) { { 'Authorization' => "Bearer #{admin_jwt_token}" } } let(:submission) { Fabricate(:submission, artist_id: 'abbas-kiarostami', title: 'rain') } let!(:submission2) { Fabricate(:submission, artist_id: 'andy-warhol', title: 'no-rain') } let(:asset) { Fabricate(:asset, submission: submission) } @@ -34,7 +35,7 @@ post '/api/graphql', params: { query: introspection_query } - expect(JSON.parse(response.body)['data']['__type']['fields'].map{|f| f['name']}).to_not include('user_id') + expect(JSON.parse(response.body)['data']['__type']['fields'].map { |f| f['name'] }).to_not include('user_id') expect(response.status).to eq 200 end @@ -52,7 +53,7 @@ post '/api/graphql', params: { query: introspection_query }, headers: headers - expect(JSON.parse(response.body)['data']['__type']['fields'].map{|f| f['name']}).to include('user_id') + expect(JSON.parse(response.body)['data']['__type']['fields'].map { |f| f['name'] }).to include('user_id') expect(response.status).to eq 200 end @@ -64,5 +65,15 @@ body = JSON.parse(response.body) expect(body['data']['submission'].count).to eq 2 end + + it 'throws an error if a user tries to access' do + post '/api/graphql', params: { + query: query_submissions + }, headers: { 'Authorization' => "Bearer #{user_jwt_token}" } + expect(response.status).to eq 200 + body = JSON.parse(response.body) + expect(body['data']['submission']).to eq nil + expect(body['errors'][0]['message']).to eq "Can't access submission" + end end end diff --git a/spec/requests/api/graphql/update_spec.rb b/spec/requests/api/graphql/update_spec.rb index c96f11f0..ca99e964 100644 --- a/spec/requests/api/graphql/update_spec.rb +++ b/spec/requests/api/graphql/update_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' describe 'Update Submission With Graphql' do - let(:jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid' }, Convection.config.jwt_secret) } + let(:jwt_token) { JWT.encode({ aud: 'gravity', sub: 'userid', roles: 'user' }, Convection.config.jwt_secret) } let(:headers) { { 'Authorization' => "Bearer #{jwt_token}" } } let(:submission) { Fabricate(:submission, artist_id: 'abbas-kiarostami', title: 'rain') } @@ -33,7 +33,10 @@ post '/api/graphql', params: { query: update_mutation }, headers: { 'Authorization' => 'Bearer foo.bar.baz' } - expect(response.status).to eq 401 + expect(response.status).to eq 200 + body = JSON.parse(response.body) + expect(body['data']['updateSubmission']).to eq nil + expect(body['errors'][0]['message']).to eq "Can't access updateSubmission" end it 'errors for unkown submission id' do