Bugfix/allowlist rm files (#1338)

* add tests for failing rm commands

* transfer validation checks need to check for nil keys for the rm command to fix #1337

apps/dashboard/app/models/transfer.rb

@@ -9,13 +9,14 @@ class Transfer
 
   validates_each :files do |record, attr, files|
     if record.action == 'mv' || record.action == 'cp'
-      conflicts = files.values.select {|f| File.exist?(f) }
+      conflicts = files.values.select { |f| File.exist?(f) }
       record.errors.add :files, "these files already exist: #{conflicts.join(', ')}" if conflicts.present?
     end
 
-    files.each do |k,v|
-      record.errors.add :files, "#{k} is not included under ALLOWLIST_PATH" unless AllowlistPolicy.default.permitted?(k)
-      record.errors.add :files, "#{v} is not included under ALLOWLIST_PATH" unless AllowlistPolicy.default.permitted?(v)
+    files.each do |k, v|
+      record.errors.add :files, "#{k} is not included under ALLOWLIST_PATH" unless AllowlistPolicy.default.permitted?(k.to_s)
+      # rm commands are [{ k => nil}] - nil values
+      record.errors.add :files, "#{v} is not included under ALLOWLIST_PATH" if !v.nil? && !AllowlistPolicy.default.permitted?(v.to_s)
     end
   end
 
@@ -40,12 +41,12 @@ def find(id)
     end
 
     def build(action:, files:)
-      if files.kind_of?(Array)
+      if files.is_a?(Array)
         # rm action will want to provide an array of files
         # so if it is an Array we convert it to a hash:
         #
         # convert [a1, a2, a3] to {a1 => nil, a2 => nil, a3 => nil}
-        files = Hash[files.map {|f| [f, nil]}]
+        files = Hash[files.map { |f| [f, nil] }]
       end
 
       Transfer.new(action: action, files: files)

apps/dashboard/test/models/transfer_test.rb

@@ -1,18 +1,18 @@
 require 'test_helper'
 
 class TransferTest < ActiveSupport::TestCase
-  test "command" do
+  test 'copy command' do
     transfer = Transfer.new action: 'cp', files: {
-      "/Users/efranz/dev/ondemand/apps/dashboard/app"=>"/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj/app",
-      "/Users/efranz/dev/ondemand/apps/dashboard/config"=>"/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj/config",
-      "/Users/efranz/dev/ondemand/apps/dashboard/manifest.yml"=>"/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj/manifest.yml"
+      '/Users/efranz/dev/ondemand/apps/dashboard/app' => '/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj/app',
+      '/Users/efranz/dev/ondemand/apps/dashboard/config' => '/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj/config',
+      '/Users/efranz/dev/ondemand/apps/dashboard/manifest.yml' => '/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj/manifest.yml'
     }
 
-
-    assert_equal ["cp", "-v", "-r", "app", "config", "manifest.yml", "/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj"], transfer.command
+    expected = ['cp', '-v', '-r', 'app', 'config', 'manifest.yml', '/var/folders/w7/fn8w83s10510pkc5j2wq8cpnhxtn3j/T/d20210201-68201-u3azoj']
+    assert_equal expected, transfer.command
   end
 
-  test "percent progress" do
+  test 'percent progress' do
     transfer = Transfer.new
     transfer.stubs(:steps).returns(100)
 
@@ -25,19 +25,37 @@ class TransferTest < ActiveSupport::TestCase
     assert_equal 100, transfer.percent
   end
 
-  test "1 step for file copy" do
-    assert_equal 1, Transfer.new(action: 'cp', files: {'config.ru' => '/tmp/config.ru'}).steps
+  test '1 step for file copy' do
+    assert_equal 1, Transfer.build(action: 'cp', files: { 'config.ru' => '/tmp/config.ru' }).steps
+  end
+
+  test '1 step for file removal' do
+    assert_equal 1, Transfer.build(action: 'rm', files: ['config.ru']).steps
   end
 
-  test "1 step for file removal" do
-    assert_equal 1, Transfer.new(action: 'rm', files: {'config.ru' => '/tmp/config.ru'}).steps
+  test '1 step for file mv' do
+    assert_equal 1, Transfer.build(action: 'mv', files: { 'config.ru' => 'config.ru.2' }).steps
   end
 
-  test "1 step for file mv" do
-    assert_equal 1, Transfer.new(action: 'mv', files: {'config.ru' => 'config.ru.2'}).steps
+  test 'steps for cp bin' do
+    assert_equal Dir['bin/*'].count + 1, Transfer.new(action: 'cp', files: { 'bin' => 'bin.2' }).steps
   end
 
-  test "steps for cp bin" do
-    assert_equal Dir['bin/*'].count+1, Transfer.new(action: 'cp', files: {'bin' => 'bin.2'}).steps
+  # This tests https://github.com/OSC/ondemand/issues/1337 and fails if it's not patched
+  test 'rm works when allowlists are enabled' do
+    Dir.mktmpdir do |tmpdir|
+      Configuration.stubs(:allowlist_paths).returns([tmpdir])
+
+      f = "#{tmpdir}/myfile"
+      FileUtils.touch f
+      assert_equal true, File.exist?(f)
+
+      t = Transfer.build(action: 'rm', files: [f])
+      assert_equal true, t.valid?, t.errors.full_messages.join('. ')
+      assert_equal 1, t.steps
+
+      t.perform
+      assert_equal false, File.exist?(f)
+    end
   end
 end