15836: Merge branch 'master'
authorTom Clegg <tom@tomclegg.ca>
Thu, 16 Jan 2020 16:59:39 +0000 (11:59 -0500)
committerTom Clegg <tom@tomclegg.ca>
Thu, 16 Jan 2020 16:59:39 +0000 (11:59 -0500)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

18 files changed:
lib/config/config.default.yml
lib/config/export.go
lib/config/generated_config.go
lib/dispatchcloud/container/queue.go
lib/dispatchcloud/container/queue_test.go
sdk/go/arvados/config.go
sdk/go/arvados/fs_project.go
sdk/go/arvados/fs_project_test.go
sdk/go/arvados/fs_site.go
sdk/python/tests/run_test_server.py
services/api/app/models/arvados_model.rb
services/api/app/models/collection.rb
services/api/app/models/group.rb
services/api/config/arvados_config.rb
services/api/test/unit/collection_test.rb
services/api/test/unit/group_test.rb
services/keep-web/handler.go
services/keep-web/handler_test.go

index bc6c57f279025519e334de15247b4796a756cb63..ab0f6488493825592033d0fa499c53cdf239d3bc 100644 (file)
@@ -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:
index e7278c5f32b9a8de1c5d5ec3e47d0d4838813603..5eeda348bf366755c6ae6b8cfc7c78765d731a9b 100644 (file)
@@ -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,
index ce7f69ccc8b853387c219e17882fcce4987ed62e..79c38471732b22147b0bcf4eb3b9e4914a489bd7 100644 (file)
@@ -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:
index 21b41ec4d86d6cdc6c909b63106511dfe548f3c4..a4a270dd1013eee601cd214eb7e336f6cd8a29e2 100644 (file)
@@ -240,19 +240,15 @@ func (cq *Queue) addEnt(uuid string, ctr arvados.Container) {
                go func() {
                        if ctr.State == arvados.ContainerStateQueued {
                                // Can't set runtime error without
-                               // locking first. If Lock() is
-                               // successful, it will call addEnt()
-                               // again itself, and we'll fall
-                               // through to the
-                               // setRuntimeError/Cancel code below.
+                               // locking first.
                                err := cq.Lock(ctr.UUID)
                                if err != nil {
                                        logger.WithError(err).Warn("lock failed")
+                                       return
                                        // ...and try again on the
                                        // next Update, if the problem
                                        // still exists.
                                }
-                               return
                        }
                        var err error
                        defer func() {
index bb8226118cb4dc91a36d5c5382fa5b3e8e5a1487..31f321488e686929080f2b1b6a9bdba7c7bfeff3 100644 (file)
@@ -134,10 +134,11 @@ func (suite *IntegrationSuite) TestCancelIfNoInstanceType(c *check.C) {
        c.Check(err, check.IsNil)
        c.Check(ctr.State, check.Equals, arvados.ContainerStateQueued)
 
+       go cq.Update()
+
        // Wait for the cancel operation to take effect. Container
        // will have state=Cancelled or just disappear from the queue.
        suite.waitfor(c, time.Second, func() bool {
-               cq.Update()
                err := client.RequestAndDecode(&ctr, "GET", "arvados/v1/containers/"+arvadostest.QueuedContainerUUID, nil, nil)
                return err == nil && ctr.State == arvados.ContainerStateCancelled
        })
index ca39d6b26531ea7b6bdfe10505e209160b526634..f34a7e00abaa83345a8b891287aa8801634979d1 100644 (file)
@@ -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
index 92995510c7eaefaa4de69ea42f6c5f050e6828bd..c5eb03360a3877b577168611a8e579329b6abfa8 100644 (file)
@@ -30,16 +30,31 @@ func (fs *customFileSystem) projectsLoadOne(parent inode, uuid, name string) (in
        }
 
        var contents CollectionList
-       err = fs.RequestAndDecode(&contents, "GET", "arvados/v1/groups/"+uuid+"/contents", nil, ResourceListParams{
-               Count: "none",
-               Filters: []Filter{
-                       {"name", "=", name},
-                       {"uuid", "is_a", []string{"arvados#collection", "arvados#group"}},
-                       {"groups.group_class", "=", "project"},
-               },
-       })
-       if err != nil {
-               return nil, err
+       for _, subst := range []string{"/", fs.forwardSlashNameSubstitution} {
+               contents = CollectionList{}
+               err = fs.RequestAndDecode(&contents, "GET", "arvados/v1/groups/"+uuid+"/contents", nil, ResourceListParams{
+                       Count: "none",
+                       Filters: []Filter{
+                               {"name", "=", strings.Replace(name, subst, "/", -1)},
+                               {"uuid", "is_a", []string{"arvados#collection", "arvados#group"}},
+                               {"groups.group_class", "=", "project"},
+                       },
+               })
+               if err != nil {
+                       return nil, err
+               }
+               if len(contents.Items) > 0 || fs.forwardSlashNameSubstitution == "/" || fs.forwardSlashNameSubstitution == "" || !strings.Contains(name, fs.forwardSlashNameSubstitution) {
+                       break
+               }
+               // If the requested name contains the configured "/"
+               // replacement string and didn't match a
+               // project/collection exactly, we'll try again with
+               // "/" in its place, so a lookup of a munged name
+               // works regardless of whether the directory listing
+               // has been populated with escaped names.
+               //
+               // Note this doesn't handle items whose names contain
+               // both "/" and the substitution string.
        }
        if len(contents.Items) == 0 {
                return nil, os.ErrNotExist
@@ -86,6 +101,9 @@ func (fs *customFileSystem) projectsLoadAll(parent inode, uuid string) ([]inode,
                }
                for _, i := range resp.Items {
                        coll := i
+                       if fs.forwardSlashNameSubstitution != "" {
+                               coll.Name = strings.Replace(coll.Name, "/", fs.forwardSlashNameSubstitution, -1)
+                       }
                        if !permittedName(coll.Name) {
                                continue
                        }
@@ -106,6 +124,9 @@ func (fs *customFileSystem) projectsLoadAll(parent inode, uuid string) ([]inode,
                        break
                }
                for _, group := range resp.Items {
+                       if fs.forwardSlashNameSubstitution != "" {
+                               group.Name = strings.Replace(group.Name, "/", fs.forwardSlashNameSubstitution, -1)
+                       }
                        if !permittedName(group.Name) {
                                continue
                        }
index 91b8222cdc631d6ac80bf005c3f07c84fe641b92..5628dcc9c43b42ef6560ddfa1eba2bc0482001d3 100644 (file)
@@ -147,6 +147,21 @@ func (s *SiteFSSuite) TestSlashInName(c *check.C) {
                c.Logf("fi.Name() == %q", fi.Name())
                c.Check(strings.Contains(fi.Name(), "/"), check.Equals, false)
        }
+
+       // Make a new fs (otherwise content will still be cached from
+       // above) and enable "/" replacement string.
+       s.fs = s.client.SiteFileSystem(s.kc)
+       s.fs.ForwardSlashNameSubstitution("___")
+       dir, err = s.fs.Open("/users/active/A Project/bad___collection")
+       if c.Check(err, check.IsNil) {
+               _, err = dir.Readdir(-1)
+               c.Check(err, check.IsNil)
+       }
+       dir, err = s.fs.Open("/users/active/A Project/bad___project")
+       if c.Check(err, check.IsNil) {
+               _, err = dir.Readdir(-1)
+               c.Check(err, check.IsNil)
+       }
 }
 
 func (s *SiteFSSuite) TestProjectUpdatedByOther(c *check.C) {
index 4264be4fa600be7abb05351f506feb00dccac6cc..7826d335c81fa93cfe54bf39a81a66335a65d336 100644 (file)
@@ -16,6 +16,7 @@ type CustomFileSystem interface {
        MountByID(mount string)
        MountProject(mount, uuid string)
        MountUsers(mount string)
+       ForwardSlashNameSubstitution(string)
 }
 
 type customFileSystem struct {
@@ -25,6 +26,8 @@ type customFileSystem struct {
 
        staleThreshold time.Time
        staleLock      sync.Mutex
+
+       forwardSlashNameSubstitution string
 }
 
 func (c *Client) CustomFileSystem(kc keepClient) CustomFileSystem {
@@ -94,6 +97,10 @@ func (fs *customFileSystem) MountUsers(mount string) {
        })
 }
 
+func (fs *customFileSystem) ForwardSlashNameSubstitution(repl string) {
+       fs.forwardSlashNameSubstitution = repl
+}
+
 // SiteFileSystem returns a FileSystem that maps collections and other
 // Arvados objects onto a filesystem layout.
 //
index 2e093b396dfa22991332368db61a8e9f5904a5d5..9e9b12f98ca6d09ed1fb65eb5c768acf23454bf5 100644 (file)
@@ -738,6 +738,7 @@ def setup_config():
                 "Collections": {
                     "BlobSigningKey": "zfhgfenhffzltr9dixws36j1yhksjoll2grmku38mi7yxd66h5j4q9w4jzanezacp8s6q0ro3hxakfye02152hncy6zml2ed0uc",
                     "TrustAllContent": True,
+                    "ForwardSlashNameSubstitution": "/",
                 },
                 "Git": {
                     "Repositories": "%s/test" % os.path.join(SERVICES_SRC_DIR, 'api', 'tmp', 'git'),
index 946c4262e3a0eeeb1297cf51f1fba9ab183a1d96..816dbf4758dd0baa6e4ca438434b0b770fd1c0b7 100644 (file)
@@ -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"
index 292cf34ccb4da99e6a8fcca06cfe69dd2c637d69..99933ba7e7df0ef3bc90ec08f8aab49760b68851 100644 (file)
@@ -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
index 7fb8fef42ba9e4c1b19967174cd30c9991383726..1f2b0d8b776a1f63ca94d0e3e7654e7f9cd5887b 100644 (file)
@@ -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
index b5fcd43414a2b26d497b977b7c81efc5f936761b..07d8834b9d64b2af5562a798622776dd097c80cd 100644 (file)
@@ -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
index 2dd6eedcfbc4b20c9a386d2473f6d90e0a415fbe..bf1ba517ebcb6bf26aec3027a084fee086ff810b 100644 (file)
@@ -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
index 8b3052e78595da57ff0b6cd487fb513fd8972166..24d7333ab515cbdf127f1c5759db36006a473db6 100644 (file)
@@ -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
index bbbbd8f97bfce63a0e22da02cc5fbe32a1e5794e..563a59df014b8f642b545a68bc23f2e72e1a57d1 100644 (file)
@@ -537,6 +537,7 @@ func (h *handler) serveSiteFS(w http.ResponseWriter, r *http.Request, tokens []s
                Insecure:  arv.ApiInsecure,
        }).WithRequestID(r.Header.Get("X-Request-Id"))
        fs := client.SiteFileSystem(kc)
+       fs.ForwardSlashNameSubstitution(h.Config.cluster.Collections.ForwardSlashNameSubstitution)
        f, err := fs.Open(r.URL.Path)
        if os.IsNotExist(err) {
                http.Error(w, err.Error(), http.StatusNotFound)
index ec3df9e298e1798f5f8f440b5ce2a1bf4ab51164..7d820c8a0adb61b97befe8d02477fb34b31615f4 100644 (file)
@@ -520,6 +520,56 @@ func (s *IntegrationSuite) TestSpecialCharsInPath(c *check.C) {
        c.Check(resp.Body.String(), check.Matches, `(?ms).*href="./https:%5c%22odd%27%20path%20chars"\S+https:\\&#34;odd&#39; path chars.*`)
 }
 
+func (s *IntegrationSuite) TestForwardSlashSubstitution(c *check.C) {
+       arv := arvados.NewClientFromEnv()
+       s.testServer.Config.cluster.Services.WebDAVDownload.ExternalURL.Host = "download.example.com"
+       s.testServer.Config.cluster.Collections.ForwardSlashNameSubstitution = "{SOLIDUS}"
+       name := "foo/bar/baz"
+       nameShown := strings.Replace(name, "/", "{SOLIDUS}", -1)
+       nameShownEscaped := strings.Replace(name, "/", "%7bSOLIDUS%7d", -1)
+
+       client := s.testServer.Config.Client
+       client.AuthToken = arvadostest.ActiveToken
+       fs, err := (&arvados.Collection{}).FileSystem(&client, nil)
+       c.Assert(err, check.IsNil)
+       f, err := fs.OpenFile("filename", os.O_CREATE, 0777)
+       c.Assert(err, check.IsNil)
+       f.Close()
+       mtxt, err := fs.MarshalManifest(".")
+       c.Assert(err, check.IsNil)
+       var coll arvados.Collection
+       err = client.RequestAndDecode(&coll, "POST", "arvados/v1/collections", nil, map[string]interface{}{
+               "collection": map[string]string{
+                       "manifest_text": mtxt,
+                       "name":          name,
+                       "owner_uuid":    arvadostest.AProjectUUID,
+               },
+       })
+       c.Assert(err, check.IsNil)
+       defer arv.RequestAndDecode(&coll, "DELETE", "arvados/v1/collections/"+coll.UUID, nil, nil)
+
+       base := "http://download.example.com/by_id/" + coll.OwnerUUID + "/"
+       for tryURL, expectRegexp := range map[string]string{
+               base:                   `(?ms).*href="./` + nameShownEscaped + `/"\S+` + nameShown + `.*`,
+               base + nameShown + "/": `(?ms).*href="./filename"\S+filename.*`,
+       } {
+               u, _ := url.Parse(tryURL)
+               req := &http.Request{
+                       Method:     "GET",
+                       Host:       u.Host,
+                       URL:        u,
+                       RequestURI: u.RequestURI(),
+                       Header: http.Header{
+                               "Authorization": {"Bearer " + client.AuthToken},
+                       },
+               }
+               resp := httptest.NewRecorder()
+               s.testServer.Handler.ServeHTTP(resp, req)
+               c.Check(resp.Code, check.Equals, http.StatusOK)
+               c.Check(resp.Body.String(), check.Matches, expectRegexp)
+       }
+}
+
 // XHRs can't follow redirect-with-cookie so they rely on method=POST
 // and disposition=attachment (telling us it's acceptable to respond
 // with content instead of a redirect) and an Origin header that gets