From 4450fb18ee9f47be366fd60ecdb24fbfe7382802 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Wed, 29 Apr 2026 13:23:43 +0200 Subject: [PATCH 01/14] Add support for mail grantees --- .../usershareprovider/usershareprovider.go | 2 +- .../usershareprovider_test.go | 23 +++++++++ pkg/storage/pkg/decomposedfs/decomposedfs.go | 7 +++ pkg/storage/pkg/decomposedfs/grants.go | 11 ++++- pkg/storage/pkg/decomposedfs/grants_test.go | 48 +++++++++++++++---- .../metadata/prefixes/prefixes.go | 2 + pkg/storage/pkg/decomposedfs/node/node.go | 11 +++-- pkg/storage/pkg/decomposedfs/spaces.go | 23 +++++++-- pkg/storage/utils/ace/ace.go | 15 ++++-- pkg/storage/utils/ace/ace_test.go | 37 ++++++++++++++ 10 files changed, 158 insertions(+), 21 deletions(-) diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index c2f3f9db6cb..f65e143bd5b 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -206,7 +206,7 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar UserId: &userpb.UserId{ OpaqueId: req.GetGrant().GetGrantee().GetUserId().GetOpaqueId(), Idp: user.GetId().GetIdp(), - Type: userpb.UserType_USER_TYPE_PRIMARY}, + Type: req.GetGrant().GetGrantee().GetUserId().GetType()}, } } // some for group grantees diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 6dd5b75af75..4f63221276a 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -436,6 +436,7 @@ var _ = Describe("user share provider service", func() { Entry("ListGrants", conversions.RoleFromName("manager").CS3ResourcePermissions(), &providerpb.ResourcePermissions{ListGrants: true}, rpcpb.Code_CODE_OK, 1), ) }) + Context("create share with tenant awareness", func() { JustBeforeEach(func() { rgrpcService := usershareprovider.New(gatewaySelector, manager, []*regexp.Regexp{}) @@ -491,6 +492,28 @@ var _ = Describe("user share provider service", func() { manager.AssertNumberOfCalls(GinkgoT(), "Share", 1) }) + + It("succeeds when sharing with a guest using an email address", func() { + createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{ + ResourceInfo: &providerpb.ResourceInfo{ + PermissionSet: conversions.RoleFromName("manager").CS3ResourcePermissions(), + }, + Grant: &collaborationpb.ShareGrant{ + Grantee: &providerpb.Grantee{ + Type: providerpb.GranteeType_GRANTEE_TYPE_GUEST, + Id: &providerpb.Grantee_Email{Email: "guest@example.com"}, + }, + Permissions: &collaborationpb.SharePermissions{ + Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), + }, + }, + }) + + Expect(err).ToNot(HaveOccurred()) + Expect(createShareResponse.Status.Code).To(Equal(rpcpb.Code_CODE_OK)) + + manager.AssertNumberOfCalls(GinkgoT(), "Share", 1) + }) }) }) diff --git a/pkg/storage/pkg/decomposedfs/decomposedfs.go b/pkg/storage/pkg/decomposedfs/decomposedfs.go index ccd7f05d34e..9ae5f281556 100644 --- a/pkg/storage/pkg/decomposedfs/decomposedfs.go +++ b/pkg/storage/pkg/decomposedfs/decomposedfs.go @@ -130,6 +130,7 @@ type Decomposedfs struct { UserCache *ttlcache.Cache userSpaceIndex *spaceidindex.Index groupSpaceIndex *spaceidindex.Index + mailSpaceIndex *spaceidindex.Index spaceTypeIndex *spaceidindex.Index log *zerolog.Logger @@ -218,6 +219,11 @@ func New(o *options.Options, aspects aspects.Aspects, log *zerolog.Logger) (stor if err != nil { return nil, err } + mailSpaceIndex := spaceidindex.New(filepath.Join(o.Root, lookup.IndexesDir), "by-mail") + err = mailSpaceIndex.Init() + if err != nil { + return nil, err + } spaceTypeIndex := spaceidindex.New(filepath.Join(o.Root, lookup.IndexesDir), "by-type") err = spaceTypeIndex.Init() if err != nil { @@ -244,6 +250,7 @@ func New(o *options.Options, aspects aspects.Aspects, log *zerolog.Logger) (stor UserCache: ttlcache.NewCache(), userSpaceIndex: userSpaceIndex, groupSpaceIndex: groupSpaceIndex, + mailSpaceIndex: mailSpaceIndex, spaceTypeIndex: spaceTypeIndex, log: log, } diff --git a/pkg/storage/pkg/decomposedfs/grants.go b/pkg/storage/pkg/decomposedfs/grants.go index 94cc53c355e..3b9ba51534a 100644 --- a/pkg/storage/pkg/decomposedfs/grants.go +++ b/pkg/storage/pkg/decomposedfs/grants.go @@ -23,6 +23,7 @@ import ( "path/filepath" "strings" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/opencloud-eu/reva/v2/internal/grpc/services/storageprovider" "github.com/opencloud-eu/reva/v2/pkg/appctx" @@ -235,8 +236,14 @@ func (fs *Decomposedfs) RemoveGrant(ctx context.Context, ref *provider.Reference switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: // remove from user index - if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), grantNode.SpaceID); err != nil { - return err + if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { + if err := fs.mailSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), grantNode.SpaceID); err != nil { + return err + } + } else { + if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), grantNode.SpaceID); err != nil { + return err + } } case provider.GranteeType_GRANTEE_TYPE_GROUP: // remove from group index diff --git a/pkg/storage/pkg/decomposedfs/grants_test.go b/pkg/storage/pkg/decomposedfs/grants_test.go index d3b32909759..7751d635606 100644 --- a/pkg/storage/pkg/decomposedfs/grants_test.go +++ b/pkg/storage/pkg/decomposedfs/grants_test.go @@ -35,19 +35,24 @@ import ( var _ = Describe("Grants", func() { var ( - env *helpers.DecomposedTestEnv - ref *provider.Reference - grant *provider.Grant + env *helpers.DecomposedTestEnv + ref *provider.Reference + grant *provider.Grant + userid *userpb.UserId ) BeforeEach(func() { + userid = &userpb.UserId{ + OpaqueId: "4c510ada-c86b-4815-8820-42cdf82c3d51", + } + }) + + JustBeforeEach(func() { grant = &provider.Grant{ Grantee: &provider.Grantee{ Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{ - UserId: &userpb.UserId{ - OpaqueId: "4c510ada-c86b-4815-8820-42cdf82c3d51", - }, + UserId: userid, }, }, Permissions: &provider.ResourcePermissions{ @@ -60,9 +65,7 @@ var _ = Describe("Grants", func() { OpaqueId: helpers.OwnerID, }, } - }) - JustBeforeEach(func() { var err error env, err = helpers.NewTestEnv(nil) Expect(err).ToNot(HaveOccurred()) @@ -181,5 +184,34 @@ var _ = Describe("Grants", func() { Expect(len(grants)).To(Equal(0)) }) }) + + Context("with guest grants", func() { + BeforeEach(func() { + userid = &userpb.UserId{ + OpaqueId: "foo@example.com", + Type: userpb.UserType_USER_TYPE_GUEST, + } + }) + + It("adds, lists and removes the guest grant", func() { + err := env.Fs.AddGrant(env.Ctx, ref, grant) + Expect(err).ToNot(HaveOccurred()) + + grants, err := env.Fs.ListGrants(env.Ctx, ref) + Expect(err).ToNot(HaveOccurred()) + Expect(len(grants)).To(Equal(1)) + + g := grants[0] + Expect(g.Grantee.GetUserId().OpaqueId).To(Equal(grant.Grantee.GetUserId().OpaqueId)) + Expect(g.Grantee.GetUserId().Type).To(Equal(userpb.UserType_USER_TYPE_GUEST)) + + err = env.Fs.RemoveGrant(env.Ctx, ref, grant) + Expect(err).ToNot(HaveOccurred()) + + grants, err = env.Fs.ListGrants(env.Ctx, ref) + Expect(err).ToNot(HaveOccurred()) + Expect(len(grants)).To(Equal(0)) + }) + }) }) }) diff --git a/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go b/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go index 0c364973736..8d0c94d6050 100644 --- a/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go +++ b/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go @@ -55,6 +55,7 @@ const ( GrantPrefix string = OcPrefix + "grant." GrantUserAcePrefix string = OcPrefix + "grant." + UserAcePrefix GrantGroupAcePrefix string = OcPrefix + "grant." + GroupAcePrefix + GrantMailAcePrefix string = OcPrefix + "grant." + MailAcePrefix MetadataPrefix string = OcPrefix + "md." // favorite flag, per user @@ -106,6 +107,7 @@ const ( UserAcePrefix string = "u:" GroupAcePrefix string = "g:" + MailAcePrefix string = "m:" ) func FavoriteKey(uid *userpb.UserId) string { diff --git a/pkg/storage/pkg/decomposedfs/node/node.go b/pkg/storage/pkg/decomposedfs/node/node.go index dc03ad6b85c..a11b122318c 100644 --- a/pkg/storage/pkg/decomposedfs/node/node.go +++ b/pkg/storage/pkg/decomposedfs/node/node.go @@ -1304,10 +1304,15 @@ func (n *Node) ReadGrant(ctx context.Context, grantee string) (g *provider.Grant func (n *Node) DeleteGrant(ctx context.Context, g *provider.Grant) (err error) { var attr string - if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_GROUP { + switch g.Grantee.Type { + case provider.GranteeType_GRANTEE_TYPE_GROUP: attr = prefixes.GrantGroupAcePrefix + g.Grantee.GetGroupId().OpaqueId - } else { - attr = prefixes.GrantUserAcePrefix + g.Grantee.GetUserId().OpaqueId + default: + prefix := prefixes.GrantUserAcePrefix + if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { + prefix = prefixes.GrantMailAcePrefix + } + attr = prefix + g.Grantee.GetUserId().OpaqueId } if err = n.RemoveXattr(ctx, attr); err != nil { diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index 00dd5e52460..ed5860453d0 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -894,6 +894,9 @@ func (fs *Decomposedfs) updateIndexes(ctx context.Context, grantee *provider.Gra // create space grant index switch grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: + if grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { + return fs.linkSpaceByMail(ctx, grantee.GetUserId().GetOpaqueId(), spaceID, target) + } return fs.linkSpaceByUser(ctx, grantee.GetUserId().GetOpaqueId(), spaceID, target) case provider.GranteeType_GRANTEE_TYPE_GROUP: return fs.linkSpaceByGroup(ctx, grantee.GetGroupId().GetOpaqueId(), spaceID, target) @@ -910,6 +913,10 @@ func (fs *Decomposedfs) linkSpaceByGroup(ctx context.Context, groupID, spaceID, return fs.groupSpaceIndex.Add(groupID, spaceID, target) } +func (fs *Decomposedfs) linkSpaceByMail(ctx context.Context, mail, spaceID, target string) error { + return fs.mailSpaceIndex.Add(mail, spaceID, target) +} + func (fs *Decomposedfs) linkStorageSpaceType(ctx context.Context, spaceType, spaceID, target string) error { return fs.spaceTypeIndex.Add(spaceType, spaceID, target) } @@ -983,10 +990,18 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, // invalidate space grant switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - // remove from user index - if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), n.GetSpaceID()); err != nil { - sublog.Error().Err(err).Str("grantee", id). - Msg("failed to delete expired user space index") + if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { + // remove from mail index + if err := fs.mailSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), n.GetSpaceID()); err != nil { + sublog.Error().Err(err).Str("grantee", id). + Msg("failed to delete expired mail space index") + } + } else { + // remove from user index + if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), n.GetSpaceID()); err != nil { + sublog.Error().Err(err).Str("grantee", id). + Msg("failed to delete expired user space index") + } } case provider.GranteeType_GRANTEE_TYPE_GROUP: // remove from group index diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index ef3da2ee29e..e90592982f1 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -215,7 +215,12 @@ func FromGrant(g *provider.Grant) *ACE { } func UserAce(id *userpb.UserId) string { - return "u:" + id.OpaqueId + switch id.GetType() { + case userpb.UserType_USER_TYPE_GUEST: + return "m:" + id.OpaqueId + default: + return "u:" + id.OpaqueId + } } // Principal returns the principal of the ACE, eg. `u:` or `g:` @@ -260,7 +265,7 @@ func Unmarshal(principal string, v []byte) (e *ACE, err error) { return nil, fmt.Errorf("inconsistent ace: expected group") } } else { - if principal[:1] != "u" { + if principal[:1] != "u" && principal[:1] != "m" { return nil, fmt.Errorf("inconsistent ace: expected user") } } @@ -288,7 +293,11 @@ func (e *ACE) Grant() *provider.Grant { if e.granteeType() == provider.GranteeType_GRANTEE_TYPE_GROUP { g.Grantee.Id = &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: id}} } else if e.granteeType() == provider.GranteeType_GRANTEE_TYPE_USER { - g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id}} + if strings.HasPrefix(e.principal, "m:") { + g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_GUEST}} + } else { + g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_PRIMARY}} + } } if e.expires != 0 { diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index a98ae5aa106..5651cf94bbc 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -63,6 +63,20 @@ var _ = Describe("ACE", func() { Permissions: &provider.ResourcePermissions{ CreateContainer: true, }, + Creator: &userpb.UserId{}, + } + + guestGrant = &provider.Grant{ + Grantee: &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_MAIL, + Id: &provider.Grantee_Mail{ + Mail: "guest@example.com", + }, + }, + Permissions: &provider.ResourcePermissions{ + CreateContainer: true, + }, + Creator: &userpb.UserId{}, } ) @@ -76,6 +90,12 @@ var _ = Describe("ACE", func() { ace := ace.FromGrant(groupGrant) Expect(ace.Principal()).To(Equal("g:foo")) }) + + It("creates an ACE from a guest grant", func() { + guestGrant.Grantee.Id = &provider.Grantee_Mail{Mail: "GUEST@example.com"} + ace := ace.FromGrant(guestGrant) + Expect(ace.Principal()).To(Equal("m:guest@example.com")) + }) }) Describe("Grant", func() { @@ -86,6 +106,23 @@ var _ = Describe("ACE", func() { grant.Grantee.Opaque = nil Expect(grant).To(BeComparableTo(userGrant, protocmp.Transform())) }) + + It("returns a proper Grant for group ACE", func() { + ace := ace.FromGrant(groupGrant) + grant := ace.Grant() + // do not check opaque values + grant.Grantee.Opaque = nil + Expect(grant).To(BeComparableTo(groupGrant, protocmp.Transform())) + }) + + It("returns a proper Grant for guest ACE", func() { + guestGrant.Grantee.Id = &provider.Grantee_Mail{Mail: "guest@example.com"} + ace := ace.FromGrant(guestGrant) + grant := ace.Grant() + // do not check opaque values + grant.Grantee.Opaque = nil + Expect(grant).To(BeComparableTo(guestGrant, protocmp.Transform())) + }) }) Describe("marshalling", func() { From ec0840e87f17ca18c91f0ec07ad21abf0bcee341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 11 May 2026 09:26:16 +0200 Subject: [PATCH 02/14] Fix tests --- pkg/storage/utils/ace/ace.go | 2 +- pkg/storage/utils/ace/ace_test.go | 43 ++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index e90592982f1..4913a9da7e2 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -217,7 +217,7 @@ func FromGrant(g *provider.Grant) *ACE { func UserAce(id *userpb.UserId) string { switch id.GetType() { case userpb.UserType_USER_TYPE_GUEST: - return "m:" + id.OpaqueId + return "m:" + strings.ToLower(id.OpaqueId) default: return "u:" + id.OpaqueId } diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index 5651cf94bbc..bba5a122622 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -20,6 +20,7 @@ package ace_test import ( "fmt" + "strings" grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -34,12 +35,19 @@ import ( var _ = Describe("ACE", func() { var ( + userGrant *provider.Grant + groupGrant *provider.Grant + guestGrant *provider.Grant + ) + + BeforeEach(func() { userGrant = &provider.Grant{ Grantee: &provider.Grantee{ Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{ UserId: &userpb.UserId{ OpaqueId: "foo", + Type: userpb.UserType_USER_TYPE_PRIMARY, }, }, }, @@ -68,9 +76,12 @@ var _ = Describe("ACE", func() { guestGrant = &provider.Grant{ Grantee: &provider.Grantee{ - Type: provider.GranteeType_GRANTEE_TYPE_MAIL, - Id: &provider.Grantee_Mail{ - Mail: "guest@example.com", + Type: provider.GranteeType_GRANTEE_TYPE_USER, + Id: &provider.Grantee_UserId{ + UserId: &userpb.UserId{ + OpaqueId: "GUEST@example.com", + Type: userpb.UserType_USER_TYPE_GUEST, + }, }, }, Permissions: &provider.ResourcePermissions{ @@ -78,7 +89,7 @@ var _ = Describe("ACE", func() { }, Creator: &userpb.UserId{}, } - ) + }) Describe("FromGrant", func() { It("creates an ACE from a user grant", func() { @@ -92,7 +103,12 @@ var _ = Describe("ACE", func() { }) It("creates an ACE from a guest grant", func() { - guestGrant.Grantee.Id = &provider.Grantee_Mail{Mail: "GUEST@example.com"} + guestGrant.Grantee.Id = &provider.Grantee_UserId{ + UserId: &userpb.UserId{ + OpaqueId: "GUEST@example.com", + Type: userpb.UserType_USER_TYPE_GUEST, + }, + } ace := ace.FromGrant(guestGrant) Expect(ace.Principal()).To(Equal("m:guest@example.com")) }) @@ -107,6 +123,16 @@ var _ = Describe("ACE", func() { Expect(grant).To(BeComparableTo(userGrant, protocmp.Transform())) }) + It("returns a proper guest Grant with lowercased OpaqueId", func() { + ace := ace.FromGrant(guestGrant) + grant := ace.Grant() + // do not check opaque values + grant.Grantee.Opaque = nil + g := guestGrant + g.Grantee.Id.(*provider.Grantee_UserId).UserId.OpaqueId = strings.ToLower(g.Grantee.Id.(*provider.Grantee_UserId).UserId.OpaqueId) + Expect(grant).To(BeComparableTo(g, protocmp.Transform())) + }) + It("returns a proper Grant for group ACE", func() { ace := ace.FromGrant(groupGrant) grant := ace.Grant() @@ -116,7 +142,12 @@ var _ = Describe("ACE", func() { }) It("returns a proper Grant for guest ACE", func() { - guestGrant.Grantee.Id = &provider.Grantee_Mail{Mail: "guest@example.com"} + guestGrant.Grantee.Id = &provider.Grantee_UserId{ + UserId: &userpb.UserId{ + OpaqueId: "guest@example.com", + Type: userpb.UserType_USER_TYPE_GUEST, + }, + } ace := ace.FromGrant(guestGrant) grant := ace.Grant() // do not check opaque values From 677bd464fd7bfeecf2fe10b90e031f16c64e122b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Duffeck?= Date: Mon, 11 May 2026 11:11:58 +0200 Subject: [PATCH 03/14] Do not check the tenant id of guests --- .../grpc/services/usershareprovider/usershareprovider.go | 5 +++-- .../services/usershareprovider/usershareprovider_test.go | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index f65e143bd5b..e7d4972c3eb 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -173,8 +173,9 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar user := ctxpkg.ContextMustGetUser(ctx) // check if the grantee is a user or group if req.GetGrant().GetGrantee().GetType() == provider.GranteeType_GRANTEE_TYPE_USER { - // check if the tenantId of the user matches the tenantId of the target user - if user.GetId().GetTenantId() != req.GetGrant().GetGrantee().GetUserId().GetTenantId() { + // check if the tenantId of the user matches the tenantId of the target user, skipping guests + if req.GetGrant().GetGrantee().GetUserId().GetType() != userpb.UserType_USER_TYPE_GUEST && + user.GetId().GetTenantId() != req.GetGrant().GetGrantee().GetUserId().GetTenantId() { log.Warn().Msg("user tenantId does not match the target user tenantId, this is not supported yet") return &collaboration.CreateShareResponse{ Status: status.NewPermissionDenied(ctx, nil, "user tenantId does not match the target user tenantId"), diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 4f63221276a..3424f1ad8bb 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -500,8 +500,8 @@ var _ = Describe("user share provider service", func() { }, Grant: &collaborationpb.ShareGrant{ Grantee: &providerpb.Grantee{ - Type: providerpb.GranteeType_GRANTEE_TYPE_GUEST, - Id: &providerpb.Grantee_Email{Email: "guest@example.com"}, + Type: providerpb.GranteeType_GRANTEE_TYPE_USER, + Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "GUEST@example.com", Type: userpb.UserType_USER_TYPE_GUEST}}, }, Permissions: &collaborationpb.SharePermissions{ Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), From 5c1ae4b39039ebc77ac92f4970adbc6d29fbad06 Mon Sep 17 00:00:00 2001 From: Alex Ababii Date: Tue, 25 Aug 2026 18:33:01 +0200 Subject: [PATCH 04/14] reverted userprovider check for create share, guest now is handled like a normal user --- .../grpc/services/usershareprovider/usershareprovider.go | 5 ++--- .../services/usershareprovider/usershareprovider_test.go | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index e7d4972c3eb..f65e143bd5b 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -173,9 +173,8 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar user := ctxpkg.ContextMustGetUser(ctx) // check if the grantee is a user or group if req.GetGrant().GetGrantee().GetType() == provider.GranteeType_GRANTEE_TYPE_USER { - // check if the tenantId of the user matches the tenantId of the target user, skipping guests - if req.GetGrant().GetGrantee().GetUserId().GetType() != userpb.UserType_USER_TYPE_GUEST && - user.GetId().GetTenantId() != req.GetGrant().GetGrantee().GetUserId().GetTenantId() { + // check if the tenantId of the user matches the tenantId of the target user + if user.GetId().GetTenantId() != req.GetGrant().GetGrantee().GetUserId().GetTenantId() { log.Warn().Msg("user tenantId does not match the target user tenantId, this is not supported yet") return &collaboration.CreateShareResponse{ Status: status.NewPermissionDenied(ctx, nil, "user tenantId does not match the target user tenantId"), diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 3424f1ad8bb..90e9e21499f 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -501,7 +501,7 @@ var _ = Describe("user share provider service", func() { Grant: &collaborationpb.ShareGrant{ Grantee: &providerpb.Grantee{ Type: providerpb.GranteeType_GRANTEE_TYPE_USER, - Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "GUEST@example.com", Type: userpb.UserType_USER_TYPE_GUEST}}, + Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "GUEST@example.com", TenantId: "tenant1", Type: userpb.UserType_USER_TYPE_GUEST}}, }, Permissions: &collaborationpb.SharePermissions{ Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), From 119ffa47858739259342ee832ff65182d0b96057 Mon Sep 17 00:00:00 2001 From: Alex Ababii Date: Tue, 25 Aug 2026 20:27:47 +0200 Subject: [PATCH 05/14] guest users can see shared with them spaces --- pkg/storage/pkg/decomposedfs/spaces.go | 15 ++++++ pkg/storage/pkg/decomposedfs/spaces_test.go | 52 +++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index ed5860453d0..66c150e7cbc 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -357,6 +357,21 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide matches[allMatches[entry]] = allMatches[entry] } + // guests are granted shares via their mail address, search the mail index as well + if requestedUserID.GetType() == userv1beta1.UserType_USER_TYPE_GUEST { + allMatches, err = fs.mailSpaceIndex.Load(strings.ToLower(requestedUserID.GetOpaqueId())) + // do not return an error if the guest has no spaces + if err != nil && !os.IsNotExist(err) { + return nil, errors.Wrap(err, "error reading mail index") + } + + if entry == spaceIDAny { + maps.Copy(matches, allMatches) + } else { + matches[allMatches[entry]] = allMatches[entry] + } + } + // get Groups for userid user := ctxpkg.ContextMustGetUser(ctx) // TODO the user from context may not have groups populated diff --git a/pkg/storage/pkg/decomposedfs/spaces_test.go b/pkg/storage/pkg/decomposedfs/spaces_test.go index 799b3bb6063..53c12ba60a1 100644 --- a/pkg/storage/pkg/decomposedfs/spaces_test.go +++ b/pkg/storage/pkg/decomposedfs/spaces_test.go @@ -30,6 +30,7 @@ import ( typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/opencloud-eu/reva/v2/internal/grpc/services/storageprovider" ctxpkg "github.com/opencloud-eu/reva/v2/pkg/ctx" "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/node" "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/spaceidindex" @@ -636,4 +637,55 @@ var _ = Describe("Spaces", func() { ), ) }) + + Describe("List storage spaces for guests", func() { + var ( + env *helpers.DecomposedTestEnv + ) + + BeforeEach(func() { + var err error + env, err = helpers.NewTestEnv(nil) + Expect(err).ToNot(HaveOccurred()) + env.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything).Unset() + env.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything).Return(&provider.ResourcePermissions{ + Stat: true, + AddGrant: true, + GetQuota: true, + }, nil) + env.PermissionsClient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}, nil) + }) + + AfterEach(func() { + if env != nil { + env.Cleanup() + } + }) + + It("finds project spaces granted to a guest via the by-mail index", func() { + // create a project space as the owner + resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Guest Space", Type: "project"}) + Expect(err).ToNot(HaveOccurred()) + ref := &provider.Reference{ResourceId: resp.StorageSpace.GetRoot()} + + // grant the space to a guest. A space-typed context makes the storage + // link the space in the by-mail index, like the real share flow. + guest := &userv1beta1.UserId{OpaqueId: "guest@example.com", Type: userv1beta1.UserType_USER_TYPE_GUEST} + ctx := storageprovider.WithSpaceType(ctxpkg.ContextSetUser(context.Background(), env.Users[0]), "project") + Expect(env.Fs.AddGrant(ctx, ref, &provider.Grant{ + Grantee: &provider.Grantee{Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{UserId: guest}}, + Permissions: &provider.ResourcePermissions{Stat: true}, + Creator: &userv1beta1.UserId{OpaqueId: helpers.OwnerID}, + })).To(Succeed()) + + // the guest can discover the granted space when listing their spaces + spaces, err := env.Fs.ListStorageSpaces(ctxpkg.ContextSetUser(context.Background(), &userv1beta1.User{Id: guest}), []*provider.ListStorageSpacesRequest_Filter{ + {Type: provider.ListStorageSpacesRequest_Filter_TYPE_USER, Term: &provider.ListStorageSpacesRequest_Filter_User{User: guest}}, + }, false) + Expect(err).ToNot(HaveOccurred()) + Expect(len(spaces)).To(Equal(1)) + Expect(spaces[0].SpaceType).To(Equal("project")) + Expect(spaces[0].GetId().GetOpaqueId()).To(Equal(resp.StorageSpace.GetId().GetOpaqueId())) + }) + }) }) From 47a380469abc22a40e99b6df111862770e93be13 Mon Sep 17 00:00:00 2001 From: Alex Ababii Date: Tue, 25 Aug 2026 22:14:51 +0200 Subject: [PATCH 06/14] lowercase normalization for grants --- pkg/storage/pkg/decomposedfs/grants.go | 9 +++++++-- pkg/storage/pkg/decomposedfs/grants_test.go | 6 ++++-- pkg/storage/pkg/decomposedfs/node/node.go | 12 +++++++++--- pkg/storage/pkg/decomposedfs/spaces.go | 4 ++-- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/pkg/storage/pkg/decomposedfs/grants.go b/pkg/storage/pkg/decomposedfs/grants.go index 3b9ba51534a..fab100d2169 100644 --- a/pkg/storage/pkg/decomposedfs/grants.go +++ b/pkg/storage/pkg/decomposedfs/grants.go @@ -237,7 +237,7 @@ func (fs *Decomposedfs) RemoveGrant(ctx context.Context, ref *provider.Reference case provider.GranteeType_GRANTEE_TYPE_USER: // remove from user index if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { - if err := fs.mailSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), grantNode.SpaceID); err != nil { + if err := fs.mailSpaceIndex.Remove(strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()), grantNode.SpaceID); err != nil { return err } } else { @@ -327,7 +327,12 @@ func (fs *Decomposedfs) loadGrant(ctx context.Context, ref *provider.Reference, for _, grant := range grants { switch grant.Grantee.GetType() { case provider.GranteeType_GRANTEE_TYPE_USER: - if g.Grantee.GetUserId().GetOpaqueId() == grant.Grantee.GetUserId().GetOpaqueId() { + // guest grants are persisted with a lowercased mail as the principal + if grant.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { + if strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()) == grant.Grantee.GetUserId().GetOpaqueId() { + return n, unlockFunc, grant, nil + } + } else if g.Grantee.GetUserId().GetOpaqueId() == grant.Grantee.GetUserId().GetOpaqueId() { return n, unlockFunc, grant, nil } case provider.GranteeType_GRANTEE_TYPE_GROUP: diff --git a/pkg/storage/pkg/decomposedfs/grants_test.go b/pkg/storage/pkg/decomposedfs/grants_test.go index 7751d635606..edb1c348b23 100644 --- a/pkg/storage/pkg/decomposedfs/grants_test.go +++ b/pkg/storage/pkg/decomposedfs/grants_test.go @@ -22,6 +22,7 @@ import ( "fmt" "os" "path/filepath" + "strings" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -188,7 +189,7 @@ var _ = Describe("Grants", func() { Context("with guest grants", func() { BeforeEach(func() { userid = &userpb.UserId{ - OpaqueId: "foo@example.com", + OpaqueId: "Foo@Example.COM", Type: userpb.UserType_USER_TYPE_GUEST, } }) @@ -202,9 +203,10 @@ var _ = Describe("Grants", func() { Expect(len(grants)).To(Equal(1)) g := grants[0] - Expect(g.Grantee.GetUserId().OpaqueId).To(Equal(grant.Grantee.GetUserId().OpaqueId)) + Expect(g.Grantee.GetUserId().OpaqueId).To(Equal(strings.ToLower(grant.Grantee.GetUserId().OpaqueId))) Expect(g.Grantee.GetUserId().Type).To(Equal(userpb.UserType_USER_TYPE_GUEST)) + // the mixed-case request must match the lowercased persisted grant err = env.Fs.RemoveGrant(env.Ctx, ref, grant) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/storage/pkg/decomposedfs/node/node.go b/pkg/storage/pkg/decomposedfs/node/node.go index a11b122318c..441a0e44048 100644 --- a/pkg/storage/pkg/decomposedfs/node/node.go +++ b/pkg/storage/pkg/decomposedfs/node/node.go @@ -1300,19 +1300,25 @@ func (n *Node) ReadGrant(ctx context.Context, grantee string) (g *provider.Grant return e.Grant(), nil } -// ReadGrant reads a CS3 grant +// DeleteGrant reads a CS3 grant func (n *Node) DeleteGrant(ctx context.Context, g *provider.Grant) (err error) { var attr string switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_GROUP: attr = prefixes.GrantGroupAcePrefix + g.Grantee.GetGroupId().OpaqueId - default: + case provider.GranteeType_GRANTEE_TYPE_USER: prefix := prefixes.GrantUserAcePrefix + opaqueID := strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()) if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { prefix = prefixes.GrantMailAcePrefix + opaqueID = strings.ToLower(opaqueID) } - attr = prefix + g.Grantee.GetUserId().OpaqueId + attr = prefix + opaqueID + default: + // Note: We shouldn't actually get here as this is already caught in the upper + // layers (e.g. the storage provider) + return errors.New("grantee type is invalid") } if err = n.RemoveXattr(ctx, attr); err != nil { diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index 66c150e7cbc..5d6669f43f2 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -910,7 +910,7 @@ func (fs *Decomposedfs) updateIndexes(ctx context.Context, grantee *provider.Gra switch grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: if grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { - return fs.linkSpaceByMail(ctx, grantee.GetUserId().GetOpaqueId(), spaceID, target) + return fs.linkSpaceByMail(ctx, strings.ToLower(grantee.GetUserId().GetOpaqueId()), spaceID, target) } return fs.linkSpaceByUser(ctx, grantee.GetUserId().GetOpaqueId(), spaceID, target) case provider.GranteeType_GRANTEE_TYPE_GROUP: @@ -1007,7 +1007,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, case provider.GranteeType_GRANTEE_TYPE_USER: if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index - if err := fs.mailSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), n.GetSpaceID()); err != nil { + if err := fs.mailSpaceIndex.Remove(strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()), n.GetSpaceID()); err != nil { sublog.Error().Err(err).Str("grantee", id). Msg("failed to delete expired mail space index") } From 8d7b86b3830e347b66aaeb08f5954b895807b42e Mon Sep 17 00:00:00 2001 From: Alex Ababii Date: Wed, 26 Aug 2026 08:59:50 +0200 Subject: [PATCH 07/14] ability to remove guest grants from index --- pkg/storage/pkg/decomposedfs/spaces.go | 13 ++++++++++--- pkg/storage/pkg/decomposedfs/spaces_test.go | 13 ++++++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index 5d6669f43f2..dd7e94a3882 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -803,9 +803,16 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De for _, g := range grants { switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - // remove from user index - if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), spaceID); err != nil { - sublog.Error().Err(err).Str("grantee", g.Grantee.GetUserId().GetOpaqueId()).Msg("could not remove user from user index") + if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { + // remove from mail index + if err := fs.mailSpaceIndex.Remove(strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()), spaceID); err != nil { + sublog.Error().Err(err).Str("grantee", g.Grantee.GetUserId().GetOpaqueId()).Msg("could not remove guest from mail index") + } + } else { + // remove from user index + if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), spaceID); err != nil { + sublog.Error().Err(err).Str("grantee", g.Grantee.GetUserId().GetOpaqueId()).Msg("could not remove user from user index") + } } case provider.GranteeType_GRANTEE_TYPE_GROUP: // remove from group index diff --git a/pkg/storage/pkg/decomposedfs/spaces_test.go b/pkg/storage/pkg/decomposedfs/spaces_test.go index 53c12ba60a1..8b3d714b4ca 100644 --- a/pkg/storage/pkg/decomposedfs/spaces_test.go +++ b/pkg/storage/pkg/decomposedfs/spaces_test.go @@ -275,6 +275,7 @@ var _ = Describe("Spaces", func() { userGrantee := helpers.User0ID groupGrantee := "11111111-2222-3333-4444-555555555555" + guestGrantee := "Guest@Example.COM" Expect(env.Fs.AddGrant(env.Ctx, ref, &provider.Grant{ Grantee: &provider.Grantee{Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{UserId: &userv1beta1.UserId{OpaqueId: userGrantee}}}, Permissions: &provider.ResourcePermissions{Stat: true}, @@ -285,6 +286,11 @@ var _ = Describe("Spaces", func() { Permissions: &provider.ResourcePermissions{Stat: true}, Creator: &userv1beta1.UserId{OpaqueId: helpers.OwnerID}, })).To(Succeed()) + Expect(env.Fs.AddGrant(env.Ctx, ref, &provider.Grant{ + Grantee: &provider.Grantee{Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{UserId: &userv1beta1.UserId{OpaqueId: guestGrantee, Type: userv1beta1.UserType_USER_TYPE_GUEST}}}, + Permissions: &provider.ResourcePermissions{Stat: true}, + Creator: &userv1beta1.UserId{OpaqueId: helpers.OwnerID}, + })).To(Succeed()) // seed the grantee space-index entries. The real share flow writes // these via a space-typed context; here we add them directly so the @@ -296,6 +302,9 @@ var _ = Describe("Spaces", func() { groupIdx := spaceidindex.New(filepath.Join(env.Root, "indexes"), "by-group-id") Expect(groupIdx.Init()).To(Succeed()) Expect(groupIdx.Add(groupGrantee, spaceID, target)).To(Succeed()) + mailIdx := spaceidindex.New(filepath.Join(env.Root, "indexes"), "by-mail") + Expect(mailIdx.Init()).To(Succeed()) + Expect(mailIdx.Add("guest@example.com", spaceID, target)).To(Succeed()) load := func(indexName, key string) map[string]string { idx := spaceidindex.New(filepath.Join(env.Root, "indexes"), indexName) @@ -305,9 +314,10 @@ var _ = Describe("Spaces", func() { return m } - // precondition: both grantee indexes reference the space + // precondition: the grantee indexes reference the space Expect(load("by-user-id", userGrantee)).To(HaveKey(spaceID)) Expect(load("by-group-id", groupGrantee)).To(HaveKey(spaceID)) + Expect(load("by-mail", "guest@example.com")).To(HaveKey(spaceID)) // disable, then purge as the space admin ctx := ctxpkg.ContextSetUser(context.Background(), env.DeleteAllSpacesUser) @@ -320,6 +330,7 @@ var _ = Describe("Spaces", func() { // the grantee index entries must be gone after the purge Expect(load("by-user-id", userGrantee)).ToNot(HaveKey(spaceID)) Expect(load("by-group-id", groupGrantee)).ToNot(HaveKey(spaceID)) + Expect(load("by-mail", "guest@example.com")).ToNot(HaveKey(spaceID)) }) // purging one space must remove only that space's entry: a sibling // space of the same owner must stay in the by-user-id index. From 08184dc7f6cfda9632d48e7f795bf645c02d0081 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 31 Aug 2026 14:29:12 +0000 Subject: [PATCH 08/14] refactor(storage): centralize ACE prefix constants Move the ACE prefix constants to the ace package and get rid of hard coded prefixes spread throughout the code. --- .../decomposedfs/metadata/prefixes/prefixes.go | 11 ++++------- pkg/storage/utils/ace/ace.go | 18 ++++++++++++------ pkg/storage/utils/ace/ace_test.go | 12 ++++++------ .../decomposedfs/metadata/prefixes/prefixes.go | 9 ++++----- 4 files changed, 26 insertions(+), 24 deletions(-) diff --git a/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go b/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go index 8d0c94d6050..45851fa794e 100644 --- a/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go +++ b/pkg/storage/pkg/decomposedfs/metadata/prefixes/prefixes.go @@ -20,6 +20,7 @@ package prefixes import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + "github.com/opencloud-eu/reva/v2/pkg/storage/utils/ace" ) // Declare a list of xattr keys @@ -53,9 +54,9 @@ const ( // grantPrefix is the prefix for sharing related extended attributes GrantPrefix string = OcPrefix + "grant." - GrantUserAcePrefix string = OcPrefix + "grant." + UserAcePrefix - GrantGroupAcePrefix string = OcPrefix + "grant." + GroupAcePrefix - GrantMailAcePrefix string = OcPrefix + "grant." + MailAcePrefix + GrantUserAcePrefix string = OcPrefix + "grant." + ace.UserAcePrefix + GrantGroupAcePrefix string = OcPrefix + "grant." + ace.GroupAcePrefix + GrantMailAcePrefix string = OcPrefix + "grant." + ace.MailAcePrefix MetadataPrefix string = OcPrefix + "md." // favorite flag, per user @@ -104,10 +105,6 @@ const ( SpaceAliasAttr string = OcPrefix + "space.alias" SpaceTenantIDAttr string = OcPrefix + "space.tenantid" SpaceContentTypeAttr string = OcPrefix + "space.contenttype" - - UserAcePrefix string = "u:" - GroupAcePrefix string = "g:" - MailAcePrefix string = "m:" ) func FavoriteKey(uid *userpb.UserId) string { diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index 4913a9da7e2..89897ba0c97 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -33,6 +33,12 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/storage/utils/grants" ) +const ( + UserAcePrefix = "u:" + GroupAcePrefix = "g:" + MailAcePrefix = "m:" +) + /* ACE represents an Access Control Entry, mimicing NFSv4 ACLs The difference is tht grant ACEs are not propagated down the tree when being set on a dir. @@ -202,7 +208,7 @@ func FromGrant(g *provider.Grant) *ACE { } if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_GROUP { e.flags = "g" - e.principal = "g:" + g.Grantee.GetGroupId().OpaqueId + e.principal = GroupAcePrefix + g.Grantee.GetGroupId().OpaqueId } else { e.principal = UserAce(g.Grantee.GetUserId()) } @@ -217,9 +223,9 @@ func FromGrant(g *provider.Grant) *ACE { func UserAce(id *userpb.UserId) string { switch id.GetType() { case userpb.UserType_USER_TYPE_GUEST: - return "m:" + strings.ToLower(id.OpaqueId) + return MailAcePrefix + strings.ToLower(id.GetOpaqueId()) default: - return "u:" + id.OpaqueId + return UserAcePrefix + id.GetOpaqueId() } } @@ -261,11 +267,11 @@ func Unmarshal(principal string, v []byte) (e *ACE, err error) { } // check consistency of Flags and principal type if strings.Contains(e.flags, "g") { - if principal[:1] != "g" { + if !strings.HasPrefix(principal, GroupAcePrefix) { return nil, fmt.Errorf("inconsistent ace: expected group") } } else { - if principal[:1] != "u" && principal[:1] != "m" { + if !strings.HasPrefix(principal, UserAcePrefix) && !strings.HasPrefix(principal, MailAcePrefix) { return nil, fmt.Errorf("inconsistent ace: expected user") } } @@ -293,7 +299,7 @@ func (e *ACE) Grant() *provider.Grant { if e.granteeType() == provider.GranteeType_GRANTEE_TYPE_GROUP { g.Grantee.Id = &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: id}} } else if e.granteeType() == provider.GranteeType_GRANTEE_TYPE_USER { - if strings.HasPrefix(e.principal, "m:") { + if strings.HasPrefix(e.principal, MailAcePrefix) { g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_GUEST}} } else { g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_PRIMARY}} diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index bba5a122622..508cbc1015f 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -93,13 +93,13 @@ var _ = Describe("ACE", func() { Describe("FromGrant", func() { It("creates an ACE from a user grant", func() { - ace := ace.FromGrant(userGrant) - Expect(ace.Principal()).To(Equal("u:foo")) + aceValue := ace.FromGrant(userGrant) + Expect(aceValue.Principal()).To(Equal(ace.UserAcePrefix + "foo")) }) It("creates an ACE from a group grant", func() { - ace := ace.FromGrant(groupGrant) - Expect(ace.Principal()).To(Equal("g:foo")) + aceValue := ace.FromGrant(groupGrant) + Expect(aceValue.Principal()).To(Equal(ace.GroupAcePrefix + "foo")) }) It("creates an ACE from a guest grant", func() { @@ -109,8 +109,8 @@ var _ = Describe("ACE", func() { Type: userpb.UserType_USER_TYPE_GUEST, }, } - ace := ace.FromGrant(guestGrant) - Expect(ace.Principal()).To(Equal("m:guest@example.com")) + aceValue := ace.FromGrant(guestGrant) + Expect(aceValue.Principal()).To(Equal(ace.MailAcePrefix + "guest@example.com")) }) }) diff --git a/pkg/storage/utils/decomposedfs/metadata/prefixes/prefixes.go b/pkg/storage/utils/decomposedfs/metadata/prefixes/prefixes.go index fb0cbef28bb..855b3998efd 100644 --- a/pkg/storage/utils/decomposedfs/metadata/prefixes/prefixes.go +++ b/pkg/storage/utils/decomposedfs/metadata/prefixes/prefixes.go @@ -18,6 +18,8 @@ package prefixes +import "github.com/opencloud-eu/reva/v2/pkg/storage/utils/ace" + // Declare a list of xattr keys // TODO the below comment is currently copied from the owncloud driver, revisit // Currently,extended file attributes have four separated @@ -49,8 +51,8 @@ const ( // grantPrefix is the prefix for sharing related extended attributes GrantPrefix string = OcisPrefix + "grant." - GrantUserAcePrefix string = OcisPrefix + "grant." + UserAcePrefix - GrantGroupAcePrefix string = OcisPrefix + "grant." + GroupAcePrefix + GrantUserAcePrefix string = OcisPrefix + "grant." + ace.UserAcePrefix + GrantGroupAcePrefix string = OcisPrefix + "grant." + ace.GroupAcePrefix MetadataPrefix string = OcisPrefix + "md." // favorite flag, per user @@ -97,7 +99,4 @@ const ( SpaceReadmeAttr string = OcisPrefix + "space.readme" SpaceImageAttr string = OcisPrefix + "space.image" SpaceAliasAttr string = OcisPrefix + "space.alias" - - UserAcePrefix string = "u:" - GroupAcePrefix string = "g:" ) From 3c4b5456915ec4990018025d2b9eed25df6f27f4 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 31 Aug 2026 17:40:58 +0200 Subject: [PATCH 09/14] fix(decomposedfs): helper for guest ID normalization Introduce a helper for getting a canonical representation for a UserId. Currently this is mainly relevant for the USER_TYPE_GUEST users which user a mail address as the UserId. Also adapts the existing UserIDEqual() helper to make use of the new function. --- pkg/storage/pkg/decomposedfs/grants.go | 12 ++++-------- pkg/storage/pkg/decomposedfs/node/node.go | 8 +------- pkg/storage/pkg/decomposedfs/spaces.go | 8 ++++---- pkg/storage/utils/ace/ace.go | 5 +++-- pkg/storage/utils/ace/ace_test.go | 9 +++------ pkg/utils/utils.go | 19 +++++++++++++++++-- pkg/utils/utils_test.go | 21 +++++++++++++++++++++ 7 files changed, 53 insertions(+), 29 deletions(-) diff --git a/pkg/storage/pkg/decomposedfs/grants.go b/pkg/storage/pkg/decomposedfs/grants.go index fab100d2169..7c59e8d995f 100644 --- a/pkg/storage/pkg/decomposedfs/grants.go +++ b/pkg/storage/pkg/decomposedfs/grants.go @@ -236,12 +236,13 @@ func (fs *Decomposedfs) RemoveGrant(ctx context.Context, ref *provider.Reference switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: // remove from user index + canonicalUserID := utils.CanonicalUserID(g.Grantee.GetUserId()) if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { - if err := fs.mailSpaceIndex.Remove(strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()), grantNode.SpaceID); err != nil { + if err := fs.mailSpaceIndex.Remove(canonicalUserID, grantNode.SpaceID); err != nil { return err } } else { - if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), grantNode.SpaceID); err != nil { + if err := fs.userSpaceIndex.Remove(canonicalUserID, grantNode.SpaceID); err != nil { return err } } @@ -327,12 +328,7 @@ func (fs *Decomposedfs) loadGrant(ctx context.Context, ref *provider.Reference, for _, grant := range grants { switch grant.Grantee.GetType() { case provider.GranteeType_GRANTEE_TYPE_USER: - // guest grants are persisted with a lowercased mail as the principal - if grant.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { - if strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()) == grant.Grantee.GetUserId().GetOpaqueId() { - return n, unlockFunc, grant, nil - } - } else if g.Grantee.GetUserId().GetOpaqueId() == grant.Grantee.GetUserId().GetOpaqueId() { + if utils.UserIDEqual(g.Grantee.GetUserId(), grant.Grantee.GetUserId()) { return n, unlockFunc, grant, nil } case provider.GranteeType_GRANTEE_TYPE_GROUP: diff --git a/pkg/storage/pkg/decomposedfs/node/node.go b/pkg/storage/pkg/decomposedfs/node/node.go index 441a0e44048..ba2c2d42c1a 100644 --- a/pkg/storage/pkg/decomposedfs/node/node.go +++ b/pkg/storage/pkg/decomposedfs/node/node.go @@ -1308,13 +1308,7 @@ func (n *Node) DeleteGrant(ctx context.Context, g *provider.Grant) (err error) { case provider.GranteeType_GRANTEE_TYPE_GROUP: attr = prefixes.GrantGroupAcePrefix + g.Grantee.GetGroupId().OpaqueId case provider.GranteeType_GRANTEE_TYPE_USER: - prefix := prefixes.GrantUserAcePrefix - opaqueID := strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()) - if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { - prefix = prefixes.GrantMailAcePrefix - opaqueID = strings.ToLower(opaqueID) - } - attr = prefix + opaqueID + attr = prefixes.GrantPrefix + ace.UserAce(g.Grantee.GetUserId()) default: // Note: We shouldn't actually get here as this is already caught in the upper // layers (e.g. the storage provider) diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index dd7e94a3882..e6b0eb93c93 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -359,7 +359,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide // guests are granted shares via their mail address, search the mail index as well if requestedUserID.GetType() == userv1beta1.UserType_USER_TYPE_GUEST { - allMatches, err = fs.mailSpaceIndex.Load(strings.ToLower(requestedUserID.GetOpaqueId())) + allMatches, err = fs.mailSpaceIndex.Load(utils.CanonicalUserID(requestedUserID)) // do not return an error if the guest has no spaces if err != nil && !os.IsNotExist(err) { return nil, errors.Wrap(err, "error reading mail index") @@ -805,7 +805,7 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De case provider.GranteeType_GRANTEE_TYPE_USER: if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index - if err := fs.mailSpaceIndex.Remove(strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()), spaceID); err != nil { + if err := fs.mailSpaceIndex.Remove(utils.CanonicalUserID(g.Grantee.GetUserId()), spaceID); err != nil { sublog.Error().Err(err).Str("grantee", g.Grantee.GetUserId().GetOpaqueId()).Msg("could not remove guest from mail index") } } else { @@ -917,7 +917,7 @@ func (fs *Decomposedfs) updateIndexes(ctx context.Context, grantee *provider.Gra switch grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: if grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { - return fs.linkSpaceByMail(ctx, strings.ToLower(grantee.GetUserId().GetOpaqueId()), spaceID, target) + return fs.linkSpaceByMail(ctx, utils.CanonicalUserID(grantee.GetUserId()), spaceID, target) } return fs.linkSpaceByUser(ctx, grantee.GetUserId().GetOpaqueId(), spaceID, target) case provider.GranteeType_GRANTEE_TYPE_GROUP: @@ -1014,7 +1014,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, case provider.GranteeType_GRANTEE_TYPE_USER: if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index - if err := fs.mailSpaceIndex.Remove(strings.ToLower(g.Grantee.GetUserId().GetOpaqueId()), n.GetSpaceID()); err != nil { + if err := fs.mailSpaceIndex.Remove(utils.CanonicalUserID(g.Grantee.GetUserId()), n.GetSpaceID()); err != nil { sublog.Error().Err(err).Str("grantee", id). Msg("failed to delete expired mail space index") } diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index 89897ba0c97..505517121a2 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.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/opencloud-eu/reva/v2/pkg/storage/utils/grants" + "github.com/opencloud-eu/reva/v2/pkg/utils" ) const ( @@ -223,9 +224,9 @@ func FromGrant(g *provider.Grant) *ACE { func UserAce(id *userpb.UserId) string { switch id.GetType() { case userpb.UserType_USER_TYPE_GUEST: - return MailAcePrefix + strings.ToLower(id.GetOpaqueId()) + return MailAcePrefix + utils.CanonicalUserID(id) default: - return UserAcePrefix + id.GetOpaqueId() + return UserAcePrefix + utils.CanonicalUserID(id) } } diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index 508cbc1015f..c06b16e7794 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -20,7 +20,6 @@ package ace_test import ( "fmt" - "strings" grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -126,11 +125,9 @@ var _ = Describe("ACE", func() { It("returns a proper guest Grant with lowercased OpaqueId", func() { ace := ace.FromGrant(guestGrant) grant := ace.Grant() - // do not check opaque values - grant.Grantee.Opaque = nil - g := guestGrant - g.Grantee.Id.(*provider.Grantee_UserId).UserId.OpaqueId = strings.ToLower(g.Grantee.Id.(*provider.Grantee_UserId).UserId.OpaqueId) - Expect(grant).To(BeComparableTo(g, protocmp.Transform())) + Expect(grant.GetGrantee().GetUserId().GetOpaqueId()).To(Equal("guest@example.com")) + Expect(grant.GetGrantee().GetUserId().GetType()).To(Equal(userpb.UserType_USER_TYPE_GUEST)) + Expect(grant.GetPermissions()).To(BeComparableTo(guestGrant.GetPermissions(), protocmp.Transform())) }) It("returns a proper Grant for group ACE", func() { diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index bc4833dd27f..52ff5973c5c 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -218,12 +218,27 @@ func ExtractGranteeID(grantee *provider.Grantee) (*userpb.UserId, *grouppb.Group // UserEqual returns whether two users have the same field values. func UserEqual(u, v *userpb.UserId) bool { - return u != nil && v != nil && u.Idp == v.Idp && u.OpaqueId == v.OpaqueId + return u != nil && v != nil && u.Idp == v.Idp && UserIDEqual(u, v) } // UserIDEqual returns whether two users have the same opaqueid values. The idp is ignored func UserIDEqual(u, v *userpb.UserId) bool { - return u != nil && v != nil && u.OpaqueId == v.OpaqueId + if u == nil || v == nil { + return false + } + return CanonicalUserID(u) == CanonicalUserID(v) +} + +// CanonicalUserID returns the stable representation of a UserId as e.g. used for user storage keys. +// Currently this is only relevant for UserId of the USER_TYPE_GUEST, which are matched case-insensitively. +func CanonicalUserID(id *userpb.UserId) string { + if id == nil { + return "" + } + if id.GetType() == userpb.UserType_USER_TYPE_GUEST { + return strings.ToLower(id.GetOpaqueId()) + } + return id.GetOpaqueId() } // GroupEqual returns whether two groups have the same field values. diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index ca138ead72b..83a7338e825 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -21,6 +21,7 @@ package utils import ( "testing" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" ) @@ -171,3 +172,23 @@ func TestMakeRelativePath(t *testing.T) { } } } + +func TestCanonicalUserID(t *testing.T) { + tests := []struct { + name string + id *userpb.UserId + expected string + }{ + {name: "nil", expected: ""}, + {name: "regular user", id: &userpb.UserId{OpaqueId: "MixedCase"}, expected: "MixedCase"}, + {name: "guest", id: &userpb.UserId{OpaqueId: "Guest@Example.COM", Type: userpb.UserType_USER_TYPE_GUEST}, expected: "guest@example.com"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if actual := CanonicalUserID(tt.id); actual != tt.expected { + t.Errorf("expected %q, got %q", tt.expected, actual) + } + }) + } +} From c76ccb801571fd2b9f48dc893f17d62c9b97faf0 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 7 Sep 2026 17:33:24 +0200 Subject: [PATCH 10/14] fix: require filesystem safe keys for the share and space caches As the cache keys end up as parts of file names the access methods for the caches now accept the new "FilenameEncoder" interface. The interface implements a single method "SafeFilename()". The new wrapper types FSSafeUserId and FSSafeGroupId implement that interface for the cs3 UserId and GroupId Types. Currently only UserIds of the type GUEST are actually encoded (simple base64 encoding) all other IDs are assumed to be safe filenames. Similar to the share manager caches we now encode the IDs for mail user invites when using them as names for attributes in the ACE string or as file/directory names for the space indexes. --- .../usershareprovider/usershareprovider.go | 13 +++- .../usershareprovider_test.go | 44 +++++++++++ pkg/share/manager/jsoncs3/jsoncs3.go | 44 ++++++----- .../receivedsharecache/receivedsharecache.go | 71 +++++++++-------- .../receivedsharecache_test.go | 15 +++- .../manager/jsoncs3/sharecache/sharecache.go | 69 +++++++++-------- .../jsoncs3/sharecache/sharecache_test.go | 14 +++- pkg/storage/pkg/decomposedfs/grants.go | 7 +- pkg/storage/pkg/decomposedfs/grants_test.go | 7 ++ pkg/storage/pkg/decomposedfs/node/node.go | 4 +- pkg/storage/pkg/decomposedfs/spaces.go | 20 +++-- pkg/storage/pkg/decomposedfs/spaces_test.go | 39 +++++++++- pkg/storage/utils/ace/ace.go | 14 +++- pkg/storage/utils/ace/ace_test.go | 12 +-- pkg/utils/filename_encoder.go | 54 +++++++++++++ pkg/utils/filename_encoder_suite_test.go | 13 ++++ pkg/utils/filename_encoder_test.go | 76 +++++++++++++++++++ 17 files changed, 399 insertions(+), 117 deletions(-) create mode 100644 pkg/utils/filename_encoder.go create mode 100644 pkg/utils/filename_encoder_suite_test.go create mode 100644 pkg/utils/filename_encoder_test.go diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index f65e143bd5b..d8521638231 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -20,6 +20,7 @@ package usershareprovider import ( "context" + "net/mail" "path/filepath" "regexp" "slices" @@ -180,8 +181,18 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar Status: status.NewPermissionDenied(ctx, nil, "user tenantId does not match the target user tenantId"), }, nil } - } + // guests are identified by their mail address, which is used as a path + // segment in the share manager storage; reject invalid mails before they + // end up in any index or cache. + _, err := mail.ParseAddress(req.GetGrant().GetGrantee().GetUserId().GetOpaqueId()) + if req.GetGrant().GetGrantee().GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST && err != nil { + + return &collaboration.CreateShareResponse{ + Status: status.NewInvalidArg(ctx, "invalid mail address for guest grantee"), + }, nil + } + } gatewayClient, err := s.gatewaySelector.Next() if err != nil { return nil, err diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 90e9e21499f..0477a97eaab 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -514,6 +514,50 @@ var _ = Describe("user share provider service", func() { manager.AssertNumberOfCalls(GinkgoT(), "Share", 1) }) + + It("fails when sharing with a guest using an invalid mail address", func() { + createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{ + ResourceInfo: &providerpb.ResourceInfo{ + PermissionSet: conversions.RoleFromName("manager").CS3ResourcePermissions(), + }, + Grant: &collaborationpb.ShareGrant{ + Grantee: &providerpb.Grantee{ + Type: providerpb.GranteeType_GRANTEE_TYPE_USER, + Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "not-a-mail", TenantId: "tenant1", Type: userpb.UserType_USER_TYPE_GUEST}}, + }, + Permissions: &collaborationpb.SharePermissions{ + Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), + }, + }, + }) + + Expect(err).ToNot(HaveOccurred()) + Expect(createShareResponse.Status.Code).To(Equal(rpcpb.Code_CODE_INVALID_ARGUMENT)) + + manager.AssertNumberOfCalls(GinkgoT(), "Share", 0) + }) + + It("fails when sharing with a guest using a mail address with path separators", func() { + createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{ + ResourceInfo: &providerpb.ResourceInfo{ + PermissionSet: conversions.RoleFromName("manager").CS3ResourcePermissions(), + }, + Grant: &collaborationpb.ShareGrant{ + Grantee: &providerpb.Grantee{ + Type: providerpb.GranteeType_GRANTEE_TYPE_USER, + Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "my/../email@email.com", TenantId: "tenant1", Type: userpb.UserType_USER_TYPE_GUEST}}, + }, + Permissions: &collaborationpb.SharePermissions{ + Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), + }, + }, + }) + + Expect(err).ToNot(HaveOccurred()) + Expect(createShareResponse.Status.Code).To(Equal(rpcpb.Code_CODE_INVALID_ARGUMENT)) + + manager.AssertNumberOfCalls(GinkgoT(), "Share", 0) + }) }) }) diff --git a/pkg/share/manager/jsoncs3/jsoncs3.go b/pkg/share/manager/jsoncs3/jsoncs3.go index 2e90b062ea2..3b6df2bf797 100644 --- a/pkg/share/manager/jsoncs3/jsoncs3.go +++ b/pkg/share/manager/jsoncs3/jsoncs3.go @@ -26,6 +26,7 @@ import ( "time" gatewayv1beta1 "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" @@ -482,7 +483,7 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla }) eg.Go(func() error { - err := m.CreatedCache.Add(ctx, s.GetCreator().GetOpaqueId(), shareID) + err := m.CreatedCache.Add(ctx, utils.FSSafeUserID{ID: s.GetCreator()}, shareID) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -496,13 +497,12 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: eg.Go(func() error { - userid := g.Grantee.GetUserId().GetOpaqueId() rs := &collaboration.ReceivedShare{ Share: s, State: collaboration.ShareState_SHARE_STATE_PENDING, } - err := m.UserReceivedStates.Add(ctx, userid, spaceID, rs) + err := m.UserReceivedStates.Add(ctx, utils.FSSafeUserID{ID: g.GetGrantee().GetUserId()}, spaceID, rs) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -512,8 +512,7 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla }) case provider.GranteeType_GRANTEE_TYPE_GROUP: eg.Go(func() error { - groupid := g.Grantee.GetGroupId().GetOpaqueId() - err := m.GroupReceivedCache.Add(ctx, groupid, shareID) + err := m.GroupReceivedCache.Add(ctx, utils.FSSafeGroupID{ID: g.GetGrantee().GetGroupId()}, shareID) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -879,7 +878,7 @@ func (m *Manager) listCreatedShares(ctx context.Context, user *userv1beta1.User, defer span.End() sublog := appctx.GetLogger(ctx).With().Str("userid", user.GetId().GetOpaqueId()).Str("useridp", user.GetId().GetIdp()).Str("driver", "jsoncs3").Str("handler", "listCreatedShares").Logger() - list, err := m.CreatedCache.List(ctx, user.Id.OpaqueId) + list, err := m.CreatedCache.List(ctx, utils.FSSafeUserID{ID: user.GetId()}) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -1024,7 +1023,10 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati // first collect all spaceids the user has access to as a group member for _, group := range user.Groups { - list, err := m.GroupReceivedCache.List(ctx, group) + groupid := grouppb.GroupId{ + OpaqueId: group, + } + list, err := m.GroupReceivedCache.List(ctx, utils.FSSafeGroupID{ID: &groupid}) if err != nil { continue // ignore error, cache will be updated on next read } @@ -1049,7 +1051,7 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati } // add all spaces the user has receved shares for, this includes mount points and share state for groups - spaces, err := m.UserReceivedStates.List(ctx, user.Id.OpaqueId) + spaces, err := m.UserReceivedStates.List(ctx, utils.FSSafeUserID{ID: user.GetId()}) if err != nil { return nil, err } @@ -1112,7 +1114,7 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati } if s == nil { sublogr.Warn().Str("shareid", shareID).Msg("share not found. cleaning up") - _ = m.UserReceivedStates.Remove(ctx, user.Id.OpaqueId, w.ssid, shareID) + _ = m.UserReceivedStates.Remove(ctx, utils.FSSafeUserID{ID: user.GetId()}, w.ssid, shareID) continue } sublogr = sublogr.With().Str("shareid", shareID).Logger() @@ -1190,7 +1192,7 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati } // convert must be called in a lock-controlled block. -func (m *Manager) convert(ctx context.Context, userID string, s *collaboration.Share) *collaboration.ReceivedShare { +func (m *Manager) convert(ctx context.Context, userID *userv1beta1.UserId, s *collaboration.Share) *collaboration.ReceivedShare { ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "convert") defer span.End() @@ -1201,7 +1203,7 @@ func (m *Manager) convert(ctx context.Context, userID string, s *collaboration.S storageID, spaceID, _ := shareid.Decode(s.Id.OpaqueId) - state, err := m.UserReceivedStates.Get(ctx, userID, storageID+shareid.IDDelimiter+spaceID, s.Id.GetOpaqueId()) + state, err := m.UserReceivedStates.Get(ctx, utils.FSSafeUserID{ID: userID}, storageID+shareid.IDDelimiter+spaceID, s.Id.GetOpaqueId()) if err == nil && state != nil { rs.State = state.State rs.MountPoint = state.MountPoint @@ -1261,7 +1263,7 @@ func (m *Manager) getReceived(ctx context.Context, ref *collaboration.ShareRefer } } } - return m.convert(ctx, user.Id.GetOpaqueId(), s), nil + return m.convert(ctx, user.GetId(), s), nil } // UpdateReceivedShare updates the received share with share state. @@ -1296,12 +1298,12 @@ func (m *Manager) UpdateReceivedShare(ctx context.Context, receivedShare *collab // write back u := ctxpkg.ContextMustGetUser(ctx) - uid := u.GetId().GetOpaqueId() + uid := u.GetId() if u.GetId().GetType() == userv1beta1.UserType_USER_TYPE_SERVICE { - uid = forUser.GetOpaqueId() + uid = forUser } - err = m.UserReceivedStates.Add(ctx, uid, rs.Share.ResourceId.StorageId+shareid.IDDelimiter+rs.Share.ResourceId.SpaceId, rs) + err = m.UserReceivedStates.Add(ctx, utils.FSSafeUserID{ID: uid}, rs.Share.ResourceId.StorageId+shareid.IDDelimiter+rs.Share.ResourceId.SpaceId, rs) if err != nil { return nil, err } @@ -1339,7 +1341,7 @@ func (m *Manager) Load(ctx context.Context, shareChan <-chan *collaboration.Shar } else { l.Debug().Str("storageid", s.GetResourceId().GetStorageId()).Str("spaceid", s.GetResourceId().GetSpaceId()).Str("shareid", s.Id.OpaqueId).Msg("imported share") } - if err := m.CreatedCache.Add(ctx, s.GetCreator().GetOpaqueId(), s.Id.OpaqueId); err != nil { + if err := m.CreatedCache.Add(ctx, utils.FSSafeUserID{ID: s.GetCreator()}, s.Id.OpaqueId); err != nil { l.Error().Err(err).Interface("share", s).Msg("error persisting created cache") } else { l.Debug().Str("creatorid", s.GetCreator().GetOpaqueId()).Str("shareid", s.Id.OpaqueId).Msg("updated created cache") @@ -1355,14 +1357,14 @@ func (m *Manager) Load(ctx context.Context, shareChan <-chan *collaboration.Shar } if s.UserID != nil { spaceid := s.ReceivedShare.GetShare().GetResourceId().GetStorageId() + shareid.IDDelimiter + s.ReceivedShare.GetShare().GetResourceId().GetSpaceId() - if err := m.UserReceivedStates.Add(context.Background(), s.UserID.GetOpaqueId(), spaceid, s.ReceivedShare); err != nil { + if err := m.UserReceivedStates.Add(context.Background(), utils.FSSafeUserID{ID: s.UserID}, spaceid, s.ReceivedShare); err != nil { l.Error().Err(err).Interface("received share", s).Msg("error persisting received share for user") } else { l.Debug().Str("userid", s.UserID.GetOpaqueId()).Str("spaceid", spaceid).Str("shareid", s.ReceivedShare.GetShare().Id.OpaqueId).Msg("updated received share userdata") } } if s.ReceivedShare.Share.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_GROUP && s.UserID == nil { - if err := m.GroupReceivedCache.Add(context.Background(), s.ReceivedShare.GetShare().GetGrantee().GetGroupId().GetOpaqueId(), s.ReceivedShare.GetShare().GetId().GetOpaqueId()); err != nil { + if err := m.GroupReceivedCache.Add(context.Background(), utils.FSSafeGroupID{ID: s.ReceivedShare.GetShare().GetGrantee().GetGroupId()}, s.ReceivedShare.GetShare().GetId().GetOpaqueId()); err != nil { l.Error().Err(err).Interface("received share", s).Msg("error persisting received share to group cache") } else { l.Debug().Str("groupid", s.ReceivedShare.GetShare().GetGrantee().GetGroupId().GetOpaqueId()).Str("shareid", s.ReceivedShare.GetShare().Id.OpaqueId).Msg("updated received share group cache") @@ -1418,15 +1420,15 @@ func (m *Manager) removeShare(ctx context.Context, s *collaboration.Share, skipS eg.Go(func() error { // remove from created cache - return m.CreatedCache.Remove(ctx, s.GetCreator().GetOpaqueId(), s.Id.OpaqueId) + return m.CreatedCache.Remove(ctx, utils.FSSafeUserID{ID: s.GetCreator()}, s.Id.OpaqueId) }) eg.Go(func() error { // remove from user received states if s.GetGrantee().Type == provider.GranteeType_GRANTEE_TYPE_USER { - return m.UserReceivedStates.Remove(ctx, s.GetGrantee().GetUserId().GetOpaqueId(), s.GetResourceId().GetStorageId()+shareid.IDDelimiter+s.GetResourceId().GetSpaceId(), s.Id.OpaqueId) + return m.UserReceivedStates.Remove(ctx, utils.FSSafeUserID{ID: s.GetGrantee().GetUserId()}, s.GetResourceId().GetStorageId()+shareid.IDDelimiter+s.GetResourceId().GetSpaceId(), s.Id.OpaqueId) } else if s.GetGrantee().Type == provider.GranteeType_GRANTEE_TYPE_GROUP { - return m.GroupReceivedCache.Remove(ctx, s.GetGrantee().GetGroupId().GetOpaqueId(), s.Id.OpaqueId) + return m.GroupReceivedCache.Remove(ctx, utils.FSSafeGroupID{ID: s.GetGrantee().GetGroupId()}, s.Id.OpaqueId) } return nil }) diff --git a/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache.go b/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache.go index a0e94361a90..b8e536b0821 100644 --- a/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache.go +++ b/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache.go @@ -34,6 +34,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/errtypes" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/decomposedfs/mtimesyncedcache" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/metadata" + "github.com/opencloud-eu/reva/v2/pkg/utils" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" ) @@ -82,8 +83,8 @@ func New(s metadata.Storage, ttl time.Duration) Cache { } } -func (c *Cache) lockUser(userID string) func() { - v, _ := c.lockMap.LoadOrStore(userID, &sync.Mutex{}) +func (c *Cache) lockUser(userID utils.FilenameEncoder) func() { + v, _ := c.lockMap.LoadOrStore(userID.SafeFilename(), &sync.Mutex{}) lock := v.(*sync.Mutex) lock.Lock() @@ -91,14 +92,15 @@ func (c *Cache) lockUser(userID string) func() { } // Add adds a new entry to the cache -func (c *Cache) Add(ctx context.Context, userID, spaceID string, rs *collaboration.ReceivedShare) error { +func (c *Cache) Add(ctx context.Context, userID utils.FilenameEncoder, spaceID string, rs *collaboration.ReceivedShare) error { + userIDKey := userID.SafeFilename() ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") unlock := c.lockUser(userID) span.End() - span.SetAttributes(attribute.String("cs3.userid", userID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey)) defer unlock() - if _, ok := c.ReceivedSpaces.Load(userID); !ok { + if _, ok := c.ReceivedSpaces.Load(userIDKey); !ok { err := c.syncWithLock(ctx, userID) if err != nil { return err @@ -107,12 +109,12 @@ func (c *Cache) Add(ctx context.Context, userID, spaceID string, rs *collaborati ctx, span = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Add") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userID), attribute.String("cs3.spaceid", spaceID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey), attribute.String("cs3.spaceid", spaceID)) persistFunc := func() error { - c.initializeIfNeeded(userID, spaceID) + c.initializeIfNeeded(userIDKey, spaceID) - rss, _ := c.ReceivedSpaces.Load(userID) + rss, _ := c.ReceivedSpaces.Load(userIDKey) receivedSpace := rss.Spaces[spaceID] if receivedSpace.States == nil { receivedSpace.States = map[string]*State{} @@ -128,7 +130,7 @@ func (c *Cache) Add(ctx context.Context, userID, spaceID string, rs *collaborati log := appctx.GetLogger(ctx).With(). Str("hostname", os.Getenv("HOSTNAME")). - Str("userID", userID). + Str("userIDKey", userIDKey). Str("spaceID", spaceID).Logger() var err error @@ -167,18 +169,19 @@ func (c *Cache) Add(ctx context.Context, userID, spaceID string, rs *collaborati } // Get returns one entry from the cache -func (c *Cache) Get(ctx context.Context, userID, spaceID, shareID string) (*State, error) { +func (c *Cache) Get(ctx context.Context, userID utils.FilenameEncoder, spaceID, shareID string) (*State, error) { ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") + userIDKey := userID.SafeFilename() unlock := c.lockUser(userID) span.End() - span.SetAttributes(attribute.String("cs3.userid", userID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey)) defer unlock() err := c.syncWithLock(ctx, userID) if err != nil { return nil, err } - rss, ok := c.ReceivedSpaces.Load(userID) + rss, ok := c.ReceivedSpaces.Load(userIDKey) if !ok || rss.Spaces[spaceID] == nil { return nil, nil } @@ -186,21 +189,22 @@ func (c *Cache) Get(ctx context.Context, userID, spaceID, shareID string) (*Stat } // Remove removes an entry from the cache -func (c *Cache) Remove(ctx context.Context, userID, spaceID, shareID string) error { +func (c *Cache) Remove(ctx context.Context, userID utils.FilenameEncoder, spaceID, shareID string) error { ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") + userIDKey := userID.SafeFilename() unlock := c.lockUser(userID) span.End() - span.SetAttributes(attribute.String("cs3.userid", userID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey)) defer unlock() ctx, span = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Add") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userID), attribute.String("cs3.spaceid", spaceID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey), attribute.String("cs3.spaceid", spaceID)) persistFunc := func() error { - c.initializeIfNeeded(userID, spaceID) + c.initializeIfNeeded(userIDKey, spaceID) - rss, _ := c.ReceivedSpaces.Load(userID) + rss, _ := c.ReceivedSpaces.Load(userIDKey) receivedSpace := rss.Spaces[spaceID] if receivedSpace.States == nil { receivedSpace.States = map[string]*State{} @@ -215,7 +219,7 @@ func (c *Cache) Remove(ctx context.Context, userID, spaceID, shareID string) err log := appctx.GetLogger(ctx).With(). Str("hostname", os.Getenv("HOSTNAME")). - Str("userID", userID). + Str("userIDKey", userIDKey). Str("spaceID", spaceID).Logger() var err error @@ -255,11 +259,12 @@ func (c *Cache) Remove(ctx context.Context, userID, spaceID, shareID string) err // List returns a list of received shares for a given user // The return list is guaranteed to be thread-safe -func (c *Cache) List(ctx context.Context, userID string) (map[string]*Space, error) { +func (c *Cache) List(ctx context.Context, userID utils.FilenameEncoder) (map[string]*Space, error) { + userIDKey := userID.SafeFilename() ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") unlock := c.lockUser(userID) span.End() - span.SetAttributes(attribute.String("cs3.userid", userID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey)) defer unlock() err := c.syncWithLock(ctx, userID) @@ -268,7 +273,7 @@ func (c *Cache) List(ctx context.Context, userID string) (map[string]*Space, err } spaces := map[string]*Space{} - rss, _ := c.ReceivedSpaces.Load(userID) + rss, _ := c.ReceivedSpaces.Load(userIDKey) for spaceID, space := range rss.Spaces { spaceCopy := &Space{ States: map[string]*State{}, @@ -285,19 +290,20 @@ func (c *Cache) List(ctx context.Context, userID string) (map[string]*Space, err return spaces, nil } -func (c *Cache) syncWithLock(ctx context.Context, userID string) error { +func (c *Cache) syncWithLock(ctx context.Context, userID utils.FilenameEncoder) error { + userIDKey := userID.SafeFilename() ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Sync") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey)) - log := appctx.GetLogger(ctx).With().Str("userID", userID).Logger() + log := appctx.GetLogger(ctx).With().Str("userIDKey", userIDKey).Logger() - c.initializeIfNeeded(userID, "") + c.initializeIfNeeded(userIDKey, "") - jsonPath := userJSONPath(userID) + jsonPath := userJSONPath(userIDKey) span.AddEvent("updating cache") // - update cached list of created shares for the user in memory if changed - rss, _ := c.ReceivedSpaces.Load(userID) + rss, _ := c.ReceivedSpaces.Load(userIDKey) dlres, err := c.storage.Download(ctx, metadata.DownloadRequest{ Path: jsonPath, IfNoneMatch: []string{rss.etag}, @@ -326,18 +332,19 @@ func (c *Cache) syncWithLock(ctx context.Context, userID string) error { } newSpaces.etag = dlres.Etag - c.ReceivedSpaces.Store(userID, newSpaces) + c.ReceivedSpaces.Store(userIDKey, newSpaces) span.SetStatus(codes.Ok, "") return nil } // persist persists the data for one user to the storage -func (c *Cache) persist(ctx context.Context, userID string) error { +func (c *Cache) persist(ctx context.Context, userID utils.FilenameEncoder) error { + userIDKey := userID.SafeFilename() ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Persist") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userID)) + span.SetAttributes(attribute.String("cs3.userid.key", userIDKey)) - rss, ok := c.ReceivedSpaces.Load(userID) + rss, ok := c.ReceivedSpaces.Load(userIDKey) if !ok { span.SetStatus(codes.Ok, "no received shares") return nil @@ -349,7 +356,7 @@ func (c *Cache) persist(ctx context.Context, userID string) error { span.SetStatus(codes.Error, err.Error()) return err } - jsonPath := userJSONPath(userID) + jsonPath := userJSONPath(userIDKey) if err := c.storage.MakeDirIfNotExist(ctx, path.Dir(jsonPath)); err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) diff --git a/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go b/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go index 93d5cab0677..eb78756d1f6 100644 --- a/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go +++ b/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go @@ -23,9 +23,11 @@ import ( "os" "time" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" "github.com/opencloud-eu/reva/v2/pkg/share/manager/jsoncs3/receivedsharecache" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/metadata" + "github.com/opencloud-eu/reva/v2/pkg/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -36,7 +38,11 @@ var _ = Describe("Cache", func() { c receivedsharecache.Cache storage metadata.Storage - userID = "user" + userID = utils.FSSafeUserID{ + ID: &userpb.UserId{ + OpaqueId: "userid", + }, + } spaceID = "spaceid" shareID = "storageid$spaceid!share1" share = &collaboration.Share{ @@ -111,7 +117,12 @@ var _ = Describe("Cache", func() { Describe("Get", func() { It("handles unknown users", func() { - s, err := c.Get(ctx, "something", spaceID, shareID) + someUserID := utils.FSSafeUserID{ + ID: &userpb.UserId{ + OpaqueId: "something", + }, + } + s, err := c.Get(ctx, someUserID, spaceID, shareID) Expect(err).ToNot(HaveOccurred()) Expect(s).To(BeNil()) }) diff --git a/pkg/share/manager/jsoncs3/sharecache/sharecache.go b/pkg/share/manager/jsoncs3/sharecache/sharecache.go index d7a830594a7..c5a812dc163 100644 --- a/pkg/share/manager/jsoncs3/sharecache/sharecache.go +++ b/pkg/share/manager/jsoncs3/sharecache/sharecache.go @@ -37,6 +37,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/share/manager/jsoncs3/shareid" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/decomposedfs/mtimesyncedcache" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/metadata" + "github.com/opencloud-eu/reva/v2/pkg/utils" ) // name is the Tracer name used to identify this instrumentation library. @@ -89,15 +90,17 @@ func New(s metadata.Storage, namespace, filename string, ttl time.Duration) Cach } // Add adds a share to the cache -func (c *Cache) Add(ctx context.Context, userid, shareID string) error { +func (c *Cache) Add(ctx context.Context, id utils.FilenameEncoder, shareID string) error { + key := id.SafeFilename() + ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") - unlock := c.lockUser(userid) + unlock := c.lockUser(key) span.End() - span.SetAttributes(attribute.String("cs3.userid", userid)) + span.SetAttributes(attribute.String("cs3.userid", key)) defer unlock() - if _, ok := c.UserShares.Load(userid); !ok { - err := c.syncWithLock(ctx, userid) + if _, ok := c.UserShares.Load(key); !ok { + err := c.syncWithLock(ctx, key) if err != nil { return err } @@ -105,23 +108,23 @@ func (c *Cache) Add(ctx context.Context, userid, shareID string) error { ctx, span = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Add") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userid), attribute.String("cs3.shareid", shareID)) + span.SetAttributes(attribute.String("cs3.userid", key), attribute.String("cs3.shareid", shareID)) storageid, spaceid, _ := shareid.Decode(shareID) ssid := storageid + shareid.IDDelimiter + spaceid persistFunc := func() error { - c.initializeIfNeeded(userid, ssid) + c.initializeIfNeeded(key, ssid) // add share id - us, _ := c.UserShares.Load(userid) + us, _ := c.UserShares.Load(key) us.UserShares[ssid].IDs[shareID] = struct{}{} - return c.Persist(ctx, userid) + return c.Persist(ctx, key) } log := appctx.GetLogger(ctx).With(). Str("hostname", os.Getenv("HOSTNAME")). - Str("userID", userid). + Str("userID", key). Str("shareID", shareID).Logger() var err error @@ -149,7 +152,7 @@ func (c *Cache) Add(ctx context.Context, userid, shareID string) error { log.Error().Err(err).Msg("persisting added share failed") return err } - if err := c.syncWithLock(ctx, userid); err != nil { + if err := c.syncWithLock(ctx, key); err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) log.Error().Err(err).Msg("persisting added share failed. giving up.") @@ -160,15 +163,16 @@ func (c *Cache) Add(ctx context.Context, userid, shareID string) error { } // Remove removes a share for the given user -func (c *Cache) Remove(ctx context.Context, userid, shareID string) error { +func (c *Cache) Remove(ctx context.Context, id utils.FilenameEncoder, shareID string) error { ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") - unlock := c.lockUser(userid) + key := id.SafeFilename() + unlock := c.lockUser(key) span.End() - span.SetAttributes(attribute.String("cs3.userid", userid)) + span.SetAttributes(attribute.String("cs3.userid", key)) defer unlock() - if _, ok := c.UserShares.Load(userid); ok { - err := c.syncWithLock(ctx, userid) + if _, ok := c.UserShares.Load(key); ok { + err := c.syncWithLock(ctx, key) if err != nil { return err } @@ -176,13 +180,13 @@ func (c *Cache) Remove(ctx context.Context, userid, shareID string) error { ctx, span = appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Remove") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userid), attribute.String("cs3.shareid", shareID)) + span.SetAttributes(attribute.String("cs3.userid", key), attribute.String("cs3.shareid", shareID)) storageid, spaceid, _ := shareid.Decode(shareID) ssid := storageid + shareid.IDDelimiter + spaceid persistFunc := func() error { - us, loaded := c.UserShares.LoadOrStore(userid, &UserShareCache{ + us, loaded := c.UserShares.LoadOrStore(key, &UserShareCache{ UserShares: map[string]*SpaceShareIDs{}, }) @@ -191,12 +195,12 @@ func (c *Cache) Remove(ctx context.Context, userid, shareID string) error { delete(us.UserShares[ssid].IDs, shareID) } - return c.Persist(ctx, userid) + return c.Persist(ctx, key) } log := appctx.GetLogger(ctx).With(). Str("hostname", os.Getenv("HOSTNAME")). - Str("userID", userid). + Str("userID", key). Str("shareID", shareID).Logger() var err error @@ -224,7 +228,7 @@ func (c *Cache) Remove(ctx context.Context, userid, shareID string) error { log.Error().Err(err).Msg("persisting removed share failed") return err } - if err := c.syncWithLock(ctx, userid); err != nil { + if err := c.syncWithLock(ctx, key); err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) return err @@ -235,18 +239,19 @@ func (c *Cache) Remove(ctx context.Context, userid, shareID string) error { } // List return the list of spaces/shares for the given user/group -func (c *Cache) List(ctx context.Context, userid string) (map[string]SpaceShareIDs, error) { +func (c *Cache) List(ctx context.Context, id utils.FilenameEncoder) (map[string]SpaceShareIDs, error) { ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Grab lock") - unlock := c.lockUser(userid) + key := id.SafeFilename() + unlock := c.lockUser(key) span.End() - span.SetAttributes(attribute.String("cs3.userid", userid)) + span.SetAttributes(attribute.String("cs3.userid", key)) defer unlock() - if err := c.syncWithLock(ctx, userid); err != nil { + if err := c.syncWithLock(ctx, key); err != nil { return nil, err } r := map[string]SpaceShareIDs{} - us, ok := c.UserShares.Load(userid) + us, ok := c.UserShares.Load(key) if !ok { return r, nil } @@ -309,12 +314,12 @@ func (c *Cache) syncWithLock(ctx context.Context, userID string) error { } // Persist persists the data for one user/group to the storage -func (c *Cache) Persist(ctx context.Context, userid string) error { +func (c *Cache) Persist(ctx context.Context, key string) error { ctx, span := appctx.GetTracerProvider(ctx).Tracer(tracerName).Start(ctx, "Persist") defer span.End() - span.SetAttributes(attribute.String("cs3.userid", userid)) + span.SetAttributes(attribute.String("cs3.userid", key)) - us, ok := c.UserShares.Load(userid) + us, ok := c.UserShares.Load(key) if !ok { span.SetStatus(codes.Ok, "no user shares") return nil @@ -325,7 +330,7 @@ func (c *Cache) Persist(ctx context.Context, userid string) error { span.SetStatus(codes.Error, err.Error()) return err } - jsonPath := c.userCreatedPath(userid) + jsonPath := c.userCreatedPath(key) if err := c.storage.MakeDirIfNotExist(ctx, path.Dir(jsonPath)); err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -355,8 +360,8 @@ func (c *Cache) Persist(ctx context.Context, userid string) error { return nil } -func (c *Cache) userCreatedPath(userid string) string { - return filepath.Join("/", c.namespace, userid, c.filename) +func (c *Cache) userCreatedPath(key string) string { + return filepath.Join("/", c.namespace, key, c.filename) } func (c *Cache) initializeIfNeeded(userid, ssid string) { diff --git a/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go b/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go index 342339a47d3..7a00bcecc62 100644 --- a/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go +++ b/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go @@ -26,8 +26,10 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" "github.com/opencloud-eu/reva/v2/pkg/share/manager/jsoncs3/sharecache" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/metadata" + "github.com/opencloud-eu/reva/v2/pkg/utils" ) var _ = Describe("Sharecache", func() { @@ -35,7 +37,11 @@ var _ = Describe("Sharecache", func() { c sharecache.Cache storage metadata.Storage - userid = "user" + userid = utils.FSSafeUserID{ + ID: &userpb.UserId{ + OpaqueId: "userid", + }, + } shareID = "storageid$spaceid!share1" ctx context.Context tmpdir string @@ -71,13 +77,13 @@ var _ = Describe("Sharecache", func() { }) It("updates the etag", func() { - uc, _ := c.UserShares.Load(userid) + uc, _ := c.UserShares.Load(userid.SafeFilename()) oldEtag := uc.Etag Expect(oldEtag).ToNot(BeEmpty()) - Expect(c.Persist(ctx, userid)).To(Succeed()) + Expect(c.Persist(ctx, userid.SafeFilename())).To(Succeed()) - uc, _ = c.UserShares.Load(userid) + uc, _ = c.UserShares.Load(userid.SafeFilename()) Expect(uc.Etag).ToNot(Equal(oldEtag)) }) }) diff --git a/pkg/storage/pkg/decomposedfs/grants.go b/pkg/storage/pkg/decomposedfs/grants.go index 7c59e8d995f..b21a496358e 100644 --- a/pkg/storage/pkg/decomposedfs/grants.go +++ b/pkg/storage/pkg/decomposedfs/grants.go @@ -235,14 +235,13 @@ func (fs *Decomposedfs) RemoveGrant(ctx context.Context, ref *provider.Reference // invalidate space grant switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - // remove from user index - canonicalUserID := utils.CanonicalUserID(g.Grantee.GetUserId()) + filename := (utils.FSSafeUserID{ID: g.Grantee.GetUserId()}).SafeFilename() if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { - if err := fs.mailSpaceIndex.Remove(canonicalUserID, grantNode.SpaceID); err != nil { + if err := fs.mailSpaceIndex.Remove(filename, grantNode.SpaceID); err != nil { return err } } else { - if err := fs.userSpaceIndex.Remove(canonicalUserID, grantNode.SpaceID); err != nil { + if err := fs.userSpaceIndex.Remove(filename, grantNode.SpaceID); err != nil { return err } } diff --git a/pkg/storage/pkg/decomposedfs/grants_test.go b/pkg/storage/pkg/decomposedfs/grants_test.go index edb1c348b23..838bd8207d1 100644 --- a/pkg/storage/pkg/decomposedfs/grants_test.go +++ b/pkg/storage/pkg/decomposedfs/grants_test.go @@ -31,6 +31,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/lookup" "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/metadata/prefixes" helpers "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/testhelpers" + "github.com/opencloud-eu/reva/v2/pkg/utils" "github.com/stretchr/testify/mock" ) @@ -198,6 +199,12 @@ var _ = Describe("Grants", func() { err := env.Fs.AddGrant(env.Ctx, ref, grant) Expect(err).ToNot(HaveOccurred()) + n, err := env.Lookup.NodeFromResource(env.Ctx, ref) + Expect(err).ToNot(HaveOccurred()) + principal := prefixes.GrantMailAcePrefix + (utils.FSSafeUserID{ID: grant.Grantee.GetUserId()}).SafeFilename() + _, err = n.XattrString(env.Ctx, principal) + Expect(err).ToNot(HaveOccurred()) + grants, err := env.Fs.ListGrants(env.Ctx, ref) Expect(err).ToNot(HaveOccurred()) Expect(len(grants)).To(Equal(1)) diff --git a/pkg/storage/pkg/decomposedfs/node/node.go b/pkg/storage/pkg/decomposedfs/node/node.go index ba2c2d42c1a..33516a6a649 100644 --- a/pkg/storage/pkg/decomposedfs/node/node.go +++ b/pkg/storage/pkg/decomposedfs/node/node.go @@ -1238,7 +1238,7 @@ func (n *Node) IsDenied(ctx context.Context) bool { isExecutant := func(g *provider.Grantee) bool { switch g.GetType() { case provider.GranteeType_GRANTEE_TYPE_USER: - return g.GetUserId().GetOpaqueId() == u.GetId().GetOpaqueId() + return utils.UserIDEqual(g.GetUserId(), u.GetId()) case provider.GranteeType_GRANTEE_TYPE_GROUP: // check gid gid := g.GetGroupId().GetOpaqueId() @@ -1358,7 +1358,7 @@ func (n *Node) getGranteeTypes(ctx context.Context) []provider.GranteeType { hasUserShares, hasGroupShares := false, false for i := range g { switch { - case !hasUserShares && strings.HasPrefix(g[i], prefixes.GrantUserAcePrefix): + case !hasUserShares && (strings.HasPrefix(g[i], prefixes.GrantUserAcePrefix) || strings.HasPrefix(g[i], prefixes.GrantMailAcePrefix)): hasUserShares = true case !hasGroupShares && strings.HasPrefix(g[i], prefixes.GrantGroupAcePrefix): hasGroupShares = true diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index e6b0eb93c93..22394a902de 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -345,7 +345,8 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide var err error if requestedUserID != nil { - allMatches, err = fs.userSpaceIndex.Load(requestedUserID.GetOpaqueId()) + filename := (utils.FSSafeUserID{ID: requestedUserID}).SafeFilename() + allMatches, err = fs.userSpaceIndex.Load(filename) // do not return an error if the user has no spaces if err != nil && !os.IsNotExist(err) { return nil, errors.Wrap(err, "error reading user index") @@ -359,7 +360,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide // guests are granted shares via their mail address, search the mail index as well if requestedUserID.GetType() == userv1beta1.UserType_USER_TYPE_GUEST { - allMatches, err = fs.mailSpaceIndex.Load(utils.CanonicalUserID(requestedUserID)) + allMatches, err = fs.mailSpaceIndex.Load(filename) // do not return an error if the guest has no spaces if err != nil && !os.IsNotExist(err) { return nil, errors.Wrap(err, "error reading mail index") @@ -803,14 +804,15 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De for _, g := range grants { switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: + filename := (utils.FSSafeUserID{ID: g.Grantee.GetUserId()}).SafeFilename() if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index - if err := fs.mailSpaceIndex.Remove(utils.CanonicalUserID(g.Grantee.GetUserId()), spaceID); err != nil { + if err := fs.mailSpaceIndex.Remove(filename, spaceID); err != nil { sublog.Error().Err(err).Str("grantee", g.Grantee.GetUserId().GetOpaqueId()).Msg("could not remove guest from mail index") } } else { // remove from user index - if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), spaceID); err != nil { + if err := fs.userSpaceIndex.Remove(filename, spaceID); err != nil { sublog.Error().Err(err).Str("grantee", g.Grantee.GetUserId().GetOpaqueId()).Msg("could not remove user from user index") } } @@ -916,10 +918,11 @@ func (fs *Decomposedfs) updateIndexes(ctx context.Context, grantee *provider.Gra // create space grant index switch grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: + filename := (utils.FSSafeUserID{ID: grantee.GetUserId()}).SafeFilename() if grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { - return fs.linkSpaceByMail(ctx, utils.CanonicalUserID(grantee.GetUserId()), spaceID, target) + return fs.linkSpaceByMail(ctx, filename, spaceID, target) } - return fs.linkSpaceByUser(ctx, grantee.GetUserId().GetOpaqueId(), spaceID, target) + return fs.linkSpaceByUser(ctx, filename, spaceID, target) case provider.GranteeType_GRANTEE_TYPE_GROUP: return fs.linkSpaceByGroup(ctx, grantee.GetGroupId().GetOpaqueId(), spaceID, target) default: @@ -1012,15 +1015,16 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, // invalidate space grant switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: + filename := (utils.FSSafeUserID{ID: g.Grantee.GetUserId()}).SafeFilename() if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index - if err := fs.mailSpaceIndex.Remove(utils.CanonicalUserID(g.Grantee.GetUserId()), n.GetSpaceID()); err != nil { + if err := fs.mailSpaceIndex.Remove(filename, n.GetSpaceID()); err != nil { sublog.Error().Err(err).Str("grantee", id). Msg("failed to delete expired mail space index") } } else { // remove from user index - if err := fs.userSpaceIndex.Remove(g.Grantee.GetUserId().GetOpaqueId(), n.GetSpaceID()); err != nil { + if err := fs.userSpaceIndex.Remove(filename, n.GetSpaceID()); err != nil { sublog.Error().Err(err).Str("grantee", id). Msg("failed to delete expired user space index") } diff --git a/pkg/storage/pkg/decomposedfs/spaces_test.go b/pkg/storage/pkg/decomposedfs/spaces_test.go index 8b3d714b4ca..4c22e14f850 100644 --- a/pkg/storage/pkg/decomposedfs/spaces_test.go +++ b/pkg/storage/pkg/decomposedfs/spaces_test.go @@ -36,6 +36,7 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/spaceidindex" helpers "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/testhelpers" "github.com/opencloud-eu/reva/v2/pkg/storagespace" + "github.com/opencloud-eu/reva/v2/pkg/utils" "github.com/stretchr/testify/mock" "google.golang.org/grpc" ) @@ -304,7 +305,8 @@ var _ = Describe("Spaces", func() { Expect(groupIdx.Add(groupGrantee, spaceID, target)).To(Succeed()) mailIdx := spaceidindex.New(filepath.Join(env.Root, "indexes"), "by-mail") Expect(mailIdx.Init()).To(Succeed()) - Expect(mailIdx.Add("guest@example.com", spaceID, target)).To(Succeed()) + guestFilename := (utils.FSSafeUserID{ID: &userv1beta1.UserId{OpaqueId: guestGrantee, Type: userv1beta1.UserType_USER_TYPE_GUEST}}).SafeFilename() + Expect(mailIdx.Add(guestFilename, spaceID, target)).To(Succeed()) load := func(indexName, key string) map[string]string { idx := spaceidindex.New(filepath.Join(env.Root, "indexes"), indexName) @@ -317,7 +319,7 @@ var _ = Describe("Spaces", func() { // precondition: the grantee indexes reference the space Expect(load("by-user-id", userGrantee)).To(HaveKey(spaceID)) Expect(load("by-group-id", groupGrantee)).To(HaveKey(spaceID)) - Expect(load("by-mail", "guest@example.com")).To(HaveKey(spaceID)) + Expect(load("by-mail", guestFilename)).To(HaveKey(spaceID)) // disable, then purge as the space admin ctx := ctxpkg.ContextSetUser(context.Background(), env.DeleteAllSpacesUser) @@ -330,7 +332,7 @@ var _ = Describe("Spaces", func() { // the grantee index entries must be gone after the purge Expect(load("by-user-id", userGrantee)).ToNot(HaveKey(spaceID)) Expect(load("by-group-id", groupGrantee)).ToNot(HaveKey(spaceID)) - Expect(load("by-mail", "guest@example.com")).ToNot(HaveKey(spaceID)) + Expect(load("by-mail", guestFilename)).ToNot(HaveKey(spaceID)) }) // purging one space must remove only that space's entry: a sibling // space of the same owner must stay in the by-user-id index. @@ -698,5 +700,36 @@ var _ = Describe("Spaces", func() { Expect(spaces[0].SpaceType).To(Equal("project")) Expect(spaces[0].GetId().GetOpaqueId()).To(Equal(resp.StorageSpace.GetId().GetOpaqueId())) }) + + It("safely indexes a project space granted to a guest with a path-like ID", func() { + // create a project space as the owner + resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Guest Space", Type: "project"}) + Expect(err).ToNot(HaveOccurred()) + ref := &provider.Reference{ResourceId: resp.StorageSpace.GetRoot()} + + ctx := storageprovider.WithSpaceType(ctxpkg.ContextSetUser(context.Background(), env.Users[0]), "project") + guest := &userv1beta1.UserId{OpaqueId: "my/../email@email.com", Type: userv1beta1.UserType_USER_TYPE_GUEST} + err = env.Fs.AddGrant(ctx, ref, &provider.Grant{ + Grantee: &provider.Grantee{Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{UserId: guest}}, + Permissions: &provider.ResourcePermissions{Stat: true}, + Creator: &userv1beta1.UserId{OpaqueId: helpers.OwnerID}, + }) + Expect(err).NotTo(HaveOccurred()) + + mailIdx := spaceidindex.New(filepath.Join(env.Root, "indexes"), "by-mail") + Expect(mailIdx.Init()).To(Succeed()) + filename := (utils.FSSafeUserID{ID: guest}).SafeFilename() + index, err := mailIdx.Load(filename) + Expect(err).NotTo(HaveOccurred()) + _, spaceID, _, err := storagespace.SplitID(resp.StorageSpace.GetId().GetOpaqueId()) + Expect(err).NotTo(HaveOccurred()) + Expect(index).To(HaveKey(spaceID)) + + spaces, err := env.Fs.ListStorageSpaces(ctxpkg.ContextSetUser(context.Background(), &userv1beta1.User{Id: guest}), []*provider.ListStorageSpacesRequest_Filter{ + {Type: provider.ListStorageSpacesRequest_Filter_TYPE_USER, Term: &provider.ListStorageSpacesRequest_Filter_User{User: guest}}, + }, false) + Expect(err).NotTo(HaveOccurred()) + Expect(spaces).To(HaveLen(1)) + }) }) }) diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index 505517121a2..8b8edd999da 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -222,11 +222,12 @@ func FromGrant(g *provider.Grant) *ACE { } func UserAce(id *userpb.UserId) string { + filename := (utils.FSSafeUserID{ID: id}).SafeFilename() switch id.GetType() { case userpb.UserType_USER_TYPE_GUEST: - return MailAcePrefix + utils.CanonicalUserID(id) + return MailAcePrefix + filename default: - return UserAcePrefix + utils.CanonicalUserID(id) + return UserAcePrefix + filename } } @@ -275,6 +276,12 @@ func Unmarshal(principal string, v []byte) (e *ACE, err error) { if !strings.HasPrefix(principal, UserAcePrefix) && !strings.HasPrefix(principal, MailAcePrefix) { return nil, fmt.Errorf("inconsistent ace: expected user") } + if strings.HasPrefix(principal, MailAcePrefix) { + id := &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST} + if _, err := (utils.FSSafeUserID{ID: id}).Decode(strings.TrimPrefix(principal, MailAcePrefix)); err != nil { + return nil, fmt.Errorf("invalid guest ace principal: %w", err) + } + } } default: return nil, fmt.Errorf("unknown ace encoding") @@ -301,7 +308,8 @@ func (e *ACE) Grant() *provider.Grant { g.Grantee.Id = &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: id}} } else if e.granteeType() == provider.GranteeType_GRANTEE_TYPE_USER { if strings.HasPrefix(e.principal, MailAcePrefix) { - g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_GUEST}} + userID, _ := (utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}}).Decode(id) + g.Grantee.Id = &provider.Grantee_UserId{UserId: userID} } else { g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_PRIMARY}} } diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index c06b16e7794..70eb031a49c 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -25,6 +25,7 @@ import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/opencloud-eu/reva/v2/pkg/storage/utils/ace" + "github.com/opencloud-eu/reva/v2/pkg/utils" "google.golang.org/protobuf/testing/protocmp" . "github.com/onsi/ginkgo/v2" @@ -102,14 +103,15 @@ var _ = Describe("ACE", func() { }) It("creates an ACE from a guest grant", func() { + userid := &userpb.UserId{ + OpaqueId: "GUEST@example.com", + Type: userpb.UserType_USER_TYPE_GUEST, + } guestGrant.Grantee.Id = &provider.Grantee_UserId{ - UserId: &userpb.UserId{ - OpaqueId: "GUEST@example.com", - Type: userpb.UserType_USER_TYPE_GUEST, - }, + UserId: userid, } aceValue := ace.FromGrant(guestGrant) - Expect(aceValue.Principal()).To(Equal(ace.MailAcePrefix + "guest@example.com")) + Expect(aceValue.Principal()).To(Equal(ace.MailAcePrefix + utils.FSSafeUserID{ID: userid}.SafeFilename())) }) }) diff --git a/pkg/utils/filename_encoder.go b/pkg/utils/filename_encoder.go new file mode 100644 index 00000000000..54bedf566a0 --- /dev/null +++ b/pkg/utils/filename_encoder.go @@ -0,0 +1,54 @@ +package utils + +import ( + "encoding/base64" + "strings" + + grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + "google.golang.org/protobuf/proto" +) + +type FilenameEncoder interface { + SafeFilename() string +} + +type FSSafeUserID struct { + ID *userpb.UserId +} + +func (id FSSafeUserID) SafeFilename() string { + opaqueID := id.ID.GetOpaqueId() + if id.ID.GetType() == userpb.UserType_USER_TYPE_GUEST { + return base64.RawURLEncoding.EncodeToString([]byte(strings.ToLower(opaqueID))) + } + return opaqueID +} + +// Decode returns a copy of the wrapped user ID with the filename restored as its opaque ID. +// The decoding decision is done based on the Type attribute of the Receiver id +func (id FSSafeUserID) Decode(filename string) (*userpb.UserId, error) { + opaqueID := filename + if id.ID.GetType() == userpb.UserType_USER_TYPE_GUEST { + decoded, err := base64.RawURLEncoding.DecodeString(filename) + if err != nil { + return nil, err + } + opaqueID = string(decoded) + } + + decodedID := &userpb.UserId{} + if id.ID != nil { + decodedID = proto.Clone(id.ID).(*userpb.UserId) + } + decodedID.OpaqueId = opaqueID + return decodedID, nil +} + +type FSSafeGroupID struct { + ID *grouppb.GroupId +} + +func (id FSSafeGroupID) SafeFilename() string { + return id.ID.GetOpaqueId() +} diff --git a/pkg/utils/filename_encoder_suite_test.go b/pkg/utils/filename_encoder_suite_test.go new file mode 100644 index 00000000000..98f7fb6f1dc --- /dev/null +++ b/pkg/utils/filename_encoder_suite_test.go @@ -0,0 +1,13 @@ +package utils_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestFilenameEncoder(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Filename Encoder Suite") +} diff --git a/pkg/utils/filename_encoder_test.go b/pkg/utils/filename_encoder_test.go new file mode 100644 index 00000000000..8c36c71df4c --- /dev/null +++ b/pkg/utils/filename_encoder_test.go @@ -0,0 +1,76 @@ +package utils_test + +import ( + "encoding/base64" + "strings" + + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/opencloud-eu/reva/v2/pkg/utils" +) + +var _ = Describe("FSSafeUserID", func() { + DescribeTable("SafeFilename", + func(userType userpb.UserType, opaqueID string, encoded bool) { + id := &utils.FSSafeUserID{ID: &userpb.UserId{ + Type: userType, + OpaqueId: opaqueID, + }} + if encoded { + Expect(id.SafeFilename()).To(Equal(base64.RawURLEncoding.EncodeToString([]byte(strings.ToLower(opaqueID))))) + } else { + Expect(id.SafeFilename()).To(Equal(opaqueID)) + } + }, + Entry("preserves a primary user ID", userpb.UserType_USER_TYPE_PRIMARY, "MixedCase/ID", false), + Entry("preserves another non-guest user ID", userpb.UserType_USER_TYPE_FEDERATED, "Federated@Example.COM", false), + Entry("lowercases and encodes a guest user ID", userpb.UserType_USER_TYPE_GUEST, "Guest@Example.COM", true), + Entry("uses URL-safe raw base64 for a guest user ID", userpb.UserType_USER_TYPE_GUEST, "???", true), + Entry("encodes a file system path used as a guest user ID", userpb.UserType_USER_TYPE_GUEST, "../../etc/passwd", true), + Entry("encodes a guest user ID with a quoted local part", userpb.UserType_USER_TYPE_GUEST, `"Guest.User"@Example.COM`, true), + Entry("handles an empty primary user ID", userpb.UserType_USER_TYPE_PRIMARY, "", false), + Entry("handles an empty guest user ID", userpb.UserType_USER_TYPE_GUEST, "", false), + ) + + Describe("Decode", func() { + It("decodes a guest ID and preserves the wrapped ID", func() { + original := &userpb.UserId{ + Type: userpb.UserType_USER_TYPE_GUEST, + OpaqueId: "original@example.com", + Idp: "idp.example.com", + TenantId: "tenant", + } + + decoded, err := (utils.FSSafeUserID{ID: original}).Decode( + base64.RawURLEncoding.EncodeToString([]byte(strings.ToLower("../../etc/passwd"))), + ) + + Expect(err).NotTo(HaveOccurred()) + Expect(decoded.GetOpaqueId()).To(Equal("../../etc/passwd")) + Expect(decoded.GetType()).To(Equal(userpb.UserType_USER_TYPE_GUEST)) + Expect(decoded.GetIdp()).To(Equal("idp.example.com")) + Expect(decoded.GetTenantId()).To(Equal("tenant")) + Expect(original.GetOpaqueId()).To(Equal("original@example.com")) + }) + + It("does not decode a non-guest ID", func() { + id := utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_PRIMARY}} + + someb64 := base64.RawURLEncoding.EncodeToString([]byte("userid")) + decoded, err := id.Decode(someb64) + + Expect(err).NotTo(HaveOccurred()) + Expect(decoded.GetOpaqueId()).To(Equal(someb64)) + }) + + It("rejects malformed Base64 for a guest ID", func() { + id := utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}} + + decoded, err := id.Decode("not valid base64") + + Expect(err).To(HaveOccurred()) + Expect(decoded).To(BeNil()) + }) + }) +}) From b3bdf9cdaf0d7a2418af9b991d6b48ed9a4bc14b Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 7 Sep 2026 17:18:58 +0200 Subject: [PATCH 11/14] fix: only accept bare mail addresses --- .../usershareprovider/usershareprovider.go | 18 ++++++------- .../usershareprovider_test.go | 26 +++++++++++++++++-- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index d8521638231..466a500973b 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -182,15 +182,15 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar }, nil } - // guests are identified by their mail address, which is used as a path - // segment in the share manager storage; reject invalid mails before they - // end up in any index or cache. - _, err := mail.ParseAddress(req.GetGrant().GetGrantee().GetUserId().GetOpaqueId()) - if req.GetGrant().GetGrantee().GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST && err != nil { - - return &collaboration.CreateShareResponse{ - Status: status.NewInvalidArg(ctx, "invalid mail address for guest grantee"), - }, nil + if req.GetGrant().GetGrantee().GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { + // guests are identified by their mail address, lets be strict here and only accept bare mail addresses + // and reject the "Mailbox"-Format that also contains a Display name + addr, err := mail.ParseAddress(req.GetGrant().GetGrantee().GetUserId().GetOpaqueId()) + if err != nil || addr.Name != "" { + return &collaboration.CreateShareResponse{ + Status: status.NewInvalidArg(ctx, "invalid mail address for guest grantee"), + }, nil + } } } gatewayClient, err := s.gatewaySelector.Next() diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 0477a97eaab..e4974cd256a 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -515,6 +515,28 @@ var _ = Describe("user share provider service", func() { manager.AssertNumberOfCalls(GinkgoT(), "Share", 1) }) + It("succeeds when sharing with a guest using a mail address with a quoted localpart including path separators", func() { + createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{ + ResourceInfo: &providerpb.ResourceInfo{ + PermissionSet: conversions.RoleFromName("manager").CS3ResourcePermissions(), + }, + Grant: &collaborationpb.ShareGrant{ + Grantee: &providerpb.Grantee{ + Type: providerpb.GranteeType_GRANTEE_TYPE_USER, + Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "\"my/../email\"@email.com", TenantId: "tenant1", Type: userpb.UserType_USER_TYPE_GUEST}}, + }, + Permissions: &collaborationpb.SharePermissions{ + Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), + }, + }, + }) + + Expect(err).ToNot(HaveOccurred()) + Expect(createShareResponse.Status.Code).To(Equal(rpcpb.Code_CODE_OK)) + + manager.AssertNumberOfCalls(GinkgoT(), "Share", 1) + }) + It("fails when sharing with a guest using an invalid mail address", func() { createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{ ResourceInfo: &providerpb.ResourceInfo{ @@ -537,7 +559,7 @@ var _ = Describe("user share provider service", func() { manager.AssertNumberOfCalls(GinkgoT(), "Share", 0) }) - It("fails when sharing with a guest using a mail address with path separators", func() { + It("fails when sharing with a guest using a mail address including a Mailbox name", func() { createShareResponse, err := provider.CreateShare(ctx, &collaborationpb.CreateShareRequest{ ResourceInfo: &providerpb.ResourceInfo{ PermissionSet: conversions.RoleFromName("manager").CS3ResourcePermissions(), @@ -545,7 +567,7 @@ var _ = Describe("user share provider service", func() { Grant: &collaborationpb.ShareGrant{ Grantee: &providerpb.Grantee{ Type: providerpb.GranteeType_GRANTEE_TYPE_USER, - Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "my/../email@email.com", TenantId: "tenant1", Type: userpb.UserType_USER_TYPE_GUEST}}, + Id: &providerpb.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "Test Useer ", TenantId: "tenant1", Type: userpb.UserType_USER_TYPE_GUEST}}, }, Permissions: &collaborationpb.SharePermissions{ Permissions: conversions.RoleFromName("viewer").CS3ResourcePermissions(), From 0026eefafe5dd9e2dcf4999987ab0efd9ee9d681 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 9 Sep 2026 12:21:09 +0200 Subject: [PATCH 12/14] docs: add clarifying comments about why we're lowercasing guest user ids Co-authored-by: Pascal Bleser --- pkg/utils/filename_encoder.go | 4 ++++ pkg/utils/utils.go | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/pkg/utils/filename_encoder.go b/pkg/utils/filename_encoder.go index 54bedf566a0..65f5bfb2669 100644 --- a/pkg/utils/filename_encoder.go +++ b/pkg/utils/filename_encoder.go @@ -20,6 +20,10 @@ type FSSafeUserID struct { func (id FSSafeUserID) SafeFilename() string { opaqueID := id.ID.GetOpaqueId() if id.ID.GetType() == userpb.UserType_USER_TYPE_GUEST { + // Guest opaqueID is an email address, which is why we choose to make + // it lowercase: RFC 5321 does specify that email address local-parts + // are case sensitive but, in practice, it's a de-facto standard that + // email providers consider them to be case insensitive: return base64.RawURLEncoding.EncodeToString([]byte(strings.ToLower(opaqueID))) } return opaqueID diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 52ff5973c5c..78a3c960b17 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -236,6 +236,10 @@ func CanonicalUserID(id *userpb.UserId) string { return "" } if id.GetType() == userpb.UserType_USER_TYPE_GUEST { + // Guest user IDs are lowercased to enable case insensitive + // comparisons: those are email addresses, and while RFC 5321 states + // that the local-part is case sensitive, in practice, it's a de facto + // standard that email providers consider them to be case insensitive. return strings.ToLower(id.GetOpaqueId()) } return id.GetOpaqueId() From 2e693bbfa4987f1a9e4f19231e289fc5c0378967 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 9 Sep 2026 10:59:05 +0000 Subject: [PATCH 13/14] docs: explain ignored guest principal decode error --- pkg/storage/utils/ace/ace.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index 8b8edd999da..fb89f206b62 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -308,6 +308,8 @@ func (e *ACE) Grant() *provider.Grant { g.Grantee.Id = &provider.Grantee_GroupId{GroupId: &grouppb.GroupId{OpaqueId: id}} } else if e.granteeType() == provider.GranteeType_GRANTEE_TYPE_USER { if strings.HasPrefix(e.principal, MailAcePrefix) { + // Guest principals are validated by Unmarshal or encoded by FromGrant, + // so decoding cannot fail for a valid ACE. userID, _ := (utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}}).Decode(id) g.Grantee.Id = &provider.Grantee_UserId{UserId: userID} } else { From 05b61f2518f5e90b6584fc47113a90b8ca387d76 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 9 Sep 2026 11:06:36 +0000 Subject: [PATCH 14/14] refactor: use constructor for FSSafeUserID Also makes the "id" struct field private. --- pkg/share/manager/jsoncs3/jsoncs3.go | 22 +++++++++---------- .../receivedsharecache_test.go | 16 +++++--------- .../jsoncs3/sharecache/sharecache_test.go | 8 +++---- pkg/storage/pkg/decomposedfs/grants.go | 2 +- pkg/storage/pkg/decomposedfs/grants_test.go | 2 +- pkg/storage/pkg/decomposedfs/spaces.go | 8 +++---- pkg/storage/pkg/decomposedfs/spaces_test.go | 4 ++-- pkg/storage/utils/ace/ace.go | 6 ++--- pkg/storage/utils/ace/ace_test.go | 2 +- pkg/utils/filename_encoder.go | 16 +++++++++----- pkg/utils/filename_encoder_test.go | 10 ++++----- 11 files changed, 47 insertions(+), 49 deletions(-) diff --git a/pkg/share/manager/jsoncs3/jsoncs3.go b/pkg/share/manager/jsoncs3/jsoncs3.go index 3b6df2bf797..492ec38009e 100644 --- a/pkg/share/manager/jsoncs3/jsoncs3.go +++ b/pkg/share/manager/jsoncs3/jsoncs3.go @@ -483,7 +483,7 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla }) eg.Go(func() error { - err := m.CreatedCache.Add(ctx, utils.FSSafeUserID{ID: s.GetCreator()}, shareID) + err := m.CreatedCache.Add(ctx, utils.NewFSSafeUserID(s.GetCreator()), shareID) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -502,7 +502,7 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla Share: s, State: collaboration.ShareState_SHARE_STATE_PENDING, } - err := m.UserReceivedStates.Add(ctx, utils.FSSafeUserID{ID: g.GetGrantee().GetUserId()}, spaceID, rs) + err := m.UserReceivedStates.Add(ctx, utils.NewFSSafeUserID(g.GetGrantee().GetUserId()), spaceID, rs) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -878,7 +878,7 @@ func (m *Manager) listCreatedShares(ctx context.Context, user *userv1beta1.User, defer span.End() sublog := appctx.GetLogger(ctx).With().Str("userid", user.GetId().GetOpaqueId()).Str("useridp", user.GetId().GetIdp()).Str("driver", "jsoncs3").Str("handler", "listCreatedShares").Logger() - list, err := m.CreatedCache.List(ctx, utils.FSSafeUserID{ID: user.GetId()}) + list, err := m.CreatedCache.List(ctx, utils.NewFSSafeUserID(user.GetId())) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) @@ -1051,7 +1051,7 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati } // add all spaces the user has receved shares for, this includes mount points and share state for groups - spaces, err := m.UserReceivedStates.List(ctx, utils.FSSafeUserID{ID: user.GetId()}) + spaces, err := m.UserReceivedStates.List(ctx, utils.NewFSSafeUserID(user.GetId())) if err != nil { return nil, err } @@ -1114,7 +1114,7 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati } if s == nil { sublogr.Warn().Str("shareid", shareID).Msg("share not found. cleaning up") - _ = m.UserReceivedStates.Remove(ctx, utils.FSSafeUserID{ID: user.GetId()}, w.ssid, shareID) + _ = m.UserReceivedStates.Remove(ctx, utils.NewFSSafeUserID(user.GetId()), w.ssid, shareID) continue } sublogr = sublogr.With().Str("shareid", shareID).Logger() @@ -1203,7 +1203,7 @@ func (m *Manager) convert(ctx context.Context, userID *userv1beta1.UserId, s *co storageID, spaceID, _ := shareid.Decode(s.Id.OpaqueId) - state, err := m.UserReceivedStates.Get(ctx, utils.FSSafeUserID{ID: userID}, storageID+shareid.IDDelimiter+spaceID, s.Id.GetOpaqueId()) + state, err := m.UserReceivedStates.Get(ctx, utils.NewFSSafeUserID(userID), storageID+shareid.IDDelimiter+spaceID, s.Id.GetOpaqueId()) if err == nil && state != nil { rs.State = state.State rs.MountPoint = state.MountPoint @@ -1303,7 +1303,7 @@ func (m *Manager) UpdateReceivedShare(ctx context.Context, receivedShare *collab uid = forUser } - err = m.UserReceivedStates.Add(ctx, utils.FSSafeUserID{ID: uid}, rs.Share.ResourceId.StorageId+shareid.IDDelimiter+rs.Share.ResourceId.SpaceId, rs) + err = m.UserReceivedStates.Add(ctx, utils.NewFSSafeUserID(uid), rs.Share.ResourceId.StorageId+shareid.IDDelimiter+rs.Share.ResourceId.SpaceId, rs) if err != nil { return nil, err } @@ -1341,7 +1341,7 @@ func (m *Manager) Load(ctx context.Context, shareChan <-chan *collaboration.Shar } else { l.Debug().Str("storageid", s.GetResourceId().GetStorageId()).Str("spaceid", s.GetResourceId().GetSpaceId()).Str("shareid", s.Id.OpaqueId).Msg("imported share") } - if err := m.CreatedCache.Add(ctx, utils.FSSafeUserID{ID: s.GetCreator()}, s.Id.OpaqueId); err != nil { + if err := m.CreatedCache.Add(ctx, utils.NewFSSafeUserID(s.GetCreator()), s.Id.OpaqueId); err != nil { l.Error().Err(err).Interface("share", s).Msg("error persisting created cache") } else { l.Debug().Str("creatorid", s.GetCreator().GetOpaqueId()).Str("shareid", s.Id.OpaqueId).Msg("updated created cache") @@ -1357,7 +1357,7 @@ func (m *Manager) Load(ctx context.Context, shareChan <-chan *collaboration.Shar } if s.UserID != nil { spaceid := s.ReceivedShare.GetShare().GetResourceId().GetStorageId() + shareid.IDDelimiter + s.ReceivedShare.GetShare().GetResourceId().GetSpaceId() - if err := m.UserReceivedStates.Add(context.Background(), utils.FSSafeUserID{ID: s.UserID}, spaceid, s.ReceivedShare); err != nil { + if err := m.UserReceivedStates.Add(context.Background(), utils.NewFSSafeUserID(s.UserID), spaceid, s.ReceivedShare); err != nil { l.Error().Err(err).Interface("received share", s).Msg("error persisting received share for user") } else { l.Debug().Str("userid", s.UserID.GetOpaqueId()).Str("spaceid", spaceid).Str("shareid", s.ReceivedShare.GetShare().Id.OpaqueId).Msg("updated received share userdata") @@ -1420,13 +1420,13 @@ func (m *Manager) removeShare(ctx context.Context, s *collaboration.Share, skipS eg.Go(func() error { // remove from created cache - return m.CreatedCache.Remove(ctx, utils.FSSafeUserID{ID: s.GetCreator()}, s.Id.OpaqueId) + return m.CreatedCache.Remove(ctx, utils.NewFSSafeUserID(s.GetCreator()), s.Id.OpaqueId) }) eg.Go(func() error { // remove from user received states if s.GetGrantee().Type == provider.GranteeType_GRANTEE_TYPE_USER { - return m.UserReceivedStates.Remove(ctx, utils.FSSafeUserID{ID: s.GetGrantee().GetUserId()}, s.GetResourceId().GetStorageId()+shareid.IDDelimiter+s.GetResourceId().GetSpaceId(), s.Id.OpaqueId) + return m.UserReceivedStates.Remove(ctx, utils.NewFSSafeUserID(s.GetGrantee().GetUserId()), s.GetResourceId().GetStorageId()+shareid.IDDelimiter+s.GetResourceId().GetSpaceId(), s.Id.OpaqueId) } else if s.GetGrantee().Type == provider.GranteeType_GRANTEE_TYPE_GROUP { return m.GroupReceivedCache.Remove(ctx, utils.FSSafeGroupID{ID: s.GetGrantee().GetGroupId()}, s.Id.OpaqueId) } diff --git a/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go b/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go index eb78756d1f6..fdc45638b99 100644 --- a/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go +++ b/pkg/share/manager/jsoncs3/receivedsharecache/receivedsharecache_test.go @@ -38,11 +38,9 @@ var _ = Describe("Cache", func() { c receivedsharecache.Cache storage metadata.Storage - userID = utils.FSSafeUserID{ - ID: &userpb.UserId{ - OpaqueId: "userid", - }, - } + userID = utils.NewFSSafeUserID(&userpb.UserId{ + OpaqueId: "userid", + }) spaceID = "spaceid" shareID = "storageid$spaceid!share1" share = &collaboration.Share{ @@ -117,11 +115,9 @@ var _ = Describe("Cache", func() { Describe("Get", func() { It("handles unknown users", func() { - someUserID := utils.FSSafeUserID{ - ID: &userpb.UserId{ - OpaqueId: "something", - }, - } + someUserID := utils.NewFSSafeUserID(&userpb.UserId{ + OpaqueId: "something", + }) s, err := c.Get(ctx, someUserID, spaceID, shareID) Expect(err).ToNot(HaveOccurred()) Expect(s).To(BeNil()) diff --git a/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go b/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go index 7a00bcecc62..6fa457a2bd9 100644 --- a/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go +++ b/pkg/share/manager/jsoncs3/sharecache/sharecache_test.go @@ -37,11 +37,9 @@ var _ = Describe("Sharecache", func() { c sharecache.Cache storage metadata.Storage - userid = utils.FSSafeUserID{ - ID: &userpb.UserId{ - OpaqueId: "userid", - }, - } + userid = utils.NewFSSafeUserID(&userpb.UserId{ + OpaqueId: "userid", + }) shareID = "storageid$spaceid!share1" ctx context.Context tmpdir string diff --git a/pkg/storage/pkg/decomposedfs/grants.go b/pkg/storage/pkg/decomposedfs/grants.go index b21a496358e..4a1eb82ed86 100644 --- a/pkg/storage/pkg/decomposedfs/grants.go +++ b/pkg/storage/pkg/decomposedfs/grants.go @@ -235,7 +235,7 @@ func (fs *Decomposedfs) RemoveGrant(ctx context.Context, ref *provider.Reference // invalidate space grant switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - filename := (utils.FSSafeUserID{ID: g.Grantee.GetUserId()}).SafeFilename() + filename := utils.NewFSSafeUserID(g.Grantee.GetUserId()).SafeFilename() if g.Grantee.GetUserId().GetType() == userpb.UserType_USER_TYPE_GUEST { if err := fs.mailSpaceIndex.Remove(filename, grantNode.SpaceID); err != nil { return err diff --git a/pkg/storage/pkg/decomposedfs/grants_test.go b/pkg/storage/pkg/decomposedfs/grants_test.go index 838bd8207d1..6cf2fa2926f 100644 --- a/pkg/storage/pkg/decomposedfs/grants_test.go +++ b/pkg/storage/pkg/decomposedfs/grants_test.go @@ -201,7 +201,7 @@ var _ = Describe("Grants", func() { n, err := env.Lookup.NodeFromResource(env.Ctx, ref) Expect(err).ToNot(HaveOccurred()) - principal := prefixes.GrantMailAcePrefix + (utils.FSSafeUserID{ID: grant.Grantee.GetUserId()}).SafeFilename() + principal := prefixes.GrantMailAcePrefix + utils.NewFSSafeUserID(grant.Grantee.GetUserId()).SafeFilename() _, err = n.XattrString(env.Ctx, principal) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/storage/pkg/decomposedfs/spaces.go b/pkg/storage/pkg/decomposedfs/spaces.go index 22394a902de..2421c3c0742 100644 --- a/pkg/storage/pkg/decomposedfs/spaces.go +++ b/pkg/storage/pkg/decomposedfs/spaces.go @@ -345,7 +345,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide var err error if requestedUserID != nil { - filename := (utils.FSSafeUserID{ID: requestedUserID}).SafeFilename() + filename := utils.NewFSSafeUserID(requestedUserID).SafeFilename() allMatches, err = fs.userSpaceIndex.Load(filename) // do not return an error if the user has no spaces if err != nil && !os.IsNotExist(err) { @@ -804,7 +804,7 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De for _, g := range grants { switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - filename := (utils.FSSafeUserID{ID: g.Grantee.GetUserId()}).SafeFilename() + filename := utils.NewFSSafeUserID(g.Grantee.GetUserId()).SafeFilename() if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index if err := fs.mailSpaceIndex.Remove(filename, spaceID); err != nil { @@ -918,7 +918,7 @@ func (fs *Decomposedfs) updateIndexes(ctx context.Context, grantee *provider.Gra // create space grant index switch grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - filename := (utils.FSSafeUserID{ID: grantee.GetUserId()}).SafeFilename() + filename := utils.NewFSSafeUserID(grantee.GetUserId()).SafeFilename() if grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { return fs.linkSpaceByMail(ctx, filename, spaceID, target) } @@ -1015,7 +1015,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, // invalidate space grant switch g.Grantee.Type { case provider.GranteeType_GRANTEE_TYPE_USER: - filename := (utils.FSSafeUserID{ID: g.Grantee.GetUserId()}).SafeFilename() + filename := utils.NewFSSafeUserID(g.Grantee.GetUserId()).SafeFilename() if g.Grantee.GetUserId().GetType() == userv1beta1.UserType_USER_TYPE_GUEST { // remove from mail index if err := fs.mailSpaceIndex.Remove(filename, n.GetSpaceID()); err != nil { diff --git a/pkg/storage/pkg/decomposedfs/spaces_test.go b/pkg/storage/pkg/decomposedfs/spaces_test.go index 4c22e14f850..0e4971f841d 100644 --- a/pkg/storage/pkg/decomposedfs/spaces_test.go +++ b/pkg/storage/pkg/decomposedfs/spaces_test.go @@ -305,7 +305,7 @@ var _ = Describe("Spaces", func() { Expect(groupIdx.Add(groupGrantee, spaceID, target)).To(Succeed()) mailIdx := spaceidindex.New(filepath.Join(env.Root, "indexes"), "by-mail") Expect(mailIdx.Init()).To(Succeed()) - guestFilename := (utils.FSSafeUserID{ID: &userv1beta1.UserId{OpaqueId: guestGrantee, Type: userv1beta1.UserType_USER_TYPE_GUEST}}).SafeFilename() + guestFilename := utils.NewFSSafeUserID(&userv1beta1.UserId{OpaqueId: guestGrantee, Type: userv1beta1.UserType_USER_TYPE_GUEST}).SafeFilename() Expect(mailIdx.Add(guestFilename, spaceID, target)).To(Succeed()) load := func(indexName, key string) map[string]string { @@ -718,7 +718,7 @@ var _ = Describe("Spaces", func() { mailIdx := spaceidindex.New(filepath.Join(env.Root, "indexes"), "by-mail") Expect(mailIdx.Init()).To(Succeed()) - filename := (utils.FSSafeUserID{ID: guest}).SafeFilename() + filename := utils.NewFSSafeUserID(guest).SafeFilename() index, err := mailIdx.Load(filename) Expect(err).NotTo(HaveOccurred()) _, spaceID, _, err := storagespace.SplitID(resp.StorageSpace.GetId().GetOpaqueId()) diff --git a/pkg/storage/utils/ace/ace.go b/pkg/storage/utils/ace/ace.go index fb89f206b62..96f4fa180e2 100644 --- a/pkg/storage/utils/ace/ace.go +++ b/pkg/storage/utils/ace/ace.go @@ -222,7 +222,7 @@ func FromGrant(g *provider.Grant) *ACE { } func UserAce(id *userpb.UserId) string { - filename := (utils.FSSafeUserID{ID: id}).SafeFilename() + filename := utils.NewFSSafeUserID(id).SafeFilename() switch id.GetType() { case userpb.UserType_USER_TYPE_GUEST: return MailAcePrefix + filename @@ -278,7 +278,7 @@ func Unmarshal(principal string, v []byte) (e *ACE, err error) { } if strings.HasPrefix(principal, MailAcePrefix) { id := &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST} - if _, err := (utils.FSSafeUserID{ID: id}).Decode(strings.TrimPrefix(principal, MailAcePrefix)); err != nil { + if _, err := utils.NewFSSafeUserID(id).Decode(strings.TrimPrefix(principal, MailAcePrefix)); err != nil { return nil, fmt.Errorf("invalid guest ace principal: %w", err) } } @@ -310,7 +310,7 @@ func (e *ACE) Grant() *provider.Grant { if strings.HasPrefix(e.principal, MailAcePrefix) { // Guest principals are validated by Unmarshal or encoded by FromGrant, // so decoding cannot fail for a valid ACE. - userID, _ := (utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}}).Decode(id) + userID, _ := utils.NewFSSafeUserID(&userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}).Decode(id) g.Grantee.Id = &provider.Grantee_UserId{UserId: userID} } else { g.Grantee.Id = &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: id, Type: userpb.UserType_USER_TYPE_PRIMARY}} diff --git a/pkg/storage/utils/ace/ace_test.go b/pkg/storage/utils/ace/ace_test.go index 70eb031a49c..89f5cac5c80 100644 --- a/pkg/storage/utils/ace/ace_test.go +++ b/pkg/storage/utils/ace/ace_test.go @@ -111,7 +111,7 @@ var _ = Describe("ACE", func() { UserId: userid, } aceValue := ace.FromGrant(guestGrant) - Expect(aceValue.Principal()).To(Equal(ace.MailAcePrefix + utils.FSSafeUserID{ID: userid}.SafeFilename())) + Expect(aceValue.Principal()).To(Equal(ace.MailAcePrefix + utils.NewFSSafeUserID(userid).SafeFilename())) }) }) diff --git a/pkg/utils/filename_encoder.go b/pkg/utils/filename_encoder.go index 65f5bfb2669..13fdf392579 100644 --- a/pkg/utils/filename_encoder.go +++ b/pkg/utils/filename_encoder.go @@ -14,12 +14,16 @@ type FilenameEncoder interface { } type FSSafeUserID struct { - ID *userpb.UserId + id *userpb.UserId +} + +func NewFSSafeUserID(id *userpb.UserId) FSSafeUserID { + return FSSafeUserID{id: id} } func (id FSSafeUserID) SafeFilename() string { - opaqueID := id.ID.GetOpaqueId() - if id.ID.GetType() == userpb.UserType_USER_TYPE_GUEST { + opaqueID := id.id.GetOpaqueId() + if id.id.GetType() == userpb.UserType_USER_TYPE_GUEST { // Guest opaqueID is an email address, which is why we choose to make // it lowercase: RFC 5321 does specify that email address local-parts // are case sensitive but, in practice, it's a de-facto standard that @@ -33,7 +37,7 @@ func (id FSSafeUserID) SafeFilename() string { // The decoding decision is done based on the Type attribute of the Receiver id func (id FSSafeUserID) Decode(filename string) (*userpb.UserId, error) { opaqueID := filename - if id.ID.GetType() == userpb.UserType_USER_TYPE_GUEST { + if id.id.GetType() == userpb.UserType_USER_TYPE_GUEST { decoded, err := base64.RawURLEncoding.DecodeString(filename) if err != nil { return nil, err @@ -42,8 +46,8 @@ func (id FSSafeUserID) Decode(filename string) (*userpb.UserId, error) { } decodedID := &userpb.UserId{} - if id.ID != nil { - decodedID = proto.Clone(id.ID).(*userpb.UserId) + if id.id != nil { + decodedID = proto.Clone(id.id).(*userpb.UserId) } decodedID.OpaqueId = opaqueID return decodedID, nil diff --git a/pkg/utils/filename_encoder_test.go b/pkg/utils/filename_encoder_test.go index 8c36c71df4c..91ccbedbfd6 100644 --- a/pkg/utils/filename_encoder_test.go +++ b/pkg/utils/filename_encoder_test.go @@ -13,10 +13,10 @@ import ( var _ = Describe("FSSafeUserID", func() { DescribeTable("SafeFilename", func(userType userpb.UserType, opaqueID string, encoded bool) { - id := &utils.FSSafeUserID{ID: &userpb.UserId{ + id := utils.NewFSSafeUserID(&userpb.UserId{ Type: userType, OpaqueId: opaqueID, - }} + }) if encoded { Expect(id.SafeFilename()).To(Equal(base64.RawURLEncoding.EncodeToString([]byte(strings.ToLower(opaqueID))))) } else { @@ -42,7 +42,7 @@ var _ = Describe("FSSafeUserID", func() { TenantId: "tenant", } - decoded, err := (utils.FSSafeUserID{ID: original}).Decode( + decoded, err := utils.NewFSSafeUserID(original).Decode( base64.RawURLEncoding.EncodeToString([]byte(strings.ToLower("../../etc/passwd"))), ) @@ -55,7 +55,7 @@ var _ = Describe("FSSafeUserID", func() { }) It("does not decode a non-guest ID", func() { - id := utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_PRIMARY}} + id := utils.NewFSSafeUserID(&userpb.UserId{Type: userpb.UserType_USER_TYPE_PRIMARY}) someb64 := base64.RawURLEncoding.EncodeToString([]byte("userid")) decoded, err := id.Decode(someb64) @@ -65,7 +65,7 @@ var _ = Describe("FSSafeUserID", func() { }) It("rejects malformed Base64 for a guest ID", func() { - id := utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}} + id := utils.NewFSSafeUserID(&userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}) decoded, err := id.Decode("not valid base64")