[ARVADOS] created: 1.3.0-2050-gfd221b400
Git user
git at public.arvados.org
Tue Jan 7 15:36:02 UTC 2020
at fd221b4003c306095d6056458e313c74c7a80dc4 (commit)
commit fd221b4003c306095d6056458e313c74c7a80dc4
Author: Tom Clegg <tom at tomclegg.ca>
Date: Tue Jan 7 10:28:03 2020 -0500
15836: Substitute configured string for "/" in WebDAV listings.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>
diff --git a/sdk/go/arvados/fs_project.go b/sdk/go/arvados/fs_project.go
index 92995510c..c5eb03360 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 91b8222cd..5628dcc9c 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 4264be4fa..7826d335c 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 2e093b396..9e9b12f98 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/keep-web/handler.go b/services/keep-web/handler.go
index bbbbd8f97..563a59df0 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 ec3df9e29..7d820c8a0 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
commit 8f01e971dd6868ed734736dba75c4e5dad5910a5
Author: Tom Clegg <tom at tomclegg.ca>
Date: Mon Jan 6 16:06:51 2020 -0500
15836: Refuse project/collection names that fuse/webdav can't show.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>
diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index bc6c57f27..ab0f64884 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 e7278c5f3..5eeda348b 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 ce7f69ccc..79c384717 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/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index ca39d6b26..f34a7e00a 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/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 946c4262e..816dbf475 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 292cf34cc..99933ba7e 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 7fb8fef42..1f2b0d8b7 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 b5fcd4341..07d8834b9 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 2dd6eedcf..bf1ba517e 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 8b3052e78..24d7333ab 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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list