From: Tom Clegg Date: Thu, 16 Jan 2020 16:59:39 +0000 (-0500) Subject: 15836: Merge branch 'master' X-Git-Tag: 2.0.0~47^2~3 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/7c7b9ab80e1b33a108c55a23b3dfe793a73ecc95?hp=5e8d75bc3b942b7f0dc2c0129a8e8a0e10d598e5 15836: Merge branch 'master' Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index bc6c57f279..ab0f648849 100644 --- 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 e7278c5f32..5eeda348bf 100644 --- 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 ce7f69ccc8..79c3847173 100644 --- 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/lib/dispatchcloud/container/queue.go b/lib/dispatchcloud/container/queue.go index 21b41ec4d8..a4a270dd10 100644 --- a/lib/dispatchcloud/container/queue.go +++ b/lib/dispatchcloud/container/queue.go @@ -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() { diff --git a/lib/dispatchcloud/container/queue_test.go b/lib/dispatchcloud/container/queue_test.go index bb8226118c..31f321488e 100644 --- a/lib/dispatchcloud/container/queue_test.go +++ b/lib/dispatchcloud/container/queue_test.go @@ -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 }) diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index ca39d6b265..f34a7e00ab 100644 --- 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/sdk/go/arvados/fs_project.go b/sdk/go/arvados/fs_project.go index 92995510c7..c5eb03360a 100644 --- a/sdk/go/arvados/fs_project.go +++ b/sdk/go/arvados/fs_project.go @@ -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 } diff --git a/sdk/go/arvados/fs_project_test.go b/sdk/go/arvados/fs_project_test.go index 91b8222cdc..5628dcc9c4 100644 --- a/sdk/go/arvados/fs_project_test.go +++ b/sdk/go/arvados/fs_project_test.go @@ -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) { diff --git a/sdk/go/arvados/fs_site.go b/sdk/go/arvados/fs_site.go index 4264be4fa6..7826d335c8 100644 --- a/sdk/go/arvados/fs_site.go +++ b/sdk/go/arvados/fs_site.go @@ -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. // diff --git a/sdk/python/tests/run_test_server.py b/sdk/python/tests/run_test_server.py index 2e093b396d..9e9b12f98c 100644 --- a/sdk/python/tests/run_test_server.py +++ b/sdk/python/tests/run_test_server.py @@ -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'), diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 946c4262e3..816dbf4758 100644 --- 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 292cf34ccb..99933ba7e7 100644 --- 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 7fb8fef42b..1f2b0d8b77 100644 --- 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 b5fcd43414..07d8834b9d 100644 --- 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 2dd6eedcfb..bf1ba517eb 100644 --- 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 8b3052e785..24d7333ab5 100644 --- 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 diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go index bbbbd8f97b..563a59df01 100644 --- a/services/keep-web/handler.go +++ b/services/keep-web/handler.go @@ -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) diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go index ec3df9e298..7d820c8a0a 100644 --- a/services/keep-web/handler_test.go +++ b/services/keep-web/handler_test.go @@ -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:\\"odd' 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