From 1e6ee492216b921e6fbf30a893cdc15dc42ce21f Mon Sep 17 00:00:00 2001 From: Viktor Scharf Date: Wed, 29 Oct 2025 12:08:48 +0100 Subject: [PATCH 1/3] bump-reva-2.29.5 --- go.mod | 2 +- go.sum | 4 +- .../storage/fs/posix/blobstore/blobstore.go | 149 ++++++++++++------ .../pkg/storage/fs/posix/trashbin/trashbin.go | 11 +- .../reva/v2/pkg/storage/fs/posix/tree/tree.go | 8 +- .../storage/pkg/decomposedfs/upload/upload.go | 11 ++ vendor/modules.txt | 2 +- 7 files changed, 130 insertions(+), 57 deletions(-) diff --git a/go.mod b/go.mod index f9fbd3af61..1de54981ab 100644 --- a/go.mod +++ b/go.mod @@ -63,7 +63,7 @@ require ( github.com/onsi/ginkgo/v2 v2.23.3 github.com/onsi/gomega v1.36.3 github.com/open-policy-agent/opa v1.2.0 - github.com/opencloud-eu/reva/v2 v2.29.4 + github.com/opencloud-eu/reva/v2 v2.29.5 github.com/orcaman/concurrent-map v1.0.0 github.com/owncloud/libre-graph-api-go v1.0.5-0.20240829135935-80dc00d6f5ea github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index 082b61110b..6358f06499 100644 --- a/go.sum +++ b/go.sum @@ -865,8 +865,8 @@ github.com/onsi/gomega v1.36.3 h1:hID7cr8t3Wp26+cYnfcjR6HpJ00fdogN6dqZ1t6IylU= github.com/onsi/gomega v1.36.3/go.mod h1:8D9+Txp43QWKhM24yyOBEdpkzN8FvJyAwecBgsU4KU0= github.com/open-policy-agent/opa v1.2.0 h1:88NDVCM0of1eO6Z4AFeL3utTEtMuwloFmWWU7dRV1z0= github.com/open-policy-agent/opa v1.2.0/go.mod h1:30euUmOvuBoebRCcJ7DMF42bRBOPznvt0ACUMYDUGVY= -github.com/opencloud-eu/reva/v2 v2.29.4 h1:UaykCqG3FNEpaeZzixsJBGi+j/Ihl3qASQ1WgcYTDb0= -github.com/opencloud-eu/reva/v2 v2.29.4/go.mod h1:+nkCU7w6E6cyNSsKRYj1rb0cCI7QswEQ7KOPljctebM= +github.com/opencloud-eu/reva/v2 v2.29.5 h1:T4RjTSDk650PVn0hAL8HpF+61ChqQ/UwNoWMYYAMOGU= +github.com/opencloud-eu/reva/v2 v2.29.5/go.mod h1:+nkCU7w6E6cyNSsKRYj1rb0cCI7QswEQ7KOPljctebM= github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs= github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYrxe9dPLANfrWvHYVTgc= diff --git a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/blobstore/blobstore.go b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/blobstore/blobstore.go index 8501196b5c..d24d162409 100644 --- a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/blobstore/blobstore.go +++ b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/blobstore/blobstore.go @@ -20,13 +20,21 @@ package blobstore import ( - "bufio" + "context" + "fmt" "io" "os" "path/filepath" + "time" + + "github.com/pkg/errors" + "github.com/pkg/xattr" "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/node" - "github.com/pkg/errors" +) + +const ( + TMPDir = ".oc-tmp" ) // Blobstore provides an interface to an filesystem based blobstore @@ -41,61 +49,106 @@ func New(root string) (*Blobstore, error) { }, nil } -// Upload stores some data in the blobstore under the given key -func (bs *Blobstore) Upload(node *node.Node, source, copyTarget string) error { - path := node.InternalPath() +// Upload is responsible for transferring data from a source file (upload) to its final location; +// the file operation is done atomically using a temporary file followed by a rename +func (bs *Blobstore) Upload(n *node.Node, source, copyTarget string) error { + tempName := filepath.Join(n.SpaceRoot.InternalPath(), TMPDir, filepath.Base(source)) - // preserve the mtime of the file - fi, _ := os.Stat(path) - - file, err := os.Open(source) - if err != nil { - return errors.Wrap(err, "Decomposedfs: posix blobstore: Can not open source file to upload") - } - defer file.Close() - - f, err := os.OpenFile(node.InternalPath(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0700) - if err != nil { - return errors.Wrapf(err, "could not open blob '%s' for writing", node.InternalPath()) - } - defer f.Close() - - w := bufio.NewWriter(f) - _, err = w.ReadFrom(file) - if err != nil { - return errors.Wrapf(err, "could not write blob '%s'", node.InternalPath()) - } - err = w.Flush() - if err != nil { - return err - } - err = os.Chtimes(path, fi.ModTime(), fi.ModTime()) - if err != nil { + // there is no guarantee that the space root TMPDir exists at this point, so we create the directory if needed + if err := os.MkdirAll(filepath.Dir(tempName), 0700); err != nil { return err } - if copyTarget != "" { - // also "upload" the file to a local path, e.g. for keeping the "current" version of the file - err := os.MkdirAll(filepath.Dir(copyTarget), 0700) - if err != nil { - return err + sourceFile, err := os.Open(source) + if err != nil { + return fmt.Errorf("failed to open source file '%s': %v", source, err) + } + defer func() { + _ = sourceFile.Close() + }() + + tempFile, err := os.OpenFile(tempName, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0700) + if err != nil { + return fmt.Errorf("unable to create temp file '%s': %v", tempName, err) + } + + if _, err := tempFile.ReadFrom(sourceFile); err != nil { + return fmt.Errorf("failed to write data from source file '%s' to temp file '%s' - %v", source, tempName, err) + } + + if err := tempFile.Sync(); err != nil { + return fmt.Errorf("failed to sync temp file '%s' - %v", tempName, err) + } + + if err := tempFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file '%s' - %v", tempName, err) + } + + nodeAttributes, err := n.Xattrs(context.Background()) + if err != nil { + return fmt.Errorf("failed to get xattrs for node '%s': %v", n.InternalPath(), err) + } + + var mtime *time.Time + for k, v := range nodeAttributes { + if err := xattr.Set(tempName, k, v); err != nil { + return fmt.Errorf("failed to set xattr '%s' on temp file '%s' - %v", k, tempName, err) } - _, err = file.Seek(0, 0) - if err != nil { - return err + if k == "user.oc.mtime" { + tv, err := time.Parse(time.RFC3339Nano, string(v)) + if err == nil { + mtime = &tv + } + } + } + + // the extended attributes should always contain a mtime, but in case they don't, we fetch it from the node + if mtime == nil { + switch nodeMtime, err := n.GetMTime(context.Background()); { + case err != nil: + return fmt.Errorf("failed to get mtime for node '%s' - %v", n.InternalPath(), err) + default: + mtime = &nodeMtime } - copyFile, err := os.OpenFile(copyTarget, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) - if err != nil { - return errors.Wrapf(err, "could not open copy target '%s' for writing", copyTarget) - } - defer copyFile.Close() + } - _, err = copyFile.ReadFrom(file) - if err != nil { - return errors.Wrapf(err, "could not write blob copy of '%s' to '%s'", node.InternalPath(), copyTarget) - } + // etags rely on the id and the mtime, so we need to ensure the mtime is set correctly + if err := os.Chtimes(tempName, *mtime, *mtime); err != nil { + return fmt.Errorf("failed to set mtime on temp file '%s' - %v", tempName, err) + } + + // atomically move the file to its final location, + // on Windows systems (unsupported oc os) os.Rename is not atomic + if err := os.Rename(tempName, n.InternalPath()); err != nil { + return fmt.Errorf("failed to move temp file '%s' to node '%s' - %v", tempName, n.InternalPath(), err) + } + + // upload successfully, now handle the copy target if set + if copyTarget == "" { + return nil + } + + // also "upload" the file to a local path, e.g., for keeping the "current" version of the file + if err := os.MkdirAll(filepath.Dir(copyTarget), 0700); err != nil { + return err + } + + if _, err := sourceFile.Seek(0, 0); err != nil { + return err + } + + copyFile, err := os.OpenFile(copyTarget, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) + if err != nil { + return errors.Wrapf(err, "could not open copy target '%s' for writing", copyTarget) + } + defer func() { + _ = copyFile.Close() + }() + + if _, err := copyFile.ReadFrom(sourceFile); err != nil { + return errors.Wrapf(err, "could not write blob copy of '%s' to '%s'", n.InternalPath(), copyTarget) } return nil diff --git a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/trashbin/trashbin.go b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/trashbin/trashbin.go index b4cf116532..bfc02c2224 100644 --- a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/trashbin/trashbin.go +++ b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/trashbin/trashbin.go @@ -32,6 +32,7 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/opencloud-eu/reva/v2/pkg/errtypes" "github.com/opencloud-eu/reva/v2/pkg/storage" "github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/lookup" @@ -333,10 +334,12 @@ func (tb *Trashbin) RestoreRecycleItem(ctx context.Context, spaceID string, key, return nil, fmt.Errorf("trashbin: parent id not found for %s", restorePath) } - trashNode := &trashNode{spaceID: spaceID, id: id, path: trashPath} - err = tb.lu.MetadataBackend().Set(ctx, trashNode, prefixes.ParentidAttr, []byte(parentID)) - if err != nil { - return nil, err + trashedNode := &trashNode{spaceID: spaceID, id: id, path: trashPath} + if err = tb.lu.MetadataBackend().SetMultiple(ctx, trashedNode, map[string][]byte{ + prefixes.NameAttr: []byte(filepath.Base(restorePath)), + prefixes.ParentidAttr: []byte(parentID), + }, true); err != nil { + return nil, fmt.Errorf("posixfs: failed to update trashed node metadata: %w", err) } // restore the item diff --git a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/tree/tree.go b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/tree/tree.go index 590db5f10f..2bd1f433e8 100644 --- a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/tree/tree.go +++ b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/tree/tree.go @@ -38,9 +38,11 @@ import ( "golang.org/x/sync/errgroup" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/opencloud-eu/reva/v2/pkg/appctx" "github.com/opencloud-eu/reva/v2/pkg/errtypes" "github.com/opencloud-eu/reva/v2/pkg/events" + "github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/blobstore" "github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/lookup" "github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/options" "github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/trashbin" @@ -649,6 +651,10 @@ func (t *Tree) createDirNode(ctx context.Context, n *node.Node) (err error) { return n.SetXattrsWithContext(ctx, attributes, false) } +func (t *Tree) isTemporary(path string) bool { + return path == blobstore.TMPDir +} + func (t *Tree) isIgnored(path string) bool { return isLockFile(path) || isTrash(path) || t.isUpload(path) || t.isInternal(path) } @@ -664,7 +670,7 @@ func (t *Tree) isIndex(path string) bool { func (t *Tree) isInternal(path string) bool { return path == t.options.Root || path == filepath.Join(t.options.Root, "users") || - t.isIndex(path) || strings.Contains(path, lookup.MetadataDir) + t.isIndex(path) || strings.Contains(path, lookup.MetadataDir) || t.isTemporary(path) } func isLockFile(path string) bool { diff --git a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/upload/upload.go b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/upload/upload.go index c37f5216b0..79313ca4b3 100644 --- a/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/upload/upload.go +++ b/vendor/github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/upload/upload.go @@ -286,6 +286,17 @@ func (session *DecomposedFsSession) Finalize(ctx context.Context) (err error) { revisionNode := node.New(session.SpaceID(), session.NodeID(), "", "", session.Size(), session.ID(), provider.ResourceType_RESOURCE_TYPE_FILE, session.SpaceOwner(), session.store.lu) + switch spaceRoot, err := session.store.lu.NodeFromSpaceID(ctx, session.SpaceID()); { + case err != nil: + return fmt.Errorf("failed to get space root for space id %s: %v", session.SpaceID(), err) + case spaceRoot == nil: + return fmt.Errorf("space root for space id %s not found", session.SpaceID()) + case spaceRoot.InternalPath() == "": + return fmt.Errorf("space root for space id %s has no valid internal path", session.SpaceID()) + default: + revisionNode.SpaceRoot = spaceRoot + } + // upload the data to the blobstore _, subspan := tracer.Start(ctx, "WriteBlob") err = session.store.tp.WriteBlob(revisionNode, session.binPath()) diff --git a/vendor/modules.txt b/vendor/modules.txt index 5b72ae6d69..e4bb5c22f6 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1198,7 +1198,7 @@ github.com/open-policy-agent/opa/v1/types github.com/open-policy-agent/opa/v1/util github.com/open-policy-agent/opa/v1/util/decoding github.com/open-policy-agent/opa/v1/version -# github.com/opencloud-eu/reva/v2 v2.29.4 +# github.com/opencloud-eu/reva/v2 v2.29.5 ## explicit; go 1.24.1 github.com/opencloud-eu/reva/v2/cmd/revad/internal/grace github.com/opencloud-eu/reva/v2/cmd/revad/runtime From ddaf1f86b0f61156d0c05490f03895dc262ddd24 Mon Sep 17 00:00:00 2001 From: Viktor Scharf Date: Tue, 14 Oct 2025 14:54:07 +0200 Subject: [PATCH 2/3] apiTest-coverage for #1523 --- .../trashbinRestore.feature | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature b/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature index 5d96844756..1281f0067f 100644 --- a/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature +++ b/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature @@ -567,3 +567,44 @@ Feature: restore deleted files/folders | dav-path-version | | spaces | | new | + + @issue-1523 + Scenario Outline: restore deleted folder when folder with same name exists + Given using DAV path + And user "Alice" has created folder "new" + And user "Alice" has uploaded file with content "content" to "new/test.txt" + And user "Alice" has deleted folder "new" + And user "Alice" has created folder "new" + And user "Alice" has uploaded file with content "new content" to "new/new-file.txt" + When user "Alice" restores the folder with original path "/new" to "/new (1)" using the trashbin API + Then the HTTP status code should be "201" + And as "Alice" folder "new (1)" should exist + And as "Alice" file "new (1)/test.txt" should exist + And as "Alice" folder "new" should exist + And as "Alice" file "new/new-file.txt" should exist + Examples: + | dav-path-version | + | spaces | + | new | + + @issue-1523 + Scenario Outline: restore deleted folder with files when folder with same name exists + Given using DAV path + And user "Alice" has created folder "folder-a" + And user "Alice" has uploaded file with content "content b" to "folder-a/b.txt" + And user "Alice" has uploaded file with content "content c" to "folder-a/c.txt" + And user "Alice" has deleted file "folder-a/b.txt" + And user "Alice" has deleted folder "folder-a" + And user "Alice" has created folder "folder-a" + When user "Alice" restores the file with original path "folder-a/b.txt" using the trashbin API + Then the HTTP status code should be "201" + When user "Alice" restores the folder with original path "/folder-a" to "/folder-a (1)" using the trashbin API + Then the HTTP status code should be "201" + And as "Alice" folder "folder-a" should exist + And as "Alice" file "folder-a/b.txt" should exist + And as "Alice" folder "folder-a (1)" should exist + And as "Alice" file "folder-a (1)/c.txt" should exist + Examples: + | dav-path-version | + | spaces | + | new | From 723284193dc803a23adb68b595846a0311dde409 Mon Sep 17 00:00:00 2001 From: Viktor Scharf Date: Thu, 16 Oct 2025 13:18:51 +0200 Subject: [PATCH 3/3] check propfind contans correct files name --- .../features/apiContract/propfind.feature | 23 ++++++++++++++++++ .../trashbinRestore.feature | 24 ++++++++++++------- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/tests/acceptance/features/apiContract/propfind.feature b/tests/acceptance/features/apiContract/propfind.feature index 63a9c79311..e01900f94e 100644 --- a/tests/acceptance/features/apiContract/propfind.feature +++ b/tests/acceptance/features/apiContract/propfind.feature @@ -120,3 +120,26 @@ Feature: Propfind test | Manager | RDNVWZP | | Space Editor | DNVW | | Space Viewer | | + + @issue-1523 + Scenario: propfind response contains a restored folder with correct name + Given user "Alice" has created a folder "folderMain" in space "Personal" + And user "Alice" has deleted folder "folderMain" + And user "Alice" has created a folder "folderMain" in space "Personal" + When user "Alice" restores the folder with original path "/folderMain" to "/folderMain (1)" using the trashbin API + And user "Alice" sends PROPFIND request to space "Personal" using the WebDAV API + Then the HTTP status code should be "207" + And as user "Alice" the PROPFIND response should contain a resource "folderMain" with these key and value pairs: + | key | value | + | oc:fileid | %file_id_pattern% | + | oc:file-parent | %file_id_pattern% | + | oc:name | folderMain | + | oc:permissions | RDNVCKZP | + | oc:size | 0 | + And as user "Alice" the PROPFIND response should contain a resource "folderMain (1)" with these key and value pairs: + | key | value | + | oc:fileid | %file_id_pattern% | + | oc:file-parent | %file_id_pattern% | + | oc:name | folderMain (1) | + | oc:permissions | RDNVCKZP | + | oc:size | 0 | diff --git a/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature b/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature index 1281f0067f..edbb31bb0c 100644 --- a/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature +++ b/tests/acceptance/features/coreApiTrashbinRestore/trashbinRestore.feature @@ -578,10 +578,14 @@ Feature: restore deleted files/folders And user "Alice" has uploaded file with content "new content" to "new/new-file.txt" When user "Alice" restores the folder with original path "/new" to "/new (1)" using the trashbin API Then the HTTP status code should be "201" - And as "Alice" folder "new (1)" should exist - And as "Alice" file "new (1)/test.txt" should exist - And as "Alice" folder "new" should exist - And as "Alice" file "new/new-file.txt" should exist + And as "Alice" the following folders should exist + | path | + | /new | + | /new (1) | + And as "Alice" the following files should exist + | path | + | /new/new-file.txt | + | /new (1)/test.txt | Examples: | dav-path-version | | spaces | @@ -600,10 +604,14 @@ Feature: restore deleted files/folders Then the HTTP status code should be "201" When user "Alice" restores the folder with original path "/folder-a" to "/folder-a (1)" using the trashbin API Then the HTTP status code should be "201" - And as "Alice" folder "folder-a" should exist - And as "Alice" file "folder-a/b.txt" should exist - And as "Alice" folder "folder-a (1)" should exist - And as "Alice" file "folder-a (1)/c.txt" should exist + And as "Alice" the following folders should exist + | path | + | /folder-a | + | /folder-a (1) | + And as "Alice" the following files should exist + | path | + | /folder-a/b.txt | + | /folder-a (1)/c.txt | Examples: | dav-path-version | | spaces |