From b067d5f5619a38418184cc429cd3cef624d375ae Mon Sep 17 00:00:00 2001 From: Pat Allan Date: Thu, 5 Oct 2017 14:08:15 +1100 Subject: [PATCH 01/30] Update for frozen-string-literal friendliness. --- lib/omniauth/form.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/form.rb b/lib/omniauth/form.rb index fecc63660..b838d3f20 100644 --- a/lib/omniauth/form.rb +++ b/lib/omniauth/form.rb @@ -9,7 +9,7 @@ def initialize(options = {}) options[:header_info] ||= '' self.options = options - @html = '' + @html = ''.dup @with_custom_button = false @footer = nil header(options[:title], options[:header_info]) From 3e4e716506cdbe1b208be813b9c3ed36dfafe7e0 Mon Sep 17 00:00:00 2001 From: Jonathan Hinkle Date: Wed, 26 Jun 2019 16:19:26 -0700 Subject: [PATCH 02/30] limit Builder#provider's symbol lookup to constants under OmniAuth::Strategies i.e., prevent it from finding top-level constants --- lib/omniauth/builder.rb | 2 +- spec/omniauth/builder_spec.rb | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/builder.rb b/lib/omniauth/builder.rb index a513571f8..fcb0af1ba 100644 --- a/lib/omniauth/builder.rb +++ b/lib/omniauth/builder.rb @@ -50,7 +50,7 @@ def provider(klass, *args, &block) middleware = klass else begin - middleware = OmniAuth::Strategies.const_get(OmniAuth::Utils.camelize(klass.to_s).to_s) + middleware = OmniAuth::Strategies.const_get(OmniAuth::Utils.camelize(klass.to_s).to_s, false) rescue NameError raise(LoadError.new("Could not find matching strategy for #{klass.inspect}. You may need to install an additional gem (such as omniauth-#{klass}).")) end diff --git a/spec/omniauth/builder_spec.rb b/spec/omniauth/builder_spec.rb index 92d13fb1b..65411dd1d 100644 --- a/spec/omniauth/builder_spec.rb +++ b/spec/omniauth/builder_spec.rb @@ -3,7 +3,7 @@ describe OmniAuth::Builder do describe '#provider' do it 'translates a symbol to a constant' do - expect(OmniAuth::Strategies).to receive(:const_get).with('MyStrategy').and_return(Class.new) + expect(OmniAuth::Strategies).to receive(:const_get).with('MyStrategy', false).and_return(Class.new) OmniAuth::Builder.new(nil) do provider :my_strategy end @@ -26,6 +26,16 @@ class ExampleClass; end end end.to raise_error(LoadError, 'Could not find matching strategy for :lorax. You may need to install an additional gem (such as omniauth-lorax).') end + + it "doesn't translate a symbol to a top-level constant" do + class MyStrategy; end + + expect do + OmniAuth::Builder.new(nil) do + provider :my_strategy + end + end.to raise_error(LoadError, 'Could not find matching strategy for :my_strategy. You may need to install an additional gem (such as omniauth-my_strategy).') + end end describe '#options' do From e35a14995a88d47c346671b6b649d0280b8e0882 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Wed, 20 Nov 2019 11:56:43 -0500 Subject: [PATCH 03/30] Fix code climate badge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 554f430a9..476f06eab 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Gem Version](http://img.shields.io/gem/v/omniauth.svg)][gem] [![Build Status](http://img.shields.io/travis/omniauth/omniauth.svg)][travis] -[![Code Climate](http://img.shields.io/codeclimate/github/omniauth/omniauth.svg)][codeclimate] +[![Code Climate](https://api.codeclimate.com/v1/badges/ffd33970723587806744/maintainability)][codeclimate] [![Coverage Status](http://img.shields.io/coveralls/omniauth/omniauth.svg)][coveralls] [![Security](https://hakiri.io/github/omniauth/omniauth/master.svg)](https://hakiri.io/github/omniauth/omniauth/master) From 13fc9c3e0679b04d3f8673dfa6cb673015bcd309 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Mon, 7 Oct 2019 14:44:48 -0500 Subject: [PATCH 04/30] Change first-person to second-person in README This changes the few instances of "I" to "you" to match the rest of the README. --- README.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 0d3f5e450..9c7df85b7 100644 --- a/README.md +++ b/README.md @@ -32,8 +32,8 @@ development and easily swap in other strategies later. ## Getting Started Each OmniAuth strategy is a Rack Middleware. That means that you can use it the same way that you use any other Rack middleware. For example, to -use the built-in Developer strategy in a Sinatra application I might do -this: +use the built-in Developer strategy in a Sinatra application you might +do this: ```ruby require 'sinatra' @@ -45,7 +45,7 @@ class MyApplication < Sinatra::Base end ``` -Because OmniAuth is built for *multi-provider* authentication, I may +Because OmniAuth is built for *multi-provider* authentication, you may want to leave room to run multiple strategies. For this, the built-in `OmniAuth::Builder` class gives you an easy way to specify multiple strategies. Note that there is **no difference** between the following @@ -82,14 +82,14 @@ environment of a request to `/auth/:provider/callback`. This hash contains as much information about the user as OmniAuth was able to glean from the utilized strategy. You should set up an endpoint in your application that matches to the callback URL and then performs whatever -steps are necessary for your application. For example, in a Rails app I -would add a line in my `routes.rb` file like this: +steps are necessary for your application. For example, in a Rails app +you would add a line in your `routes.rb` file like this: ```ruby get '/auth/:provider/callback', to: 'sessions#create' ``` -And I might then have a `SessionsController` with code that looks +And you might then have a `SessionsController` with code that looks something like this: ```ruby @@ -108,7 +108,7 @@ class SessionsController < ApplicationController end ``` -The `omniauth.auth` key in the environment hash gives me my +The `omniauth.auth` key in the environment hash provides an Authentication Hash which will contain information about the just authenticated user including a unique id, the strategy they just used for authentication, and personal details such as name and email address @@ -163,7 +163,7 @@ a `session_store.rb` initializer, add `use ActionDispatch::Session::CookieStore` and have sessions functioning as normal. To be clear: sessions may work, but your session options will be ignored -(i.e the session key will default to `_session_id`). Instead of the +(i.e. the session key will default to `_session_id`). Instead of the initializer, you'll have to set the relevant options somewhere before your middleware is built (like `application.rb`) and pass them to your preferred middleware, like this: From eb5047ad51eda33fa083be08b59295bc2a88864c Mon Sep 17 00:00:00 2001 From: Cyril Rohr Date: Thu, 15 Mar 2018 12:14:50 +0100 Subject: [PATCH 05/30] Make the FailureEndpoint respect the strategy's path_prefix. Previously, it just fetched the global path_prefix. --- lib/omniauth/failure_endpoint.rb | 10 +++++++++- spec/omniauth/failure_endpoint_spec.rb | 8 +++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/failure_endpoint.rb b/lib/omniauth/failure_endpoint.rb index 7c39a347b..947a7fd8d 100644 --- a/lib/omniauth/failure_endpoint.rb +++ b/lib/omniauth/failure_endpoint.rb @@ -27,10 +27,18 @@ def raise_out! def redirect_to_failure message_key = env['omniauth.error.type'] - new_path = "#{env['SCRIPT_NAME']}#{OmniAuth.config.path_prefix}/failure?message=#{message_key}#{origin_query_param}#{strategy_name_query_param}" + new_path = "#{env['SCRIPT_NAME']}#{strategy_path_prefix}/failure?message=#{message_key}#{origin_query_param}#{strategy_name_query_param}" Rack::Response.new(['302 Moved'], 302, 'Location' => new_path).finish end + def strategy_path_prefix + if env['omniauth.error.strategy'] + env['omniauth.error.strategy'].path_prefix + else + OmniAuth.config.path_prefix + end + end + def strategy_name_query_param return '' unless env['omniauth.error.strategy'] diff --git a/spec/omniauth/failure_endpoint_spec.rb b/spec/omniauth/failure_endpoint_spec.rb index 714e02962..4b75c1adb 100644 --- a/spec/omniauth/failure_endpoint_spec.rb +++ b/spec/omniauth/failure_endpoint_spec.rb @@ -43,12 +43,18 @@ expect(head['Location']).to eq('/random/auth/failure?message=invalid_request&strategy=test') end - it 'respects the configured path prefix' do + it 'respects the globally configured path prefix' do allow(OmniAuth.config).to receive(:path_prefix).and_return('/boo') _, head, = *subject.call(env) expect(head['Location']).to eq('/boo/failure?message=invalid_request&strategy=test') end + it 'respects the custom path prefix configured on the strategy' do + env['omniauth.error.strategy'] = ExampleStrategy.new({}, path_prefix: "/some/custom/path") + _, head, = *subject.call(env) + expect(head['Location']).to eq('/some/custom/path/failure?message=invalid_request&strategy=test') + end + it 'includes the origin (escaped) if one is provided' do env['omniauth.origin'] = '/origin-example' _, head, = *subject.call(env) From 4c960045e3ab92653e6f4ef76f3c2225fc5a4a05 Mon Sep 17 00:00:00 2001 From: Garen Torikian Date: Mon, 17 Feb 2020 18:49:58 +0000 Subject: [PATCH 06/30] Remove Hakiri badge --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 0d3f5e450..4b1c2e4fb 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,6 @@ [![Build Status](http://img.shields.io/travis/omniauth/omniauth.svg)][travis] [![Code Climate](http://img.shields.io/codeclimate/github/omniauth/omniauth.svg)][codeclimate] [![Coverage Status](http://img.shields.io/coveralls/omniauth/omniauth.svg)][coveralls] -[![Security](https://hakiri.io/github/omniauth/omniauth/master.svg)](https://hakiri.io/github/omniauth/omniauth/master) [gem]: https://rubygems.org/gems/omniauth [travis]: http://travis-ci.org/omniauth/omniauth From 790d354c8826d2c2fabc3524478f81099291f441 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Mon, 2 Mar 2020 15:07:25 -0500 Subject: [PATCH 07/30] OmniAuth::Utils.camelize now properly handles passing false param --- lib/omniauth.rb | 2 +- spec/omniauth_spec.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/omniauth.rb b/lib/omniauth.rb index dc315ee2c..f9cfaf806 100644 --- a/lib/omniauth.rb +++ b/lib/omniauth.rb @@ -159,7 +159,7 @@ def camelize(word, first_letter_in_uppercase = true) if first_letter_in_uppercase word.to_s.gsub(%r{/(.?)}) { '::' + Regexp.last_match[1].upcase }.gsub(/(^|_)(.)/) { Regexp.last_match[2].upcase } else - word.first + camelize(word)[1..-1] + camelize(word).tap { |w| w[0] = w[0].downcase } end end end diff --git a/spec/omniauth_spec.rb b/spec/omniauth_spec.rb index 12f316125..8013ccb7c 100644 --- a/spec/omniauth_spec.rb +++ b/spec/omniauth_spec.rb @@ -148,6 +148,10 @@ class ExampleStrategy OmniAuth.config.add_camelization('oauth', 'OAuth') expect(OmniAuth::Utils.camelize(:oauth)).to eq('OAuth') end + + it 'doesn\'t uppercase the first letter when passed false' do + expect(OmniAuth::Utils.camelize('apple_jack', false)).to eq('appleJack') + end end end end From 5c48888e850ca81cfd10586fe7a23bb81e1f05c7 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Mon, 2 Mar 2020 15:09:14 -0500 Subject: [PATCH 08/30] Increase test coverage --- spec/helper.rb | 4 ++-- spec/omniauth/auth_hash_spec.rb | 8 ++++++++ spec/omniauth/form_spec.rb | 33 ++++++++++++++++++++++++++++++++- spec/omniauth/key_store_spec.rb | 1 + spec/omniauth/strategy_spec.rb | 29 +++++++++++++++++++++++++++++ spec/omniauth_spec.rb | 7 +++++++ 6 files changed, 79 insertions(+), 3 deletions(-) diff --git a/spec/helper.rb b/spec/helper.rb index a0975eed4..8565335ab 100644 --- a/spec/helper.rb +++ b/spec/helper.rb @@ -45,7 +45,7 @@ def initialize(*args, &block) def request_phase options[:mutate_on_request].call(options) if options[:mutate_on_request] - @fail = fail!(options[:failure]) if options[:failure] + @fail = fail!(options[:failure], options[:failure_exception]) if options[:failure] @last_env = env return @fail if @fail @@ -54,7 +54,7 @@ def request_phase def callback_phase options[:mutate_on_callback].call(options) if options[:mutate_on_callback] - @fail = fail!(options[:failure]) if options[:failure] + @fail = fail!(options[:failure], options[:failure_exception]) if options[:failure] @last_env = env return @fail if @fail diff --git a/spec/omniauth/auth_hash_spec.rb b/spec/omniauth/auth_hash_spec.rb index 8a1a3c784..9467e86d6 100644 --- a/spec/omniauth/auth_hash_spec.rb +++ b/spec/omniauth/auth_hash_spec.rb @@ -13,6 +13,10 @@ expect(subject.weird_field.info).to eq 'string' end + it 'has a subkey_class' do + expect(OmniAuth::AuthHash.subkey_class).to eq Hashie::Mash + end + describe '#valid?' do subject { OmniAuth::AuthHash.new(:uid => '123', :provider => 'example', :info => {:name => 'Steven'}) } @@ -111,6 +115,10 @@ end end + it 'has a subkey_class' do + expect(OmniAuth::AuthHash::InfoHash.subkey_class).to eq Hashie::Mash + end + require 'hashie/version' if Gem::Version.new(Hashie::VERSION) >= Gem::Version.new('3.5.1') context 'with Hashie 3.5.1+' do diff --git a/spec/omniauth/form_spec.rb b/spec/omniauth/form_spec.rb index 69b91d947..1b9c88c70 100644 --- a/spec/omniauth/form_spec.rb +++ b/spec/omniauth/form_spec.rb @@ -7,7 +7,8 @@ end it 'evaluates in the instance when called with a block and no argument' do - OmniAuth::Form.build { |f| expect(f.class).to eq(OmniAuth::Form) } + f = OmniAuth::Form.build { @html = '

