diff --git a/.idea/altmedia.iml b/.idea/altmedia.iml index e99c7d52..d53b377f 100644 --- a/.idea/altmedia.iml +++ b/.idea/altmedia.iml @@ -54,7 +54,7 @@ - + @@ -134,8 +134,9 @@ - - + + + @@ -154,6 +155,7 @@ + diff --git a/Gemfile b/Gemfile index a69eb312..e73fe580 100644 --- a/Gemfile +++ b/Gemfile @@ -24,8 +24,9 @@ gem 'lograge', '>=0.11.2' gem 'netaddr', '~> 1.5', '>= 1.5.1' gem 'net-ssh' gem 'okcomputer', '~> 1.19' -gem 'omniauth', '~> 1.9', '>= 1.9.2' -gem 'omniauth-cas', '~> 2.0' +gem 'omniauth', '~> 2.1' +gem 'omniauth-cas', '~> 3.0' +gem 'omniauth-rails_csrf_protection', '~> 1.0' gem 'pg', '~> 1.2' gem 'prawn', '~> 2.4' gem 'puma', '~> 4.3', '>= 4.3.12' diff --git a/Gemfile.lock b/Gemfile.lock index 89a112d2..45b26b09 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -276,13 +276,18 @@ GEM ostruct (>= 0.2) okcomputer (1.19.1) benchmark - omniauth (1.9.2) + omniauth (2.1.4) hashie (>= 3.4.6) - rack (>= 1.6.2, < 3) - omniauth-cas (2.0.0) - addressable (~> 2.3) - nokogiri (~> 1.5) - omniauth (~> 1.2) + logger + rack (>= 2.2.3) + rack-protection + omniauth-cas (3.0.2) + addressable (~> 2.8) + nokogiri (~> 1.12) + omniauth (~> 2.1) + omniauth-rails_csrf_protection (1.0.2) + actionpack (>= 4.2) + omniauth (~> 2.0) ostruct (0.6.3) ougai (2.0.0) oj (~> 3.10) @@ -310,6 +315,9 @@ GEM raabro (1.4.0) racc (1.8.1) rack (2.2.17) + rack-protection (3.2.0) + base64 (>= 0.1.0) + rack (~> 2.2, >= 2.2.4) rack-session (1.0.2) rack (< 3) rack-test (2.2.0) @@ -533,8 +541,9 @@ DEPENDENCIES net-ssh netaddr (~> 1.5, >= 1.5.1) okcomputer (~> 1.19) - omniauth (~> 1.9, >= 1.9.2) - omniauth-cas (~> 2.0) + omniauth (~> 2.1) + omniauth-cas (~> 3.0) + omniauth-rails_csrf_protection (~> 1.0) pg (~> 1.2) prawn (~> 2.4) puma (~> 4.3, >= 4.3.12) diff --git a/app/controllers/concerns/exception_handling.rb b/app/controllers/concerns/exception_handling.rb index 44f7c15b..6c921b13 100644 --- a/app/controllers/concerns/exception_handling.rb +++ b/app/controllers/concerns/exception_handling.rb @@ -64,7 +64,7 @@ def log_error(error) # this isn't really an error condition, it just means the user's # not logged in, so we don't need the full stack trace etc. logger.info(error.message) - redirect_to main_app.login_path(url: request.fullpath) + render :unauthorized, status: :unauthorized end end # rubocop:enable Metrics/BlockLength diff --git a/app/controllers/reference_card_forms_controller.rb b/app/controllers/reference_card_forms_controller.rb index b36d7c16..b55084b2 100644 --- a/app/controllers/reference_card_forms_controller.rb +++ b/app/controllers/reference_card_forms_controller.rb @@ -85,6 +85,6 @@ def validate_recaptcha! def require_admin! @user_is_admin = current_user.role?(Role.stackpass_admin) - redirect_to login_path(url: request.fullpath) unless @user_is_admin + raise Error::ForbiddenError unless @user_is_admin end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 04a6582b..22be13eb 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -10,12 +10,6 @@ # # @see https://github.com/omniauth/omniauth class SessionsController < ApplicationController - # Redirect the user to Calnet for authentication - def new - redirect_args = { origin: params[:url] || home_path }.to_query - redirect_to "/auth/calnet?#{redirect_args}" - end - # Generate a new user session using data returned from a valid Calnet login def callback logger.debug({ msg: 'Received omniauth callback', omniauth: auth_params }) diff --git a/app/controllers/stack_pass_forms_controller.rb b/app/controllers/stack_pass_forms_controller.rb index e15822bb..64c32aee 100644 --- a/app/controllers/stack_pass_forms_controller.rb +++ b/app/controllers/stack_pass_forms_controller.rb @@ -87,6 +87,6 @@ def validate_recaptcha! def require_admin! @user_is_admin = current_user.role?(Role.stackpass_admin) - redirect_to login_path(url: request.fullpath) unless @user_is_admin + raise Error::ForbiddenError unless @user_is_admin end end diff --git a/app/views/application/unauthorized.html.erb b/app/views/application/unauthorized.html.erb new file mode 100644 index 00000000..68abf8c2 --- /dev/null +++ b/app/views/application/unauthorized.html.erb @@ -0,0 +1,7 @@ +

