diff --git a/changelog/unreleased/bugfix-posix-trashbin-permissions.md b/changelog/unreleased/bugfix-posix-trashbin-permissions.md new file mode 100644 index 00000000000..8345ceac227 --- /dev/null +++ b/changelog/unreleased/bugfix-posix-trashbin-permissions.md @@ -0,0 +1,9 @@ +Bugfix: Check permissions in the trashbin of the posix driver + +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. + +https://github.com/owncloud/reva/pull/729 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..45e27c09aae --- /dev/null +++ b/pkg/storage/fs/posix/trashbin/trashbin_test.go @@ -0,0 +1,160 @@ +// 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", + // 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()) + + // 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(""))) + }) + }) +})