10181: Permit dispatcher to update log while container is running.
authorTom Clegg <tclegg@veritasgenetics.com>
Thu, 20 Sep 2018 17:52:47 +0000 (13:52 -0400)
committerTom Clegg <tclegg@veritasgenetics.com>
Fri, 21 Sep 2018 19:20:51 +0000 (15:20 -0400)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

doc/api/methods/containers.html.textile.liquid
services/api/app/models/container.rb
services/api/test/unit/container_test.rb

index 9ebd91d2b185821701dcb2df1e1011b79dac5cd9..e073d1c6ad8ce0de92c4b1e0441621506bb84ac0 100644 (file)
@@ -28,7 +28,7 @@ table(table table-bordered table-condensed).
 |state|string|The allowed states are "Queued", "Locked", "Running", "Cancelled" and "Complete".|See "Container states":#container_states for more details.|
 |started_at|datetime|When this container started running.|Null if container has not yet started.|
 |finished_at|datetime|When this container finished.|Null if container has not yet finished.|
-|log|string|Portable data hash of the collection containing logs from a completed container run.|Null if the container is not yet finished.|
+|log|string|UUID or portable data hash of a collection containing the log messages produced when executing the container.|PDH after the container is finished, otherwise UUID or null.|
 |environment|hash|Environment variables and values that should be set in the container environment (@docker run --env@). This augments and (when conflicts exist) overrides environment variables given in the image's Dockerfile.|Must be equal to a ContainerRequest's environment in order to satisfy the ContainerRequest.|
 |cwd|string|Initial working directory.|Must be equal to a ContainerRequest's cwd in order to satisfy the ContainerRequest|
 |command|array of strings|Command to execute.| Must be equal to a ContainerRequest's command in order to satisfy the ContainerRequest.|
index 5fea2d5bc7d6ce6400db4755e2d5d7592ff227ab..e8a70499a929ed0192015044e6abd0fe6eb46bcd 100644 (file)
@@ -377,24 +377,11 @@ class Container < ArvadosModel
     current_user.andand.is_admin
   end
 
-  def permission_to_update
-    # Override base permission check to allow auth_uuid to set progress and
-    # output (only).  Whether it is legal to set progress and output in the current
-    # state has already been checked in validate_change.
-    current_user.andand.is_admin ||
-      (!current_api_client_authorization.nil? and
-       [self.auth_uuid, self.locked_by_uuid].include? current_api_client_authorization.uuid)
-  end
-
   def ensure_owner_uuid_is_permitted
-    # Override base permission check to allow auth_uuid to set progress and
-    # output (only).  Whether it is legal to set progress and output in the current
-    # state has already been checked in validate_change.
-    if !current_api_client_authorization.nil? and self.auth_uuid == current_api_client_authorization.uuid
-      check_update_whitelist [:progress, :output]
-    else
-      super
-    end
+    # validate_change ensures owner_uuid can't be changed at all --
+    # except during create, which requires admin privileges. Checking
+    # permission here would be superfluous.
+    true
   end
 
   def set_timestamps
@@ -420,6 +407,8 @@ class Container < ArvadosModel
 
   def validate_change
     permitted = [:state]
