15836: Refuse project/collection names that fuse/webdav can't show.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index bc6c57f279025519e334de15247b4796a756cb63..ab0f6488493825592033d0fa499c53cdf239d3bc 100644 (file)
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -452,6 +452,24 @@ Clusters:
       # > 0s = auto-create a new version when older than the specified number of seconds.
       PreserveVersionIfIdle: -1s
 
+      # If non-empty, allow project and collection names to contain
+      # the "/" character (slash/stroke/solidus), and replace "/" with
+      # the given string in the filesystem hierarchy presented by
+      # WebDAV. Possible values include "%2f" and "{slash}". Names
+      # that contain the substitution string itself may result in
+      # confusing behavior.
+      #
+      # If the default empty value is used, names containing "/"
+      # cannot be used when creating or renaming a collection or
+      # project.
+      #
+      # If the value "/" is used, project and collection names
+      # containing "/" will be allowed, but they will not be
+      # accessible via WebDAV.
+      #
+      # Use of this feature is not recommended, if it can be avoided.
+      ForwardSlashNameSubstitution: ""
+
       # Managed collection properties. At creation time, if the client didn't
       # provide the listed keys, they will be automatically populated following
       # one of the following behaviors:

diff --git a/lib/config/export.go b/lib/config/export.go
index e7278c5f32b9a8de1c5d5ec3e47d0d4838813603..5eeda348bf366755c6ae6b8cfc7c78765d731a9b 100644 (file)
--- a/lib/config/export.go
+++ b/lib/config/export.go
@@ -92,6 +92,7 @@ var whitelist = map[string]bool{
        "Collections.CollectionVersioning":             false,
        "Collections.DefaultReplication":               true,
        "Collections.DefaultTrashLifetime":             true,
+       "Collections.ForwardSlashNameSubstitution":     true,
        "Collections.ManagedProperties":                true,
        "Collections.ManagedProperties.*":              true,
        "Collections.ManagedProperties.*.*":            true,

diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go
index ce7f69ccc8b853387c219e17882fcce4987ed62e..79c38471732b22147b0bcf4eb3b9e4914a489bd7 100644 (file)
--- a/lib/config/generated_config.go
+++ b/lib/config/generated_config.go
@@ -458,6 +458,24 @@ Clusters:
       # > 0s = auto-create a new version when older than the specified number of seconds.
       PreserveVersionIfIdle: -1s
 
+      # If non-empty, allow project and collection names to contain
+      # the "/" character (slash/stroke/solidus), and replace "/" with
+      # the given string in the filesystem hierarchy presented by
+      # WebDAV. Possible values include "%2f" and "{slash}". Names
+      # that contain the substitution string itself may result in
+      # confusing behavior.
+      #
+      # If the default empty value is used, names containing "/"
+      # cannot be used when creating or renaming a collection or
+      # project.
+      #
+      # If the value "/" is used, project and collection names
+      # containing "/" will be allowed, but they will not be
+      # accessible via WebDAV.
+      #
+      # Use of this feature is not recommended, if it can be avoided.
+      ForwardSlashNameSubstitution: ""
+
       # Managed collection properties. At creation time, if the client didn't
       # provide the listed keys, they will be automatically populated following
       # one of the following behaviors:

diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index ca39d6b26531ea7b6bdfe10505e209160b526634..f34a7e00abaa83345a8b891287aa8801634979d1 100644 (file)
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -115,9 +115,10 @@ type Cluster struct {
                        Function  string
                        Protected bool
                }
-               PreserveVersionIfIdle Duration
-               TrashSweepInterval    Duration
-               TrustAllContent       bool
+               PreserveVersionIfIdle        Duration
+               TrashSweepInterval           Duration
+               TrustAllContent              bool
+               ForwardSlashNameSubstitution string
 
                BlobMissingReport        string
                BalancePeriod            Duration

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 946c4262e3a0eeeb1297cf51f1fba9ab183a1d96..816dbf4758dd0baa6e4ca438434b0b770fd1c0b7 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -738,6 +738,14 @@ class ArvadosModel < ApplicationRecord
     end
   end
 
+  def ensure_filesystem_compatible_name
+    if name == "." || name == ".."
+      errors.add(:name, "cannot be '.' or '..'")
+    elsif Rails.configuration.Collections.ForwardSlashNameSubstitution == "" && !name.nil? && name.index('/')
+      errors.add(:name, "cannot contain a '/' character")
+    end
+  end
+
   class Email
     def self.kind
       "email"

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 292cf34ccb4da99e6a8fcca06cfe69dd2c637d69..99933ba7e7df0ef3bc90ec08f8aab49760b68851 100644 (file)
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -28,6 +28,7 @@ class Collection < ArvadosModel
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
   before_validation :name_null_if_empty
+  validate :ensure_filesystem_compatible_name
   validate :ensure_pdh_matches_manifest_text
   validate :ensure_storage_classes_desired_is_not_empty
   validate :ensure_storage_classes_contain_non_empty_strings

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 7fb8fef42ba9e4c1b19967174cd30c9991383726..1f2b0d8b776a1f63ca94d0e3e7654e7f9cd5887b 100644 (file)
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -16,6 +16,7 @@ class Group < ArvadosModel
   # already know how to properly treat them.
   attribute :properties, :jsonbHash, default: {}
 
+  validate :ensure_filesystem_compatible_name
   after_create :invalidate_permissions_cache
   after_update :maybe_invalidate_permissions_cache
   before_create :assign_name
@@ -31,6 +32,12 @@ class Group < ArvadosModel
     t.add :properties
   end
 
+  def ensure_filesystem_compatible_name
+    # project groups need filesystem-compatible names, but others
+    # don't.
+    super if group_class == 'project'
+  end
+
   def maybe_invalidate_permissions_cache
     if uuid_changed? or owner_uuid_changed? or is_trashed_changed?
       # This can change users' permissions on other groups as well as

diff --git a/services/api/config/arvados_config.rb b/services/api/config/arvados_config.rb
index b5fcd43414a2b26d497b977b7c81efc5f936761b..07d8834b9d64b2af5562a798622776dd097c80cd 100644 (file)
--- a/services/api/config/arvados_config.rb
+++ b/services/api/config/arvados_config.rb
@@ -124,6 +124,7 @@ arvcfg.declare_config "Collections.TrashSweepInterval", ActiveSupport::Duration,
 arvcfg.declare_config "Collections.BlobSigningKey", NonemptyString, :blob_signing_key
 arvcfg.declare_config "Collections.BlobSigningTTL", ActiveSupport::Duration, :blob_signature_ttl
 arvcfg.declare_config "Collections.BlobSigning", Boolean, :permit_create_collection_with_unsigned_manifest, ->(cfg, k, v) { ConfigLoader.set_cfg cfg, "Collections.BlobSigning", !v }
+arvcfg.declare_config "Collections.ForwardSlashNameSubstitution", String
 arvcfg.declare_config "Containers.SupportedDockerImageFormats", Hash, :docker_image_formats, ->(cfg, k, v) { arrayToHash cfg, "Containers.SupportedDockerImageFormats", v }
 arvcfg.declare_config "Containers.LogReuseDecisions", Boolean, :log_reuse_decisions
 arvcfg.declare_config "Containers.DefaultKeepCacheRAM", Integer, :container_default_keep_cache_ram

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 2dd6eedcfbc4b20c9a386d2473f6d90e0a415fbe..bf1ba517ebcb6bf26aec3027a084fee086ff810b 100644 (file)
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -1090,4 +1090,25 @@ class CollectionTest < ActiveSupport::TestCase
       end
     end
   end
+
+  test "collection names must be displayable in a filesystem" do
+    set_user_from_auth :active
+    ["", "{SOLIDUS}"].each do |subst|
+      Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
+      c = Collection.create
+      [[nil, true],
+       ["", true],
+       [".", false],
+       ["..", false],
+       ["...", true],
+       ["..z..", true],
+       ["foo/bar", subst != ""],
+       ["../..", subst != ""],
+       ["/", subst != ""],
+      ].each do |name, valid|
+        c.name = name
+        assert_equal valid, c.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
+      end
+    end
+  end
 end

diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 8b3052e78595da57ff0b6cd487fb513fd8972166..24d7333ab515cbdf127f1c5759db36006a473db6 100644 (file)
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -232,4 +232,28 @@ class GroupTest < ActiveSupport::TestCase
     assert_equal cr_nr_was-1, ContainerRequest.all.length
     assert_equal job_nr_was-1, Job.all.length
   end
+
+  test "project names must be displayable in a filesystem" do
+    set_user_from_auth :active
+    ["", "{SOLIDUS}"].each do |subst|
+      Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
+      g = Group.create
+      [[nil, true],
+       ["", true],
+       [".", false],
+       ["..", false],
+       ["...", true],
+       ["..z..", true],
+       ["foo/bar", subst != ""],
+       ["../..", subst != ""],
+       ["/", subst != ""],
+      ].each do |name, valid|
+        g.name = name
+        g.group_class = "role"
+        assert_equal true, g.valid?
+        g.group_class = "project"
+        assert_equal valid, g.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
+      end
+    end
+  end
 end