diff --git a/.rubocop_rspec.yml b/.rubocop_rspec.yml index 4e366ed4..347777dc 100644 --- a/.rubocop_rspec.yml +++ b/.rubocop_rspec.yml @@ -21,3 +21,6 @@ RSpec/InstanceVariable: RSpec/NestedGroups: Enabled: false + +RSpec/ExpectInHook: + Enabled: false diff --git a/Gemfile b/Gemfile index 91bbbf40..8a6e97d0 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ group :test do gem 'rubocop-rspec', '~> 1.24.0' end gem 'pry', '~> 0.11' if ruby_version >= Gem::Version.new('2.0') + gem 'rspec-pending_for' end # Specify non-special dependencies in oauth2.gemspec diff --git a/gemfiles/jruby_1.7.gemfile b/gemfiles/jruby_1.7.gemfile index e3f9afd5..f31532b0 100644 --- a/gemfiles/jruby_1.7.gemfile +++ b/gemfiles/jruby_1.7.gemfile @@ -11,6 +11,7 @@ group :test do # gem "rubocop", "~> 0.41.2" gem 'rake' gem 'rspec' + gem 'rspec-pending_for' end gemspec :path => '../' diff --git a/gemfiles/jruby_9.0.gemfile b/gemfiles/jruby_9.0.gemfile index e924355b..9cd4d582 100644 --- a/gemfiles/jruby_9.0.gemfile +++ b/gemfiles/jruby_9.0.gemfile @@ -3,6 +3,7 @@ source 'https://rubygems.org' group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/jruby_9.1.gemfile b/gemfiles/jruby_9.1.gemfile index 920dabea..60080c62 100644 --- a/gemfiles/jruby_9.1.gemfile +++ b/gemfiles/jruby_9.1.gemfile @@ -7,6 +7,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/jruby_head.gemfile b/gemfiles/jruby_head.gemfile index 671290d6..64436169 100644 --- a/gemfiles/jruby_head.gemfile +++ b/gemfiles/jruby_head.gemfile @@ -9,6 +9,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' # rubocop:disable Bundler/DuplicatedGem gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' diff --git a/gemfiles/ruby_1.9.gemfile b/gemfiles/ruby_1.9.gemfile index 49c0a414..369c14a6 100644 --- a/gemfiles/ruby_1.9.gemfile +++ b/gemfiles/ruby_1.9.gemfile @@ -8,6 +8,7 @@ gem 'tins', '< 1.7' group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' end gemspec :path => '../' diff --git a/gemfiles/ruby_2.0.gemfile b/gemfiles/ruby_2.0.gemfile index 6244eb2f..8fba443a 100644 --- a/gemfiles/ruby_2.0.gemfile +++ b/gemfiles/ruby_2.0.gemfile @@ -9,6 +9,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' end gemspec :path => '../' diff --git a/gemfiles/ruby_2.1.gemfile b/gemfiles/ruby_2.1.gemfile index 20f56b70..41632192 100644 --- a/gemfiles/ruby_2.1.gemfile +++ b/gemfiles/ruby_2.1.gemfile @@ -9,6 +9,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/ruby_2.2.gemfile b/gemfiles/ruby_2.2.gemfile index 920dabea..60080c62 100644 --- a/gemfiles/ruby_2.2.gemfile +++ b/gemfiles/ruby_2.2.gemfile @@ -7,6 +7,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/ruby_2.3.gemfile b/gemfiles/ruby_2.3.gemfile index 920dabea..60080c62 100644 --- a/gemfiles/ruby_2.3.gemfile +++ b/gemfiles/ruby_2.3.gemfile @@ -7,6 +7,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/ruby_2.4.gemfile b/gemfiles/ruby_2.4.gemfile index 920dabea..60080c62 100644 --- a/gemfiles/ruby_2.4.gemfile +++ b/gemfiles/ruby_2.4.gemfile @@ -7,6 +7,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/ruby_2.5.gemfile b/gemfiles/ruby_2.5.gemfile index 920dabea..60080c62 100644 --- a/gemfiles/ruby_2.5.gemfile +++ b/gemfiles/ruby_2.5.gemfile @@ -7,6 +7,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/gemfiles/ruby_head.gemfile b/gemfiles/ruby_head.gemfile index 920dabea..60080c62 100644 --- a/gemfiles/ruby_head.gemfile +++ b/gemfiles/ruby_head.gemfile @@ -7,6 +7,7 @@ end group :test do gem 'rake' gem 'rspec' + gem 'rspec-pending_for' gem 'rubocop', '~> 0.53.0' gem 'rubocop-rspec', '~> 1.24.0' end diff --git a/lib/oauth2/error.rb b/lib/oauth2/error.rb index b77355f3..2eab7dfd 100644 --- a/lib/oauth2/error.rb +++ b/lib/oauth2/error.rb @@ -7,34 +7,44 @@ class Error < StandardError def initialize(response) response.error = self @response = response + message_opts = {} if response.parsed.is_a?(Hash) @code = response.parsed['error'] @description = response.parsed['error_description'] - error_description = "#{@code}: #{@description}" + message_opts = parse_error_description(@code, @description) end - super(error_message(response.body, :error_description => error_description)) + super(error_message(response.body, message_opts)) end - # Makes a error message - # @param [String] response_body response body of request - # @param [String] opts :error_description error description to show first line + private + def error_message(response_body, opts = {}) - message = [] + lines = [] + + lines << opts[:error_description] if opts[:error_description] + + error_string = if response_body.respond_to?(:encode) && opts[:error_description].respond_to?(:encoding) + script_encoding = opts[:error_description].encoding + response_body.encode(script_encoding, :invalid => :replace, :undef => :replace) + else + response_body + end - opts[:error_description] && message << opts[:error_description] + lines << error_string + + lines.join("\n") + end - error_message = if opts[:error_description] && opts[:error_description].respond_to?(:encoding) - script_encoding = opts[:error_description].encoding - response_body.encode(script_encoding, :invalid => :replace, :undef => :replace) - else - response_body - end + def parse_error_description(code, description) + return {} unless code || description - message << error_message + error_description = '' + error_description << "#{code}: " if code + error_description << description if description - message.join("\n") + {:error_description => error_description} end end end diff --git a/spec/oauth2/error_spec.rb b/spec/oauth2/error_spec.rb new file mode 100644 index 00000000..85122b6f --- /dev/null +++ b/spec/oauth2/error_spec.rb @@ -0,0 +1,160 @@ +# encoding: UTF-8 + +RSpec.describe OAuth2::Error do + let(:subject) { described_class.new(response) } + let(:response) do + fake_response = double( + 'response', + :status => 418, + :headers => response_headers, + :body => response_body + ) + + OAuth2::Response.new(fake_response) + end + + let(:response_headers) { {'Content-Type' => 'application/json'} } + let(:response_body) { {:text => 'Coffee brewing failed'}.to_json } + + it 'sets self to #error on the response object' do + expect(response.error).to be_nil + error = described_class.new(response) + expect(response.error).to equal(error) + end + + it 'sets the response object to #response on self' do + error = described_class.new(response) + expect(error.response).to equal(response) + end + + describe 'attr_readers' do + it 'has code' do + expect(subject).to respond_to(:code) + end + + it 'has description' do + expect(subject).to respond_to(:description) + end + + it 'has response' do + expect(subject).to respond_to(:response) + end + end + + context 'when the response is parseable as a hash' do + let(:response_body) { response_hash.to_json } + let(:response_hash) { {:text => 'Coffee brewing failed'} } + + context 'when the response has an error and error_description' do + before do + response_hash[:error_description] = 'Short and stout' + response_hash[:error] = 'i_am_a_teapot' + end + + it 'prepends to the error message with a return character' do + expect(subject.message.each_line.to_a).to eq( + [ + 'i_am_a_teapot: Short and stout' + "\n", + '{"text":"Coffee brewing failed","error_description":"Short and stout","error":"i_am_a_teapot"}', + ] + ) + end + + context 'when the response needs to be encoded' do + let(:response_body) { MultiJson.encode(response_hash).force_encoding('ASCII-8BIT') } + + context 'with invalid characters present' do + before do + response_body.gsub!('stout', "\255 invalid \255") + end + + it 'replaces them' do + # The skip can be removed once support for < 2.1 is dropped. + encoding = {:reason => 'encode/scrub only works as of Ruby 2.1'} + skip_for(encoding.merge(:engine => 'ruby', :versions => %w[1.8.7 1.9.3 2.0.0])) + skip_for(encoding.merge(:engine => 'jruby')) + # See https://bibwild.wordpress.com/2013/03/12/removing-illegal-bytes-for-encoding-in-ruby-1-9-strings/ + + raise 'Invalid characters not replaced' unless subject.message.include?('� invalid �') + # This will fail if {:invalid => replace} is not passed into `encode` + end + end + + context 'with undefined characters present' do + before do + response_hash[:error_description] << ": 'A magical voyage of tea 🍵'" + end + + it 'replaces them' do + raise 'Undefined characters not replaced' unless subject.message.include?('tea �') + # This will fail if {:undef => replace} is not passed into `encode` + end + end + end + + context 'when the response is not an encodable thing' do + let(:response_headers) { {'Content-Type' => 'who knows'} } + let(:response_body) { {:text => 'Coffee brewing failed'} } + + before do + expect(response_body).not_to respond_to(:encode) + # i.e. a Ruby hash + end + + it 'does not try to encode the message string' do + expect(subject.message).to eq(response_body.to_s) + end + end + + it 'sets the code attribute' do + expect(subject.code).to eq('i_am_a_teapot') + end + + it 'sets the description attribute' do + expect(subject.description).to eq('Short and stout') + end + end + + context 'when there is no error description' do + before do + expect(response_hash).not_to have_key(:error) + expect(response_hash).not_to have_key(:error_description) + end + + it 'does not prepend anything to the message' do + expect(subject.message.lines.count).to eq(1) + expect(subject.message).to eq '{"text":"Coffee brewing failed"}' + end + + it 'does not set code' do + expect(subject.code).to be_nil + end + + it 'does not set description' do + expect(subject.description).to be_nil + end + end + end + + context 'when the response does not parse to a hash' do + let(:response_headers) { {'Content-Type' => 'text/html'} } + let(:response_body) { 'Hello, I am a teapot' } + + before do + expect(response.parsed).not_to be_a(Hash) + end + + it 'does not do anything to the message' do + expect(subject.message.lines.count).to eq(1) + expect(subject.message).to eq(response_body) + end + + it 'does not set code' do + expect(subject.code).to be_nil + end + + it 'does not set description' do + expect(subject.description).to be_nil + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 70f64474..0618ddf9 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,6 +1,7 @@ require 'bundler/setup' require 'oauth2' require 'helper' +require 'rspec/pending_for' RSpec.configure do |config| # Enable flags like --only-failures and --next-failure