+    progress_attrs = [:progress, :runtime_status, :log, :output]
+    final_attrs = [:exit_code, :finished_at]
 
     if self.new_record?
       permitted.push(:owner_uuid, :command, :container_image, :cwd,
@@ -430,26 +419,26 @@ class Container < ArvadosModel
 
     case self.state
     when Locked
-      permitted.push :priority, :runtime_status
+      permitted.push :priority, :runtime_status, :log
 
     when Queued
       permitted.push :priority
 
     when Running
-      permitted.push :priority, :progress, :output, :runtime_status
+      permitted.push :priority, *progress_attrs
       if self.state_changed?
         permitted.push :started_at
       end
 
     when Complete
       if self.state_was == Running
-        permitted.push :finished_at, :output, :log, :exit_code
+        permitted.push *final_attrs, *progress_attrs
       end
 
     when Cancelled
       case self.state_was
       when Running
-        permitted.push :finished_at, :output, :log
+        permitted.push :finished_at, *progress_attrs
       when Queued, Locked
         permitted.push :finished_at, :log
       end
@@ -459,6 +448,15 @@ class Container < ArvadosModel
       return false
     end
 
+    if current_api_client_authorization.andand.uuid.andand == self.auth_uuid
+      # The contained process itself can update progress indicators,
+      # but can't change priority etc.
+      permitted = permitted & [:state, :progress, :output]
+    elsif self.locked_by_uuid && self.locked_by_uuid != current_api_client_authorization.andand.uuid
+      # When locked, progress fields cannot be updated by the wrong
+      # dispatcher, even though it has admin privileges.
+      permitted = permitted - progress_attrs
+    end
     check_update_whitelist permitted
   end
 
index b8acd4fd092b2cf788f3bc699fb9f9678a04f125..0c5e2e7ad7a8e25ad3c1127ed7d9c20599de1cfe 100644 (file)
@@ -652,32 +652,41 @@ class ContainerTest < ActiveSupport::TestCase
     assert c.update_attributes(exit_code: 1, state: Container::Complete)
   end
 
-  test "locked_by_uuid can set output on running container" do
+  test "locked_by_uuid can update log when locked/running, and output when running" do
     c, _ = minimal_new
     set_user_from_auth :dispatch1
     c.lock
-    c.update_attributes! state: Container::Running
-
     assert_equal c.locked_by_uuid, Thread.current[:api_client_authorization].uuid
+    assert c.update_attributes(log: collections(:real_log_collection).portable_data_hash)
+    assert c.update_attributes(state: Container::Running)
 
-    assert c.update_attributes output: collections(:collection_owned_by_active).portable_data_hash
-    assert c.update_attributes! state: Container::Complete
+    assert c.update_attributes(output: collections(:collection_owned_by_active).portable_data_hash)
+    assert c.update_attributes(log: nil)
+    assert c.update_attributes(log: collections(:real_log_collection).portable_data_hash)
+    assert c.update_attributes(state: Container::Complete, log: collections(:real_log_collection).portable_data_hash)
+    c.reload
+    assert_equal c.output, collections(:collection_owned_by_active).portable_data_hash
+    assert_equal c.log, collections(:real_log_collection).portable_data_hash
+    refute c.update_attributes(output: nil)
+    refute c.update_attributes(log: nil)
   end
 
-  test "auth_uuid can set output on running container, but not change container state" do
+  test "auth_uuid can set output, progress on running container -- but not state, log" do
     c, _ = minimal_new
     set_user_from_auth :dispatch1
     c.lock
     c.update_attributes! state: Container::Running
 
-    Thread.current[:api_client_authorization] = ApiClientAuthorization.find_by_uuid(c.auth_uuid)
-    Thread.current[:user] = User.find_by_id(Thread.current[:api_client_authorization].user_id)
-    assert c.update_attributes output: collections(:collection_owned_by_active).portable_data_hash
+    auth = ApiClientAuthorization.find_by_uuid(c.auth_uuid)
+    Thread.current[:api_client_authorization] = auth
+    Thread.current[:api_client] = auth.api_client
+    Thread.current[:token] = auth.token
+    Thread.current[:user] = auth.user
 
-    assert_raises ArvadosModel::PermissionDeniedError do
-      # auth_uuid cannot set container state
-      c.update_attributes state: Container::Complete
-    end
+    assert c.update_attributes(output: collections(:collection_owned_by_active).portable_data_hash)
+    assert c.update_attributes(progress: 0.5)
+    refute c.update_attributes(log: collections(:real_log_collection).portable_data_hash)
+    refute c.update_attributes(state: Container::Complete)
   end
 
   test "not allowed to set output that is not readable by current user" do
@@ -701,9 +710,7 @@ class ContainerTest < ActiveSupport::TestCase
     c.update_attributes! state: Container::Running
 
     set_user_from_auth :running_to_be_deleted_container_auth
-    assert_raises ArvadosModel::PermissionDeniedError do
-      c.update_attributes! output: collections(:foo_file).portable_data_hash
-    end
+    refute c.update_attributes(output: collections(:foo_file).portable_data_hash)
   end
 
   test "can set trashed output on running container" do