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 <[email protected]>

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'

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)

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?

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

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

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: "[email protected]", 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