From ac8c7b6d32821d7e4589ea5210567481fd95c25f Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Tue, 15 Sep 2026 21:18:48 +0200 Subject: [PATCH 1/4] feat: Support permanent_delete on drive PATCH The drive PATCH handler parsed but silently ignored `permanent_delete`: the `docPatch` of `web/sharings` had no `Delete` field, so a permanent delete requested through `/sharings/drives/:id/:file-id` was a no-op returning 200. Add the `Delete` field and handle it in `applyPatch` with `fs.DestroyDirAndContent(dir, fs.EnsureErased)` for directories and `fs.DestroyFile` for files, bringing the drive route to parity with the `/files` API in a single call. Authorization needs nothing new: `guardSharedDriveRouteForMember` already requires effective PATCH access on the target, so read-only members get a 403. --- model/sharing/indexer.go | 5 ++ model/vfs/couchdb_indexer.go | 43 +++++++++++++----- model/vfs/vfs.go | 9 ++++ model/vfs/vfsafero/impl.go | 2 +- model/vfs/vfss3/impl.go | 9 ++-- model/vfs/vfsswift/impl_v3.go | 9 ++-- web/sharings/drives.go | 15 ++++++ web/sharings/drives_test.go | 86 +++++++++++++++++++++++++++++++++++ 8 files changed, 160 insertions(+), 18 deletions(-) diff --git a/model/sharing/indexer.go b/model/sharing/indexer.go index c33f2183a8e..77547932385 100644 --- a/model/sharing/indexer.go +++ b/model/sharing/indexer.go @@ -294,6 +294,11 @@ func (s *sharingIndexer) DeleteFileDoc(doc *vfs.FileDoc) error { return s.indexer.DeleteFileDoc(doc) } +func (s *sharingIndexer) DestroyFileDoc(doc *vfs.FileDoc) error { + s.log.Errorf("Unexpected call to DestroyFileDoc") + return ErrInternalServerError +} + func (s *sharingIndexer) CreateDirDoc(doc *vfs.DirDoc) error { s.log.Errorf("Unexpected call to CreateDirDoc") return ErrInternalServerError diff --git a/model/vfs/couchdb_indexer.go b/model/vfs/couchdb_indexer.go index 8adf9e769ad..262d145fe36 100644 --- a/model/vfs/couchdb_indexer.go +++ b/model/vfs/couchdb_indexer.go @@ -224,7 +224,7 @@ func (c *couchdbIndexer) UpdateFileDoc(olddoc, newdoc *FileDoc) error { } if !olddoc.Trashed && newdoc.Trashed { - c.checkTrashedFileIsShared(newdoc) + c.revokeSharingRefsOnFile(newdoc) } newdoc.SetID(olddoc.ID()) @@ -245,6 +245,17 @@ func (c *couchdbIndexer) DeleteFileDoc(doc *FileDoc) error { return couchdb.DeleteDoc(c.db, doc) } +// DestroyFileDoc is called by the VFS backends when they destroy a file: +// destroying bypasses the trash, where the revocation of a shared file +// normally happens. It revokes the sharings for which this file is the main +// file. It is a no-op for shortcuts (destroying them is part of the sharing +// cleanup itself) and for files already in the trash, whose sharing +// references were removed when they were trashed. +func (c *couchdbIndexer) DestroyFileDoc(doc *FileDoc) error { + c.revokeSharingRefsOnFile(doc) + return c.DeleteFileDoc(doc) +} + func (c *couchdbIndexer) CreateDirDoc(doc *DirDoc) error { return couchdb.CreateDoc(c.db, doc) } @@ -264,7 +275,7 @@ func (c *couchdbIndexer) UpdateDirDoc(olddoc, newdoc *DirDoc) error { isTrashed := !oldTrashed && newTrashed if isTrashed { - c.checkTrashedDirIsShared(newdoc) + c.revokeSharingRefsOnDir(newdoc) if err := c.setTrashedForFilesInsideDir(olddoc, true); err != nil { return err } @@ -290,12 +301,18 @@ func (c *couchdbIndexer) UpdateDirDoc(olddoc, newdoc *DirDoc) error { } func (c *couchdbIndexer) DeleteDirDoc(doc *DirDoc) error { + // No sharing revocation here: this is also the path used to dissociate a + // doc from a sharing, which must not revoke it. Directory destruction + // goes through DeleteDirDocAndContent, which revokes. return couchdb.DeleteDoc(c.db, doc) } func (c *couchdbIndexer) DeleteDirDocAndContent(doc *DirDoc, onlyContent bool) (files []*FileDoc, n int64, err error) { var docs []couchdb.Doc if !onlyContent { + // Destroying a dir bypasses the trash: revoke the sharing of the dir + // and of any nested shared dir/file before removing them. + c.revokeSharingRefsOnDir(doc) docs = append(docs, doc) } err = walk(c, doc.Name(), doc, nil, func(name string, dir *DirDoc, file *FileDoc, err error) error { @@ -306,8 +323,10 @@ func (c *couchdbIndexer) DeleteDirDocAndContent(doc *DirDoc, onlyContent bool) ( if dir.ID() == doc.ID() { return nil } + c.revokeSharingRefsOnDir(dir) docs = append(docs, dir.Clone()) } else { + c.revokeSharingRefsOnFile(file) cloned := file.Clone() docs = append(docs, cloned) files = append(files, cloned.(*FileDoc)) @@ -400,7 +419,7 @@ func (c *couchdbIndexer) MoveDir(oldpath, newpath string) error { } cloned := child.Clone() if isTrashed { - c.checkTrashedDirIsShared(child) + c.revokeSharingRefsOnDir(child) } olddocs = append(olddocs, cloned) child.Fullpath = path.Join(newpath, child.Fullpath[len(oldpath)+1:]) @@ -664,7 +683,7 @@ func (c *couchdbIndexer) setTrashedForFilesInsideDir(doc *DirDoc, trashed bool) fullpath = strings.TrimPrefix(fullpath, TrashDirName) trashpath := strings.Replace(fullpath, doc.Fullpath, TrashDirName, 1) if trashed { - c.checkTrashedFileIsShared(cloned) + c.revokeSharingRefsOnFile(cloned) cloned.fullpath = fullpath file.fullpath = trashpath } else { @@ -761,10 +780,11 @@ func (c *couchdbIndexer) ListNotSynchronizedOn(clientID string) ([]DirDoc, error return docs, nil } -// checkTrashedDirIsShared will look for a dir going to the trash if it was the -// main dir of a sharing. If it is the case, the sharing is revoked and the +// revokeSharingRefsOnDir looks for io.cozy.sharings references on a dir being +// removed from the VFS, either by going to the trash or by being destroyed. If +// the dir is the main dir of a sharing, the sharing is revoked and the // reference to the sharing is removed. -func (c *couchdbIndexer) checkTrashedDirIsShared(doc *DirDoc) { +func (c *couchdbIndexer) revokeSharingRefsOnDir(doc *DirDoc) { refs := doc.ReferencedBy[:0] for _, ref := range doc.ReferencedBy { if ref.Type == consts.Sharings { @@ -776,10 +796,11 @@ func (c *couchdbIndexer) checkTrashedDirIsShared(doc *DirDoc) { doc.ReferencedBy = refs } -// checkTrashedFileIsShared will look for a file going to the trash if it was -// the main file of a sharing. If it is the case, the sharing is revoked and -// the reference to the sharing is removed. -func (c *couchdbIndexer) checkTrashedFileIsShared(doc *FileDoc) { +// revokeSharingRefsOnFile looks for io.cozy.sharings references on a file +// being removed from the VFS, either by going to the trash or by being +// destroyed. If the file is the main file of a sharing, the sharing is revoked +// and the reference to the sharing is removed. +func (c *couchdbIndexer) revokeSharingRefsOnFile(doc *FileDoc) { // A shortcut is created for a sharing not yet accepted if the owner of the // sharing knows the Cozy URL of a recipient. Normally, this shortcut is // removed when the sharing is accepted, but it is safer to avoid revoking diff --git a/model/vfs/vfs.go b/model/vfs/vfs.go index 2a83cd2f1fe..7e3f8141c6c 100644 --- a/model/vfs/vfs.go +++ b/model/vfs/vfs.go @@ -186,7 +186,16 @@ type Indexer interface { // representing the current revision of the file. UpdateFileDoc(olddoc, newdoc *FileDoc) error // DeleteFileDoc removes from the index the specified file document. + // + // Warning: no sharing revocation here. This is also the path used to + // dissociate a doc from a sharing, which must not revoke it. File + // destruction goes through DestroyFileDoc, which revokes. DeleteFileDoc(doc *FileDoc) error + // DestroyFileDoc removes from the index the specified file document and + // revokes the sharings for which this file is the main file. It is used + // when destroying a file, as destroying bypasses the trash where the + // revocation of a shared file normally happens. + DestroyFileDoc(doc *FileDoc) error // CreateDirDoc creates and add in the index a new directory document. CreateDirDoc(doc *DirDoc) error diff --git a/model/vfs/vfsafero/impl.go b/model/vfs/vfsafero/impl.go index 4f9320fb278..73065f5c296 100644 --- a/model/vfs/vfsafero/impl.go +++ b/model/vfs/vfsafero/impl.go @@ -464,7 +464,7 @@ func (afs *aferoVFS) DestroyFile(doc *vfs.FileDoc) error { if err != nil && !os.IsNotExist(err) { return err } - if err = afs.Indexer.DeleteFileDoc(doc); err != nil { + if err = afs.Indexer.DestroyFileDoc(doc); err != nil { return err } versions, err := vfs.VersionsFor(afs, doc.DocID) diff --git a/model/vfs/vfss3/impl.go b/model/vfs/vfss3/impl.go index e47723b154a..68ca541a0da 100644 --- a/model/vfs/vfss3/impl.go +++ b/model/vfs/vfss3/impl.go @@ -349,6 +349,9 @@ func (sfs *s3VFS) DissociateFile(src, dst *vfs.FileDoc) error { return err } + if err := sfs.Indexer.DeleteFileDoc(src); err != nil { + return err + } return sfs.destroyFileLocked(src) } @@ -413,6 +416,9 @@ func (sfs *s3VFS) DestroyFile(doc *vfs.FileDoc) error { return lockerr } defer sfs.mu.Unlock() + if err := sfs.Indexer.DestroyFileDoc(doc); err != nil { + return err + } return sfs.destroyFileLocked(doc) } @@ -421,9 +427,6 @@ func (sfs *s3VFS) destroyFileLocked(doc *vfs.FileDoc) error { objNames := []string{ MakeObjectKey(sfs.keyPrefix, doc.DocID, doc.InternalID), } - if err := sfs.Indexer.DeleteFileDoc(doc); err != nil { - return err - } destroyed := doc.ByteSize if versions, errv := vfs.VersionsFor(sfs, doc.DocID); errv == nil { for _, v := range versions { diff --git a/model/vfs/vfsswift/impl_v3.go b/model/vfs/vfsswift/impl_v3.go index e06304c7c2c..cd6e4201881 100644 --- a/model/vfs/vfsswift/impl_v3.go +++ b/model/vfs/vfsswift/impl_v3.go @@ -353,6 +353,9 @@ func (sfs *swiftVFSV3) DissociateFile(src, dst *vfs.FileDoc) error { if err := thumbsFS.RemoveThumbs(src, vfs.ThumbnailFormatNames); err != nil { sfs.log.Infof("Cleaning thumbnails in DissociateFile %s has failed: %s", src.ID(), err) } + if err := sfs.Indexer.DeleteFileDoc(src); err != nil { + return err + } return sfs.destroyFileLocked(src) } @@ -418,6 +421,9 @@ func (sfs *swiftVFSV3) DestroyFile(doc *vfs.FileDoc) error { return lockerr } defer sfs.mu.Unlock() + if err := sfs.Indexer.DestroyFileDoc(doc); err != nil { + return err + } return sfs.destroyFileLocked(doc) } @@ -426,9 +432,6 @@ func (sfs *swiftVFSV3) destroyFileLocked(doc *vfs.FileDoc) error { objNames := []string{ MakeObjectNameV3(doc.DocID, doc.InternalID), } - if err := sfs.Indexer.DeleteFileDoc(doc); err != nil { - return err - } destroyed := doc.ByteSize if versions, errv := vfs.VersionsFor(sfs, doc.DocID); errv == nil { for _, v := range versions { diff --git a/web/sharings/drives.go b/web/sharings/drives.go index ef2027f4a47..c4ba89dc408 100644 --- a/web/sharings/drives.go +++ b/web/sharings/drives.go @@ -38,6 +38,7 @@ type docPatch struct { docID string vfs.DocPatch + Delete bool `json:"permanent_delete,omitempty"` } // ListSharedDrives returns the list of the shared drives. @@ -404,6 +405,13 @@ func ModifyMetadataByIDHandler(c echo.Context, inst *instance.Instance, s *shari if err != nil { return files.WrapVfsError(err) } + if patch.Delete { + if rootID, err := s.DriveRootID(); err == nil && c.Param("file-id") == rootID { + if member := GetSharedDriveMember(c); member != nil { + return jsonapi.Forbidden(errors.New("only the owner can destroy the root of a shared drive")) + } + } + } if patch.DirID != nil { rootID, err := s.DriveRootID() if err == nil && c.Param("file-id") == rootID { @@ -469,6 +477,13 @@ func applyPatch(c echo.Context, fs vfs.VFS, patch *docPatch) (err error) { } } + if patch.Delete { + if dir != nil { + return fs.DestroyDirAndContent(dir, fs.EnsureErased) + } + return fs.DestroyFile(file) + } + if patch.DirID != nil { newParent, _, err := fs.DirOrFileByID(*patch.DirID) if err != nil { diff --git a/web/sharings/drives_test.go b/web/sharings/drives_test.go index 9781916c6d9..302b1ca750c 100644 --- a/web/sharings/drives_test.go +++ b/web/sharings/drives_test.go @@ -6267,6 +6267,92 @@ func TestSharedDriveEffectiveAccessOnTrashRoutes(t *testing.T) { }) } +func TestSharedDrivePermanentDeleteViaPatch(t *testing.T) { + if testing.Short() { + t.Skip("an instance is required for this test: test skipped due to the use of --short flag") + } + + env := setupSharedDrivesEnv(t) + eA, _, eD := env.createClients(t) + + // D1: Dave is a read-only member of the whole drive. + d1ID, d1RootID, _ := createSharedDrive(t, DriveCreationMethodFromFolder, + env.acme, env.acmeToken, env.tsA.URL, "PermanentDelete D1", "d1", + []RecipientInfo{{Name: "Dave", Email: "dave@example.net", ReadOnly: true}}) + roFileID := createFile(t, eA, d1RootID, "readonly.txt", env.acmeToken) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, d1ID) + + // D2: Dave has write access. + d2ID, d2RootID, _ := createSharedDrive(t, DriveCreationMethodFromFolder, + env.acme, env.acmeToken, env.tsA.URL, "PermanentDelete D2", "d2", + []RecipientInfo{{Name: "Dave", Email: "dave@example.net", ReadOnly: false}}) + rwFileID := createFile(t, eA, d2RootID, "writable.txt", env.acmeToken) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, d2ID) + + permanentDeleteBody := func(fileID string) []byte { + return []byte(`{ + "data": { + "type": "io.cozy.files", + "id": "` + fileID + `", + "attributes": { "permanent_delete": true } + } + }`) + } + + t.Run("ReadOnlyMemberDenied", func(t *testing.T) { + eD.PATCH("/sharings/drives/"+d1ID+"/"+roFileID). + WithHeader("Authorization", "Bearer "+env.daveToken). + WithHeader("Content-Type", "application/json"). + WithBytes(permanentDeleteBody(roFileID)). + Expect().Status(403) + + eA.GET("/files/"+roFileID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(200) + }) + + t.Run("ReadWriteMemberAllowed", func(t *testing.T) { + eD.PATCH("/sharings/drives/"+d2ID+"/"+rwFileID). + WithHeader("Authorization", "Bearer "+env.daveToken). + WithHeader("Content-Type", "application/json"). + WithBytes(permanentDeleteBody(rwFileID)). + Expect().Status(200) + + eA.GET("/files/"+rwFileID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(404) + }) + + t.Run("ReadWriteMemberRootDenied", func(t *testing.T) { + eD.PATCH("/sharings/drives/"+d2ID+"/"+d2RootID). + WithHeader("Authorization", "Bearer "+env.daveToken). + WithHeader("Content-Type", "application/json"). + WithBytes(permanentDeleteBody(d2RootID)). + Expect().Status(403) + + eA.GET("/files/"+d2RootID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(200) + }) + + // The owner can destroy the root: the destroy-time hook revokes and + // deletes the drive sharing. + t.Run("OwnerRootAllowedRevokesSharing", func(t *testing.T) { + d3ID, d3RootID, _ := createSharedDrive(t, DriveCreationMethodFromFolder, + env.acme, env.acmeToken, env.tsA.URL, "PermanentDelete D3", "d3", nil) + + eA.PATCH("/sharings/drives/"+d3ID+"/"+d3RootID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + WithHeader("Content-Type", "application/json"). + WithBytes(permanentDeleteBody(d3RootID)). + Expect().Status(200) + + eA.GET("/sharings/"+d3ID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(404) + }) +} + func TestSharedDriveEffectiveAccessOnMetadataByPath(t *testing.T) { if testing.Short() { t.Skip("an instance is required for this test: test skipped due to the use of --short flag") From 8cb9d8e68bab0fee5630d0f3ea947c7763489e1d Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Mon, 21 Sep 2026 12:47:36 +0200 Subject: [PATCH 2/4] feat: Route shared drive moves via drive routes Cross-stack moves involving a shared drive were executed through the direct `/files` routes with the share-interact token. That token carries all verbs on the drive content, so the member authorization enforced by `guardSharedDriveRouteForMember` on the dedicated drive routes was bypassed: a read-only member could add files to a drive or permanently delete from it. Remote errors were also mishandled: logged and swallowed, or mapped to 500 by `files.WrapVfsError`. The files API operations move from `Client` to a new `FilesClient`, created with `NewFilesClient`: when scoped to a drive, its `filesPath` helper swaps the `/files` prefix for `/sharings/drives/` on every operation, so the stack hosting the drive authorizes each request; an empty drive ID keeps routing through `/files`. The move flows wrap their remote clients accordingly, and `wrapRemoteErr` surfaces the remote status faithfully instead of the previous log-and-continue, with `remoteStatusTextCodes` covering the common texts. The per-file deletions inside the copy loop of `moveDirBetweenSharedDrives` are dropped: the recursive root directory deletion after the loop already covers them, and no transient per-file deletion error can abort the copy loop midway. A failed source deletion after a successful copy now returns an error instead of being logged, leaving the copy at the destination; `docs/shared-drives.md` documents this non-atomic behavior. The files tests move from `client_test.go` to `files_test.go`, `web/sharings/move.go` and the `cmd` tests are adapted to the new type, and a `newFilesClient` helper in `cmd/root.go` centralizes the creation of wrapped clients for the `files` and `fix` commands. --- client/client_test.go | 93 ------------------------- client/files.go | 109 ++++++++++++++++++----------- client/files_test.go | 105 ++++++++++++++++++++++++++++ cmd/cmd_test.go | 4 +- cmd/files.go | 26 +++---- cmd/fix.go | 2 +- cmd/root.go | 5 ++ docs/shared-drives.md | 1 + web/sharings/move.go | 155 ++++++++++++++++++++++++++++++------------ 9 files changed, 305 insertions(+), 195 deletions(-) create mode 100644 client/files_test.go diff --git a/client/client_test.go b/client/client_test.go index 412e9080b13..82c593e75a1 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -4,22 +4,13 @@ import ( "encoding/json" "errors" "io" - "net" "net/http" "net/url" "strings" "testing" - "time" "github.com/cozy/cozy-stack/client/auth" "github.com/cozy/cozy-stack/client/request" - "github.com/cozy/cozy-stack/pkg/config/config" - "github.com/cozy/cozy-stack/pkg/consts" - "github.com/cozy/cozy-stack/tests/testutils" - weberrors "github.com/cozy/cozy-stack/web/errors" - webfiles "github.com/cozy/cozy-stack/web/files" - "github.com/cozy/cozy-stack/web/middlewares" - "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -135,87 +126,3 @@ func TestReadInstanceStorageState(t *testing.T) { assert.Contains(t, string(output), `"fs_scheme":"s3"`) assert.Contains(t, string(output), `"blocking_reason":"MOVING"`) } - -// filesTestClient creates a minimal live files server and an authenticated -// client ready to call files routes. -func filesTestClient(t *testing.T) *Client { - config.UseTestFile(t) - testutils.NeedCouchdb(t) - setup := testutils.NewSetup(t, t.Name()) - config.GetConfig().Fs.URL = &url.URL{Scheme: "file", Host: "localhost", Path: t.TempDir()} - - inst := setup.GetTestInstance() - _, token := setup.GetTestClient(consts.Files + " " + consts.CertifiedCarbonCopy + " " + consts.CertifiedElectronicSafe) - ts := setup.GetTestServer("/files", webfiles.Routes, func(r *echo.Echo) *echo.Echo { - secure := middlewares.Secure(&middlewares.SecureConfig{CSPDefaultSrc: []middlewares.CSPSource{middlewares.CSPSrcSelf}, CSPFrameAncestors: []middlewares.CSPSource{middlewares.CSPSrcNone}}) - r.Use(secure) - return r - }) - ts.Config.Handler.(*echo.Echo).HTTPErrorHandler = weberrors.ErrorHandler - t.Cleanup(ts.Close) - - u, err := url.Parse(ts.URL) - require.NoError(t, err) - hostPort := u.Host - if _, _, e := net.SplitHostPort(hostPort); e != nil { - hostPort = u.Host - } - - return &Client{ - Addr: hostPort, - Domain: inst.Domain, - Scheme: u.Scheme, - Client: &http.Client{Timeout: 10 * time.Second}, - Authorizer: &request.BearerAuthorizer{Token: token}, - } -} - -// withListChildrenPageSize temporarily overrides ListChildrenPageSize. -func withListChildrenPageSize(t *testing.T, size string) { - old := ListChildrenPageSize - ListChildrenPageSize = size - t.Cleanup(func() { ListChildrenPageSize = old }) -} - -func TestListChildrenByDirID_Pagination(t *testing.T) { - if testing.Short() { - t.Skip("requires instance; skipped with --short") - } - - c := filesTestClient(t) - - parent, err := c.Mkdir("/client-pagination-root") - require.NoError(t, err) - - withListChildrenPageSize(t, "2") - - names := []string{"a.txt", "b.txt", "c.txt"} - for _, name := range names { - _, err := c.Upload(&Upload{ - Name: name, - DirID: parent.ID, - Contents: strings.NewReader("foo"), - ContentType: "text/plain", - }) - require.NoError(t, err) - } - - _, err = c.Req(&request.Options{ - Method: "POST", - Path: "/files/" + url.PathEscape(parent.ID), - Queries: url.Values{"Type": {"directory"}, "Name": {"child"}}, - }) - require.NoError(t, err) - - children, err := c.ListChildrenByDirID(parent.ID) - require.NoError(t, err) - assert.Equal(t, 4, len(children)) - gotNames := make(map[string]bool) - for _, ch := range children { - gotNames[ch.Attrs.Name] = true - } - assert.True(t, gotNames["a.txt"]) - assert.True(t, gotNames["b.txt"]) - assert.True(t, gotNames["c.txt"]) - assert.True(t, gotNames["child"]) -} diff --git a/client/files.go b/client/files.go index 9d443980d23..d5c4a898613 100644 --- a/client/files.go +++ b/client/files.go @@ -97,11 +97,38 @@ type FilePatch struct { // listing directory children. Tests can override this to force pagination. var ListChildrenPageSize = "100" +// FilesClient is a specialized client for the files API. It embeds Client +// for authentication and transport, and routes its operations either through +// /files (default) or through the /sharings/drives/ endpoints of the +// given shared drive, so the stack hosting the drive enforces member +// authorization. All the operations routed through filesPath have a +// drive-route equivalent (reads, creation, metadata patch, upload, +// overwrite, trash, restore, destroy). +type FilesClient struct { + *Client + driveID string +} + +// NewFilesClient wraps a Client into a FilesClient scoped to the given +// shared drive. An empty driveID routes the operations through /files. +func NewFilesClient(c *Client, driveID string) *FilesClient { + return &FilesClient{Client: c, driveID: driveID} +} + +// filesPath prefixes p with the files API root: the shared-drive endpoints +// when the client is scoped to a drive, /files otherwise. +func (c *FilesClient) filesPath(p string) string { + if c.driveID != "" { + return "/sharings/drives/" + url.PathEscape(c.driveID) + p + } + return "/files" + p +} + // GetFileByID returns a File given the specified ID -func (c *Client) GetFileByID(id string) (*File, error) { +func (c *FilesClient) GetFileByID(id string) (*File, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/" + url.PathEscape(id), + Path: c.filesPath("/" + url.PathEscape(id)), }) if err != nil { return nil, err @@ -110,10 +137,10 @@ func (c *Client) GetFileByID(id string) (*File, error) { } // GetFileByPath returns a File given the specified path -func (c *Client) GetFileByPath(name string) (*File, error) { +func (c *FilesClient) GetFileByPath(name string) (*File, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/metadata", + Path: c.filesPath("/metadata"), Queries: url.Values{"Path": {name}}, }) if err != nil { @@ -123,10 +150,10 @@ func (c *Client) GetFileByPath(name string) (*File, error) { } // GetDirByID returns a Dir given the specified ID -func (c *Client) GetDirByID(id string) (*Dir, error) { +func (c *FilesClient) GetDirByID(id string) (*Dir, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/" + url.PathEscape(id), + Path: c.filesPath("/" + url.PathEscape(id)), }) if err != nil { return nil, err @@ -135,10 +162,10 @@ func (c *Client) GetDirByID(id string) (*Dir, error) { } // GetDirByPath returns a Dir given the specified path -func (c *Client) GetDirByPath(name string) (*Dir, error) { +func (c *FilesClient) GetDirByPath(name string) (*Dir, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/metadata", + Path: c.filesPath("/metadata"), Queries: url.Values{"Path": {name}}, }) if err != nil { @@ -148,10 +175,10 @@ func (c *Client) GetDirByPath(name string) (*Dir, error) { } // GetDirOrFileByPath returns a DirOrFile given the specified path -func (c *Client) GetDirOrFileByPath(name string) (*DirOrFile, error) { +func (c *FilesClient) GetDirOrFileByPath(name string) (*DirOrFile, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/metadata", + Path: c.filesPath("/metadata"), Queries: url.Values{"Path": {name}}, }) if err != nil { @@ -162,20 +189,20 @@ func (c *Client) GetDirOrFileByPath(name string) (*DirOrFile, error) { // Mkdir creates a directory with the specified path. If the directory's parent // does not exist, an error is returned. -func (c *Client) Mkdir(name string) (*Dir, error) { +func (c *FilesClient) Mkdir(name string) (*Dir, error) { return c.mkdir(name, "") } // Mkdirall creates a directory with the specified path. If the directory's // parent does not exist, all intermediary parents are created. -func (c *Client) Mkdirall(name string) (*Dir, error) { +func (c *FilesClient) Mkdirall(name string) (*Dir, error) { return c.mkdir(name, "true") } -func (c *Client) mkdir(name string, recur string) (*Dir, error) { +func (c *FilesClient) mkdir(name string, recur string) (*Dir, error) { res, err := c.Req(&request.Options{ Method: "POST", - Path: "/files/", + Path: c.filesPath("/"), Queries: url.Values{ "Path": {name}, "Type": {"directory"}, @@ -190,10 +217,10 @@ func (c *Client) mkdir(name string, recur string) (*Dir, error) { // DownloadByID is used to download a file's content given its ID. It returns // a io.ReadCloser that you can read from. -func (c *Client) DownloadByID(id string) (io.ReadCloser, error) { +func (c *FilesClient) DownloadByID(id string) (io.ReadCloser, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/download/" + url.PathEscape(id), + Path: c.filesPath("/download/" + url.PathEscape(id)), }) if err != nil { return nil, err @@ -203,10 +230,10 @@ func (c *Client) DownloadByID(id string) (io.ReadCloser, error) { // DownloadByPath is used to download a file's content given its path. It // returns a io.ReadCloser that you can read from. -func (c *Client) DownloadByPath(name string) (io.ReadCloser, error) { +func (c *FilesClient) DownloadByPath(name string) (io.ReadCloser, error) { res, err := c.Req(&request.Options{ Method: "GET", - Path: "/files/download", + Path: c.filesPath("/download"), Queries: url.Values{"Path": {name}}, }) if err != nil { @@ -217,7 +244,7 @@ func (c *Client) DownloadByPath(name string) (io.ReadCloser, error) { // Upload is used to upload a new file from an using a Upload instance. If the // ContentMD5 field is not nil, the file integrity is checked. -func (c *Client) Upload(u *Upload) (*File, error) { +func (c *FilesClient) Upload(u *Upload) (*File, error) { headers := make(request.Headers) if u.ContentMD5 != nil { headers["Content-MD5"] = base64.StdEncoding.EncodeToString(u.ContentMD5) @@ -237,13 +264,13 @@ func (c *Client) Upload(u *Upload) (*File, error) { if u.Overwrite { opts.Method = "PUT" - opts.Path = "/files/" + url.PathEscape(u.FileID) + opts.Path = c.filesPath("/" + url.PathEscape(u.FileID)) if u.FileRev != "" { headers["If-Match"] = u.FileRev } } else { opts.Method = "POST" - opts.Path = "/files/" + url.PathEscape(u.DirID) + opts.Path = c.filesPath("/" + url.PathEscape(u.DirID)) opts.Queries = url.Values{ "Type": {"file"}, "Name": {u.Name}, @@ -258,7 +285,7 @@ func (c *Client) Upload(u *Upload) (*File, error) { // UpdateAttrsByID is used to update the attributes of a file or directory // of the specified ID -func (c *Client) UpdateAttrsByID(id string, patch *FilePatch) (*DirOrFile, error) { +func (c *FilesClient) UpdateAttrsByID(id string, patch *FilePatch) (*DirOrFile, error) { body, err := writeJSONAPI(patch) if err != nil { return nil, err @@ -269,7 +296,7 @@ func (c *Client) UpdateAttrsByID(id string, patch *FilePatch) (*DirOrFile, error } res, err := c.Req(&request.Options{ Method: "PATCH", - Path: "/files/" + id, + Path: c.filesPath("/" + id), Body: body, Headers: headers, }) @@ -281,7 +308,7 @@ func (c *Client) UpdateAttrsByID(id string, patch *FilePatch) (*DirOrFile, error // UpdateAttrsByPath is used to update the attributes of a file or directory // of the specified path -func (c *Client) UpdateAttrsByPath(name string, patch *FilePatch) (*DirOrFile, error) { +func (c *FilesClient) UpdateAttrsByPath(name string, patch *FilePatch) (*DirOrFile, error) { body, err := writeJSONAPI(patch) if err != nil { return nil, err @@ -292,7 +319,7 @@ func (c *Client) UpdateAttrsByPath(name string, patch *FilePatch) (*DirOrFile, e } res, err := c.Req(&request.Options{ Method: "PATCH", - Path: "/files/metadata", + Path: c.filesPath("/metadata"), Headers: headers, Body: body, Queries: url.Values{"Path": {name}}, @@ -305,7 +332,7 @@ func (c *Client) UpdateAttrsByPath(name string, patch *FilePatch) (*DirOrFile, e // Move is used to move a file or directory from a given path to the other // given path -func (c *Client) Move(from, to string) error { +func (c *FilesClient) Move(from, to string) error { doc, err := c.GetDirByPath(path.Dir(to)) if err != nil { return err @@ -322,10 +349,10 @@ func (c *Client) Move(from, to string) error { // TrashByID is used to move a file or directory specified by its ID to the // trash -func (c *Client) TrashByID(id string) error { +func (c *FilesClient) TrashByID(id string) error { _, err := c.Req(&request.Options{ Method: "DELETE", - Path: "/files/" + url.PathEscape(id), + Path: c.filesPath("/" + url.PathEscape(id)), NoResponse: true, }) return err @@ -333,7 +360,7 @@ func (c *Client) TrashByID(id string) error { // TrashByPath is used to move a file or directory specified by its path to the // trash -func (c *Client) TrashByPath(name string) error { +func (c *FilesClient) TrashByPath(name string) error { doc, err := c.GetDirOrFileByPath(name) if err != nil { return err @@ -343,10 +370,10 @@ func (c *Client) TrashByPath(name string) error { // RestoreByID is used to restore a file or directory from the trash given its // ID -func (c *Client) RestoreByID(id string) error { +func (c *FilesClient) RestoreByID(id string) error { _, err := c.Req(&request.Options{ Method: "POST", - Path: "/files/trash/" + url.PathEscape(id), + Path: c.filesPath("/trash/" + url.PathEscape(id)), NoResponse: true, }) return err @@ -354,7 +381,7 @@ func (c *Client) RestoreByID(id string) error { // RestoreByPath is used to restore a file or directory from the trash given its // path -func (c *Client) RestoreByPath(name string) error { +func (c *FilesClient) RestoreByPath(name string) error { doc, err := c.GetDirOrFileByPath(name) if err != nil { return err @@ -364,10 +391,10 @@ func (c *Client) RestoreByPath(name string) error { // PermanentDeleteByID is used to delete a file or directory specified by its // ID, not just putting it in the trash -func (c *Client) PermanentDeleteByID(id string) error { +func (c *FilesClient) PermanentDeleteByID(id string) error { _, err := c.Req(&request.Options{ Method: "PATCH", - Path: "/files/" + url.PathEscape(id), + Path: c.filesPath("/" + url.PathEscape(id)), Body: strings.NewReader(`{"data": {"attributes": {"permanent_delete": true}}}`), NoResponse: true, }) @@ -376,7 +403,7 @@ func (c *Client) PermanentDeleteByID(id string) error { // PermanentDeleteByPath is used to delete a file or directory specified by its // path, not just putting it in the trash -func (c *Client) PermanentDeleteByPath(name string) error { +func (c *FilesClient) PermanentDeleteByPath(name string) error { doc, err := c.GetDirOrFileByPath(name) if err != nil { return err @@ -386,7 +413,7 @@ func (c *Client) PermanentDeleteByPath(name string) error { // getIncludedPage performs a GET request on reqPath with reqQuery and returns // the page's included DirOrFile items and the next link (empty if none). -func (c *Client) getIncludedPage(reqPath string, reqQuery url.Values) ([]*DirOrFile, string, error) { +func (c *FilesClient) getIncludedPage(reqPath string, reqQuery url.Values) ([]*DirOrFile, string, error) { res, err := c.Req(&request.Options{ Method: "GET", Path: reqPath, @@ -406,8 +433,8 @@ func (c *Client) getIncludedPage(reqPath string, reqQuery url.Values) ([]*DirOrF // ListChildrenByDirID returns all direct child items (files and directories) // of the directory identified by its ID. It transparently follows pagination // and returns the complete list. -func (c *Client) ListChildrenByDirID(id string) ([]*DirOrFile, error) { - reqPath := "/files/" + url.PathEscape(id) +func (c *FilesClient) ListChildrenByDirID(id string) ([]*DirOrFile, error) { + reqPath := c.filesPath("/" + url.PathEscape(id)) reqQuery := url.Values{"page[limit]": {ListChildrenPageSize}} var all []*DirOrFile for { @@ -434,7 +461,7 @@ type WalkFn func(name string, doc *DirOrFile, err error) error // WalkByPath is used to walk along the filesystem tree originated at the // specified root path. -func (c *Client) WalkByPath(root string, walkFn WalkFn) error { +func (c *FilesClient) WalkByPath(root string, walkFn WalkFn) error { doc, err := c.GetDirOrFileByPath(path.Clean(root)) root = path.Clean(root) if err != nil { @@ -443,7 +470,7 @@ func (c *Client) WalkByPath(root string, walkFn WalkFn) error { return walk(c, root, doc, walkFn) } -func walk(c *Client, name string, doc *DirOrFile, walkFn WalkFn) error { +func walk(c *FilesClient, name string, doc *DirOrFile, walkFn WalkFn) error { isDir := doc.Attrs.Type == DirType err := walkFn(name, doc, nil) @@ -458,7 +485,7 @@ func walk(c *Client, name string, doc *DirOrFile, walkFn WalkFn) error { return nil } - reqPath := "/files/" + url.PathEscape(doc.ID) + reqPath := c.filesPath("/" + url.PathEscape(doc.ID)) reqQuery := url.Values{"page[limit]": {ListChildrenPageSize}} for { included, next, err := c.getIncludedPage(reqPath, reqQuery) diff --git a/client/files_test.go b/client/files_test.go new file mode 100644 index 00000000000..e70a9a9c50f --- /dev/null +++ b/client/files_test.go @@ -0,0 +1,105 @@ +package client + +import ( + "net" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "github.com/cozy/cozy-stack/client/request" + "github.com/cozy/cozy-stack/pkg/config/config" + "github.com/cozy/cozy-stack/pkg/consts" + "github.com/cozy/cozy-stack/tests/testutils" + weberrors "github.com/cozy/cozy-stack/web/errors" + webfiles "github.com/cozy/cozy-stack/web/files" + "github.com/cozy/cozy-stack/web/middlewares" + "github.com/labstack/echo/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// filesTestClient creates a minimal live files server and an authenticated +// client ready to call files routes. +func filesTestClient(t *testing.T) *Client { + config.UseTestFile(t) + testutils.NeedCouchdb(t) + setup := testutils.NewSetup(t, t.Name()) + config.GetConfig().Fs.URL = &url.URL{Scheme: "file", Host: "localhost", Path: t.TempDir()} + + inst := setup.GetTestInstance() + _, token := setup.GetTestClient(consts.Files + " " + consts.CertifiedCarbonCopy + " " + consts.CertifiedElectronicSafe) + ts := setup.GetTestServer("/files", webfiles.Routes, func(r *echo.Echo) *echo.Echo { + secure := middlewares.Secure(&middlewares.SecureConfig{CSPDefaultSrc: []middlewares.CSPSource{middlewares.CSPSrcSelf}, CSPFrameAncestors: []middlewares.CSPSource{middlewares.CSPSrcNone}}) + r.Use(secure) + return r + }) + ts.Config.Handler.(*echo.Echo).HTTPErrorHandler = weberrors.ErrorHandler + t.Cleanup(ts.Close) + + u, err := url.Parse(ts.URL) + require.NoError(t, err) + hostPort := u.Host + if _, _, e := net.SplitHostPort(hostPort); e != nil { + hostPort = u.Host + } + + return &Client{ + Addr: hostPort, + Domain: inst.Domain, + Scheme: u.Scheme, + Client: &http.Client{Timeout: 10 * time.Second}, + Authorizer: &request.BearerAuthorizer{Token: token}, + } +} + +// withListChildrenPageSize temporarily overrides ListChildrenPageSize. +func withListChildrenPageSize(t *testing.T, size string) { + old := ListChildrenPageSize + ListChildrenPageSize = size + t.Cleanup(func() { ListChildrenPageSize = old }) +} + +func TestListChildrenByDirID_Pagination(t *testing.T) { + if testing.Short() { + t.Skip("requires instance; skipped with --short") + } + + c := NewFilesClient(filesTestClient(t), "") + + parent, err := c.Mkdir("/client-pagination-root") + require.NoError(t, err) + + withListChildrenPageSize(t, "2") + + names := []string{"a.txt", "b.txt", "c.txt"} + for _, name := range names { + _, err := c.Upload(&Upload{ + Name: name, + DirID: parent.ID, + Contents: strings.NewReader("foo"), + ContentType: "text/plain", + }) + require.NoError(t, err) + } + + _, err = c.Req(&request.Options{ + Method: "POST", + Path: "/files/" + url.PathEscape(parent.ID), + Queries: url.Values{"Type": {"directory"}, "Name": {"child"}}, + }) + require.NoError(t, err) + + children, err := c.ListChildrenByDirID(parent.ID) + require.NoError(t, err) + assert.Equal(t, 4, len(children)) + gotNames := make(map[string]bool) + for _, ch := range children { + gotNames[ch.Attrs.Name] = true + } + assert.True(t, gotNames["a.txt"]) + assert.True(t, gotNames["b.txt"]) + assert.True(t, gotNames["c.txt"]) + assert.True(t, gotNames["child"]) +} diff --git a/cmd/cmd_test.go b/cmd/cmd_test.go index 8d0a42b85b8..559d93adcd9 100644 --- a/cmd/cmd_test.go +++ b/cmd/cmd_test.go @@ -70,11 +70,11 @@ func TestExecCommand(t *testing.T) { } buf := new(bytes.Buffer) - err = execCommand(testClient, "mkdir /hello-test", buf) + err = execCommand(client.NewFilesClient(testClient, ""), "mkdir /hello-test", buf) assert.NoError(t, err) buf = new(bytes.Buffer) - err = execCommand(testClient, "ls /", buf) + err = execCommand(client.NewFilesClient(testClient, ""), "ls /", buf) assert.NoError(t, err) assert.True(t, bytes.Contains(buf.Bytes(), []byte("hello-test"))) } diff --git a/cmd/files.go b/cmd/files.go index 3c9b16d436e..f2fda31b1db 100644 --- a/cmd/files.go +++ b/cmd/files.go @@ -72,7 +72,7 @@ var execFilesCmd = &cobra.Command{ errPrintfln("%s", errMissingDomain) return cmd.Usage() } - c := newClient(flagDomain, consts.Files) + c := newFilesClient(flagDomain, "") command := args[0] err := execCommand(c, command, os.Stdout) if errors.Is(err, errFilesExec) { @@ -103,7 +103,7 @@ var importFilesCmd = &cobra.Command{ } } - c := newClient(flagDomain, consts.Files) + c := newFilesClient(flagDomain, "") return importFiles(c, flagImportFrom, flagImportTo, match) }, } @@ -149,7 +149,7 @@ var usageFilesCmd = &cobra.Command{ }, } -func execCommand(c *client.Client, command string, w io.Writer) error { +func execCommand(c *client.FilesClient, command string, w io.Writer) error { args := splitArgs(command) if len(args) == 0 { return errFilesExec @@ -214,7 +214,7 @@ func execCommand(c *client.Client, command string, w io.Writer) error { return errFilesExec } -func mkdirCmd(c *client.Client, name string, mkdirP bool) error { +func mkdirCmd(c *client.FilesClient, name string, mkdirP bool) error { var err error if mkdirP { _, err = c.Mkdirall(name) @@ -224,7 +224,7 @@ func mkdirCmd(c *client.Client, name string, mkdirP bool) error { return err } -func lsCmd(c *client.Client, root string, w io.Writer, verbose, human, all bool) error { +func lsCmd(c *client.FilesClient, root string, w io.Writer, verbose, human, all bool) error { type filePrint struct { id string typ string @@ -330,7 +330,7 @@ func lsCmd(c *client.Client, root string, w io.Writer, verbose, human, all bool) return nil } -func treeCmd(c *client.Client, root string, w io.Writer, verbose bool) error { +func treeCmd(c *client.FilesClient, root string, w io.Writer, verbose bool) error { root = path.Clean(root) return c.WalkByPath(root, func(name string, doc *client.DirOrFile, err error) error { @@ -363,7 +363,7 @@ func treeCmd(c *client.Client, root string, w io.Writer, verbose bool) error { }) } -func attrsCmd(c *client.Client, name string, w io.Writer) error { +func attrsCmd(c *client.FilesClient, name string, w io.Writer) error { doc, err := c.GetDirOrFileByPath(name) if err != nil { return err @@ -373,7 +373,7 @@ func attrsCmd(c *client.Client, name string, w io.Writer) error { return enc.Encode(doc) } -func catCmd(c *client.Client, name string, w io.Writer) error { +func catCmd(c *client.FilesClient, name string, w io.Writer) error { r, err := c.DownloadByPath(name) if err != nil { return err @@ -385,23 +385,23 @@ func catCmd(c *client.Client, name string, w io.Writer) error { return err } -func mvCmd(c *client.Client, from, to string) error { +func mvCmd(c *client.FilesClient, from, to string) error { return c.Move(from, to) } -func rmCmd(c *client.Client, name string, force, recur bool) error { +func rmCmd(c *client.FilesClient, name string, force, recur bool) error { if force { return c.PermanentDeleteByPath(name) } return c.TrashByPath(name) } -func restoreCmd(c *client.Client, name string) error { +func restoreCmd(c *client.FilesClient, name string) error { return c.RestoreByPath(name) } type importer struct { - c *client.Client + c *client.FilesClient paths map[string]string } @@ -447,7 +447,7 @@ func (i *importer) upload(localname, distname string) error { return err } -func importFiles(c *client.Client, from, to string, match *regexp.Regexp) error { +func importFiles(c *client.FilesClient, from, to string, match *regexp.Regexp) error { from = path.Clean(from) to = path.Clean(to) diff --git a/cmd/fix.go b/cmd/fix.go index bfdbbd99c21..333c0404437 100644 --- a/cmd/fix.go +++ b/cmd/fix.go @@ -44,7 +44,7 @@ var mimeFixerCmd = &cobra.Command{ if len(args) == 0 { return cmd.Usage() } - c := newClient(args[0], consts.Files) + c := newFilesClient(args[0], "") return c.WalkByPath("/", func(name string, doc *client.DirOrFile, err error) error { if err != nil { return err diff --git a/cmd/root.go b/cmd/root.go index 2a1d2c4e485..61becf75bb8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -12,6 +12,7 @@ import ( "github.com/cozy/cozy-stack/client/request" build "github.com/cozy/cozy-stack/pkg/config" "github.com/cozy/cozy-stack/pkg/config/config" + "github.com/cozy/cozy-stack/pkg/consts" "github.com/cozy/cozy-stack/pkg/tlsclient" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -69,6 +70,10 @@ func newClient(domain string, scopes ...string) *client.Client { return client } +func newFilesClient(domain, driveID string) *client.FilesClient { + return client.NewFilesClient(newClient(domain, consts.Files), driveID) +} + func newAdminClient() *client.AdminClient { pass := []byte(os.Getenv("COZY_ADMIN_PASSPHRASE")) if len(pass) == 0 { diff --git a/docs/shared-drives.md b/docs/shared-drives.md index be0cb55050a..8c7c40766cd 100644 --- a/docs/shared-drives.md +++ b/docs/shared-drives.md @@ -732,6 +732,7 @@ Notes on behavior: - Cross-stack operations perform a remote download/upload and delete the remote source upon success (only for move operations). - When `copy: true`, source files and directories are preserved in their original location. - When `copy: false` (default), source files and directories are deleted after successful copy to destination. +- Moves are not atomic: the source is deleted only after the copy succeeded. If that deletion fails, the request returns an error, but the copy remains at the destination (retrying the move creates a suffixed duplicate, e.g. `name (2)`). Additional rules for file-root shared drives: diff --git a/web/sharings/move.go b/web/sharings/move.go index cda807d4066..eafa4e899f7 100644 --- a/web/sharings/move.go +++ b/web/sharings/move.go @@ -219,6 +219,63 @@ func MoveHandler(c echo.Context) error { } } +// remoteStatusTextCodes maps the English status texts a remote stack may +// return as an unparseable error body (e.g. an HTML error page from a proxy) +// back to a numeric status code. Only the codes meaningful to a move are +// listed. +// We need this because request.Error keeps only strings. Drop this if +// request.Error gains the numeric code +// (.opencode/issues/client-request-error-loses-status-code.md). +var remoteStatusTextCodes = map[string]int{ + "Bad Request": http.StatusBadRequest, + "Forbidden": http.StatusForbidden, + "Not Found": http.StatusNotFound, + "Conflict": http.StatusConflict, + "Unprocessable Entity": http.StatusUnprocessableEntity, + "Bad Gateway": http.StatusBadGateway, + "Service Unavailable": http.StatusServiceUnavailable, + "Gateway Timeout": http.StatusGatewayTimeout, +} + +// wrapRemoteErr converts a remote client error into a jsonapi error that +// preserves the remote status code, and logs the raw error for the server +// side. request.Error carries the status only as a string: the numeric form +// ("403") for structured jsonapi errors, the English status text as a last +// resort. A remote 401 is mapped to 502: it means the member token was +// rejected by the remote stack, not that the caller's credentials are bad. +// Errors that cannot be classified (remote stack failure, +// network-level error) yield a generic 500: the raw error may carry internal +// URLs, it stays in the logs. `what` describes the failing remote operation +// for the error detail. +func wrapRemoteErr(inst *instance.Instance, err error, what string) error { + inst.Logger().WithNamespace("move").Warnf("%s: %v", what, err) + var reqErr *request.Error + if !errors.As(err, &reqErr) { + return jsonapi.Errorf(http.StatusInternalServerError, "%s", what) + } + code := http.StatusInternalServerError + numeric := false + if c, cerr := strconv.Atoi(reqErr.Status); cerr == nil { + code = c + numeric = true + } else if c, ok := remoteStatusTextCodes[reqErr.Status]; ok { + code = c + } + // A remote 401 means the member token was rejected by the remote stack + // (expired/revoked credentials), not that the caller's own credentials + // are bad: echoing 401 back could trigger a token refresh against the + // wrong stack. Surface it as a bad gateway instead. + if code == http.StatusUnauthorized { + code = http.StatusBadGateway + } + if numeric { + return jsonapi.Errorf(code, "%s: %s", what, reqErr.Error()) + } + // The status text means the body could not be parsed (e.g. an HTML error + // page): only echo back the title, not the raw body. + return jsonapi.Errorf(code, "%s: %s", what, reqErr.Title) +} + // Same-stack moves (instance ↔ instance) func moveDirSameStack(c echo.Context, srcInst *instance.Instance, destInst *instance.Instance, sourceDirID string, destDirID string, destSharing *sharing.Sharing, copy bool) error { @@ -399,11 +456,11 @@ func moveFileFromSharedDriveCore(inst *instance.Instance, sourceInstanceURL stri if err != nil { return nil, err } - srcClient := NewRemoteClient(u, bearer) + srcClient := client.NewFilesClient(NewRemoteClient(u, bearer), s.ID()) srcFile, err := srcClient.GetFileByID(fileID) if err != nil { - return nil, files.WrapVfsError(err) + return nil, wrapRemoteErr(inst, err, "could not read source file on the remote stack") } newFileDoc, err := createFileDocFromRemoteFile(srcFile, destDir.DocID) @@ -431,7 +488,7 @@ func moveFileFromSharedDriveCore(inst *instance.Instance, sourceInstanceURL stri if err != nil { // Best-effort close to avoid leaking descriptors on error paths _ = fd.Close() - return nil, files.WrapVfsError(err) + return nil, wrapRemoteErr(inst, err, "could not download source file on the remote stack") } defer rc.Close() @@ -444,8 +501,11 @@ func moveFileFromSharedDriveCore(inst *instance.Instance, sourceInstanceURL stri return nil, err } if delete { + // fail-after-copy: the destination copy is already in place, so a + // failed source delete reports an error but leaves the copy (a retry + // creates a suffixed duplicate). if err := srcClient.PermanentDeleteByID(fileID); err != nil { - inst.Logger().WithNamespace("move").Warnf("Could not delete source file: %v", err) + return nil, wrapRemoteErr(inst, err, "could not delete source file on the remote stack") } } return newFileDoc, nil @@ -466,12 +526,12 @@ func moveDirFromSharedDrive(c echo.Context, inst *instance.Instance, sourceInsta if err != nil { return err } - srcClient := NewRemoteClient(u, bearer) + srcClient := client.NewFilesClient(NewRemoteClient(u, bearer), s.ID()) // Get the remote directory structure dirs, filesToMove, err := remoteContentToMove(srcClient, sourceDirID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read source directory on the remote stack") } // Map from source dirID to destination dirID @@ -519,8 +579,11 @@ func moveDirFromSharedDrive(c echo.Context, inst *instance.Instance, sourceInsta // Delete source directories bottom-up (reverse order) only if not copying if !copy { + // fail-after-copy: the destination tree is already in place, so a + // failed source delete reports an error but leaves it (a retry + // creates suffixed duplicates). if err := srcClient.PermanentDeleteByID(sourceDirID); err != nil { - inst.Logger().WithNamespace("move").Warnf("Could not delete source directory: %v", err) + return wrapRemoteErr(inst, err, "could not delete source directory on the remote stack") } } @@ -568,11 +631,11 @@ func moveDirToSharedDrive(c echo.Context, srcInst *instance.Instance, } dstDir, err := dstClient.GetDirByID(parentDestID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(srcInst, err, "could not read destination directory on the remote stack") } newID, err := ensureRemoteChildDir(dstClient, dstDir, d.DocName) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(srcInst, err, "could not create directory on the remote stack") } srcToDstDirID[d.DocID] = newID } @@ -599,11 +662,11 @@ func moveDirToSharedDrive(c echo.Context, srcInst *instance.Instance, dstDir, err := dstClient.GetDirByID(destDirID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(srcInst, err, "could not read destination directory on the remote stack") } movedDir, err := dstClient.GetDirByPath(dstDir.Attrs.Fullpath + "/" + srcRoot.DocName) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(srcInst, err, "could not read moved directory on the remote stack") } return respondRemoteUploadDir(c, movedDir, s.ID()) @@ -639,11 +702,11 @@ func moveFileToSharedDriveCore(inst *instance.Instance, // Optimistic upload with conflict retry dstParent, err := dstClient.GetDirByID(targetDirID) if err != nil { - return nil, files.WrapVfsError(err) + return nil, wrapRemoteErr(inst, err, "could not read destination directory on the remote stack") } uploaded, err := uploadWithConflictRetry(dstClient, dstParent.Attrs.Fullpath, targetDirID, localSrcFile.DocName, localSrcFile.MD5Sum, srcHandle, localSrcFile.Mime, localSrcFile.ByteSize) if err != nil { - return nil, files.WrapVfsError(err) + return nil, wrapRemoteErr(inst, err, "could not upload file to the remote stack") } if delete { @@ -655,7 +718,7 @@ func moveFileToSharedDriveCore(inst *instance.Instance, return uploaded, nil } -func remoteContentToMove(remoteClient *client.Client, dirID string) ([]*client.DirOrFile, []*client.DirOrFile, error) { +func remoteContentToMove(remoteClient *client.FilesClient, dirID string) ([]*client.DirOrFile, []*client.DirOrFile, error) { var dirs []*client.DirOrFile var filesToMove []*client.DirOrFile @@ -715,7 +778,7 @@ func moveDirBetweenSharedDrives(c echo.Context, sourceInstanceURL, sourceDirID s if err != nil { return err } - sourceClient := NewRemoteClient(sourceURL, sourceBearer) + sourceClient := client.NewFilesClient(NewRemoteClient(sourceURL, sourceBearer), sourceSharing.ID()) destURL, err := url.Parse(destInstanceURL) if err != nil { @@ -725,12 +788,12 @@ func moveDirBetweenSharedDrives(c echo.Context, sourceInstanceURL, sourceDirID s if err != nil { return err } - destClient := NewRemoteClient(destURL, destBearer) + destClient := client.NewFilesClient(NewRemoteClient(destURL, destBearer), destSharing.ID()) // Get the remote directory structure to move dirs, filesToMove, err := remoteContentToMove(sourceClient, sourceDirID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read source directory on the remote stack") } // Map from source dirID to destination dirID @@ -752,11 +815,11 @@ func moveDirBetweenSharedDrives(c echo.Context, sourceInstanceURL, sourceDirID s // Resolve parent path then create the directory remotely dstParent, err := destClient.GetDirByID(parentDestID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read destination directory on the remote stack") } newID, err := ensureRemoteChildDir(destClient, dstParent, d.Attrs.Name) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not create directory on the remote stack") } srcToDstDirID[d.ID] = newID } @@ -771,34 +834,33 @@ func moveDirBetweenSharedDrives(c echo.Context, sourceInstanceURL, sourceDirID s // Fetch source file metadata and content srcFile, err := sourceClient.GetFileByID(f.ID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read source file on the remote stack") } srcReader, err := sourceClient.DownloadByID(f.ID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not download source file on the remote stack") } defer srcReader.Close() // Optimistic upload with conflict retry dstParent, err := destClient.GetDirByID(destParentID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read destination directory on the remote stack") } _, err = uploadWithConflictRetry(destClient, dstParent.Attrs.Fullpath, destParentID, srcFile.Attrs.Name, srcFile.Attrs.MD5Sum, srcReader, srcFile.Attrs.Mime, srcFile.Attrs.Size) if err != nil { - return files.WrapVfsError(err) - } - - if !copy { - if err := sourceClient.PermanentDeleteByID(f.ID); err != nil { - inst.Logger().WithNamespace("move").Warnf("Could not delete source file: %v", err) - } + return wrapRemoteErr(inst, err, "could not upload file to the remote stack") } } - // Delete source directory (remote) after files are moved only if not copying + // Delete source directory (remote) after files are moved only if not + // copying. The delete is recursive: per-file deletions during the copy + // loop would only risk aborting it midway on a transient error. if !copy { + // fail-after-copy: the destination tree is already in place, so a + // failed source delete reports an error but leaves it (a retry + // creates suffixed duplicates). if err := sourceClient.PermanentDeleteByID(sourceDirID); err != nil { - inst.Logger().WithNamespace("move").Warnf("Could not delete source directory: %v", err) + return wrapRemoteErr(inst, err, "could not delete source directory on the remote stack") } } @@ -806,7 +868,7 @@ func moveDirBetweenSharedDrives(c echo.Context, sourceInstanceURL, sourceDirID s newRootID := srcToDstDirID[sourceDirID] dstRootDir, err := destClient.GetDirByID(newRootID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read created directory on the remote stack") } return respondRemoteUploadDir(c, dstRootDir, destSharing.ID()) } @@ -821,7 +883,7 @@ func moveFileBetweenSharedDrives(c echo.Context, sourceInstanceURL, fileID strin if err != nil { return err } - sourceClient := NewRemoteClient(sourceURL, sourceBearer) + sourceClient := client.NewFilesClient(NewRemoteClient(sourceURL, sourceBearer), sourceSharing.ID()) destURL, err := url.Parse(destInstanceURL) if err != nil { @@ -831,26 +893,26 @@ func moveFileBetweenSharedDrives(c echo.Context, sourceInstanceURL, fileID strin if err != nil { return err } - destClient := NewRemoteClient(destURL, destBearer) + destClient := client.NewFilesClient(NewRemoteClient(destURL, destBearer), destSharing.ID()) srcFile, err := sourceClient.GetFileByID(fileID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read source file on the remote stack") } srcReader, err := sourceClient.DownloadByID(fileID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not download source file on the remote stack") } defer srcReader.Close() // Resolve destination parent fullpath for conflict checks dstParent, err := destClient.GetDirByID(destDirID) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not read destination directory on the remote stack") } uniqueName, err := ensureRemoteUniqueChildName(destClient, dstParent.Attrs.Fullpath, srcFile.Attrs.Name, true) if err != nil { - return err + return wrapRemoteErr(inst, err, "could not resolve name conflict on the remote stack") } uploaded, err := destClient.Upload(&client.Upload{ Name: uniqueName, @@ -861,12 +923,15 @@ func moveFileBetweenSharedDrives(c echo.Context, sourceInstanceURL, fileID strin ContentLength: srcFile.Attrs.Size, }) if err != nil { - return files.WrapVfsError(err) + return wrapRemoteErr(inst, err, "could not upload file to the remote stack") } if !copy { + // fail-after-copy: the destination copy is already in place, so a + // failed source delete reports an error but leaves the copy (a retry + // creates a suffixed duplicate). if err := sourceClient.PermanentDeleteByID(fileID); err != nil { - inst.Logger().WithNamespace("move").Warnf("Could not delete source file: %v", err) + return wrapRemoteErr(inst, err, "could not delete source file on the remote stack") } } @@ -897,7 +962,7 @@ var NewRemoteClient = func(u *url.URL, bearerToken string) *client.Client { return c } -var NewSharedDriveClient = func(s *sharing.Sharing) (*client.Client, error) { +var NewSharedDriveClient = func(s *sharing.Sharing) (*client.FilesClient, error) { if len(s.Members) == 0 || s.Members[0].Instance == "" { return nil, jsonapi.Forbidden(errors.New("invalid sharing: missing member instance")) } @@ -909,7 +974,7 @@ var NewSharedDriveClient = func(s *sharing.Sharing) (*client.Client, error) { if err != nil { return nil, files.WrapVfsError(err) } - return NewRemoteClient(destURL, bearer), nil + return client.NewFilesClient(NewRemoteClient(destURL, bearer), s.ID()), nil } // respondRemoteUpload returns a minimal JSONAPI-like response for a file created @@ -1043,7 +1108,7 @@ func createLocalDir(v vfs.VFS, newDir *vfs.DirDoc) error { // ensureRemoteChildDir creates (or reuses) a child dir under the given parent remote dir. // It tries Mkdir first, falling back to GetDirByPath if it already exists. -func ensureRemoteChildDir(c *client.Client, parent *client.Dir, name string) (string, error) { +func ensureRemoteChildDir(c *client.FilesClient, parent *client.Dir, name string) (string, error) { targetPath := parent.Attrs.Fullpath + "/" + name newDir, err := c.Mkdir(targetPath) if err == nil { @@ -1071,7 +1136,7 @@ func ensureRemoteChildDir(c *client.Client, parent *client.Dir, name string) (st // parent fullpath on the remote instance. For files, the numeric suffix is added // before the file extension (e.g., "name (2).txt"). For directories, it is // appended at the end (e.g., "name (2)"). -func ensureRemoteUniqueChildName(c *client.Client, parentFullpath, name string, isFile bool) (string, error) { +func ensureRemoteUniqueChildName(c *client.FilesClient, parentFullpath, name string, isFile bool) (string, error) { candidate := name // quick existence check if _, err := c.GetDirOrFileByPath(parentFullpath + "/" + candidate); err != nil { @@ -1106,7 +1171,7 @@ func ensureRemoteUniqueChildName(c *client.Client, parentFullpath, name string, // uploadWithConflictRetry attempts an upload with the provided name first; if the // server responds with a 409/Conflict, it retries with an auto-generated unique // name under parentFullpath. Returns the created file metadata. -func uploadWithConflictRetry(c *client.Client, parentFullpath, dirID, name string, md5sum []byte, contents io.Reader, contentType string, contentLength int64) (*client.File, error) { +func uploadWithConflictRetry(c *client.FilesClient, parentFullpath, dirID, name string, md5sum []byte, contents io.Reader, contentType string, contentLength int64) (*client.File, error) { // First, optimistic attempt with original name f, err := c.Upload(&client.Upload{ Name: name, From 85dc436028062da4468f6809eab0b354a4ac32bc Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Tue, 15 Sep 2026 21:49:09 +0200 Subject: [PATCH 3/4] feat: Use effective access in drive move operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move and copy on shared drives were authorized by a flat check on the sharing: membership plus the member's `ReadOnly` flag. With nested shared folders this is wrong in both directions — it rejects a member read-only at the drive root but read-write on a nested folder, and it never accumulates scopes along the target's path. `MoveHandler` now resolves effective access on the source and the destination through `AccessResolver.ResolveForMember`: the resolver runs on the instance hosting the target while membership is matched for the calling instance. Copy requires effective read on the source; move requires effective write on the source plus effective write/create on the destination parent. Targets outside every scope return 404, insufficient access returns 403, and file-backed-drive validation keeps its 422 by running before the effective checks. The owner of a drive keeps full access on it without resolving. A remote target cannot be resolved locally, so the local stack decides nothing there: the remote stack is the only authority and applies its own access rules. `checkSharedDrivePermission` still serves membership loading and the other callers. --- web/sharings/drives.go | 12 +- web/sharings/move.go | 73 ++++++- web/sharings/move_test.go | 398 +++++++++++++++++++++++++++++++++++++- 3 files changed, 463 insertions(+), 20 deletions(-) diff --git a/web/sharings/drives.go b/web/sharings/drives.go index c4ba89dc408..3e7ed9582f3 100644 --- a/web/sharings/drives.go +++ b/web/sharings/drives.go @@ -1831,9 +1831,8 @@ func sharedDrivePermissionCheck(method, path string) (shouldCheck bool, requireW // the specified shared drive. It verifies that: // 1. The sharing exists and is a drive // 2. The current user is a member of the sharing (by domain or email) -// If requireWrite is true, it also checks that the user has write permission (not read-only). // Returns the sharing if the user has the required permissions. -func checkSharedDrivePermission(inst *instance.Instance, sharingID string, requireWrite bool) (*sharing.Sharing, error) { +func checkSharedDrivePermission(inst *instance.Instance, sharingID string) (*sharing.Sharing, error) { // Find the sharing by ID s, err := sharing.FindSharing(inst, sharingID) if err != nil { @@ -1859,12 +1858,10 @@ func checkSharedDrivePermission(inst *instance.Instance, sharingID string, requi currEmail, _ := inst.SettingsEMail() isMember := false - isReadOnly := false // If this is the owner instance, they're a member with write access if s.Owner { isMember = true - isReadOnly = false } else { // On a recipient's instance, their own member entry is typically at index 1 // (index 0 is the owner). Check if there's a member with an Instance field set. @@ -1876,13 +1873,11 @@ func checkSharedDrivePermission(inst *instance.Instance, sharingID string, requi // Check by domain if memberHost == inst.Domain || memberHost == currDomain { isMember = true - isReadOnly = m.ReadOnly break } // Check by email if currEmail != "" && m.Email == currEmail { isMember = true - isReadOnly = m.ReadOnly break } } @@ -1892,11 +1887,6 @@ func checkSharedDrivePermission(inst *instance.Instance, sharingID string, requi return nil, jsonapi.Forbidden(errors.New("not a member of this sharing")) } - // If write permission is required, check that the user is not read-only - if requireWrite && isReadOnly { - return nil, jsonapi.Forbidden(errors.New("write access denied: read-only member")) - } - return s, nil } diff --git a/web/sharings/move.go b/web/sharings/move.go index eafa4e899f7..94b11088ff0 100644 --- a/web/sharings/move.go +++ b/web/sharings/move.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/url" + "os" "strconv" "strings" "time" @@ -151,17 +152,23 @@ func MoveHandler(c echo.Context) error { moveDirectory := req.Source.DirID != "" if req.Source.Instance != "" && req.Dest.Instance != "" { - sourceSharing, err := checkSharedDrivePermission(inst, req.Source.SharingID, !req.Copy) + sourceSharing, err := checkSharedDrivePermission(inst, req.Source.SharingID) if err != nil { return err } - destSharing, err := checkSharedDrivePermission(inst, req.Dest.SharingID, true) + destSharing, err := checkSharedDrivePermission(inst, req.Dest.SharingID) if err != nil { return err } if err := validateFileBackedDriveMoveCopy(req, sourceSharing, destSharing); err != nil { return err } + if err := checkMoveSidePermission(inst, sourceInstance, sourceSharing, sourceTargetID(req), sourceMoveVerb(req.Copy)); err != nil { + return err + } + if err := checkMoveSidePermission(inst, destInstance, destSharing, req.Dest.DirID, permission.POST); err != nil { + return err + } if sourceInstance != nil && destInstance != nil { if moveDirectory { return moveDirSameStack(c, sourceInstance, destInstance, req.Source.DirID, req.Dest.DirID, destSharing, req.Copy) @@ -175,13 +182,16 @@ func MoveHandler(c echo.Context) error { return moveFileBetweenSharedDrives(c, req.Source.Instance, req.Source.FileID, sourceSharing, req.Dest.Instance, req.Dest.DirID, destSharing, req.Copy) } } else if req.Source.Instance != "" && req.Dest.Instance == "" { - s, err := checkSharedDrivePermission(inst, req.Source.SharingID, !req.Copy) + s, err := checkSharedDrivePermission(inst, req.Source.SharingID) if err != nil { return err } if err := validateFileBackedDriveMoveCopy(req, s, nil); err != nil { return err } + if err := checkMoveSidePermission(inst, sourceInstance, s, sourceTargetID(req), sourceMoveVerb(req.Copy)); err != nil { + return err + } if sourceInstance != nil && destInstance != nil { if moveDirectory { return moveDirSameStack(c, sourceInstance, destInstance, req.Source.DirID, req.Dest.DirID, nil, req.Copy) @@ -195,13 +205,16 @@ func MoveHandler(c echo.Context) error { return moveFileFromSharedDrive(c, inst, req.Source.Instance, req.Source.FileID, req.Dest.DirID, s, req.Copy) } } else if req.Source.Instance == "" && req.Dest.Instance != "" { - s, err := checkSharedDrivePermission(inst, req.Dest.SharingID, true) + s, err := checkSharedDrivePermission(inst, req.Dest.SharingID) if err != nil { return err } if err := validateFileBackedDriveMoveCopy(req, nil, s); err != nil { return err } + if err := checkMoveSidePermission(inst, destInstance, s, req.Dest.DirID, permission.POST); err != nil { + return err + } if sourceInstance != nil && destInstance != nil { if moveDirectory { return moveDirSameStack(c, sourceInstance, destInstance, req.Source.DirID, req.Dest.DirID, s, req.Copy) @@ -219,6 +232,58 @@ func MoveHandler(c echo.Context) error { } } +// checkMoveSidePermission asserts the calling instance has effective access +// on one shared-drive side of a move/copy. The resolver runs on hostInst +// (which hosts the target docs) but membership is resolved for callerInst +// (the instance performing the move). For a remote target nothing can be +// decided locally: the remote stack is the only authority. +func checkMoveSidePermission(callerInst, hostInst *instance.Instance, s *sharing.Sharing, targetID string, verb permission.Verb) error { + if hostInst == nil { + return nil + } + // Resolve the caller as a member of this sharing to follow it across + // nested sharings via MemberMatching (email or instance host). + member := s.MemberFor(callerInst) + if member == nil { + return jsonapi.NotFound(errors.New("shared drive target not found")) + } + if member.Status == sharing.MemberStatusOwner { + // The owner's own instance has full access to its drive. + return nil + } + ea, err := sharing.NewAccessResolver(hostInst).ResolveForMember(targetID, member) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return jsonapi.NotFound(errors.New("shared drive target not found")) + } + return wrapErrors(err) + } + if !ea.CanRead { + return jsonapi.NotFound(errors.New("shared drive target not found")) + } + if !ea.Can(verb) { + return jsonapi.Forbidden(errors.New("insufficient access on the target file or folder")) + } + return nil +} + +// sourceMoveVerb returns the effective verb required on the source: read for +// a copy, write for a move. +func sourceMoveVerb(copy bool) permission.Verb { + if copy { + return permission.GET + } + return permission.PATCH +} + +// sourceTargetID returns the ID of the moved file or directory. +func sourceTargetID(req moveRequest) string { + if req.Source.FileID != "" { + return req.Source.FileID + } + return req.Source.DirID +} + // remoteStatusTextCodes maps the English status texts a remote stack may // return as an unparseable error body (e.g. an HTML error page from a proxy) // back to a numeric status code. Only the codes meaningful to a move are diff --git a/web/sharings/move_test.go b/web/sharings/move_test.go index cf771e0c47c..53ec16af0ab 100644 --- a/web/sharings/move_test.go +++ b/web/sharings/move_test.go @@ -1,16 +1,21 @@ package sharings_test import ( + "net/http" "net/http/httptest" "net/url" "strings" "testing" + "time" "github.com/cozy/cozy-stack/model/instance" "github.com/cozy/cozy-stack/model/instance/lifecycle" + "github.com/cozy/cozy-stack/model/sharing" "github.com/cozy/cozy-stack/pkg/assets/dynamic" build "github.com/cozy/cozy-stack/pkg/config" "github.com/cozy/cozy-stack/pkg/config/config" + "github.com/cozy/cozy-stack/pkg/consts" + "github.com/cozy/cozy-stack/pkg/couchdb" "github.com/cozy/cozy-stack/pkg/crypto" "github.com/cozy/cozy-stack/tests/testutils" "github.com/cozy/cozy-stack/web" @@ -203,6 +208,31 @@ func TestSharedDrivesMove(t *testing.T) { verifyFileDeleted(t, env.betty, srcFileDoc) }) + t.Run("SuccessfulMove_WithinOwnSharedDrive_OwnerCanMove", func(t *testing.T) { + eA, _, _ := env.createClients(t) + // The owner has full access to its own drive: a move that stays inside + // the drive must be allowed (regression test for the owner bypass in + // checkMoveSidePermission). + responseObj := postMove(t, eA, env.acmeToken, `{ + "source": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+env.firstSharingID+`", + "file_id": "`+env.checklistID+`" + }, + "dest": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+env.firstSharingID+`", + "dir_id": "`+env.productDirID+`" + } + }`) + + // Verify the response and get moved file ID + movedFileID := assertMoveResponseWithSharing(t, responseObj, "Checklist.txt", env.productDirID, env.firstSharingID) + + // Verify the file was moved and content preserved + verifyFileMove(t, env.acme, movedFileID, "Checklist.txt", env.productDirID, "foo") + }) + // Force the cross-stack path even if instances are on the same server t.Run("SuccessfulMove_ToSharedDrive_DifferentStack", func(t *testing.T) { eA, eB, _ := env.createClients(t) @@ -1149,10 +1179,12 @@ func TestSharedDrivesMove(t *testing.T) { WithHeader("Authorization", "Bearer "+env.daveToken). Expect().Status(200) - fileToMoveSameStack := createFile(t, eD, "", "file-to-upload.txt", env.daveToken) + // Cross-stack move: the upload goes through the shared-drive routes on + // the owner stack, which enforces Dave's read-only access. + fileToMoveDifferentStack := createFile(t, eD, "", "file-to-upload.txt", env.daveToken) postMoveExpectStatus(t, eD, env.daveToken, `{ "source": { - "file_id": "`+fileToMoveSameStack+`" + "file_id": "`+fileToMoveDifferentStack+`" }, "dest": { "instance": "https://`+env.acme.Domain+`", @@ -1167,7 +1199,7 @@ func TestSharedDrivesMove(t *testing.T) { // Prepare: create a second shared drive with Dave as read-only recipient secondSharingID, secondRootDirID, _ := createSharedDrive(t, DriveCreationMethodLegacy, env.acme, env.acmeToken, env.tsA.URL, "ShareDrive"+strings.ReplaceAll(t.Name(), "/", "_"), "Drive used as destination for nested dir move", nil) - fileToMoveSameStack := createFile(t, eA, "", "file-to-upload.txt", env.acmeToken) + fileToMoveSameStack := createFile(t, eA, secondRootDirID, "file-to-upload.txt", env.acmeToken) daveDirID := createDirectory(t, eD, "", "DaveDir", env.daveToken) // Dave needs to accept the sharing invitation (read-only recipients still need to accept) @@ -1196,7 +1228,8 @@ func TestSharedDrivesMove(t *testing.T) { // Prepare: create a second shared drive with Dave as read-only recipient secondSharingID, secondRootDirID, _ := createSharedDrive(t, DriveCreationMethodLegacy, env.acme, env.acmeToken, env.tsA.URL, testify(t, "ShareDrive"), "Drive used as destination for nested dir move", nil) - fileToMoveSameStack := createFile(t, eA, "", testify(t, "file-to-upload.txt"), env.acmeToken) + fileName := testify(t, "file-to-upload.txt") + fileToMoveDifferentStack := createFile(t, eA, secondRootDirID, fileName, env.acmeToken) daveDirID := createDirectory(t, eD, "", testify(t, "DaveDir"), env.daveToken) // Dave needs to accept the sharing invitation (read-only recipients still need to accept) @@ -1206,9 +1239,11 @@ func TestSharedDrivesMove(t *testing.T) { WithHeader("Authorization", "Bearer "+env.daveToken). Expect().Status(200) + // Cross-stack move: the source deletion goes through the shared-drive + // routes on the owner stack, which enforces Dave's read-only access. postMoveExpectStatus(t, eD, env.daveToken, `{ "source": { - "file_id": "`+fileToMoveSameStack+`", + "file_id": "`+fileToMoveDifferentStack+`", "sharing_id": "`+secondSharingID+`", "instance": "https://`+env.acme.Domain+`" }, @@ -1216,6 +1251,9 @@ func TestSharedDrivesMove(t *testing.T) { "dir_id": "`+daveDirID+`" } }`, 403) + + // Verify the file was not deleted from the drive + verifyFileExists(t, env.acme, fileToMoveDifferentStack, fileName, secondRootDirID, "foo") }) // Dave is a read-only member; he must not be able to move files out of a shared drive @@ -1266,6 +1304,311 @@ func TestSharedDrivesMove(t *testing.T) { // Verify the file still exists (was not deleted) verifyFileExists(t, env.acme, fileToDeleteID, testify(t, "file-to-delete.txt"), secondRootDirID, "foo") }) + + // Dave is read-only on the root drive but read-write on a nested shared + // folder: he must be able to move files inside the nested scope. + t.Run("SuccessfulMove_NestedSharedFolder_RWChild_ROParent", func(t *testing.T) { + eA, _, eD := env.createClients(t) + + // Create a root shared drive with Dave as read-only recipient + rootSharingID, rootDirID, _ := createSharedDrive(t, DriveCreationMethodLegacy, env.acme, env.acmeToken, env.tsA.URL, + testify(t, "RootDrive"), "Root drive with Dave read-only", nil) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, rootSharingID) + + // Create a nested folder inside the root drive + nestedDirID := createDirectory(t, eA, rootDirID, testify(t, "NestedFolder"), env.acmeToken) + + // Create a nested sharing on the subfolder with Dave as read-write + // recipient. We create it directly in DB to control the members. + now := time.Now() + nestedSharing := &sharing.Sharing{ + Active: true, + Owner: true, + Drive: true, + DriveRootType: sharing.DriveRootTypeDirectory, + AppSlug: "test", + AccessMode: sharing.AccessModeAdditive, + Members: []sharing.Member{ + { + Status: sharing.MemberStatusOwner, + Name: "Acme", + Email: "acme@example.net", + Instance: "https://" + env.acme.Domain, + }, + { + Status: sharing.MemberStatusReady, + Name: "Dave", + Email: "dave@example.net", + Instance: "https://" + env.dave.Domain, + ReadOnly: false, + }, + }, + Rules: []sharing.Rule{ + { + Title: "nested", + DocType: consts.Files, + Values: []string{nestedDirID}, + }, + }, + CreatedAt: now, + UpdatedAt: now, + } + require.NoError(t, couchdb.CreateDoc(env.acme, nestedSharing)) + require.NoError(t, nestedSharing.AddReferenceForSharing(env.acme, &nestedSharing.Rules[0])) + + // Create a file inside the nested folder + fileToMove := createFile(t, eA, nestedDirID, testify(t, "nested-file.txt"), env.acmeToken) + + // Dave moves the file inside the nested folder (same instance, same + // sharing scope) → should succeed because he is RW on the nested scope + destDirID := createDirectory(t, eA, nestedDirID, testify(t, "DestDir"), env.acmeToken) + postMove(t, eD, env.daveToken, `{ + "source": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+rootSharingID+`", + "file_id": "`+fileToMove+`" + }, + "dest": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+rootSharingID+`", + "dir_id": "`+destDirID+`" + } + }`) + }) + + // Alice shares the parent drive with Bob, a nested drive inside it with + // Charlie, and the other drive with Bob. Moving the parent drive into the + // other drive is a metadata-only move and must not revoke any of the + // involved sharings. + t.Run("MoveRootDriveIntoAnotherSharedDrive_DoesNotRevokeSharings", func(t *testing.T) { + eA, _, _ := env.createClients(t) + + // /parent is the drive from the env setup, already accepted by Bob + parentSharingID := env.firstSharingID + parentRootID := env.firstRootDirID + + // Nested drive /parent/nested, accepted by Charlie + nestedDirID := createDirectory(t, eA, parentRootID, testify(t, "Nested"), env.acmeToken) + nestedSharingID := createDriveSharingOnDir(t, env.acme, env.acmeToken, env.tsA.URL, + nestedDirID, testify(t, "NestedDrive"), "Drive nested in the parent drive", + []RecipientInfo{{Name: "Dave", Email: "dave@example.net"}}) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, nestedSharingID) + + // /other drive, accepted by Bob + otherSharingID, otherRootID, _ := createSharedDrive(t, DriveCreationMethodLegacy, env.acme, env.acmeToken, env.tsA.URL, + testify(t, "OtherDrive"), "Destination drive for the parent move", + []RecipientInfo{{Name: "Betty", Email: "betty@example.net"}}) + acceptSharedDriveForBetty(t, env.acme, env.betty, env.tsA.URL, env.tsB.URL, otherSharingID) + + // Sanity: Bob and Charlie can browse their drives + _, eB, eD := env.createClients(t) + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, parentSharingID, parentRootID) + assertRecipientCanBrowseDrive(t, eD, env.daveToken, nestedSharingID, nestedDirID) + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, otherSharingID, otherRootID) + + // Alice moves parent into other + postMove(t, eA, env.acmeToken, `{ + "source": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+parentSharingID+`", + "dir_id": "`+parentRootID+`" + }, + "dest": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+otherSharingID+`", + "dir_id": "`+otherRootID+`" + } + }`) + + // Alice: the moved drive sits inside the other drive + otherRoot, err := env.acme.VFS().DirByID(otherRootID) + require.NoError(t, err) + parentRoot, err := env.acme.VFS().DirByID(parentRootID) + require.NoError(t, err) + require.Equal(t, otherRoot.Fullpath+"/"+parentRoot.DocName, parentRoot.Fullpath) + + // Alice: the sharings are still active, with ready members, and the + // shared roots still reference their sharings + for _, tc := range []struct{ sharingID, dirID, email string }{ + {parentSharingID, parentRootID, "betty@example.net"}, + {nestedSharingID, nestedDirID, "dave@example.net"}, + } { + s, err := sharing.FindSharing(env.acme, tc.sharingID) + require.NoError(t, err) + require.True(t, s.Active, "sharing %s should not be revoked", tc.sharingID) + member := findSharingMemberByEmail(t, env.acme, tc.sharingID, tc.email) + require.Equal(t, sharing.MemberStatusReady, member.Status) + dir, err := env.acme.VFS().DirByID(tc.dirID) + require.NoError(t, err) + require.Contains(t, dir.ReferencedBy, couchdb.DocReference{ + ID: tc.sharingID, + Type: consts.Sharings, + }) + } + + // Bob still sees both of his drives, Charlie still sees his, and + // their sharings are still active + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, parentSharingID, parentRootID) + assertRecipientCanBrowseDrive(t, eD, env.daveToken, nestedSharingID, nestedDirID) + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, otherSharingID, otherRootID) + s, err := sharing.FindSharing(env.betty, parentSharingID) + require.NoError(t, err) + require.True(t, s.Active, "Bob's sharing of parent should not be revoked") + s, err = sharing.FindSharing(env.betty, otherSharingID) + require.NoError(t, err) + require.True(t, s.Active, "Bob's sharing of other should not be revoked") + s, err = sharing.FindSharing(env.dave, nestedSharingID) + require.NoError(t, err) + require.True(t, s.Active, "Charlie's sharing of nested should not be revoked") + }) + + // Alice shares the parent drive with Bob, a nested drive inside it with + // Charlie, and the other drive with Bob. Moving the nested drive into the + // other drive is a metadata-only move and must not revoke any of the + // involved sharings. + t.Run("MoveNestedDriveIntoAnotherSharedDrive_DoesNotRevokeSharings", func(t *testing.T) { + eA, _, _ := env.createClients(t) + + // /parent is the drive from the env setup, already accepted by Bob + parentSharingID := env.firstSharingID + parentRootID := env.firstRootDirID + + // Nested drive /parent/nested, accepted by Charlie + nestedDirID := createDirectory(t, eA, parentRootID, testify(t, "Nested"), env.acmeToken) + nestedSharingID := createDriveSharingOnDir(t, env.acme, env.acmeToken, env.tsA.URL, + nestedDirID, testify(t, "NestedDrive"), "Drive nested in the parent drive", + []RecipientInfo{{Name: "Dave", Email: "dave@example.net"}}) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, nestedSharingID) + + // /other drive, accepted by Bob + otherSharingID, otherRootID, _ := createSharedDrive(t, DriveCreationMethodLegacy, env.acme, env.acmeToken, env.tsA.URL, + testify(t, "OtherDrive"), "Destination drive for the nested move", + []RecipientInfo{{Name: "Betty", Email: "betty@example.net"}}) + acceptSharedDriveForBetty(t, env.acme, env.betty, env.tsA.URL, env.tsB.URL, otherSharingID) + + // Sanity: Bob and Charlie can browse their drives + _, eB, eD := env.createClients(t) + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, parentSharingID, parentRootID) + assertRecipientCanBrowseDrive(t, eD, env.daveToken, nestedSharingID, nestedDirID) + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, otherSharingID, otherRootID) + + // Alice moves nested into other + postMove(t, eA, env.acmeToken, `{ + "source": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+parentSharingID+`", + "dir_id": "`+nestedDirID+`" + }, + "dest": { + "instance": "https://`+env.acme.Domain+`", + "sharing_id": "`+otherSharingID+`", + "dir_id": "`+otherRootID+`" + } + }`) + + // Alice: the moved drive sits inside the other drive + otherRoot, err := env.acme.VFS().DirByID(otherRootID) + require.NoError(t, err) + nestedDir, err := env.acme.VFS().DirByID(nestedDirID) + require.NoError(t, err) + require.Equal(t, otherRoot.Fullpath+"/"+nestedDir.DocName, nestedDir.Fullpath) + + // Alice: the sharings are still active, with ready members, and the + // shared roots still reference their sharings + for _, tc := range []struct{ sharingID, dirID, email string }{ + {parentSharingID, parentRootID, "betty@example.net"}, + {nestedSharingID, nestedDirID, "dave@example.net"}, + } { + s, err := sharing.FindSharing(env.acme, tc.sharingID) + require.NoError(t, err) + require.True(t, s.Active, "sharing %s should not be revoked", tc.sharingID) + member := findSharingMemberByEmail(t, env.acme, tc.sharingID, tc.email) + require.Equal(t, sharing.MemberStatusReady, member.Status) + dir, err := env.acme.VFS().DirByID(tc.dirID) + require.NoError(t, err) + require.Contains(t, dir.ReferencedBy, couchdb.DocReference{ + ID: tc.sharingID, + Type: consts.Sharings, + }) + } + + // Bob still sees both of his drives, Charlie still sees his, and + // their sharings are still active + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, parentSharingID, parentRootID) + assertRecipientCanBrowseDrive(t, eD, env.daveToken, nestedSharingID, nestedDirID) + assertRecipientCanBrowseDrive(t, eB, env.bettyToken, otherSharingID, otherRootID) + s, err := sharing.FindSharing(env.betty, parentSharingID) + require.NoError(t, err) + require.True(t, s.Active, "Bob's sharing of parent should not be revoked") + s, err = sharing.FindSharing(env.betty, otherSharingID) + require.NoError(t, err) + require.True(t, s.Active, "Bob's sharing of other should not be revoked") + s, err = sharing.FindSharing(env.dave, nestedSharingID) + require.NoError(t, err) + require.True(t, s.Active, "Charlie's sharing of nested should not be revoked") + }) +} + +// assertRecipientCanBrowseDrive checks that a recipient can still read the +// root directory of a shared drive through the drive proxy. +func assertRecipientCanBrowseDrive(t *testing.T, e *httpexpect.Expect, token, sharingID, rootDirID string) { + t.Helper() + + e.GET("/sharings/drives/"+sharingID+"/"+rootDirID). + WithHeader("Authorization", "Bearer "+token). + Expect().Status(http.StatusOK) +} + +// createDriveSharingOnDir creates a drive sharing on an existing directory, +// like createSharedDrive with the legacy method, but without creating the +// root directory itself. All recipients are read-write. +func createDriveSharingOnDir( + t *testing.T, + inst *instance.Instance, + appToken string, + tsURL string, + dirID string, + driveName string, + description string, + recipients []RecipientInfo, +) string { + t.Helper() + + e := httpexpect.Default(t, tsURL) + + var refs []string + for _, r := range recipients { + c := createContact(t, inst, r.Name, r.Email) + require.NotNil(t, c) + refs = append(refs, `{"id": "`+c.ID()+`", "type": "`+c.DocType()+`"}`) + } + + sharingID := e.POST("/sharings/"). + WithHeader("Authorization", "Bearer "+appToken). + WithHeader("Content-Type", "application/vnd.api+json"). + WithBytes([]byte(`{ + "data": { + "type": "` + consts.Sharings + `", + "attributes": { + "description": "` + description + `", + "drive": true, + "rules": [{ + "title": "` + driveName + `", + "doctype": "` + consts.Files + `", + "values": ["` + dirID + `"] + }] + }, + "relationships": { + "recipients": { + "data": [` + strings.Join(refs, ",") + `] + } + } + } + }`)). + Expect().Status(201). + JSON(httpexpect.ContentOpts{MediaType: "application/vnd.api+json"}). + Object().Path("$.data.id").String().NotEmpty().Raw() + return sharingID } func TestSharedDrivesCopy(t *testing.T) { @@ -1596,6 +1939,51 @@ func TestSharedDrivesCopy(t *testing.T) { verifyFileExists(t, env.acme, fileToMoveID, fileToMoveName, secondRootDirID, "foo") }) + t.Run("SuccessfulCopy_DirectoryFromSharedDriveToLocal_Readonly_DifferentStack", func(t *testing.T) { + eA, _, eD := env.createClients(t) + cleanup := forceCrossStack(t, env.tsA.URL) + defer cleanup() + + // Prepare: create a second shared drive with Dave as read-only recipient + secondSharingID, secondRootDirID, _ := createSharedDrive(t, DriveCreationMethodLegacy, env.acme, env.acmeToken, env.tsA.URL, + testify(t, "ShareDrive"), "Drive used as read-only dir copy source", nil) + // Dave needs to accept the sharing invitation (read-only recipients still need to accept) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, secondSharingID) + + dirToCopy := createDirectory(t, eA, secondRootDirID, testify(t, "SharedDirToCopy"), env.acmeToken) + _ = createFile(t, eA, dirToCopy, "shared-file1.txt", env.acmeToken) + subDirID := createDirectory(t, eA, dirToCopy, "SharedSubDir", env.acmeToken) + _ = createFile(t, eA, subDirID, "shared-file2.bin", env.acmeToken) + + daveDirID := createDirectory(t, eD, "", testify(t, "DaveDir"), env.daveToken) + + // Verify Dave can access the shared drive + eD.GET("/sharings/drives/"+secondSharingID+"/"+secondRootDirID). + WithHeader("Authorization", "Bearer "+env.daveToken). + Expect().Status(200) + + responseObj := postMove(t, eD, env.daveToken, `{ + "source": { + "dir_id": "`+dirToCopy+`", + "sharing_id": "`+secondSharingID+`", + "instance": "https://`+env.acme.Domain+`" + }, + "dest": { + "dir_id": "`+daveDirID+`" + }, + "copy": true + }`) + + // Verify the response and get copied directory ID + copiedDirID := assertDirectoryResponse(t, responseObj, testify(t, "SharedDirToCopy"), daveDirID) + + // Verify the directory was copied with all its contents + verifyDirectoryCopy(t, env.dave, copiedDirID, testify(t, "SharedDirToCopy"), daveDirID) + + // Verify the original directory still exists (not deleted) + verifyDirectoryExists(t, env.acme, dirToCopy, testify(t, "SharedDirToCopy"), secondRootDirID) + }) + t.Run("Unprocessable_CopyFromFileRootSharedDriveWithNonRootFile", func(t *testing.T) { eA, eB, _ := env.createClients(t) From 0a168bbb70cfc200d3f7bd414c358611b5dde958 Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Tue, 15 Sep 2026 23:45:15 +0200 Subject: [PATCH 4/4] fix: Reserve drive root trash/destroy to the owner A read-write member could trash or destroy the root of a shared drive via `DELETE /sharings/drives/:id/:file-id` or `DELETE /sharings/drives/:id/trash/:file-id`, taking the whole drive down, as removing the root revokes the sharing. These requests are now rejected with 403 for members; only the owner keeps them. --- web/sharings/drives.go | 9 ++++++ web/sharings/drives_test.go | 64 +++++++++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/web/sharings/drives.go b/web/sharings/drives.go index 3e7ed9582f3..7139df3ca96 100644 --- a/web/sharings/drives.go +++ b/web/sharings/drives.go @@ -1650,6 +1650,15 @@ func guardSharedDriveRouteForMember(c echo.Context, inst *instance.Instance, s * reqPath := c.Request().URL.Path fileID := c.Param("file-id") + // Only the owner can trash or destroy the root of a shared drive: the VFS + // revocation hook would take the whole drive down with it. Deleting a + // version of the root file is neither: it leaves the drive in place. + if method == http.MethodDelete && c.Param("version-id") == "" { + if rootID, err := s.DriveRootID(); err == nil && fileID == rootID { + return jsonapi.Forbidden(errors.New("only the owner can trash or destroy the root of a shared drive")) + } + } + // Share-by-link routes keep their own authorization layer. Match the // registered route pattern, not the raw URL, so a future route whose // path contains "/permissions" cannot silently bypass this guard. diff --git a/web/sharings/drives_test.go b/web/sharings/drives_test.go index 302b1ca750c..9fab3ee0651 100644 --- a/web/sharings/drives_test.go +++ b/web/sharings/drives_test.go @@ -4059,19 +4059,8 @@ func TestFileRootSharedDriveMutationRoutes(t *testing.T) { }`)). Expect().Status(422) + // Only the owner can trash the root of a drive, file-root included. eB.DELETE("/sharings/drives/"+sharingID+"/"+rootFileID). - WithHeader("Authorization", "Bearer "+env.bettyToken). - Expect().Status(200). - JSON(httpexpect.ContentOpts{MediaType: "application/vnd.api+json"}). - Object(). - Path("$.data.attributes.trashed").Boolean().True() - - require.Eventually(t, func() bool { - s, err := sharing.FindSharing(env.betty, sharingID) - return err == nil && !s.Active - }, 5*time.Second, 50*time.Millisecond) - - eB.POST("/sharings/drives/"+sharingID+"/trash/"+rootFileID). WithHeader("Authorization", "Bearer "+env.bettyToken). Expect().Status(403) }) @@ -4135,8 +4124,8 @@ func TestFileRootSharedDriveMutationRoutes(t *testing.T) { ) acceptSharedDriveForBetty(t, env.acme, env.betty, env.tsA.URL, env.tsB.URL, sharingID) - eB.DELETE("/sharings/drives/"+sharingID+"/"+rootFileID). - WithHeader("Authorization", "Bearer "+env.bettyToken). + eA.DELETE("/sharings/drives/"+sharingID+"/"+rootFileID). + WithHeader("Authorization", "Bearer "+env.acmeToken). Expect().Status(200) require.Eventually(t, func() bool { @@ -6353,6 +6342,53 @@ func TestSharedDrivePermanentDeleteViaPatch(t *testing.T) { }) } +func TestSharedDriveRootProtection(t *testing.T) { + if testing.Short() { + t.Skip("an instance is required for this test: test skipped due to the use of --short flag") + } + + env := setupSharedDrivesEnv(t) + eA, _, eD := env.createClients(t) + + // D1: Dave is a read-write member: trashing or destroying the root of the + // drive is owner-only, whatever the member's write access. + d1ID, d1RootID, _ := createSharedDrive(t, DriveCreationMethodFromFolder, + env.acme, env.acmeToken, env.tsA.URL, "RootProtection D1", "d1", + []RecipientInfo{{Name: "Dave", Email: "dave@example.net", ReadOnly: false}}) + acceptSharedDrive(t, env.acme, env.dave, "Dave", env.tsA.URL, env.tsD.URL, d1ID) + + t.Run("MemberTrashRootDenied", func(t *testing.T) { + eD.DELETE("/sharings/drives/"+d1ID+"/"+d1RootID). + WithHeader("Authorization", "Bearer "+env.daveToken). + Expect().Status(403) + + eA.GET("/files/"+d1RootID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(200) + }) + + t.Run("MemberDestroyRootDenied", func(t *testing.T) { + eD.DELETE("/sharings/drives/"+d1ID+"/trash/"+d1RootID). + WithHeader("Authorization", "Bearer "+env.daveToken). + Expect().Status(403) + }) + + // The owner can trash the root: the trash hook revokes and deletes the + // drive sharing. + t.Run("OwnerTrashRootRevokesSharing", func(t *testing.T) { + d2ID, d2RootID, _ := createSharedDrive(t, DriveCreationMethodFromFolder, + env.acme, env.acmeToken, env.tsA.URL, "RootProtection D2", "d2", nil) + + eA.DELETE("/sharings/drives/"+d2ID+"/"+d2RootID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(200) + + eA.GET("/sharings/"+d2ID). + WithHeader("Authorization", "Bearer "+env.acmeToken). + Expect().Status(404) + }) +} + func TestSharedDriveEffectiveAccessOnMetadataByPath(t *testing.T) { if testing.Short() { t.Skip("an instance is required for this test: test skipped due to the use of --short flag")