OmniAuth

' } + expect(f.instance_variable_get(:@html)).to eq('

OmniAuth

') end end @@ -20,4 +21,34 @@ expect(OmniAuth::Form.new(:title => 'Something Cool').to_html).to be_include('

Something Cool

') end end + + describe '#password_field' do + it 'adds a labeled input field' do + form = OmniAuth::Form.new.password_field('pass', 'password') + form_html = form.to_html + expect(form_html).to include('') + expect(form_html).to include('') + end + end + + describe '#html' do + it 'appends to the html body' do + form = OmniAuth::Form.build { @html = '

' } + form.html('

') + + expect(form.instance_variable_get(:@html)).to eq '

' + end + end + + describe 'fieldset' do + it 'creates a fieldset with options' do + form = OmniAuth::Form.new + options = {:style => 'color: red', :id => 'fieldSetId'} + expected = "
\n legendary\n\n
" + + form.fieldset('legendary', options) {} + + expect(form.to_html).to include expected + end + end end diff --git a/spec/omniauth/key_store_spec.rb b/spec/omniauth/key_store_spec.rb index 559cba69e..54c87fac5 100644 --- a/spec/omniauth/key_store_spec.rb +++ b/spec/omniauth/key_store_spec.rb @@ -25,6 +25,7 @@ it 'does not log anything to the console' do stub_const('Hashie::VERSION', version) + allow(OmniAuth::KeyStore).to receive(:respond_to?).with(:disable_warnings).and_return(false) OmniAuth::KeyStore.override_logging expect(logger).not_to receive(:info) OmniAuth::KeyStore.new(:id => 1234) diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index de670056b..2e8f915ce 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -32,6 +32,12 @@ def make_env(path = '/auth/test', props = {}) end end + describe 'user_info' do + it 'should default to an empty hash' do + expect(fresh_strategy.new(app, :skip_info => true).user_info).to eq({}) + end + end + describe '.configure' do subject do c = Class.new @@ -63,6 +69,29 @@ def make_env(path = '/auth/test', props = {}) end end + describe '#fail!' do + it 'provides exception information when one is provided' do + env = make_env + exception = RuntimeError.new('No session!') + + expect(OmniAuth.logger).to receive(:error).with( + "(test) Authentication failure! failed: #{exception.class}, #{exception.message}" + ) + + ExampleStrategy.new(app, :failure => :failed, :failure_exception => exception).call(env) + end + + it 'provides a generic message when not provided an exception' do + env = make_env + + expect(OmniAuth.logger).to receive(:error).with( + '(test) Authentication failure! Some Issue encountered.' + ) + + ExampleStrategy.new(app, :failure => "Some Issue").call(env) + end + end + describe '#skip_info?' do it 'is true if options.skip_info is true' do expect(ExampleStrategy.new(app, :skip_info => true)).to be_skip_info diff --git a/spec/omniauth_spec.rb b/spec/omniauth_spec.rb index 8013ccb7c..02b0247cb 100644 --- a/spec/omniauth_spec.rb +++ b/spec/omniauth_spec.rb @@ -128,6 +128,13 @@ class ExampleStrategy end describe '::Utils' do + describe 'form_css' do + it 'returns a style tag with the configured form_css' do + allow(OmniAuth).to receive(:config).and_return(double(:form_css => 'css.css')) + expect(OmniAuth::Utils.form_css).to eq "" + end + end + describe '.deep_merge' do it 'combines hashes' do expect(OmniAuth::Utils.deep_merge({'abc' => {'def' => 123}}, 'abc' => {'foo' => 'bar'})).to eq('abc' => {'def' => 123, 'foo' => 'bar'}) From b918891d08161b1455c5cc20a072ea4ee8a9b8b0 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Mon, 2 Mar 2020 15:38:05 -0500 Subject: [PATCH 09/30] Add an additional test to test replacement --- spec/omniauth/strategy_spec.rb | 2 +- spec/omniauth_spec.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index 2e8f915ce..6ae5cee39 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -88,7 +88,7 @@ def make_env(path = '/auth/test', props = {}) '(test) Authentication failure! Some Issue encountered.' ) - ExampleStrategy.new(app, :failure => "Some Issue").call(env) + ExampleStrategy.new(app, :failure => 'Some Issue').call(env) end end diff --git a/spec/omniauth_spec.rb b/spec/omniauth_spec.rb index 02b0247cb..4e364ae7a 100644 --- a/spec/omniauth_spec.rb +++ b/spec/omniauth_spec.rb @@ -159,6 +159,11 @@ class ExampleStrategy it 'doesn\'t uppercase the first letter when passed false' do expect(OmniAuth::Utils.camelize('apple_jack', false)).to eq('appleJack') end + + it 'replaces / with ::' do + expect(OmniAuth::Utils.camelize('apple_jack/cereal')).to eq('AppleJack::Cereal') + expect(OmniAuth::Utils.camelize('apple_jack/cereal', false)).to eq('appleJack::Cereal') + end end end end From f3ef5f8e79ca94986a7da6696b7c2921f3c515ae Mon Sep 17 00:00:00 2001 From: Alex Blackie Date: Wed, 12 Dec 2018 09:52:59 -0800 Subject: [PATCH 10/30] Change debugging logging to use debug channel These messages are very specific to omniauth internals, and thus are only useful when debugging or developing. Having these messages on `:info` causes them to fill test suite output or other non-production logs which are at the `:info` level. We should move these messages to `:debug` so developers can still see them, but they to not pollute non-development logs. --- lib/omniauth/strategy.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index ce8b34871..807c610a3 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -202,17 +202,17 @@ def options_call # Performs the steps necessary to run the request phase of a strategy. def request_call # rubocop:disable CyclomaticComplexity, MethodLength, PerceivedComplexity setup_phase - log :info, 'Request phase initiated.' + log :debug, 'Request phase initiated.' # store query params from the request url, extracted in the callback_phase session['omniauth.params'] = request.GET OmniAuth.config.before_request_phase.call(env) if OmniAuth.config.before_request_phase if options.form.respond_to?(:call) - log :info, 'Rendering form from supplied Rack endpoint.' + log :debug, 'Rendering form from supplied Rack endpoint.' options.form.call(env) elsif options.form - log :info, 'Rendering form from underlying application.' + log :debug, 'Rendering form from underlying application.' call_app! elsif !options.origin_param request_phase @@ -230,7 +230,7 @@ def request_call # rubocop:disable CyclomaticComplexity, MethodLength, Perceived # Performs the steps necessary to run the callback phase of a strategy. def callback_call setup_phase - log :info, 'Callback phase initiated.' + log :debug, 'Callback phase initiated.' @env['omniauth.origin'] = session.delete('omniauth.origin') @env['omniauth.origin'] = nil if env['omniauth.origin'] == '' @env['omniauth.params'] = session.delete('omniauth.params') || {} @@ -312,10 +312,10 @@ def mock_callback_call # underlying application. This will default to `/auth/:provider/setup`. def setup_phase if options[:setup].respond_to?(:call) - log :info, 'Setup endpoint detected, running now.' + log :debug, 'Setup endpoint detected, running now.' options[:setup].call(env) elsif options[:setup] - log :info, 'Calling through to underlying application for setup.' + log :debug, 'Calling through to underlying application for setup.' setup_env = env.merge('PATH_INFO' => setup_path, 'REQUEST_METHOD' => 'GET') call_app!(setup_env) end From 1cd79a1769145a3d4f1f8b31957a49f1e3f97fbd Mon Sep 17 00:00:00 2001 From: zino Date: Sat, 25 Apr 2020 15:45:13 -0500 Subject: [PATCH 11/30] Remove duplicative eval of credentials and extra --- lib/omniauth/strategy.rb | 4 ++-- spec/omniauth/strategy_spec.rb | 8 +++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index ce8b34871..5172542cb 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -347,8 +347,8 @@ def extra def auth_hash hash = AuthHash.new(:provider => name, :uid => uid) hash.info = info unless skip_info? - hash.credentials = credentials if credentials - hash.extra = extra if extra + (credentials_data = credentials) && hash.credentials = credentials_data + (extra_data = extra) && hash.extra = extra_data hash end diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index de670056b..466b59e9a 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -173,19 +173,25 @@ def make_env(path = '/auth/test', props = {}) end let(:instance) { subject.new(app) } - it 'calls through to uid and info' do + it 'calls through to uid, info, credentials, and extra' do expect(instance).to receive(:uid) expect(instance).to receive(:info) + expect(instance).to receive(:credentials).and_return(expires: true).once + expect(instance).to receive(:extra).and_return(something: 'else').once instance.auth_hash end it 'returns an AuthHash' do allow(instance).to receive(:uid).and_return('123') allow(instance).to receive(:info).and_return(:name => 'Hal Awesome') + allow(instance).to receive(:credentials).and_return(expires: true) + allow(instance).to receive(:extra).and_return(something: 'else') hash = instance.auth_hash expect(hash).to be_kind_of(OmniAuth::AuthHash) expect(hash.uid).to eq('123') expect(hash.info.name).to eq('Hal Awesome') + expect(hash.credentials.expires).to eq(true) + expect(hash.extra.something).to eq('else') end end From ad6228c04ec7496777252496687b06317ae5b8cc Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Wed, 2 Dec 2020 22:16:51 -0500 Subject: [PATCH 12/30] Add cross site protection via rack-protection --- lib/omniauth.rb | 22 +++++-- lib/omniauth/authenticity_token_protection.rb | 30 ++++++++++ lib/omniauth/strategy.rb | 25 ++++++++ omniauth.gemspec | 1 + spec/helper.rb | 1 + spec/omniauth/strategies/developer_spec.rb | 2 +- spec/omniauth/strategy_spec.rb | 60 ++++++++++++++++--- spec/omniauth_spec.rb | 31 ++++++---- 8 files changed, 148 insertions(+), 24 deletions(-) create mode 100644 lib/omniauth/authenticity_token_protection.rb diff --git a/lib/omniauth.rb b/lib/omniauth.rb index dc315ee2c..f6e6c4a41 100644 --- a/lib/omniauth.rb +++ b/lib/omniauth.rb @@ -15,6 +15,7 @@ module Strategies autoload :Form, 'omniauth/form' autoload :AuthHash, 'omniauth/auth_hash' autoload :FailureEndpoint, 'omniauth/failure_endpoint' + autoload :AuthenticityTokenProtection, 'omniauth/authenticity_token_protection' def self.strategies @strategies ||= [] @@ -29,20 +30,22 @@ def self.default_logger logger end - def self.defaults + def self.defaults # rubocop:disable MethodLength @defaults ||= { :camelizations => {}, :path_prefix => '/auth', :on_failure => OmniAuth::FailureEndpoint, :failure_raise_out_environments => ['development'], + :request_validation_phase => OmniAuth::AuthenticityTokenProtection, :before_request_phase => nil, :before_callback_phase => nil, :before_options_phase => nil, :form_css => Form::DEFAULT_CSS, :test_mode => false, :logger => default_logger, - :allowed_request_methods => %i[get post], - :mock_auth => {:default => AuthHash.new('provider' => 'default', 'uid' => '1234', 'info' => {'name' => 'Example User'})} + :allowed_request_methods => %i[post], + :mock_auth => {:default => AuthHash.new('provider' => 'default', 'uid' => '1234', 'info' => {'name' => 'Example User'})}, + :silence_get_warning => false } end @@ -74,6 +77,14 @@ def before_options_phase(&block) end end + def request_validation_phase(&block) + if block_given? + @request_validation_phase = block + else + @request_validation_phase + end + end + def before_request_phase(&block) if block_given? @before_request_phase = block @@ -111,8 +122,9 @@ def add_camelization(name, camelized) camelizations[name.to_s] = camelized.to_s end - attr_writer :on_failure, :before_callback_phase, :before_options_phase, :before_request_phase - attr_accessor :failure_raise_out_environments, :path_prefix, :allowed_request_methods, :form_css, :test_mode, :mock_auth, :full_host, :camelizations, :logger + attr_writer :on_failure, :before_callback_phase, :before_options_phase, :before_request_phase, :request_validation_phase + attr_accessor :failure_raise_out_environments, :path_prefix, :allowed_request_methods, :form_css, + :test_mode, :mock_auth, :full_host, :camelizations, :logger, :silence_get_warning end def self.config diff --git a/lib/omniauth/authenticity_token_protection.rb b/lib/omniauth/authenticity_token_protection.rb new file mode 100644 index 000000000..6dea46b93 --- /dev/null +++ b/lib/omniauth/authenticity_token_protection.rb @@ -0,0 +1,30 @@ +require 'rack-protection' + +module OmniAuth + class AuthenticityError < StandardError; end + class AuthenticityTokenProtection < Rack::Protection::AuthenticityToken + def initialize(options = {}) + @options = default_options.merge(options) + end + + def self.call(env) + new.call!(env) + end + + def call!(env) + return if accepts?(env) + + instrument env + react env + end + + private + + def deny(_env) + OmniAuth.logger.send(:warn, "Attack prevented by #{self.class}") + raise AuthenticityError.new(options[:message]) + end + + alias default_reaction deny + end +end diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index ce8b34871..d201d528d 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -180,6 +180,8 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity raise(error) end + warn_if_using_get + @env = env @env['omniauth.strategy'] = self if on_auth_path? @@ -192,6 +194,25 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity @app.call(env) end + def warn_if_using_get + return unless OmniAuth.config.allowed_request_methods.include?(:get) + return if OmniAuth.config.silence_get_warning + + log :warn, <<-WARN + You are using GET as an allowed request method for OmniAuth. This may leave + you open to CSRF attacks. As of v2.0.0, OmniAuth by default allows only POST + to its own routes. You should review the following resources to guide your + mitigation: + https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284 + https://github.com/omniauth/omniauth/issues/960 + https://nvd.nist.gov/vuln/detail/CVE-2015-9284 + https://github.com/omniauth/omniauth/pull/809 + + You can ignore this warning by setting: + OmniAuth.config.silence_get_warning = true + WARN + end + # Responds to an OPTIONS request. def options_call OmniAuth.config.before_options_phase.call(env) if OmniAuth.config.before_options_phase @@ -206,6 +227,8 @@ def request_call # rubocop:disable CyclomaticComplexity, MethodLength, Perceived # store query params from the request url, extracted in the callback_phase session['omniauth.params'] = request.GET + + OmniAuth.config.request_validation_phase.call(env) if OmniAuth.config.request_validation_phase OmniAuth.config.before_request_phase.call(env) if OmniAuth.config.before_request_phase if options.form.respond_to?(:call) @@ -225,6 +248,8 @@ def request_call # rubocop:disable CyclomaticComplexity, MethodLength, Perceived request_phase end + rescue OmniAuth::AuthenticityError => e + fail!(:authenticity_error, e) end # Performs the steps necessary to run the callback phase of a strategy. diff --git a/omniauth.gemspec b/omniauth.gemspec index cd2c25a07..dc0a90f2a 100644 --- a/omniauth.gemspec +++ b/omniauth.gemspec @@ -8,6 +8,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'hashie', ['>= 3.4.6'] spec.add_dependency 'rack', ['>= 1.6.2', '< 3'] spec.add_development_dependency 'bundler', '~> 2.0' + spec.add_dependency 'rack-protection' spec.add_development_dependency 'rake', '~> 12.0' spec.authors = ['Michael Bleigh', 'Erik Michaels-Ober', 'Tom Milewski'] spec.description = 'A generalized Rack framework for multiple-provider authentication.' diff --git a/spec/helper.rb b/spec/helper.rb index 97b72bd14..9e9ab0453 100644 --- a/spec/helper.rb +++ b/spec/helper.rb @@ -24,6 +24,7 @@ require 'omniauth/test' OmniAuth.config.logger = Logger.new('/dev/null') +OmniAuth.config.request_validation_phase = nil RSpec.configure do |config| config.include Rack::Test::Methods diff --git a/spec/omniauth/strategies/developer_spec.rb b/spec/omniauth/strategies/developer_spec.rb index fdeb10970..309b8f7b8 100644 --- a/spec/omniauth/strategies/developer_spec.rb +++ b/spec/omniauth/strategies/developer_spec.rb @@ -10,7 +10,7 @@ end context 'request phase' do - before(:each) { get '/auth/developer' } + before(:each) { post '/auth/developer' } it 'displays a form' do expect(last_response.status).to eq(200) diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index de670056b..e27196460 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -2,7 +2,7 @@ def make_env(path = '/auth/test', props = {}) { - 'REQUEST_METHOD' => 'GET', + 'REQUEST_METHOD' => 'POST', 'PATH_INFO' => path, 'rack.session' => {}, 'rack.input' => StringIO.new('test=true') @@ -524,20 +524,20 @@ def make_env(path = '/auth/test', props = {}) end context 'request method restriction' do - before do - OmniAuth.config.allowed_request_methods = [:post] + before(:context) do + OmniAuth.config.allowed_request_methods = %i[put post] end it 'does not allow a request method of the wrong type' do - expect { strategy.call(make_env) }.not_to raise_error + expect { strategy.call(make_env('/auth/test', 'REQUEST_METHOD' => 'GET')) }.not_to raise_error end it 'allows a request method of the correct type' do - expect { strategy.call(make_env('/auth/test', 'REQUEST_METHOD' => 'POST')) }.to raise_error('Request Phase') + expect { strategy.call(make_env('/auth/test')) }.to raise_error('Request Phase') end - after do - OmniAuth.config.allowed_request_methods = %i[get post] + after(:context) do + OmniAuth.config.allowed_request_methods = %i[post] end end @@ -548,7 +548,7 @@ def make_env(path = '/auth/test', props = {}) end it 'sets the Allow header properly' do - expect(response[1]['Allow']).to eq('GET, POST') + expect(response[1]['Allow']).to eq('POST') end end @@ -809,6 +809,50 @@ def make_env(path = '/auth/test', props = {}) OmniAuth.config.test_mode = false end end + + context 'authenticity validation' do + let(:app) { lambda { |_env| [200, {}, ['reached our target']] } } + let(:strategy) { ExampleStrategy.new(app, :request_path => '/auth/test') } + before do + OmniAuth.config.request_validation_phase = OmniAuth::AuthenticityTokenProtection + end + + context 'with default POST only request methods' do + let!(:csrf_token) { SecureRandom.base64(32) } + let(:escaped_token) { URI.encode_www_form_component(csrf_token, Encoding::UTF_8) } + + it 'allows a request with matching authenticity_token' do + post_env = make_env('/auth/test', 'rack.session' => {:csrf => csrf_token}, 'rack.input' => StringIO.new("authenticity_token=#{escaped_token}")) + expect { strategy.call(post_env) }.to raise_error('Request Phase') + end + + it 'does not allow a request without a matching authenticity token' do + post_env = make_env('/auth/test', 'rack.input' => StringIO.new("authenticity_token=#{escaped_token}")) + expect(strategy.call(post_env)[0]).to eq(302) + expect(strategy.call(post_env)[2]).to eq(['302 Moved']) + end + end + + context 'with allowed GET' do + before(:context) do + @old_allowed_request_methods = OmniAuth.config.allowed_request_methods + OmniAuth.config.allowed_request_methods = %i[post get] + end + + it 'allows a request without authenticity token' do + get_env = make_env('/auth/test', 'REQUEST_METHOD' => 'GET') + expect { strategy.call(get_env) }.to raise_error('Request Phase') + end + + after(:context) do + OmniAuth.config.allowed_request_methods = @old_allowed_request_methods + end + end + + after do + OmniAuth.config.request_validation_phase = nil + end + end end context 'setup phase' do diff --git a/spec/omniauth_spec.rb b/spec/omniauth_spec.rb index 12f316125..7586ecf42 100644 --- a/spec/omniauth_spec.rb +++ b/spec/omniauth_spec.rb @@ -26,20 +26,22 @@ class ExampleStrategy end before do - @old_path_prefix = OmniAuth.config.path_prefix - @old_on_failure = OmniAuth.config.on_failure - @old_before_callback_phase = OmniAuth.config.before_callback_phase - @old_before_options_phase = OmniAuth.config.before_options_phase - @old_before_request_phase = OmniAuth.config.before_request_phase + @old_path_prefix = OmniAuth.config.path_prefix + @old_on_failure = OmniAuth.config.on_failure + @old_before_callback_phase = OmniAuth.config.before_callback_phase + @old_before_options_phase = OmniAuth.config.before_options_phase + @old_before_request_phase = OmniAuth.config.before_request_phase + @old_request_validation_phase = OmniAuth.config.request_validation_phase end after do OmniAuth.configure do |config| - config.path_prefix = @old_path_prefix - config.on_failure = @old_on_failure - config.before_callback_phase = @old_before_callback_phase - config.before_options_phase = @old_before_options_phase - config.before_request_phase = @old_before_request_phase + config.path_prefix = @old_path_prefix + config.on_failure = @old_on_failure + config.before_callback_phase = @old_before_callback_phase + config.before_options_phase = @old_before_options_phase + config.before_request_phase = @old_before_request_phase + config.request_validation_phase = @old_request_validation_phase end end @@ -88,6 +90,15 @@ class ExampleStrategy expect(OmniAuth.config.before_callback_phase.call).to eq('heyhey') end + it 'is able to set request_validation_phase' do + OmniAuth.configure do |config| + config.request_validation_phase do + 'validated' + end + end + expect(OmniAuth.config.request_validation_phase.call).to eq('validated') + end + describe 'mock auth' do before do @auth_hash = {:uid => '12345', :info => {:name => 'Joe', :email => 'joe@example.com'}} From ee2ef211d587fd35efac28ffd5c066cae4e7d021 Mon Sep 17 00:00:00 2001 From: Josh Jordan Date: Thu, 20 Jun 2013 16:07:10 -0400 Subject: [PATCH 13/30] Rescues from strategy calls by calling fail!, properly URL encodes message_key for FailureEndpoint handling Do not rescue around mock_call! --- lib/omniauth/failure_endpoint.rb | 2 +- lib/omniauth/strategy.rb | 13 ++- spec/omniauth/failure_endpoint_spec.rb | 5 ++ spec/omniauth/strategy_spec.rb | 112 ++++++++++++++++++------- 4 files changed, 96 insertions(+), 36 deletions(-) diff --git a/lib/omniauth/failure_endpoint.rb b/lib/omniauth/failure_endpoint.rb index 7c39a347b..3d4e3001f 100644 --- a/lib/omniauth/failure_endpoint.rb +++ b/lib/omniauth/failure_endpoint.rb @@ -27,7 +27,7 @@ def raise_out! def redirect_to_failure message_key = env['omniauth.error.type'] - new_path = "#{env['SCRIPT_NAME']}#{OmniAuth.config.path_prefix}/failure?message=#{message_key}#{origin_query_param}#{strategy_name_query_param}" + new_path = "#{env['SCRIPT_NAME']}#{OmniAuth.config.path_prefix}/failure?message=#{Rack::Utils.escape(message_key)}#{origin_query_param}#{strategy_name_query_param}" Rack::Response.new(['302 Moved'], 302, 'Location' => new_path).finish end diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index d201d528d..ec481ec90 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -186,10 +186,15 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity @env['omniauth.strategy'] = self if on_auth_path? return mock_call!(env) if OmniAuth.config.test_mode - return options_call if on_auth_path? && options_request? - return request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym) - return callback_call if on_callback_path? - return other_phase if respond_to?(:other_phase) + + begin + return options_call if on_auth_path? && options_request? + return request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym) + return callback_call if on_callback_path? + return other_phase if respond_to?(:other_phase) + rescue => ex + return fail! ex.message, ex + end @app.call(env) end diff --git a/spec/omniauth/failure_endpoint_spec.rb b/spec/omniauth/failure_endpoint_spec.rb index 714e02962..39445e46f 100644 --- a/spec/omniauth/failure_endpoint_spec.rb +++ b/spec/omniauth/failure_endpoint_spec.rb @@ -54,5 +54,10 @@ _, head, = *subject.call(env) expect(head['Location']).to be_include('&origin=%2Forigin-example') end + + it 'escapes the message key' do + _, head = *subject.call(env.merge('omniauth.error.type' => 'Connection refused!')) + expect(head['Location']).to be_include('message=Connection+refused%21') + end end end diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index e27196460..f9a7a87eb 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -312,7 +312,9 @@ def make_env(path = '/auth/test', props = {}) context 'disabled' do it 'does not set omniauth.origin' do @options = {:origin_param => false} - expect { strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + + strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq(nil) end end @@ -320,24 +322,31 @@ def make_env(path = '/auth/test', props = {}) context 'custom' do it 'sets from a custom param' do @options = {:origin_param => 'return'} - expect { strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + + strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('/foo') end end context 'default flow' do it 'is set on the request phase' do - expect { strategy.call(make_env('/auth/test', 'HTTP_REFERER' => 'http://example.com/origin')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with("Request Phase", kind_of(StandardError)) + strategy.call(make_env('/auth/test', 'HTTP_REFERER' => 'http://example.com/origin')) + expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('http://example.com/origin') end it 'is turned into an env variable on the callback phase' do - expect { strategy.call(make_env('/auth/test/callback', 'rack.session' => {'omniauth.origin' => 'http://example.com/origin'})) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with("Callback Phase", kind_of(StandardError)) + strategy.call(make_env('/auth/test/callback', 'rack.session' => {'omniauth.origin' => 'http://example.com/origin'})) + expect(strategy.last_env['omniauth.origin']).to eq('http://example.com/origin') end it 'sets from the params if provided' do - expect { strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'origin=/foo')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'origin=/foo')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('/foo') end @@ -350,7 +359,9 @@ def make_env(path = '/auth/test', props = {}) context 'with script_name' do it 'is set on the request phase, containing full path' do env = {'HTTP_REFERER' => 'http://example.com/sub_uri/origin', 'SCRIPT_NAME' => '/sub_uri'} - expect { strategy.call(make_env('/auth/test', env)) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + + strategy.call(make_env('/auth/test', env)) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('http://example.com/sub_uri/origin') end @@ -359,8 +370,9 @@ def make_env(path = '/auth/test', props = {}) 'rack.session' => {'omniauth.origin' => 'http://example.com/sub_uri/origin'}, 'SCRIPT_NAME' => '/sub_uri' } + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) - expect { strategy.call(make_env('/auth/test/callback', env)) }.to raise_error('Callback Phase') + strategy.call(make_env('/auth/test/callback', env)) expect(strategy.last_env['omniauth.origin']).to eq('http://example.com/sub_uri/origin') end end @@ -369,34 +381,41 @@ def make_env(path = '/auth/test', props = {}) context 'default paths' do it 'uses the default request path' do - expect { strategy.call(make_env) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env) end it 'is case insensitive on request path' do - expect { strategy.call(make_env('/AUTH/Test')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/AUTH/Test')) end it 'is case insensitive on callback path' do - expect { strategy.call(make_env('/AUTH/TeSt/CaLlBAck')) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + strategy.call(make_env('/AUTH/TeSt/CaLlBAck')) end it 'uses the default callback path' do - expect { strategy.call(make_env('/auth/test/callback')) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + strategy.call(make_env('/auth/test/callback')) end it 'strips trailing spaces on request' do - expect { strategy.call(make_env('/auth/test/')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/auth/test/')) end it 'strips trailing spaces on callback' do - expect { strategy.call(make_env('/auth/test/callback/')) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + strategy.call(make_env('/auth/test/callback/')) end context 'callback_url' do it 'uses the default callback_path' do expect(strategy).to receive(:full_host).and_return('http://example.com') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) - expect { strategy.call(make_env) }.to raise_error('Request Phase') + strategy.call(make_env) expect(strategy.callback_url).to eq('http://example.com/auth/test/callback') end @@ -436,12 +455,15 @@ def make_env(path = '/auth/test', props = {}) context 'dynamic paths' do it 'runs the request phase if the custom request path evaluator is truthy' do @options = {:request_path => lambda { |_env| true }} - expect { strategy.call(make_env('/asoufibasfi')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/asoufibasfi')) end it 'runs the callback phase if the custom callback path evaluator is truthy' do @options = {:callback_path => lambda { |_env| true }} - expect { strategy.call(make_env('/asoufiasod')) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + + strategy.call(make_env('/asoufiasod')) end it 'provides a custom callback path if request_path evals to a string' do @@ -451,8 +473,9 @@ def make_env(path = '/auth/test', props = {}) it 'correctly reports the callback path when the custom callback path evaluator is truthy' do strategy_instance = ExampleStrategy.new(app, :callback_path => lambda { |env| env['PATH_INFO'] == '/auth/bish/bosh/callback' }) + strategy_instance.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) - expect { strategy_instance.call(make_env('/auth/bish/bosh/callback')) }.to raise_error('Callback Phase') + strategy_instance.call(make_env('/auth/bish/bosh/callback')) expect(strategy_instance.callback_path).to eq('/auth/bish/bosh/callback') end end @@ -460,20 +483,25 @@ def make_env(path = '/auth/test', props = {}) context 'custom paths' do it 'uses a custom request_path if one is provided' do @options = {:request_path => '/awesome'} - expect { strategy.call(make_env('/awesome')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + + strategy.call(make_env('/awesome')) end it 'uses a custom callback_path if one is provided' do @options = {:callback_path => '/radical'} - expect { strategy.call(make_env('/radical')) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + + strategy.call(make_env('/radical')) end context 'callback_url' do it 'uses a custom callback_path if one is provided' do @options = {:callback_path => '/radical'} expect(strategy).to receive(:full_host).and_return('http://example.com') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) - expect { strategy.call(make_env('/radical')) }.to raise_error('Callback Phase') + strategy.call(make_env('/radical')) expect(strategy.callback_url).to eq('http://example.com/radical') end @@ -496,18 +524,20 @@ def make_env(path = '/auth/test', props = {}) end it 'uses a custom prefix for request' do - expect { strategy.call(make_env('/wowzers/test')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/wowzers/test')) end it 'uses a custom prefix for callback' do - expect { strategy.call(make_env('/wowzers/test/callback')) }.to raise_error('Callback Phase') + strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + strategy.call(make_env('/wowzers/test/callback')) end context 'callback_url' do it 'uses a custom prefix' do expect(strategy).to receive(:full_host).and_return('http://example.com') - - expect { strategy.call(make_env('/wowzers/test')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/wowzers/test')) expect(strategy.callback_url).to eq('http://example.com/wowzers/test/callback') end @@ -533,7 +563,8 @@ def make_env(path = '/auth/test', props = {}) end it 'allows a request method of the correct type' do - expect { strategy.call(make_env('/auth/test')) }.to raise_error('Request Phase') + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + strategy.call(make_env('/auth/test')) end after(:context) do @@ -579,14 +610,16 @@ def make_env(path = '/auth/test', props = {}) it 'does not affect original options' do @options[:test_option] = true @options[:mutate_on_request] = proc { |options| options.delete(:test_option) } - expect { strategy.call(make_env) }.to raise_error('Request Phase') + + strategy.call(make_env) expect(strategy.options).to have_key(:test_option) end it 'does not affect deep options' do @options[:deep_option] = {:test_option => true} @options[:mutate_on_request] = proc { |options| options[:deep_option].delete(:test_option) } - expect { strategy.call(make_env) }.to raise_error('Request Phase') + + strategy.call(make_env) expect(strategy.options[:deep_option]).to have_key(:test_option) end end @@ -595,14 +628,16 @@ def make_env(path = '/auth/test', props = {}) it 'does not affect original options' do @options[:test_option] = true @options[:mutate_on_callback] = proc { |options| options.delete(:test_option) } - expect { strategy.call(make_env('/auth/test/callback', 'REQUEST_METHOD' => 'POST')) }.to raise_error('Callback Phase') + + strategy.call(make_env('/auth/test/callback', 'REQUEST_METHOD' => 'POST')) expect(strategy.options).to have_key(:test_option) end it 'does not affect deep options' do @options[:deep_option] = {:test_option => true} @options[:mutate_on_callback] = proc { |options| options[:deep_option].delete(:test_option) } - expect { strategy.call(make_env('/auth/test/callback', 'REQUEST_METHOD' => 'POST')) }.to raise_error('Callback Phase') + + strategy.call(make_env('/auth/test/callback', 'REQUEST_METHOD' => 'POST')) expect(strategy.options[:deep_option]).to have_key(:test_option) end end @@ -822,8 +857,10 @@ def make_env(path = '/auth/test', props = {}) let(:escaped_token) { URI.encode_www_form_component(csrf_token, Encoding::UTF_8) } it 'allows a request with matching authenticity_token' do + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + post_env = make_env('/auth/test', 'rack.session' => {:csrf => csrf_token}, 'rack.input' => StringIO.new("authenticity_token=#{escaped_token}")) - expect { strategy.call(post_env) }.to raise_error('Request Phase') + strategy.call(post_env) end it 'does not allow a request without a matching authenticity token' do @@ -840,8 +877,10 @@ def make_env(path = '/auth/test', props = {}) end it 'allows a request without authenticity token' do + strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + get_env = make_env('/auth/test', 'REQUEST_METHOD' => 'GET') - expect { strategy.call(get_env) }.to raise_error('Request Phase') + strategy.call(get_env) end after(:context) do @@ -853,6 +892,17 @@ def make_env(path = '/auth/test', props = {}) OmniAuth.config.request_validation_phase = nil end end + + it 'calls fail! when encountering an unhandled exception' do + strategy.stub(:request_phase).and_raise(Errno::ECONNREFUSED) + strategy.should_receive(:fail!).with('Connection refused', kind_of(Errno::ECONNREFUSED)) + strategy.call(make_env) + end + + it 'redirects to the fail! result when encountering an unhandled exception' do + OmniAuth.config.test_mode = false + expect(strategy.call(make_env).first).to eq 302 + end end context 'setup phase' do From 83d2d306c0b2d70428222887e6034f7779e5d796 Mon Sep 17 00:00:00 2001 From: Josh Jordan Date: Mon, 7 Dec 2020 18:50:16 -0500 Subject: [PATCH 14/30] Match current code standards & practices --- lib/omniauth/strategy.rb | 2 +- spec/omniauth/strategy_spec.rb | 52 +++++++++++++++++----------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index ec481ec90..068de1ee0 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -193,7 +193,7 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity return callback_call if on_callback_path? return other_phase if respond_to?(:other_phase) rescue => ex - return fail! ex.message, ex + return fail!(ex.message, ex) end @app.call(env) diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index f9a7a87eb..8e1a2bc18 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -312,7 +312,7 @@ def make_env(path = '/auth/test', props = {}) context 'disabled' do it 'does not set omniauth.origin' do @options = {:origin_param => false} - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq(nil) @@ -322,7 +322,7 @@ def make_env(path = '/auth/test', props = {}) context 'custom' do it 'sets from a custom param' do @options = {:origin_param => 'return'} - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'return=/foo')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('/foo') @@ -331,21 +331,21 @@ def make_env(path = '/auth/test', props = {}) context 'default flow' do it 'is set on the request phase' do - strategy.should_receive(:fail!).with("Request Phase", kind_of(StandardError)) + expect(strategy).to receive(:fail!).with("Request Phase", kind_of(StandardError)) strategy.call(make_env('/auth/test', 'HTTP_REFERER' => 'http://example.com/origin')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('http://example.com/origin') end it 'is turned into an env variable on the callback phase' do - strategy.should_receive(:fail!).with("Callback Phase", kind_of(StandardError)) + expect(strategy).to receive(:fail!).with("Callback Phase", kind_of(StandardError)) strategy.call(make_env('/auth/test/callback', 'rack.session' => {'omniauth.origin' => 'http://example.com/origin'})) expect(strategy.last_env['omniauth.origin']).to eq('http://example.com/origin') end it 'sets from the params if provided' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test', 'QUERY_STRING' => 'origin=/foo')) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('/foo') end @@ -359,7 +359,7 @@ def make_env(path = '/auth/test', props = {}) context 'with script_name' do it 'is set on the request phase, containing full path' do env = {'HTTP_REFERER' => 'http://example.com/sub_uri/origin', 'SCRIPT_NAME' => '/sub_uri'} - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test', env)) expect(strategy.last_env['rack.session']['omniauth.origin']).to eq('http://example.com/sub_uri/origin') @@ -370,7 +370,7 @@ def make_env(path = '/auth/test', props = {}) 'rack.session' => {'omniauth.origin' => 'http://example.com/sub_uri/origin'}, 'SCRIPT_NAME' => '/sub_uri' } - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test/callback', env)) expect(strategy.last_env['omniauth.origin']).to eq('http://example.com/sub_uri/origin') @@ -381,39 +381,39 @@ def make_env(path = '/auth/test', props = {}) context 'default paths' do it 'uses the default request path' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env) end it 'is case insensitive on request path' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/AUTH/Test')) end it 'is case insensitive on callback path' do - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/AUTH/TeSt/CaLlBAck')) end it 'uses the default callback path' do - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test/callback')) end it 'strips trailing spaces on request' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test/')) end it 'strips trailing spaces on callback' do - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test/callback/')) end context 'callback_url' do it 'uses the default callback_path' do expect(strategy).to receive(:full_host).and_return('http://example.com') - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env) @@ -455,13 +455,13 @@ def make_env(path = '/auth/test', props = {}) context 'dynamic paths' do it 'runs the request phase if the custom request path evaluator is truthy' do @options = {:request_path => lambda { |_env| true }} - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/asoufibasfi')) end it 'runs the callback phase if the custom callback path evaluator is truthy' do @options = {:callback_path => lambda { |_env| true }} - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/asoufiasod')) end @@ -483,14 +483,14 @@ def make_env(path = '/auth/test', props = {}) context 'custom paths' do it 'uses a custom request_path if one is provided' do @options = {:request_path => '/awesome'} - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/awesome')) end it 'uses a custom callback_path if one is provided' do @options = {:callback_path => '/radical'} - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/radical')) end @@ -499,7 +499,7 @@ def make_env(path = '/auth/test', props = {}) it 'uses a custom callback_path if one is provided' do @options = {:callback_path => '/radical'} expect(strategy).to receive(:full_host).and_return('http://example.com') - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/radical')) @@ -524,19 +524,19 @@ def make_env(path = '/auth/test', props = {}) end it 'uses a custom prefix for request' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/wowzers/test')) end it 'uses a custom prefix for callback' do - strategy.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy.call(make_env('/wowzers/test/callback')) end context 'callback_url' do it 'uses a custom prefix' do expect(strategy).to receive(:full_host).and_return('http://example.com') - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/wowzers/test')) expect(strategy.callback_url).to eq('http://example.com/wowzers/test/callback') @@ -563,7 +563,7 @@ def make_env(path = '/auth/test', props = {}) end it 'allows a request method of the correct type' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) strategy.call(make_env('/auth/test')) end @@ -857,7 +857,7 @@ def make_env(path = '/auth/test', props = {}) let(:escaped_token) { URI.encode_www_form_component(csrf_token, Encoding::UTF_8) } it 'allows a request with matching authenticity_token' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) post_env = make_env('/auth/test', 'rack.session' => {:csrf => csrf_token}, 'rack.input' => StringIO.new("authenticity_token=#{escaped_token}")) strategy.call(post_env) @@ -877,7 +877,7 @@ def make_env(path = '/auth/test', props = {}) end it 'allows a request without authenticity token' do - strategy.should_receive(:fail!).with('Request Phase', kind_of(StandardError)) + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) get_env = make_env('/auth/test', 'REQUEST_METHOD' => 'GET') strategy.call(get_env) @@ -895,7 +895,7 @@ def make_env(path = '/auth/test', props = {}) it 'calls fail! when encountering an unhandled exception' do strategy.stub(:request_phase).and_raise(Errno::ECONNREFUSED) - strategy.should_receive(:fail!).with('Connection refused', kind_of(Errno::ECONNREFUSED)) + expect(strategy).to receive(:fail!).with('Connection refused', kind_of(Errno::ECONNREFUSED)) strategy.call(make_env) end From 7f20a03d92298f59e2f570061498a1d188713f22 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Tue, 8 Dec 2020 11:00:16 -0500 Subject: [PATCH 15/30] refactor auth hash method --- lib/omniauth/strategy.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index 5172542cb..763dae142 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -345,11 +345,13 @@ def extra end def auth_hash - hash = AuthHash.new(:provider => name, :uid => uid) - hash.info = info unless skip_info? - (credentials_data = credentials) && hash.credentials = credentials_data - (extra_data = extra) && hash.extra = extra_data - hash + credentials_data = credentials + extra_data = extra + AuthHash.new(:provider => name, :uid => uid).tap do |auth| + auth.info = info unless skip_info? + auth.credentials = credentials_data if credentials_data + auth.extra = extra_data if extra_data + end end # Determines whether or not user info should be retrieved. This From cf6f5e71b33b634de0d73de6c1836aa25dac591b Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Tue, 8 Dec 2020 16:28:21 -0500 Subject: [PATCH 16/30] Specify StandardError and use e instead of ex --- lib/omniauth/strategy.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index 04bec1209..2307542cf 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -192,8 +192,8 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity return request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym) return callback_call if on_callback_path? return other_phase if respond_to?(:other_phase) - rescue => ex - return fail!(ex.message, ex) + rescue StandardError => e + return fail!(e.message, e) end @app.call(env) From d16779e48f3ccc04d774dfd95bb92ef2b3ea331b Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 10:20:25 -0500 Subject: [PATCH 17/30] Test indev branch --- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e70247271..ea56823bb 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -9,9 +9,9 @@ name: Ruby on: push: - branches: [ master ] + branches: [ master, 2_0_indev ] pull_request: - branches: [ master ] + branches: [ master, 2_0_indev ] jobs: test: From 76dd0ee44e658fb30360937e136d550164c65c5e Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 10:21:53 -0500 Subject: [PATCH 18/30] Change method for mutability --- lib/omniauth/form.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/form.rb b/lib/omniauth/form.rb index b838d3f20..617f3c849 100644 --- a/lib/omniauth/form.rb +++ b/lib/omniauth/form.rb @@ -9,7 +9,7 @@ def initialize(options = {}) options[:header_info] ||= '' self.options = options - @html = ''.dup + @html = +'' # unary + string allows it to be mutable if strings are frozen @with_custom_button = false @footer = nil header(options[:title], options[:header_info]) From 9b29b0c56a8b8fa13a4babc8c3675c368f04e132 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 10:35:04 -0500 Subject: [PATCH 19/30] Branch name typo --- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ea56823bb..47ac39859 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -9,9 +9,9 @@ name: Ruby on: push: - branches: [ master, 2_0_indev ] + branches: [ master, 2_0-indev ] pull_request: - branches: [ master, 2_0_indev ] + branches: [ master, 2_0-indev ] jobs: test: From c3e327349ae9f7b9acca49a40e9b83dcafb20b44 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 10:30:10 -0500 Subject: [PATCH 20/30] Add check for frozen string compatability --- .github/workflows/main.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 47ac39859..a127308c5 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -54,6 +54,21 @@ jobs: env: JRUBY_OPTS: --debug run: bundle exec rake + frozen-string-compat: + runs-on: ubuntu-18.04 + steps: + - uses: actions/checkout@v2 + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: 2.5 + bundler-cache: true + - name: Install dependencies + run: bundle install + - name: Run tests + env: + RUBYOPT: "--enable-frozen-string-literal" + run: bundle exec rake coveralls: runs-on: ubuntu-18.04 steps: From 3d9ed07dc9db18fde26f185e1a5e438fcf5ed301 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 10:43:32 -0500 Subject: [PATCH 21/30] Run frozen check on 2.6 --- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a127308c5..b2d277584 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -61,7 +61,7 @@ jobs: - name: Set up Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: 2.5 + ruby-version: 2.6 bundler-cache: true - name: Install dependencies run: bundle install @@ -76,7 +76,7 @@ jobs: - name: Set up Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: 2.5 + ruby-version: 2.6 bundler-cache: true - name: Install dependencies run: bundle install From 97ef26f9550638fcfdf31200c82c4a28d10d85c1 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 11:01:41 -0500 Subject: [PATCH 22/30] Relax rspec dep --- Gemfile | 2 +- spec/omniauth/form_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 7c990ebde..b0581c016 100644 --- a/Gemfile +++ b/Gemfile @@ -19,7 +19,7 @@ group :test do gem 'rack', '>= 2.0.6', :platforms => %i[jruby_18 jruby_19 ruby_19 ruby_20 ruby_21] gem 'rack-test' gem 'rest-client', '~> 2.0.0', :platforms => [:jruby_18] - gem 'rspec', '~> 3.5.0' + gem 'rspec', '~> 3.5' gem 'rubocop', '>= 0.58.2', '< 0.69.0', :platforms => %i[ruby_20 ruby_21 ruby_22 ruby_23 ruby_24] gem 'simplecov-lcov' gem 'tins', '~> 1.13', :platforms => %i[jruby_18 jruby_19 ruby_19] diff --git a/spec/omniauth/form_spec.rb b/spec/omniauth/form_spec.rb index 1b9c88c70..1354a16c7 100644 --- a/spec/omniauth/form_spec.rb +++ b/spec/omniauth/form_spec.rb @@ -33,7 +33,7 @@ describe '#html' do it 'appends to the html body' do - form = OmniAuth::Form.build { @html = '