Unauthorized

+ +

You need to log in to continue.

+ +<%= form_tag('/auth/calnet', url: request.original_url, method: :post, data: { turbo: false }) do %> + <%= button_tag 'CalNet Login', class: :calnet_login, role: :link %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index be450bd1..78d41a49 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -74,7 +74,6 @@ # Omniauth automatically handles requests to /auth/:provider. We need only # implement the callback. - get '/login', to: 'sessions#new', as: :login get '/logout', to: 'sessions#destroy', as: :logout get '/auth/:provider/callback', to: 'sessions#callback', as: :omniauth_callback get '/auth/failure', to: 'sessions#failure' diff --git a/spec/calnet_helper.rb b/spec/calnet_helper.rb index 636497af..86804ee1 100644 --- a/spec/calnet_helper.rb +++ b/spec/calnet_helper.rb @@ -120,8 +120,6 @@ def without_redirects def log_in_with_omniauth(auth_hash) OmniAuth.config.mock_auth[:calnet] = auth_hash - do_get login_path - Rails.application.env_config['omniauth.auth'] = auth_hash do_get omniauth_callback_path(:calnet) end diff --git a/spec/forms_helper.rb b/spec/forms_helper.rb index 67500750..503ee9c2 100644 --- a/spec/forms_helper.rb +++ b/spec/forms_helper.rb @@ -26,10 +26,9 @@ def show_path(id) send("#{form_name}_path", id) end - it 'redirects to login' do - expected_location = login_path(url: new_form_path) + it 'requires login' do get new_form_path - expect(response).to redirect_to(expected_location) + expect(response).to have_http_status(:unauthorized) end allowed_patron_types.each do |type| diff --git a/spec/request/bibliographics_spec.rb b/spec/request/bibliographics_spec.rb index 0395f53b..31cf2591 100644 --- a/spec/request/bibliographics_spec.rb +++ b/spec/request/bibliographics_spec.rb @@ -7,32 +7,28 @@ end context 'new/create' do - it 'GET redirects to login' do - get(form_path = bibliographics_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get bibliographics_path + expect(response).to have_http_status :unauthorized end - it 'POST redirects to login' do + it 'POST requires login' do post bibliographics_path, params: { upload_file: nil } - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: bibliographics_path)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end end # context 'index' do - # it 'GET redirects to login' do - # get(form_path = bibliographics_index_path) - # login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - # expect(response).to redirect_to(login_with_callback_url) + # it 'GET requires login' do + # get bibliographics_index_path + # expect(response).to have_http_status :unauthorized # end # end context 'response' do - it 'GET redirects to login' do - get(form_path = bibliographics_response_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get bibliographics_response_path + expect(response).to have_http_status :unauthorized end end diff --git a/spec/request/departmental_card_form_request_spec.rb b/spec/request/departmental_card_form_request_spec.rb index b15ebf22..a657f0d1 100644 --- a/spec/request/departmental_card_form_request_spec.rb +++ b/spec/request/departmental_card_form_request_spec.rb @@ -7,10 +7,9 @@ end context 'new' do - it 'GET redirects to login' do - get(form_path = new_departmental_card_form_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get new_departmental_card_form_path + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/request/doemoff_patron_email_forms_spec.rb b/spec/request/doemoff_patron_email_forms_spec.rb index 870cc044..7b6652bd 100644 --- a/spec/request/doemoff_patron_email_forms_spec.rb +++ b/spec/request/doemoff_patron_email_forms_spec.rb @@ -7,10 +7,9 @@ end context 'index' do - it 'GET redirects to login' do - get(form_path = security_incident_report_forms_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get security_incident_report_forms_path + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/request/mmsid_tind_request_spec.rb b/spec/request/mmsid_tind_request_spec.rb index f6fb1e43..583e266b 100644 --- a/spec/request/mmsid_tind_request_spec.rb +++ b/spec/request/mmsid_tind_request_spec.rb @@ -9,17 +9,15 @@ end context 'new/create' do - it 'GET redirects to login' do - get(form_path = mmsid_tind_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get mmsid_tind_path + expect(response).to have_http_status :unauthorized end - it 'POST redirects to login' do + it 'POST requires login' do args = {} post mmsid_tind_path, params: args - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: mmsid_tind_path)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/request/reference_card_forms_request_spec.rb b/spec/request/reference_card_forms_request_spec.rb index 8dbe7890..370eb080 100644 --- a/spec/request/reference_card_forms_request_spec.rb +++ b/spec/request/reference_card_forms_request_spec.rb @@ -17,11 +17,11 @@ expect(response).to redirect_to(action: :new) end - it 'redirects to login if if user is not a stack pass admin' do + it 'requires login if user is not a stack pass admin' do form = ReferenceCardForm.create(id: 1, email: 'openreq@test.com', name: 'John Doe', pass_date: Date.current, pass_date_end: Date.current + 1) - get(form_path = reference_card_form_path(id: form.id)) - expect(response).to redirect_to("#{login_path}?#{URI.encode_www_form(url: form_path)}") + get reference_card_form_path(id: form.id) + expect(response).to have_http_status :forbidden end it 'rejects a submission with a captcha verification error' do diff --git a/spec/request/security_incident_report_forms_spec.rb b/spec/request/security_incident_report_forms_spec.rb index e3ab3b6f..e9fec30f 100644 --- a/spec/request/security_incident_report_forms_spec.rb +++ b/spec/request/security_incident_report_forms_spec.rb @@ -7,10 +7,9 @@ end context 'index' do - it 'GET redirects to login' do - get(form_path = security_incident_report_forms_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get security_incident_report_forms_path + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/request/stack_pass_form_request_spec.rb b/spec/request/stack_pass_form_request_spec.rb index e5af2cbc..629fffea 100644 --- a/spec/request/stack_pass_form_request_spec.rb +++ b/spec/request/stack_pass_form_request_spec.rb @@ -23,12 +23,12 @@ expect(response).to redirect_to(action: :new) end - it 'redirects to login if if user is not a stack pass admin' do + it 'requires login if user is not a stack pass admin' do form = StackPassForm.create(email: 'openreq@test.com', name: 'John Doe', phone: '925-555-1234', pass_date: Date.current, main_stack: true) - get(form_path = stack_pass_form_path(id: form.id)) - expect(response).to redirect_to("#{login_path}?#{URI.encode_www_form(url: form_path)}") + get stack_pass_form_path(id: form.id) + expect(response).to have_http_status :forbidden end it 'rejects a submission with a captcha verification error' do @@ -49,7 +49,6 @@ get response.header['Location'] expect(response.body).to match('RECaptcha Error') end - end context 'specs with admin privledges' do @@ -136,7 +135,6 @@ end context 'specs with non-admin user logged in' do - before do allow(Rails.application.config).to receive(:alma_api_key).and_return(alma_api_key) diff --git a/spec/request/tind_download_request_spec.rb b/spec/request/tind_download_request_spec.rb index 16bec080..48f5a60d 100644 --- a/spec/request/tind_download_request_spec.rb +++ b/spec/request/tind_download_request_spec.rb @@ -19,35 +19,30 @@ end describe 'form' do - it 'redirects to login' do - get(form_path = tind_download_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'requires login' do + get tind_download_path + expect(response).to have_http_status :unauthorized end end describe 'find_collection' do - it 'redirects to login' do + it 'requires login' do get tind_download_find_collection_path - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: tind_download_find_collection_path)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end end describe 'download' do - it 'POST redirects to login' do + it 'POST requires login' do post tind_download_download_path, params: { collection_name:, export_format: 'csv' } - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: tind_download_download_path)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end - it 'GET redirects to login' do + it 'GET requires login' do params = { collection_name:, export_format: 'csv' } get tind_download_download_path, params: params - callback_url = "#{tind_download_download_path}?#{URI.encode_www_form(params)}" - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: callback_url)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/request/tind_marc_batch_request_spec.rb b/spec/request/tind_marc_batch_request_spec.rb index c190833c..e54c3b94 100644 --- a/spec/request/tind_marc_batch_request_spec.rb +++ b/spec/request/tind_marc_batch_request_spec.rb @@ -10,17 +10,15 @@ end context 'new/create' do - it 'GET redirects to login' do - get(form_path = tind_marc_batch_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get tind_marc_batch_path + expect(response).to have_http_status :unauthorized end - it 'POST redirects to login' do + it 'POST requires login' do args = {} post tind_marc_batch_path, params: args - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: tind_marc_batch_path)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/request/tind_validator_request_spec.rb b/spec/request/tind_validator_request_spec.rb index 8cb9e6af..81bcf5fe 100644 --- a/spec/request/tind_validator_request_spec.rb +++ b/spec/request/tind_validator_request_spec.rb @@ -13,10 +13,9 @@ end context 'index' do - it 'GET redirects to login' do - get(form_path = tind_spread_validator_path) - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: form_path)}" - expect(response).to redirect_to(login_with_callback_url) + it 'GET requires login' do + get tind_spread_validator_path + expect(response).to have_http_status :unauthorized end end end diff --git a/spec/requests/location_requests_spec.rb b/spec/requests/location_requests_spec.rb index ec086017..0f989f66 100644 --- a/spec/requests/location_requests_spec.rb +++ b/spec/requests/location_requests_spec.rb @@ -9,10 +9,9 @@ describe :index do context 'without login' do - it 'redirects to login' do + it 'requires login' do get location_requests_url - login_with_callback_url = "#{login_path}?#{URI.encode_www_form(url: location_requests_path)}" - expect(response).to redirect_to(login_with_callback_url) + expect(response).to have_http_status :unauthorized end end diff --git a/spec/system/alma_item_set_system_spec.rb b/spec/system/alma_item_set_system_spec.rb index 1e355d19..c43fe3c2 100644 --- a/spec/system/alma_item_set_system_spec.rb +++ b/spec/system/alma_item_set_system_spec.rb @@ -6,10 +6,9 @@ let(:alma_api_key) { 'not-a-real-api-key' } describe 'unauthenticated user' do - it 'redirects to login' do + it 'requires login' do visit alma_item_set_path - expected_path = "#{omniauth_callback_path(:calnet)}?#{URI.encode_www_form(origin: alma_item_set_path)}" - expect(page).to have_current_path(expected_path) + expect(page).to have_content('You need to log in to continue.') end end diff --git a/spec/system/bibliographics_spec.rb b/spec/system/bibliographics_spec.rb index 37049376..7f6233b6 100644 --- a/spec/system/bibliographics_spec.rb +++ b/spec/system/bibliographics_spec.rb @@ -4,10 +4,9 @@ RSpec.describe 'Bibliographics', type: :system do describe 'unauthenticated user' do - it 'redirects to login' do + it 'requires login' do visit bibliographics_path - expected_path = "#{omniauth_callback_path(:calnet)}?#{URI.encode_www_form(origin: bibliographics_path)}" - expect(page).to have_current_path(expected_path) + expect(page).to have_content('You need to log in to continue.') end end diff --git a/spec/system/home_system_spec.rb b/spec/system/home_system_spec.rb index 81581ec3..495b592a 100644 --- a/spec/system/home_system_spec.rb +++ b/spec/system/home_system_spec.rb @@ -5,11 +5,9 @@ describe :home, type: :system do describe :admin do context 'without login' do - it 'redirects to login' do + it 'requires login' do visit admin_path - - expected_path = "#{omniauth_callback_path(:calnet)}?#{URI.encode_www_form(origin: admin_path)}" - expect(page).to have_current_path(expected_path) + expect(page).to have_content('You need to log in to continue.') end end end diff --git a/spec/system/location_requests_system_spec.rb b/spec/system/location_requests_system_spec.rb index fd98fc2d..05110537 100644 --- a/spec/system/location_requests_system_spec.rb +++ b/spec/system/location_requests_system_spec.rb @@ -81,9 +81,8 @@ describe :immediate do before { visit immediate_location_request_path } - it 'redirects to login' do - expected_path = "#{omniauth_callback_path(:calnet)}?#{URI.encode_www_form(origin: immediate_location_request_path)}" - expect(page).to have_current_path(expected_path) + it 'requires login' do + expect(page).to have_content('You need to log in to continue.') end end end diff --git a/spec/system/tind_download_system_spec.rb b/spec/system/tind_download_system_spec.rb index 53ff7be2..e731d5c8 100644 --- a/spec/system/tind_download_system_spec.rb +++ b/spec/system/tind_download_system_spec.rb @@ -8,10 +8,9 @@ end describe 'unauthenticated user' do - it 'redirects to login' do + it 'requires login' do visit tind_download_path - expected_path = "#{omniauth_callback_path(:calnet)}?#{URI.encode_www_form(origin: tind_download_path)}" - expect(page).to have_current_path(expected_path) + expect(page).to have_content('You need to log in to continue.') end end diff --git a/spec/system/tind_marc_batch_system_spec.rb b/spec/system/tind_marc_batch_system_spec.rb index 0bd2da2b..b5918b21 100644 --- a/spec/system/tind_marc_batch_system_spec.rb +++ b/spec/system/tind_marc_batch_system_spec.rb @@ -6,10 +6,9 @@ let(:alma_api_key) { 'not-a-real-api-key' } describe 'unauthenticated user' do - it 'redirects to login' do + it 'requires login' do visit tind_marc_batch_path - expected_path = "#{omniauth_callback_path(:calnet)}?#{URI.encode_www_form(origin: tind_marc_batch_path)}" - expect(page).to have_current_path(expected_path) + expect(page).to have_content('You need to log in to continue.') end end