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

Permission blacklist + field-level auth example #122

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/controllers/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ def set_submission
@submission = Submission.find(submission_id)
end

def set_current_user_roles
current_user_roles
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused?

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


def require_authentication
raise ApplicationController::NotAuthorized unless current_app && current_user
end
Expand All @@ -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(',')
Copy link
Contributor

Choose a reason for hiding this comment

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

niiiiice!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

end
end
end
8 changes: 4 additions & 4 deletions app/controllers/api/graphql_controller.rb
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
module Api
class GraphqlController < BaseController
before_action :require_authentication

def execute
result = RootSchema.execute(
params[:query],
variables: params[:variables],
context: {
current_application: current_app,
current_user: current_user
}
current_user: current_user,
current_user_roles: current_user_roles
},
except: Util::PermissionBlacklist
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
render json: result, status: 200
end
Expand Down
4 changes: 4 additions & 0 deletions app/graph/mutations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]))
}
Expand All @@ -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')
Expand Down
4 changes: 3 additions & 1 deletion app/graph/root_schema.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions app/graph/types/query_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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])]
Expand Down
37 changes: 37 additions & 0 deletions app/graph/util/authorization_instrumentation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
module Util
class AuthorizationInstrumentation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

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
7 changes: 7 additions & 0 deletions app/graph/util/permission_blacklist.rb
Original file line number Diff line number Diff line change
@@ -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'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@alloy alloy Nov 17, 2017

Choose a reason for hiding this comment

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

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).

end
end
end
7 changes: 5 additions & 2 deletions spec/requests/api/graphql/create_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
52 changes: 46 additions & 6 deletions spec/requests/api/graphql/query_spec.rb
Original file line number Diff line number Diff line change
@@ -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) }
Expand All @@ -20,11 +21,40 @@
end

describe 'POST /graphql' do
it 'rejects unauthorized requests' do
it 'does not return the user_id if there is no user' do
introspection_query = <<-graphql
query {
__type(name: "Submission") {
name
fields {
name
}
}
}
graphql
post '/api/graphql', params: {
query: query_submissions
}, headers: { 'Authorization' => 'Bearer foo.bar.baz' }
expect(response.status).to eq 401
query: introspection_query
}
expect(JSON.parse(response.body)['data']['__type']['fields'].map { |f| f['name'] }).to_not include('user_id')
expect(response.status).to eq 200
end

it 'includes the user_id param if there is a user present' do
introspection_query = <<-graphql
query {
__type(name: "Submission") {
name
fields {
name
}
}
}
graphql
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(response.status).to eq 200
end

it 'finds two existing submissions' do
Expand All @@ -35,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
7 changes: 5 additions & 2 deletions spec/requests/api/graphql/update_spec.rb
Original file line number Diff line number Diff line change
@@ -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') }

Expand Down Expand Up @@ -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
Expand Down