Skip to content
Merged
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
2 changes: 1 addition & 1 deletion .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ POSTGRES_USER=changeme
POSTGRES_PASSWORD=changeme

HYDRA_ADMIN_URL=http://host.docker.internal:9002
HYDRA_SECRET=
HYDRA_ADMIN_API_KEY=test-key

SMEE_TUNNEL=https://smee.io/MLq0n9kvAes2vydX

Expand Down
2 changes: 2 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,5 @@ RSpec/DescribeClass:
- "spec/graphql/queries/**"
- "spec/graphql/mutations/**"

RSpec/MultipleMemoizedHelpers:
Max: 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of upping this could you not refactor the tests a bit?

So in the images_spec you don't really need the image_filename and instead could do
let(:params) { { images: [fixture_file_upload('test_image_1.png', 'image/png')] } } etc as it's not referenced in any tests.

Same for the update spec. You could remove the default_component_params and put them straight into the params as they are not being used in the tests I don't think.

Unless we wanted it increased of course :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there are more places where we use a lot of memoized helpers. I don't feel to bad about upping the number. We should discuss this though..

Copy link
Contributor

Choose a reason for hiding this comment

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

@patch0 Yeah I'm fine with it being increased to 8. There's already a few PR's in need of that from your overly productive day yesterday 😅 Just some autocorrectable offences to fix!

2 changes: 1 addition & 1 deletion app/controllers/api/default_projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

module Api
class DefaultProjectsController < ApiController
before_action :require_oauth_user, only: %i[create]
before_action :authorize_user, only: %i[create]

def show
data = if params[:type] == 'html'
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/projects/images_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module Api
module Projects
class ImagesController < ApiController
before_action :require_oauth_user
before_action :authorize_user

def create
@project = Project.find_by!(identifier: params[:project_id])
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/api/projects/remixes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
module Api
module Projects
class RemixesController < ApiController
before_action :require_oauth_user
before_action :authorize_user

def create
result = Project::CreateRemix.call(params: remix_params,
user_id: oauth_user_id,
user_id: current_user,
original_project: project)

if result.success?
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

module Api
class ProjectsController < ApiController
before_action :require_oauth_user, only: %i[create update index destroy]
before_action :authorize_user, only: %i[create update index destroy]
before_action :load_project, only: %i[show update destroy]
before_action :load_projects, only: %i[index]
after_action :pagination_link_header, only: [:index]
Expand Down
11 changes: 3 additions & 8 deletions app/controllers/api_controller.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class ApiController < ActionController::API
include OauthUser
include Identifiable

unless Rails.application.config.consider_all_requests_local
rescue_from ActiveRecord::RecordNotFound, with: -> { notfound }
Expand All @@ -10,13 +10,8 @@ class ApiController < ActionController::API

private

def require_oauth_user
head :unauthorized unless oauth_user_id
end

def current_user
# current_user is required by CanCanCan
oauth_user_id
def authorize_user
head :unauthorized unless current_user
end

def notfound
Expand Down
16 changes: 0 additions & 16 deletions app/controllers/concerns/authentication_concern.rb

This file was deleted.

21 changes: 21 additions & 0 deletions app/controllers/concerns/identifiable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# frozen_string_literal: true

require 'hydra_admin_api'

module Identifiable
extend ActiveSupport::Concern

def identify_user
token = request.headers['Authorization']
return nil unless token

HydraAdminApi.fetch_oauth_user_id(token:)
end

def current_user_id
@current_user_id ||= identify_user
end

# current_user is required by CanCanCan
alias current_user current_user_id
end
33 changes: 5 additions & 28 deletions app/controllers/graphql_controller.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
# frozen_string_literal: true

class GraphqlController < ApplicationController
include AuthenticationConcern
include ActiveStorage::SetCurrent if Rails.env.development?

skip_before_action :verify_authenticity_token

class GraphqlController < ApiController
# If accessing from outside this domain, nullify the session
# This allows for outside API access while preventing CSRF attacks,
# but you'll have to authenticate your user separately
Expand All @@ -14,10 +9,6 @@ class GraphqlController < ApplicationController
def execute
result = EditorApiSchema.execute(query, variables:, context:, operation_name:)
render json: result
rescue StandardError => e
raise e unless Rails.env.development?

handle_error_in_development(e)
end

private
Expand All @@ -38,29 +29,15 @@ def context
def variables
variables_param = params[:variables]

case params[:variables]
return {} if variables_param.blank?

case variables_param
when String
if variables_param.present?
JSON.parse(variables_param) || {}
else
{}
end
when Hash
variables_param
JSON.parse(variables_param) || {}
when ActionController::Parameters
variables_param.to_unsafe_hash # GraphQL-Ruby will validate name and type of incoming variables.
when nil
{}
else
raise ArgumentError, "Unexpected parameter: #{variables_param}"
end
end

def handle_error_in_development(error)
logger.error error.message
logger.error error.backtrace.join("\n")

