From 61142c7cf65f39ae44d8cd15bcffecaaa042c873 Mon Sep 17 00:00:00 2001
From: Piotr Sarnacki <drogus@gmail.com>
Date: Sat, 8 Dec 2012 17:19:09 +0100
Subject: [PATCH] Return 404 unless we can find API builder for resource

In order to protect us from rendering a resource simply converted to
json, without processing it with API data class, this commit changes
JSON responder behavior to render 404 if we can't find associated data
class. The only exception to that rule is when resource is already a
Hash, meaning that it was processed before - we sometimes return for
example simple Hash responses like { result: true }.

The Hash exception could allow to accidentally pass resource.as_json to
responder, but in travis-ci/travis-support@124b8b6 I disabled default
as_json method on AR::Base classes, so the risk of such mistake is
lowered.
---
 lib/travis/api/app/responders/json.rb | 14 +++++--
 spec/unit/responders/json_spec.rb     | 54 +++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100644 spec/unit/responders/json_spec.rb

diff --git a/lib/travis/api/app/responders/json.rb b/lib/travis/api/app/responders/json.rb
index 464be510..13065d0e 100644
--- a/lib/travis/api/app/responders/json.rb
+++ b/lib/travis/api/app/responders/json.rb
@@ -8,7 +8,7 @@ class Travis::Api::App
       end
 
       def apply
-        halt result.to_json
+        halt result.to_json if result
       end
 
       private
@@ -21,11 +21,15 @@ class Travis::Api::App
         end
 
         def result
-          builder ? builder.new(resource, params).data : resource
+          builder ? builder.new(resource, params).data : basic_type_resource
         end
 
         def builder
-          @builder ||= Travis::Api.builder(resource, { :version => version }.merge(options))
+          if defined?(@builder)
+           @builder
+          else
+           @builder = Travis::Api.builder(resource, { :version => version }.merge(options))
+          end
         end
 
         def accept_params
@@ -39,6 +43,10 @@ class Travis::Api::App
         def params
           (request.params || {}).merge(accept_params)
         end
+
+        def basic_type_resource
+          resource if resource.is_a?(Hash)
+        end
     end
   end
 end
diff --git a/spec/unit/responders/json_spec.rb b/spec/unit/responders/json_spec.rb
new file mode 100644
index 00000000..eb2ffa33
--- /dev/null
+++ b/spec/unit/responders/json_spec.rb
@@ -0,0 +1,54 @@
+require 'spec_helper'
+
+module Travis::Api::App::Responders
+  describe Json do
+    class MyJson < Json
+    end
+
+    let(:request)  { stub 'request', params: {} }
+    let(:endpoint) { stub 'endpoint', request: request }
+    let(:resource) { stub 'resource' }
+    let(:accept)   { stub 'accept entry', version: '2', params: {} }
+    let(:options)  { { :accept => accept} }
+    let(:json)  { MyJson.new(endpoint, resource, options) }
+
+    context 'with resource not associated with Api data class' do
+      it 'returns nil result' do
+        json.apply.should be_false
+      end
+    end
+
+    context 'with resource being' do
+      context 'a Hash instance' do
+        let(:resource) { { foo: 'bar' } }
+
+        it 'returns resource converted to_json' do
+          json.expects(:halt).with({ foo: 'bar' }.to_json)
+          json.apply
+        end
+      end
+
+      context 'nil' do
+        let(:resource) { nil }
+
+        it 'responds with 404' do
+          json.apply?.should be_false
+          json.apply.should be_false
+        end
+      end
+    end
+
+    context 'with resource associated with Api data class' do
+      let(:builder)       { stub 'builder', data: { foo: 'bar' } }
+      let(:builder_class) { stub 'builder class', new: builder }
+      before do
+        json.stubs :builder => builder_class
+      end
+
+      it 'returns proper data converted to json' do
+        json.expects(:halt).with({ foo: 'bar' }.to_json)
+        json.apply
+      end
+    end
+  end
+end