' } + form = OmniAuth::Form.build { @html = +'

' } form.html('

') expect(form.instance_variable_get(:@html)).to eq '

' From c606a00798a7f516c37c67a708864f9a3eb9f796 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Fri, 19 Feb 2016 13:01:25 +0100 Subject: [PATCH 23/30] Deal with relative url root installations This commit allows OmniAuth to handle relative url root installations by correctly prefixing the default `Strategy#request_path` and `Strategy#callback_path` with `SCRIPT_NAME` as provided by Rack. Also, in order to detect the full current path, uses `Rack::Request#path` instead of `#path_info`, as the latter does not include SCRIPT_NAME. A deeper question remains: What should happen with custom callback and request paths? I can't think of a case where a user with a relative url root installation would *not* want to prefix its paths, regardless of setting a custom path or not. However, since some people may have configured custom paths in order to deal with relative root, including this change may be breaking. --- lib/omniauth/strategy.rb | 13 ++++++++---- spec/omniauth/strategy_spec.rb | 37 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index 340a0da74..a74ca3e27 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -421,7 +421,12 @@ def custom_path(kind) end def request_path - @request_path ||= options[:request_path].is_a?(String) ? options[:request_path] : "#{path_prefix}/#{name}" + @request_path ||= + if options[:request_path].is_a?(String) + options[:request_path] + else + "#{script_name}#{path_prefix}/#{name}" + end end def callback_path @@ -429,7 +434,7 @@ def callback_path path = options[:callback_path] if options[:callback_path].is_a?(String) path ||= current_path if options[:callback_path].respond_to?(:call) && options[:callback_path].call(env) path ||= custom_path(:request_path) - path ||= "#{path_prefix}/#{name}/callback" + path ||= "#{script_name}#{path_prefix}/#{name}/callback" path end end @@ -441,7 +446,7 @@ def setup_path CURRENT_PATH_REGEX = %r{/$}.freeze EMPTY_STRING = ''.freeze def current_path - @current_path ||= request.path_info.downcase.sub(CURRENT_PATH_REGEX, EMPTY_STRING) + @current_path ||= request.path.downcase.sub(CURRENT_PATH_REGEX, EMPTY_STRING) end def query_string @@ -473,7 +478,7 @@ def full_host end def callback_url - full_host + script_name + callback_path + query_string + full_host + callback_path + query_string end def script_name diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index 968d57527..16aa0e609 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -588,6 +588,43 @@ def make_env(path = '/auth/test', props = {}) end end + context 'with relative url root' do + let(:props) { { 'SCRIPT_NAME' => '/myapp' } } + it 'accepts the request' do + expect { strategy.call(make_env('/auth/test', props)) }.to raise_error('Request Phase') + expect(strategy.request_path).to eq('/myapp/auth/test') + end + + it 'accepts the callback' do + expect { strategy.call(make_env('/auth/test/callback', props)) }.to raise_error('Callback Phase') + end + + context 'callback_url' do + it 'redirects to the correctly prefixed callback' do + expect(strategy).to receive(:full_host).and_return('http://example.com') + + expect { strategy.call(make_env('/auth/test', props)) }.to raise_error('Request Phase') + + expect(strategy.callback_url).to eq('http://example.com/myapp/auth/test/callback') + end + end + + context 'custom request' do + before do + @options = {:request_path => '/myapp/override', :callback_path => '/myapp/override/callback'} + end + it 'does not prefix a custom request path' do + expect(strategy).to receive(:full_host).and_return('http://example.com') + + expect(strategy.request_path).to eq('/myapp/override') + expect { strategy.call(make_env('/override', props)) }.to raise_error('Request Phase') + + expect(strategy.callback_url).to eq('http://example.com/myapp/override/callback') + end + end + end + + context 'request method restriction' do before(:context) do OmniAuth.config.allowed_request_methods = %i[put post] From f9a6c0fc8a457695a86100e2caae2a1f7a4213da Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 12:23:40 -0500 Subject: [PATCH 24/30] Fix failing specs --- spec/omniauth/strategy_spec.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index 16aa0e609..12b341a28 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -589,21 +589,26 @@ def make_env(path = '/auth/test', props = {}) end context 'with relative url root' do - let(:props) { { 'SCRIPT_NAME' => '/myapp' } } + let(:props) { {'SCRIPT_NAME' => '/myapp'} } it 'accepts the request' do - expect { strategy.call(make_env('/auth/test', props)) }.to raise_error('Request Phase') + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) + + strategy.call(make_env('/auth/test', props)) expect(strategy.request_path).to eq('/myapp/auth/test') end it 'accepts the callback' do - expect { strategy.call(make_env('/auth/test/callback', props)) }.to raise_error('Callback Phase') + expect(strategy).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) + + strategy.call(make_env('/auth/test/callback', props)) end context 'callback_url' do it 'redirects to the correctly prefixed callback' do expect(strategy).to receive(:full_host).and_return('http://example.com') + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) - expect { strategy.call(make_env('/auth/test', props)) }.to raise_error('Request Phase') + strategy.call(make_env('/auth/test', props)) expect(strategy.callback_url).to eq('http://example.com/myapp/auth/test/callback') end @@ -615,9 +620,11 @@ def make_env(path = '/auth/test', props = {}) end it 'does not prefix a custom request path' do expect(strategy).to receive(:full_host).and_return('http://example.com') + expect(strategy).to receive(:fail!).with('Request Phase', kind_of(StandardError)) expect(strategy.request_path).to eq('/myapp/override') - expect { strategy.call(make_env('/override', props)) }.to raise_error('Request Phase') + + strategy.call(make_env('/override', props)) expect(strategy.callback_url).to eq('http://example.com/myapp/override/callback') end From d4c1ff0ffb0586f99490a1c3a427cfb40657cec9 Mon Sep 17 00:00:00 2001 From: Cyrille Courtiere Date: Thu, 21 Nov 2019 11:54:26 +0100 Subject: [PATCH 25/30] Dup options when a strategy is dup'd or cloned This allows strategies to safely mutate their options without affecting subsequent or concurrent requests. --- lib/omniauth/strategy.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index a74ca3e27..d94f1e98b 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -528,16 +528,15 @@ def fail!(message_key, exception = nil) OmniAuth.config.on_failure.call(env) end - def dup - super.tap do - @options = @options.dup - end - end - class Options < OmniAuth::KeyStore; end protected + def initialize_copy(*args) + super + @options = @options.dup + end + def merge_stack(stack) stack.inject({}) do |a, e| a.merge!(e) From e405613685394932bba0d1ffa8bb8fc484e4279c Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 13:04:28 -0500 Subject: [PATCH 26/30] Freeze omniauth in test to verify thread safety --- Gemfile | 1 + spec/helper.rb | 1 + spec/omniauth/builder_spec.rb | 9 +++------ 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index b0581c016..9f08f3a34 100644 --- a/Gemfile +++ b/Gemfile @@ -20,6 +20,7 @@ group :test do gem 'rack-test' gem 'rest-client', '~> 2.0.0', :platforms => [:jruby_18] gem 'rspec', '~> 3.5' + gem 'rack-freeze' gem 'rubocop', '>= 0.58.2', '< 0.69.0', :platforms => %i[ruby_20 ruby_21 ruby_22 ruby_23 ruby_24] gem 'simplecov-lcov' gem 'tins', '~> 1.13', :platforms => %i[jruby_18 jruby_19 ruby_19] diff --git a/spec/helper.rb b/spec/helper.rb index bca865cd9..65839922d 100644 --- a/spec/helper.rb +++ b/spec/helper.rb @@ -20,6 +20,7 @@ require 'rspec' require 'rack/test' +require 'rack/freeze' require 'omniauth' require 'omniauth/test' diff --git a/spec/omniauth/builder_spec.rb b/spec/omniauth/builder_spec.rb index d2dd3a9d4..da6a2e49b 100644 --- a/spec/omniauth/builder_spec.rb +++ b/spec/omniauth/builder_spec.rb @@ -121,14 +121,11 @@ class MyStrategy; end describe '#call' do it 'passes env to to_app.call' do - app = lambda { |_env| [200, {}, []] } + app = lambda { |env| [200, {}, env['CUSTOM_ENV_VALUE']] } builder = OmniAuth::Builder.new(app) - env = {'REQUEST_METHOD' => 'GET', 'PATH_INFO' => '/some/path'} - allow(app).to receive(:call).and_call_original + env = {'REQUEST_METHOD' => 'GET', 'PATH_INFO' => '/some/path', 'CUSTOM_ENV_VALUE' => 'VALUE'} - builder.call(env) - - expect(app).to have_received(:call).with(env) + expect(builder.call(env)).to eq([200, {}, 'VALUE']) end end From 1b784ffa5f128bea1a22d7d26477f73bb6b3cd08 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 23:07:38 -0500 Subject: [PATCH 27/30] Wrap mock_call in rescue --- lib/omniauth/strategy.rb | 9 +++++++-- spec/omniauth/strategy_spec.rb | 6 ++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategy.rb b/lib/omniauth/strategy.rb index d94f1e98b..d8468bcbc 100644 --- a/lib/omniauth/strategy.rb +++ b/lib/omniauth/strategy.rb @@ -298,8 +298,13 @@ def options_request? # in the event that OmniAuth has been configured to be # in test mode. def mock_call!(*) - return mock_request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym) - return mock_callback_call if on_callback_path? + begin + OmniAuth.config.request_validation_phase.call(env) if OmniAuth.config.request_validation_phase + return mock_request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym) + return mock_callback_call if on_callback_path? + rescue StandardError => e + return fail!(e.message, e) + end call_app! end diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index 12b341a28..4fed39bc8 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -885,6 +885,12 @@ def make_env(path = '/auth/test', props = {}) expect(strategy.env['omniauth.params']).to eq('foo' => 'bar') end + it 'rescues errors in request_call' do + allow(strategy).to receive(:mock_request_call).and_raise(StandardError.new('Oh no')) + expect(strategy).to receive(:fail!).with('Oh no', kind_of(StandardError)) + strategy.call(make_env) + end + after do OmniAuth.config.test_mode = false end From 1956a95e6466f0bcefbe2cd4e444a14dad60a7b4 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Thu, 10 Dec 2020 23:07:47 -0500 Subject: [PATCH 28/30] Fix deprecation warnings --- spec/omniauth/strategy_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/omniauth/strategy_spec.rb b/spec/omniauth/strategy_spec.rb index 4fed39bc8..b4a86dace 100644 --- a/spec/omniauth/strategy_spec.rb +++ b/spec/omniauth/strategy_spec.rb @@ -508,7 +508,7 @@ def make_env(path = '/auth/test', props = {}) it 'correctly reports the callback path when the custom callback path evaluator is truthy' do strategy_instance = ExampleStrategy.new(app, :callback_path => lambda { |env| env['PATH_INFO'] == '/auth/bish/bosh/callback' }) - strategy_instance.should_receive(:fail!).with('Callback Phase', kind_of(StandardError)) + expect(strategy_instance).to receive(:fail!).with('Callback Phase', kind_of(StandardError)) strategy_instance.call(make_env('/auth/bish/bosh/callback')) expect(strategy_instance.callback_path).to eq('/auth/bish/bosh/callback') @@ -979,7 +979,7 @@ def make_env(path = '/auth/test', props = {}) end it 'calls fail! when encountering an unhandled exception' do - strategy.stub(:request_phase).and_raise(Errno::ECONNREFUSED) + allow(strategy).to receive(:request_phase).and_raise(Errno::ECONNREFUSED) expect(strategy).to receive(:fail!).with('Connection refused', kind_of(Errno::ECONNREFUSED)) strategy.call(make_env) end From 97714aa6a5da8e3b7e76e52cccd82110ab204adf Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Fri, 11 Dec 2020 01:32:20 -0500 Subject: [PATCH 29/30] Tag version --- lib/omniauth/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/version.rb b/lib/omniauth/version.rb index ea777f2f1..c2b5fe54d 100644 --- a/lib/omniauth/version.rb +++ b/lib/omniauth/version.rb @@ -1,3 +1,3 @@ module OmniAuth - VERSION = '1.9.1'.freeze + VERSION = '2.0.0-rc1'.freeze end From fe26931f2e7934e0800dd3fe646bef4a1ad2e192 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Mon, 11 Jan 2021 14:36:50 -0500 Subject: [PATCH 30/30] Release 2.0.0 --- lib/omniauth/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/version.rb b/lib/omniauth/version.rb index c2b5fe54d..957ed63f4 100644 --- a/lib/omniauth/version.rb +++ b/lib/omniauth/version.rb @@ -1,3 +1,3 @@ module OmniAuth - VERSION = '2.0.0-rc1'.freeze + VERSION = '2.0.0'.freeze end