render json: { errors: [{ message: error.message, backtrace: error.backtrace }], data: {} },
status: :internal_server_error
end
end
29 changes: 0 additions & 29 deletions app/helpers/oauth_user.rb

This file was deleted.

2 changes: 0 additions & 2 deletions spec/lib/hydra_admin_api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
require 'rails_helper'
require 'hydra_admin_api'

# rubocop:disable RSpec/MultipleMemoizedHelpers
RSpec.describe HydraAdminApi do
let(:hydra_admin_url) { 'https://hydra.com/admin' }
let(:hydra_admin_api_key) { 'secret' }
Expand Down Expand Up @@ -55,4 +54,3 @@
end
end
end
# rubocop:enable RSpec/MultipleMemoizedHelpers
2 changes: 1 addition & 1 deletion spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
config.include GraphqlQueryHelpers, type: :graphql_query

config.include PhraseIdentifierMock
config.include OauthUserMock
config.include HydraAdminApiMock, type: :request
end

Shoulda::Matchers.configure do |config|
Expand Down
136 changes: 117 additions & 19 deletions spec/requests/graphql_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,136 @@
require 'rails_helper'

RSpec.describe 'POST /graphql' do
subject { response }
subject(:request) { post(graphql_path, as: :json, params:, headers:) }

let(:params) { {} }
let(:headers) { {} }
let(:json_response) { JSON.parse(response.body) }
let(:headers) { nil }
let(:params) { nil }

before { post graphql_path, params:, headers: }
before do
allow(EditorApiSchema).to receive(:execute).and_return({})
end

it { is_expected.to be_ok }
shared_examples 'no variables are set' do
it 'sets an empty hash for the variables' do
request
expect(EditorApiSchema).to have_received(:execute).with(anything, hash_including(variables: {}))
end
end

it 'returns errors' do
expect(json_response['errors']).to be_a Array
shared_examples 'correctly sets the variables' do
it 'passes them correctly' do
request
expect(EditorApiSchema).to have_received(:execute).with(anything, hash_including(variables:))
end
end

context 'with a query' do
let(:params) { { query: } }
let(:query) { '' }
shared_examples 'an unidentified request' do
it 'sets the current_user_id as nil in the context' do
request
expect(EditorApiSchema).to have_received(:execute).with(anything, hash_including(context: hash_including(current_user_id: nil)))
end
end

it 'returns OK' do
request
expect(response).to be_ok
end

it_behaves_like 'an unidentified request'

it_behaves_like 'no variables are set'

it { is_expected.to be_ok }
it 'returns a JSON response' do
request
expect(response.content_type).to start_with 'application/json;'
end

context 'when an operationName is given' do
let(:params) { { operationName: operation_name } }
let(:operation_name) { 'testOperation' }

it 'returns errors' do
expect(json_response['errors']).to be_a Array
it 'sets the operationName correctly' do
request
expect(EditorApiSchema).to have_received(:execute).with(anything, hash_including(operation_name:))
end
end

context 'with a valid query' do
let(:query) { '{ node("xyz") }' }
context 'when an Authorization header is supplied' do
let(:headers) { { Authorization: token } }
let(:token) { '' }

it { is_expected.to be_ok }
it_behaves_like 'an unidentified request'

it 'returns data' do
expect(json_response.dig('data', 'node')).to be_nil
context 'with a token' do
let(:token) { 'valid-token' }

context 'when the token is invalid' do
before do
stub_fetch_oauth_user_id(nil)
end

it_behaves_like 'an unidentified request'
end

context 'when the token is valid' do
let(:current_user_id) { SecureRandom.uuid }

before do
stub_fetch_oauth_user_id(current_user_id)
end

it 'sets the current_user_id in the context' do
request
expect(EditorApiSchema).to have_received(:execute).with(anything, hash_including(context: hash_including(current_user_id:)))
end
end
end
end

context 'when variables are given' do
let(:params) { { variables: } }
let(:variables) { { 'key' => 'value' } }

it_behaves_like 'correctly sets the variables'

context 'when they are a JSON string' do
subject(:request) { post(graphql_path, as: :url_encoded_form, params:) }

let(:params) { { variables: variables.to_json } }

it_behaves_like 'correctly sets the variables'
end

context 'when the params are encoded as url_encoded_form' do
subject(:request) { post(graphql_path, as: :url_encoded_form, params:) }

it_behaves_like 'correctly sets the variables'
end

context 'when variables are set to null' do
let(:variables) { 'null' }

it_behaves_like 'no variables are set'
end

context 'when variables are an empty string' do
let(:variables) { '' }

it_behaves_like 'no variables are set'
end
end

context 'when a query is given' do
let(:query) { '{ query { hello { id } }' }
let(:params) { { query: } }

before do
allow(EditorApiSchema).to receive(:execute).and_return({})
end

it 'passes them correctly' do
request
expect(EditorApiSchema).to have_received(:execute).with(query, anything)
end
end
end
Loading