Fix for OAuth2::Error trying to encode non-encodable objects

.rubocop_rspec.yml

@@ -21,3 +21,6 @@ RSpec/InstanceVariable:
 
 RSpec/NestedGroups:
   Enabled: false
+
+RSpec/ExpectInHook:
+  Enabled: false

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

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 => '../'

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

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

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'

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 => '../'

gemfiles/ruby_2.0.gemfile

@@ -9,6 +9,7 @@ end
 group :test do
   gem 'rake'
   gem 'rspec'
+  gem 'rspec-pending_for'
 end
 
 gemspec :path => '../'

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

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

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

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

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

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

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

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) { '<!DOCTYPE html><html><head>Hello, I am a teapot</head><body></body></html>' }
+
+    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

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