From 741993d4faff8c4c89464fac5fd6fbe52d3a6c3a Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Wed, 9 Sep 2026 15:42:36 +0200 Subject: [PATCH 1/3] fix(posix): check trash permissions in the posix trashbin --- pkg/storage/fs/posix/posix.go | 15 +- pkg/storage/fs/posix/testhelpers/helpers.go | 5 +- pkg/storage/fs/posix/trashbin/trashbin.go | 58 ++++++- .../fs/posix/trashbin/trashbin_suite_test.go | 31 ++++ .../fs/posix/trashbin/trashbin_test.go | 157 ++++++++++++++++++ 5 files changed, 255 insertions(+), 11 deletions(-) create mode 100644 pkg/storage/fs/posix/trashbin/trashbin_suite_test.go create mode 100644 pkg/storage/fs/posix/trashbin/trashbin_test.go diff --git a/pkg/storage/fs/posix/posix.go b/pkg/storage/fs/posix/posix.go index 0164bd349da..b2ec0410440 100644 --- a/pkg/storage/fs/posix/posix.go +++ b/pkg/storage/fs/posix/posix.go @@ -85,7 +85,13 @@ func New(m map[string]interface{}, stream events.Stream, log *zerolog.Logger) (s return nil, fmt.Errorf("unknown metadata backend %s, only 'messagepack' or 'xattrs' (default) supported", o.MetadataBackend) } - trashbin, err := trashbin.New(o, lu, log) + permissionsSelector, err := pool.PermissionsSelector(o.PermissionsSVC, pool.WithTLSMode(o.PermTLSMode)) + if err != nil { + return nil, err + } + p := permissions.NewPermissions(node.NewPermissions(lu), permissionsSelector) + + trashbin, err := trashbin.New(o, p, lu, log) if err != nil { return nil, err } @@ -119,13 +125,6 @@ func New(m map[string]interface{}, stream events.Stream, log *zerolog.Logger) (s return nil, err } - permissionsSelector, err := pool.PermissionsSelector(o.PermissionsSVC, pool.WithTLSMode(o.PermTLSMode)) - if err != nil { - return nil, err - } - - p := permissions.NewPermissions(node.NewPermissions(lu), permissionsSelector) - aspects := aspects.Aspects{ Lookup: lu, Tree: tp, diff --git a/pkg/storage/fs/posix/testhelpers/helpers.go b/pkg/storage/fs/posix/testhelpers/helpers.go index e79e8cb5266..ad1e62f58e5 100644 --- a/pkg/storage/fs/posix/testhelpers/helpers.go +++ b/pkg/storage/fs/posix/testhelpers/helpers.go @@ -186,14 +186,15 @@ func NewTestEnv(config map[string]interface{}) (*TestEnv, error) { if err != nil { return nil, err } - tb, err := trashbin.New(o, lu, &logger) + p := permissions.NewPermissions(pmock, permissionsSelector) + tb, err := trashbin.New(o, p, lu, &logger) if err != nil { return nil, err } aspects := aspects.Aspects{ Lookup: lu, Tree: tree, - Permissions: permissions.NewPermissions(pmock, permissionsSelector), + Permissions: p, Trashbin: tb, } fs, err := decomposedfs.New(&o.Options, aspects, &logger) diff --git a/pkg/storage/fs/posix/trashbin/trashbin.go b/pkg/storage/fs/posix/trashbin/trashbin.go index 88bd8c59fd5..88582c7ef6b 100644 --- a/pkg/storage/fs/posix/trashbin/trashbin.go +++ b/pkg/storage/fs/posix/trashbin/trashbin.go @@ -31,6 +31,7 @@ import ( provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/owncloud/reva/v2/pkg/errtypes" "github.com/owncloud/reva/v2/pkg/storage" "github.com/owncloud/reva/v2/pkg/storage/fs/posix/lookup" "github.com/owncloud/reva/v2/pkg/storage/fs/posix/options" @@ -42,6 +43,7 @@ import ( type Trashbin struct { fs storage.FS o *options.Options + p Permissions lu *lookup.Lookup log *zerolog.Logger } @@ -51,10 +53,16 @@ const ( timeFormat = "2006-01-02T15:04:05" ) +// Permissions is the interface the trashbin uses to authorize recycle operations +type Permissions interface { + AssembleTrashPermissions(ctx context.Context, n *node.Node) (*provider.ResourcePermissions, error) +} + // New returns a new Trashbin -func New(o *options.Options, lu *lookup.Lookup, log *zerolog.Logger) (*Trashbin, error) { +func New(o *options.Options, p Permissions, lu *lookup.Lookup, log *zerolog.Logger) (*Trashbin, error) { return &Trashbin{ o: o, + p: p, lu: lu, log: log, }, nil @@ -151,6 +159,18 @@ func (tb *Trashbin) ListRecycle(ctx context.Context, ref *provider.Reference, ke return nil, err } + // check permissions on the space + rp, err := tb.p.AssembleTrashPermissions(ctx, n) + switch { + case err != nil: + return nil, err + case !rp.ListRecycle: + if rp.Stat { + return nil, errtypes.PermissionDenied(key) + } + return nil, errtypes.NotFound(key) + } + trashRoot := trashRootForNode(n) base := filepath.Join(trashRoot, "files") @@ -232,6 +252,18 @@ func (tb *Trashbin) RestoreRecycleItem(ctx context.Context, ref *provider.Refere return nil, err } + // check permissions of deleted node + rp, err := tb.p.AssembleTrashPermissions(ctx, n) + switch { + case err != nil: + return nil, err + case !rp.RestoreRecycleItem: + if rp.Stat { + return nil, errtypes.PermissionDenied(key) + } + return nil, errtypes.NotFound(key) + } + trashRoot := trashRootForNode(n) trashPath := filepath.Clean(filepath.Join(trashRoot, "files", key+".trashitem", relativePath)) @@ -285,6 +317,18 @@ func (tb *Trashbin) PurgeRecycleItem(ctx context.Context, ref *provider.Referenc return err } + // check permissions of deleted node + rp, err := tb.p.AssembleTrashPermissions(ctx, n) + switch { + case err != nil: + return err + case !rp.PurgeRecycle: + if rp.Stat { + return errtypes.PermissionDenied(key) + } + return errtypes.NotFound(key) + } + trashRoot := trashRootForNode(n) err = os.RemoveAll(filepath.Clean(filepath.Join(trashRoot, "files", key+".trashitem", relativePath))) if err != nil { @@ -305,6 +349,18 @@ func (tb *Trashbin) EmptyRecycle(ctx context.Context, ref *provider.Reference) e return err } + // check permissions of deleted node + rp, err := tb.p.AssembleTrashPermissions(ctx, n) + switch { + case err != nil: + return err + case !rp.PurgeRecycle: + if rp.Stat { + return errtypes.PermissionDenied(n.ID) + } + return errtypes.NotFound(n.ID) + } + trashRoot := trashRootForNode(n) err = os.RemoveAll(filepath.Clean(filepath.Join(trashRoot, "files"))) if err != nil { diff --git a/pkg/storage/fs/posix/trashbin/trashbin_suite_test.go b/pkg/storage/fs/posix/trashbin/trashbin_suite_test.go new file mode 100644 index 00000000000..eadf42bb873 --- /dev/null +++ b/pkg/storage/fs/posix/trashbin/trashbin_suite_test.go @@ -0,0 +1,31 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package trashbin_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestTrashbin(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Trashbin Suite") +} diff --git a/pkg/storage/fs/posix/trashbin/trashbin_test.go b/pkg/storage/fs/posix/trashbin/trashbin_test.go new file mode 100644 index 00000000000..6c1e8d4b059 --- /dev/null +++ b/pkg/storage/fs/posix/trashbin/trashbin_test.go @@ -0,0 +1,157 @@ +// Copyright 2018-2024 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package trashbin_test + +import ( + "github.com/stretchr/testify/mock" + + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/owncloud/reva/v2/pkg/errtypes" + helpers "github.com/owncloud/reva/v2/pkg/storage/fs/posix/testhelpers" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// grantTrashPermissions makes the permissions mock answer every +// AssembleTrashPermissions call with the given permissions. +func grantTrashPermissions(env *helpers.TestEnv, rp *provider.ResourcePermissions) { + env.Permissions.On("AssembleTrashPermissions", mock.Anything, mock.Anything).Return(rp, nil) +} + +var _ = Describe("Trashbin", func() { + var ( + env *helpers.TestEnv + ref *provider.Reference + ) + + BeforeEach(func() { + var err error + env, err = helpers.NewTestEnv(map[string]interface{}{ + "metadata_backend": "messagepack", + }) + Expect(err).ToNot(HaveOccurred()) + + // recycle operations address the space, not the deleted item + ref = &provider.Reference{ResourceId: env.SpaceRootRes} + }) + + AfterEach(func() { + if env != nil { + env.Cleanup() + } + }) + + Context("when the user is not a member of the space", func() { + BeforeEach(func() { + // AssembleTrashPermissions accumulates grants the user actually holds, + // so a non-member ends up with no permissions at all + grantTrashPermissions(env, &provider.ResourcePermissions{}) + }) + + It("does not confirm that the space exists when listing", func() { + _, err := env.Fs.ListRecycle(env.Ctx, ref, "", "") + Expect(err).To(BeAssignableToTypeOf(errtypes.NotFound(""))) + }) + + It("does not confirm that the space exists when restoring", func() { + _, err := env.Fs.RestoreRecycleItem(env.Ctx, ref, "key", "", ref) + Expect(err).To(BeAssignableToTypeOf(errtypes.NotFound(""))) + }) + + It("does not confirm that the space exists when purging", func() { + err := env.Fs.PurgeRecycleItem(env.Ctx, ref, "key", "") + Expect(err).To(BeAssignableToTypeOf(errtypes.NotFound(""))) + }) + + It("does not confirm that the space exists when emptying the trash", func() { + err := env.Fs.EmptyRecycle(env.Ctx, ref) + Expect(err).To(BeAssignableToTypeOf(errtypes.NotFound(""))) + }) + }) + + Context("when the user is a viewer", func() { + BeforeEach(func() { + // a space viewer may browse the trash but not change it, + // see conversions.NewSpaceViewerRole() + grantTrashPermissions(env, &provider.ResourcePermissions{ + Stat: true, + ListRecycle: true, + }) + }) + + It("allows listing the trash", func() { + items, err := env.Fs.ListRecycle(env.Ctx, ref, "", "") + Expect(err).ToNot(HaveOccurred()) + Expect(items).To(BeEmpty()) + }) + + It("denies restoring", func() { + _, err := env.Fs.RestoreRecycleItem(env.Ctx, ref, "key", "", ref) + Expect(err).To(BeAssignableToTypeOf(errtypes.PermissionDenied(""))) + }) + + It("denies purging a single item", func() { + err := env.Fs.PurgeRecycleItem(env.Ctx, ref, "key", "") + Expect(err).To(BeAssignableToTypeOf(errtypes.PermissionDenied(""))) + }) + + It("denies emptying the trash", func() { + err := env.Fs.EmptyRecycle(env.Ctx, ref) + Expect(err).To(BeAssignableToTypeOf(errtypes.PermissionDenied(""))) + }) + }) + + Context("when the user is an editor", func() { + BeforeEach(func() { + grantTrashPermissions(env, &provider.ResourcePermissions{ + Stat: true, + ListRecycle: true, + RestoreRecycleItem: true, + PurgeRecycle: true, + }) + }) + + It("allows listing the trash", func() { + items, err := env.Fs.ListRecycle(env.Ctx, ref, "", "") + Expect(err).ToNot(HaveOccurred()) + Expect(items).To(BeEmpty()) + }) + + It("allows emptying the trash", func() { + Expect(env.Fs.EmptyRecycle(env.Ctx, ref)).To(Succeed()) + }) + + // the item does not exist so these still fail, but they have to fail + // on the missing item rather than on the permission check + It("lets restoring pass the permission check", func() { + _, err := env.Fs.RestoreRecycleItem(env.Ctx, ref, "does-not-exist", "", ref) + Expect(err).To(HaveOccurred()) + Expect(err).ToNot(BeAssignableToTypeOf(errtypes.PermissionDenied(""))) + Expect(err).ToNot(BeAssignableToTypeOf(errtypes.NotFound(""))) + }) + + It("lets purging pass the permission check", func() { + err := env.Fs.PurgeRecycleItem(env.Ctx, ref, "does-not-exist", "") + Expect(err).To(HaveOccurred()) + Expect(err).ToNot(BeAssignableToTypeOf(errtypes.PermissionDenied(""))) + Expect(err).ToNot(BeAssignableToTypeOf(errtypes.NotFound(""))) + }) + }) +}) From af5aa934b9aa2e8fb9a4a89feae759c4555de3fa Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Wed, 9 Sep 2026 15:47:06 +0200 Subject: [PATCH 2/3] chore(changelog): add entry for the posix trashbin permissions fix --- .../bugfix-posix-trashbin-permissions.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 changelog/unreleased/bugfix-posix-trashbin-permissions.md diff --git a/changelog/unreleased/bugfix-posix-trashbin-permissions.md b/changelog/unreleased/bugfix-posix-trashbin-permissions.md new file mode 100644 index 00000000000..30dc8596904 --- /dev/null +++ b/changelog/unreleased/bugfix-posix-trashbin-permissions.md @@ -0,0 +1,16 @@ +Bugfix: Check permissions in the trashbin of the posix driver + +The trashbin of the posix storage driver did not check any permissions when +listing, restoring, purging or emptying the trash. An authenticated user who +knew the id of a space could read, restore or permanently delete the trashed +content of a space they are not a member of. Only the posix driver was affected. +The decomposedfs based drivers (ocis, s3ng) already checked the permissions of the space. + +All four operations now assemble the trash permissions of the space and are +gated on `ListRecycle`, `RestoreRecycleItem` and `PurgeRecycle` respectively. +Callers who are not allowed to stat the space receive a not found error instead +of a permission denied error, so that the existence of a space is not +disclosed. Note that emptying the trash requires `PurgeRecycle`, which means a +space viewer can still browse the trash but can no longer empty it. + +https://github.com/owncloud/reva/pull/729 \ No newline at end of file From 4bed082ad0d3b3b9822c95395c23162701f5489a Mon Sep 17 00:00:00 2001 From: Firas Frikha Date: Wed, 9 Sep 2026 16:13:47 +0200 Subject: [PATCH 3/3] fix(posix): disable the fs watcher in the trashbin tests --- .../bugfix-posix-trashbin-permissions.md | 19 ++++++------------- .../fs/posix/trashbin/trashbin_test.go | 3 +++ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/changelog/unreleased/bugfix-posix-trashbin-permissions.md b/changelog/unreleased/bugfix-posix-trashbin-permissions.md index 30dc8596904..8345ceac227 100644 --- a/changelog/unreleased/bugfix-posix-trashbin-permissions.md +++ b/changelog/unreleased/bugfix-posix-trashbin-permissions.md @@ -1,16 +1,9 @@ Bugfix: Check permissions in the trashbin of the posix driver -The trashbin of the posix storage driver did not check any permissions when -listing, restoring, purging or emptying the trash. An authenticated user who -knew the id of a space could read, restore or permanently delete the trashed -content of a space they are not a member of. Only the posix driver was affected. -The decomposedfs based drivers (ocis, s3ng) already checked the permissions of the space. +The trashbin of the posix storage driver now checks the trash permissions of +the space on all recycle operations. Listing requires `ListRecycle`, restoring +requires `RestoreRecycleItem`, and purging a single item or emptying the trash +require `PurgeRecycle`. Note that a space viewer can therefore browse the trash +but can no longer empty it. Only the posix driver is affected. -All four operations now assemble the trash permissions of the space and are -gated on `ListRecycle`, `RestoreRecycleItem` and `PurgeRecycle` respectively. -Callers who are not allowed to stat the space receive a not found error instead -of a permission denied error, so that the existence of a space is not -disclosed. Note that emptying the trash requires `PurgeRecycle`, which means a -space viewer can still browse the trash but can no longer empty it. - -https://github.com/owncloud/reva/pull/729 \ No newline at end of file +https://github.com/owncloud/reva/pull/729 diff --git a/pkg/storage/fs/posix/trashbin/trashbin_test.go b/pkg/storage/fs/posix/trashbin/trashbin_test.go index 6c1e8d4b059..45e27c09aae 100644 --- a/pkg/storage/fs/posix/trashbin/trashbin_test.go +++ b/pkg/storage/fs/posix/trashbin/trashbin_test.go @@ -45,6 +45,9 @@ var _ = Describe("Trashbin", func() { var err error env, err = helpers.NewTestEnv(map[string]interface{}{ "metadata_backend": "messagepack", + // the permission checks do not involve the fs watcher, and starting + // one inotifywait per spec races with the test space setup + "watch_fs": false, }) Expect(err).ToNot(HaveOccurred())