From fb61bedcbc49d54708410ed394da133440a9d78f Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <ldipentima@veritasgenetics.com>
Date: Sat, 29 Sep 2018 10:20:24 -0300
Subject: [PATCH] 13561: Avoid old versions to be updated, with test.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima@veritasgenetics.com>
---
 services/api/app/models/collection.rb     | 14 ++++++++-
 services/api/test/unit/collection_test.rb | 36 +++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 61946f82b0..f051fb3445 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -27,6 +27,7 @@ class Collection < ArvadosModel
   validate :ensure_pdh_matches_manifest_text
   validate :ensure_storage_classes_desired_is_not_empty
   validate :ensure_storage_classes_contain_non_empty_strings
+  validate :old_versions_cannot_be_updated, on: :update
   before_save :set_file_names
   before_create :set_current_version_uuid
 
@@ -252,7 +253,10 @@ class Collection < ArvadosModel
           end
         end
         Collection.where('current_version_uuid = ? AND uuid != ?', uuid, uuid).each do |c|
-          c.update_attributes!(updates)
+          c.attributes = updates
+          # Use a different validation context to skip the 'old_versions_cannot_be_updated'
+          # validator, as on this case it is legal to update some fields.
+          c.save(context: :update_old_versions)
         end
         # Also update current object just in case a new version will be created,
         # as it has to receive the same values for the synced attributes.
@@ -585,4 +589,12 @@ class Collection < ArvadosModel
       end
     end
   end
+
+  def old_versions_cannot_be_updated
+    # We check for the '_was' values just in case the update operation
+    # includes a change on current_version_uuid or uuid.
+    if current_version_uuid_was != uuid_was
+      raise ArvadosModel::PermissionDeniedError.new("previous versions cannot be updated")
+    end
+  end
 end
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index e3c2a999cb..31c76efa3e 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -128,6 +128,42 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  test "older versions should no be directly updatable" do
+    Rails.configuration.collection_versioning = true
+    act_as_system_user do
+      # Set up initial collection
+      c = create_collection 'foo', Encoding::US_ASCII
+      assert c.valid?
+      # Make changes so that a new version is created
+      c.update_attributes!({'name' => 'bar'})
+      c.reload
+      assert_equal 2, c.version
+      # Get the old version
+      c_old = Collection.where(current_version_uuid: c.uuid, version: 1).first
+      assert_not_nil c_old
+      # With collection versioning still being enabled, try to update
+      assert_raises ArvadosModel::PermissionDeniedError do
+        c_old.update_attributes(name: 'this was foo')
+      end
+      c_old.reload
+      assert_equal 'foo', c_old.name
+      # Try to fool the validator attempting to make c_old to look like a
+      # current version, it should also fail.
+      assert_raises ArvadosModel::PermissionDeniedError do
+        c_old.update_attributes(current_version_uuid: c_old.uuid)
+      end
+      c_old.reload
+      assert_equal c.uuid, c_old.current_version_uuid
+      # Now disable collection versioning, it should behave the same way
+      Rails.configuration.collection_versioning = false
+      assert_raises ArvadosModel::PermissionDeniedError do
+        c_old.update_attributes(name: 'this was foo')
+      end
+      c_old.reload
+      assert_equal 'foo', c_old.name
+    end
+  end
+
   [
     ['owner_uuid', 'zzzzz-tpzed-d9tiejq69daie8f', 'zzzzz-tpzed-xurymjxw79nv3jz'],
     ['replication_desired', 2, 3],
-- 
2.30.2