From ab45e094ec8366320e2b7bdf90eb798b60e24052 Mon Sep 17 00:00:00 2001 From: Max Kadel Date: Mon, 15 Nov 2021 09:24:04 -0500 Subject: [PATCH] Omniauth redux post upgrade (#704) * Upgrade to Omniauth 2.0.x * Add csrf protection gem * Set target and origin as single parameter going out * Ensure we do not direct logged in users to other websites Co-authored-by: Ben Pennell --- Gemfile | 4 +- Gemfile.lock | 15 ++-- app/controllers/application_controller.rb | 17 +++-- app/controllers/omniauth_controller.rb | 6 +- spec/controllers/omniauth_controller_spec.rb | 2 +- spec/features/logging_in_spec.rb | 77 ++++++++++++++++++++ 6 files changed, 105 insertions(+), 16 deletions(-) create mode 100644 spec/features/logging_in_spec.rb diff --git a/Gemfile b/Gemfile index 61a0d8844..13ae212bd 100644 --- a/Gemfile +++ b/Gemfile @@ -61,9 +61,9 @@ gem 'redis', '~> 3.3.0' gem 'rsolr', '~> 2.0.2' gem 'devise', '~> 4.8.0' gem 'devise-guests', '~> 0.7.0' -gem 'omniauth', '~> 1.9.1' +gem 'omniauth', '~> 2.0' gem 'omniauth-shibboleth', '~> 1.3' - +gem 'omniauth-rails_csrf_protection' gem 'bulkrax', '~> 1.0.0' # required by bulkrax_override - rails engine for SWORDv2 gem 'willow_sword', github: 'notch8/willow_sword', ref: '0a669d7' diff --git a/Gemfile.lock b/Gemfile.lock index 08ea4400a..7ed482dac 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -365,7 +365,7 @@ GEM hamster (3.0.0) concurrent-ruby (~> 1.0) hashdiff (1.0.1) - hashie (4.1.0) + hashie (5.0.0) hiredis (0.6.3) htmlentities (4.3.4) http_logger (0.6.0) @@ -576,8 +576,8 @@ GEM sparql (~> 3.1) sparql-client (~> 3.1) listen (3.7.0) - rb-fsevent (~> 0.9, >= 0.9.4) - rb-inotify (~> 0.9, >= 0.9.7) + rb-fsevent (~> 0.10, >= 0.10.3) + rb-inotify (~> 0.9, >= 0.9.10) loofah (2.10.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) @@ -627,9 +627,13 @@ GEM multi_json (~> 1.3) multi_xml (~> 0.5) rack (>= 1.2, < 3) - omniauth (1.9.1) + omniauth (2.0.4) hashie (>= 3.4.6) rack (>= 1.6.2, < 3) + rack-protection + omniauth-rails_csrf_protection (1.0.0) + actionpack (>= 4.2) + omniauth (~> 2.0) omniauth-shibboleth (1.3.0) omniauth (>= 1.0.0) openseadragon (0.6.0) @@ -963,7 +967,8 @@ DEPENDENCIES mini_magick (~> 4.9.4) mini_racer (~> 0.2.15) nokogiri (~> 1.12.5) - omniauth (~> 1.9.1) + omniauth (~> 2.0) + omniauth-rails_csrf_protection omniauth-shibboleth (~> 1.3) passenger (= 5.3.7) pg (~> 1.2.3) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 99c3c4dab..3f10ea122 100755 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -61,14 +61,21 @@ def render_rsolr_exceptions(exception) # [hyc-override] Overriding default after_sign_in_path_for which only forward to the dashboard def after_sign_in_path_for(resource) - direct_to = stored_location_for(resource) || request.env['omniauth.origin'] || root_path - Rails.logger.debug "After sign in, direct to: #{direct_to}" - direct_to + direct_to_path = direct_to(resource) + Rails.logger.debug "After sign in, direct to: #{direct_to_path}" + direct_to_path end - private - + def direct_to(resource) + stored_location = stored_location_for(resource) + return stored_location if stored_location.present? + return root_path if params['origin'].nil? + return params['origin'] if URI.parse(params['origin']).host == request.env['SERVER_NAME'] + root_path + rescue URI::InvalidURIError + root_path + end # Redirect all deposit and edit requests with warning message when in read only mode def check_read_only return unless Flipflop.read_only? diff --git a/app/controllers/omniauth_controller.rb b/app/controllers/omniauth_controller.rb index 00b42aa8a..baadd3da2 100644 --- a/app/controllers/omniauth_controller.rb +++ b/app/controllers/omniauth_controller.rb @@ -6,9 +6,9 @@ class OmniauthController < Devise::SessionsController def new # Rails.logger.debug "SessionsController#new: request.referer = #{request.referer}" - if Rails.env.production? && (ENV['DATABASE_AUTH'] == 'false') - origin_param = CGI.escape("&origin=#{request.referer}") - shib_login_url = ENV['SSO_LOGIN_PATH'] + "?target=#{user_shibboleth_omniauth_authorize_path}#{origin_param}" + if Rails.env.production? && (AuthConfig.use_database_auth? == false) + shib_query = "target=#{user_shibboleth_omniauth_callback_path}%26origin=#{CGI.escape(request.referer.to_s)}" + shib_login_url = ENV['SSO_LOGIN_PATH'] + "?#{shib_query}" redirect_to shib_login_url else super diff --git a/spec/controllers/omniauth_controller_spec.rb b/spec/controllers/omniauth_controller_spec.rb index 49011ee7d..ba33fe8f9 100644 --- a/spec/controllers/omniauth_controller_spec.rb +++ b/spec/controllers/omniauth_controller_spec.rb @@ -23,7 +23,7 @@ it 'is successful' do get new_user_session_path - expect(response).to redirect_to "/Shibboleth.sso/Login?target=#{user_shibboleth_omniauth_authorize_path}%26origin%3D" + expect(response).to redirect_to "/Shibboleth.sso/Login?target=#{user_shibboleth_omniauth_callback_path}%26origin=" end end end diff --git a/spec/features/logging_in_spec.rb b/spec/features/logging_in_spec.rb new file mode 100644 index 000000000..d673bf96c --- /dev/null +++ b/spec/features/logging_in_spec.rb @@ -0,0 +1,77 @@ +require "rails_helper" + +RSpec.feature 'logging into the application' do + context "in production with database auth turned off" do + let(:escaped_origin) { CGI.escape("http://www.example.com/advanced?locale=en") } + let(:escaped_target) { CGI.escape("/users/auth/shibboleth/callback?locale=en") } + + before do + + allow(AuthConfig).to receive(:use_database_auth?).and_return(false) + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) + end + + it "can find the login link" do + visit "/advanced?locale=en" + expect(page).to have_link("Login", href: '/users/sign_in?locale=en') + click_link("Login") + expect(page.current_url).to eq "http://www.example.com/Shibboleth.sso/Login?target=/users/auth/shibboleth/callback?locale=en%26origin=#{escaped_origin}" + end + context "with shibboleth mocked" do + before do + OmniAuth.config.test_mode = true + OmniAuth.config.mock_auth[:shibboleth] = OmniAuth::AuthHash.new({ + :provider => 'shibboleth', + :info => { + :uid => 'atester' + } + }) + end + + it "can return to the application" do + visit "/users/auth/shibboleth/callback?locale=en&origin=http://www.example.com/advanced?locale=en" + expect(page).to have_content "Successfully authenticated from Shibboleth account." + expect(page.current_url).to eq "http://www.example.com/advanced?locale=en" + expect(page).to have_content "atester" + end + + it "returns to the root path if the origin is garbage" do + visit "/users/auth/shibboleth/callback?locale=en&origin=://sdfgdfg.com" + expect(page).to have_content "Successfully authenticated from Shibboleth account." + expect(page.current_url).to eq "http://www.example.com/?locale=en" + end + + it "returns to the root path if the origin does not match the host" do + visit "/users/auth/shibboleth/callback?locale=en&origin=http://fake.example.com" + expect(page).to have_content "Successfully authenticated from Shibboleth account." + expect(page.current_url).to eq "http://www.example.com/?locale=en" + end + end + + end + + context "in production with database auth turned on" do + let(:user) do + User.create(email: "test@example.com", guest: false, uid: "test", password: "123456") + end + + before do + allow(AuthConfig).to receive(:use_database_auth?).and_return(true) + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) + end + + it "can log in using database authentication and redirect to the homepage" do + user.reload + visit "/advanced?locale=en" + expect(page).to have_link("Login", href: '/users/sign_in?locale=en') + click_link("Login") + expect(page.current_url).to eq "http://www.example.com/users/sign_in?locale=en" + fill_in 'Onyen', with: 'test' + fill_in 'Password', with: '123456' + click_button("Log in") + expect(page).to have_content "Signed in successfully." + # The database auth does not return you to the original page + expect(page.current_url).to eq "http://www.example.com/?locale=en" + end + end +end