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

Refactor Hydra token introspection #92

Merged
merged 17 commits into from
Mar 2, 2023
Merged
Show file tree
Hide file tree
Changes from 14 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
7 changes: 1 addition & 6 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 Down
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
27 changes: 16 additions & 11 deletions spec/requests/projects/create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,42 +6,47 @@
let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' }
let(:project) { create(:project, user_id:) }

describe 'create' do
context 'when auth is correct' do
context 'when auth is correct' do
let(:headers) { { Authorization: 'dummy-token' } }

context 'when creating project is successful' do
before do
mock_oauth_user(user_id)
stub_fetch_oauth_user_id(user_id)

response = OperationResponse.new
response[:project] = project
allow(Project::Create).to receive(:call).and_return(response)
end

it 'returns success' do
post '/api/projects'
post '/api/projects', headers: headers

expect(response).to have_http_status(:ok)
end
end

context 'when creating project fails' do
before do
mock_oauth_user(user_id)
stub_fetch_oauth_user_id(user_id)

response = OperationResponse.new
response[:error] = 'Error creating project'
allow(Project::Create).to receive(:call).and_return(response)
end

it 'returns error' do
post '/api/projects'
post '/api/projects', headers: headers

expect(response).to have_http_status(:internal_server_error)
end
end
end

context 'when no auth user' do
it 'returns unauthorized' do
post '/api/projects'
expect(response).to have_http_status(:unauthorized)
end
context 'when no token is given' do
it 'returns unauthorized' do
post '/api/projects'

expect(response).to have_http_status(:unauthorized)
end
end
end
14 changes: 9 additions & 5 deletions spec/requests/projects/destroy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,22 @@

context 'when user is logged in' do
let!(:project) { create(:project, user_id:) }
let(:headers) { { Authorization: 'dummy-token' } }

before do
mock_oauth_user(user_id)
stub_fetch_oauth_user_id(user_id)
end

context 'when deleting a project the user owns' do
it 'returns success' do
delete "/api/projects/#{project.identifier}"
delete "/api/projects/#{project.identifier}", headers: headers

expect(response).to have_http_status(:ok)
end

it "deletes user's project" do
expect do
delete "/api/projects/#{project.identifier}"
delete "/api/projects/#{project.identifier}", headers:
end.to change(Project, :count).by(-1)
end
end
Expand All @@ -29,15 +31,17 @@
let(:non_owned_project) { create(:project) }

it 'returns forbidden' do
delete "/api/projects/#{non_owned_project.identifier}"
delete "/api/projects/#{non_owned_project.identifier}", headers: headers

expect(response).to have_http_status(:forbidden)
end
end
end

context 'when no user' do
context 'when no token is given' do
it 'returns unauthorized' do
delete '/api/projects/project-identifier'

expect(response).to have_http_status(:unauthorized)
end
end
Expand Down
22 changes: 13 additions & 9 deletions spec/requests/projects/images_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,47 +20,51 @@

describe 'create' do
context 'when auth is correct' do
let(:headers) { { Authorization: 'dummy-token' } }

before do
mock_oauth_user(user_id)
stub_fetch_oauth_user_id(project.user_id)
end

it 'attaches file to project' do
expect { post "/api/projects/#{project.identifier}/images", params: }.to change { project.images.count }.by(1)
expect { post "/api/projects/#{project.identifier}/images", params:, headers: }.to change { project.images.count }.by(1)
end

it 'returns file list' do
post "/api/projects/#{project.identifier}/images", params: params
post "/api/projects/#{project.identifier}/images", params: params, headers: headers

expect(response.body).to eq(expected_json)
end

it 'returns success response' do
post "/api/projects/#{project.identifier}/images", params: params
post "/api/projects/#{project.identifier}/images", params: params, headers: headers

expect(response).to have_http_status(:ok)
end

it 'returns 404 response if invalid project' do
post '/api/projects/no-such-project/images'
post '/api/projects/no-such-project/images', headers: headers

expect(response).to have_http_status(:not_found)
end
end

context 'when authed user is not creator' do
let(:headers) { { Authorization: 'dummy-token' } }

before do
mock_oauth_user
stub_fetch_oauth_user_id
end

it 'returns forbidden response' do
post "/api/projects/#{project.identifier}/images", params: params
post "/api/projects/#{project.identifier}/images", params: params, headers: headers
expect(response).to have_http_status(:forbidden)
end
end

context 'when auth is invalid' do
context 'when auth token is missing' do
it 'returns unauthorized' do
post "/api/projects/#{project.identifier}/images"
post "/api/projects/#{project.identifier}/images", headers: headers

expect(response).to have_http_status(:unauthorized)
end
Expand Down
Loading