Merge pull request #483 from stripe/brandur-disallow-protected-field-set

Don't allow protected fields in `Save.update` API operation

lib/stripe/account.rb

@@ -79,7 +79,7 @@ module Stripe
       update_hash
     end
 
-    def protected_fields
+    def self.protected_fields
       [:legal_entity]
     end
 

lib/stripe/api_operations/save.rb

@@ -15,6 +15,12 @@ module Stripe
         #   idempotency_key to be passed in the request headers, or for the
         #   api_key to be overwritten. See {APIOperations::Request.request}.
         def update(id, params={}, opts={})
+          params.each do |k, v|
+            if self.protected_fields.include?(k)
+              raise ArgumentError, "Cannot update protected field: #{k}"
+            end
+          end
+
           response, opts = request(:post, "#{resource_url}/#{id}", params, opts)
           Util.convert_to_stripe_object(response, opts)
         end

lib/stripe/stripe_object.rb

@@ -194,19 +194,24 @@ module Stripe
 
     protected
 
+    # A protected field is one that doesn't get an accessor assigned to it
+    # (i.e. `obj.public = ...`) and one which is not allowed to be updated via
+    # the class level `Model.update(id, { ... })`.
+    def self.protected_fields
+      []
+    end
+
     def metaclass
       class << self; self; end
     end
 
-    def protected_fields
-      []
-    end
-
     def remove_accessors(keys)
-      f = protected_fields
+      # not available in the #instance_eval below
+      protected_fields = self.class.protected_fields
+
       metaclass.instance_eval do
         keys.each do |k|
-          next if f.include?(k)
+          next if protected_fields.include?(k)
           next if @@permanent_attributes.include?(k)
 
           # Remove methods for the accessor's reader and writer.
@@ -220,10 +225,12 @@ module Stripe
     end
 
     def add_accessors(keys, values)
-      f = protected_fields
+      # not available in the #instance_eval below
+      protected_fields = self.class.protected_fields
+
       metaclass.instance_eval do
         keys.each do |k|
-          next if f.include?(k)
+          next if protected_fields.include?(k)
           next if @@permanent_attributes.include?(k)
 
           define_method(k) { @values[k] }

test/stripe/account_test.rb

@@ -96,26 +96,15 @@ module Stripe
     should "be updatable" do
       resp = {
         :id => 'acct_foo',
-        :legal_entity => {
-          :first_name => 'Bob',
-          :address => {
-            :line1 => '2 Three Four'
-          }
-        }
+        :business_name => 'ACME Corp',
       }
       @mock.expects(:post).
         once.
-        with('https://api.stripe.com/v1/accounts/acct_foo', nil, 'legal_entity[first_name]=Bob&legal_entity[address][line1]=2+Three+Four').
+        with('https://api.stripe.com/v1/accounts/acct_foo', nil, 'business_name=ACME+Corp').
         returns(make_response(resp))
 
-      a = Stripe::Account.update('acct_foo', :legal_entity => {
-        :first_name => 'Bob',
-        :address => {
-          :line1 => '2 Three Four'
-        }
-      })
-      assert_equal('Bob', a.legal_entity.first_name)
-      assert_equal('2 Three Four', a.legal_entity.address.line1)
+      a = Stripe::Account.update('acct_foo', :business_name => "ACME Corp")
+      assert_equal('ACME Corp', a.business_name)
     end
 
     should 'disallow direct overrides of legal_entity' do

test/stripe/api_operations_test.rb

@@ -3,18 +3,29 @@ require File.expand_path('../../test_helper', __FILE__)
 
 module Stripe
   class ApiOperationsTest < Test::Unit::TestCase
-    class Updater < APIResource
+    class UpdateableResource < APIResource
       include Stripe::APIOperations::Save
+
+      def self.protected_fields
+        [:protected]
+      end
     end
 
-    should "the Update API operation should post the correct parameters to the resource URL" do
-      @mock.expects(:post).once.
-        with("#{Stripe.api_base}/v1/updaters/id", nil, 'foo=bar').
-        returns(make_response({foo: 'bar'}))
-      resource = Updater::update("id", {foo: "bar"})
-      assert_equal('bar', resource.foo)
+    context ".update" do
+      should "post the correct parameters to the resource URL" do
+        @mock.expects(:post).once.
+          with("#{Stripe.api_base}/v1/updateableresources/id", nil, 'foo=bar').
+          returns(make_response({foo: 'bar'}))
+        resource = UpdateableResource::update("id", { foo: "bar" })
+        assert_equal('bar', resource.foo)
+      end
+
+      should "error on protected fields" do
+        e = assert_raises do
+          UpdateableResource::update("id", { protected: "bar" })
+        end
+        assert_equal "Cannot update protected field: protected", e.message
+      end
     end
   end
 end
-
-