Add support for mail grantees by rhafer · Pull Request #785 · opencloud-eu/reva

rhafer · 2026-08-25T10:05:55Z

revived and rebased #616 on top of current master

This PR implements sharing with email addresses as per opencloud-eu/opencloud#2516

rhafer

Here's some findings that a quick review with the help of opencode revealed. I haven't looked into them a depth. But I think they're mostly valid:

High: pkg/storage/pkg/decomposedfs/spaces.go:897 indexes guest spaces by mail, but ListStorageSpaces only reads userSpaceIndex at line 348. Guests cannot discover granted project spaces.

Medium: Guest emails are lowercased during persistence in pkg/storage/utils/ace/ace.go:220, but lookup and deletion use original casing at grants.go:330 and node/node.go:1315. Mixed-case guest grants cannot be updated or removed.

Medium: Space purge removes user grants only from userSpaceIndex at spaces.go:792, leaving stale guest entries in mailSpaceIndex.

rhafer · 2026-08-31T12:41:13Z

rebased once more. And made the lowercase normalization for a bit more explicit just for mail grants (Usertype Guest).

To me this looks good now. Unfortunately I can't approve since I created the PR, even though most of the code is originally from @aduffeck .

rhafer · 2026-09-02T14:07:36Z

The more I look into this the more I think that this is not ready for prime time yet. I guess I wanted to rush this through a little too fast, after it has been sitting idle for so long.

After some discussion and more code review, my main concerns are around using the mail address as the value of the OpaqueId in the cs3 UserId and the consequences that this has for the different places, where we persist the id in the filesystem:

the share manager is using the id as the name of the directory in which it stores the share-caches for that user (the created.json and received.json files)
the decomposedfs storageprovider uses the mail address for the filename of the by-mail index file for that mail address
for storing the grants the storageprovider also uses the main address as part of the name of an extended attribute oc.user.grant.m.<mailaddress>

Mail addresses, especially the local-part, can contain all kinds of weird characters. E.g. Bad Idea <"../../../a/bad-idea"@example.org> is actually a valid Address (mail.ParseAddress) actually accepts it.

Each of the above places has it's own restriction about what a valid value for a directory-, file- or extended attribute name can be. So we need to ensure that we won't break stuff.

There are some options:

wrap the addresses in some safe encoding ( e.g. base64)
- Where would that happen
  - Everywhere the address is written disk (lots of places)?
  - at some higher level, e.g. require the caller to already provide a base64 encoded address in the OpaqueId, somewhat defeats the Id for being "opaque"
refrain from the idea of using the mail as an id, maybe create a user provider for mail invitees that maps mail-addresses to uuids? (Sound like some bigger effort)

To not completely stall efforts on the mail-invites, and as I think the above should have no impact on the user-facing (graph) API for now, I'd suggest to do the following with this PR:

instead of main, target a feature branch for now, as we already did with the original PR
adjust this PR to make sure that the supplied id (mail address) is actually safe to be used as a directory and/or filename (e.g. by checking that filepath.Base(mail) equals mail and is not . , .. or /) (AFAICS the names that get through this validation would also be safe for the user.oc.grant.m:<id> xattr name.
before merging the feature branch into main figure out a workable solution

@pbleser-oc @micbar @maki5 Opinions?

maki5 · 2026-09-02T14:17:26Z

The more I look into this the more I think that this is not ready for prime time yet. I guess I wanted to rush this through a little too fast, after it has been sitting idle for so long.

After some discussion and more code review, my main concerns are around using the mail address as the value of the OpaqueId in the cs3 UserId and the consequences that this has for the different places, where we persist the id in the filesystem:

the share manager is using the id as the name of the directory in which it stores the share-caches for that user (the created.json and received.json files)

the decomposedfs storageprovider uses the mail address for the filename of the by-mail index file for that mail address

for storing the grants the storageprovider also uses the main address as part of the name of an extended attribute oc.user.grant.m.<mailaddress>

Mail addresses, especially the local-part, can contain all kinds of weird characters. E.g. Bad Idea <"../../../a/bad-idea"@example.org> is actually a valid Address (mail.ParseAddress) actually accepts it.

Each of the above places has it's own restriction about what a valid value for a directory-, file- or extended attribute name can be. So we need to ensure that we won't break stuff.

There are some options:

wrap the addresses in some safe encoding ( e.g. base64)

Where would that happen

Everywhere the address is written disk (lots of places)?

at some higher level, e.g. require the caller to already provide a base64 encoded address in the OpaqueId, somewhat defeats the Id for being "opaque"

refrain from the idea of using the mail as an id, maybe create a user provider for mail invitees that maps mail-addresses to uuids? (Sound like some bigger effort)

To not completely stall efforts on the mail-invites, and as I think the above should have no impact on the user-facing (graph) API for now, I'd suggest to do the following with this PR:

instead of main, target a feature branch for now, as we already did with the original PR

adjust this PR to make sure that the supplied id (mail address) is actually safe to be used as a directory and/or filename (e.g. by checking that filepath.Base(mail) equals mail and is not . , .. or /) (AFAICS the names that get through this validation would also be safe for the user.oc.grant.m:<id> xattr name.

before merging the feature branch into main figure out a workable solution

@pbleser-oc @micbar @maki5 Opinions?

How about introducing the concept of anonymous user, which have restricted access but still has the same structure as normal user, means has an real uuid and email. This might be a bigger change but in this case I assume we will not need to change the whole storage and handling logic. Maybe a new role for this kind of users with access to just shared with it resource will potentially solve the issue.

pbleser-oc · 2026-09-02T14:25:51Z

There are some options:
* wrap the addresses in some safe encoding ( e.g. base64)
* Where would that happen
* Everywhere the address is written disk (lots of places)?

Yes, absolutely, because of how/where it is written on disk.

In theory we could even use different strategies for different use-cases:

written on disk as part of a filename: requires protection/escaping/encoding, e.g. base64
written as a value within a file, e.g. in a JSON object: doesn't need any escaping, the JSON encoder will take care of the string representation and escaping " and such, but no need for base64
in some cases we might even consider them worthy of secrecy/privacy and base64 + encrypt them

Those are just examples to highlight that the decision to encode, and how to encode, is really up to the functions/layers that actually store those email addresses.

    * at some higher level, e.g. require the caller to already provide a base64 encoded address in the OpaqueId, somewhat defeats the Id for being "opaque"

If it's opaque, then it's up to the storage level to encode it, as it's only the concern of the storage that

it is stored at all
how it is stored, requiring escaping or encoding (if the value wasn't used as a filename but only as a value within a file, e.g. as a string value in a JSON object, then there wouldn't be any need to encode it)

I wouldn't put that on the caller, it's not their concern/problem/business.

One point though: are those opaque values always email addresses? or are the some sort of IDs, and in the case of guest shares we use an email address?

Point being:

are we going to always encode those IDs with base64 ?
or only if it's an email address for a guest share?
if it's the latter: does only the caller know that it's an email address? or does this run through specific lower-level functions that know that those are email addresses?
if we base64 those IDs for everything and not just for guest share IDs, then are we going to have to implement a significant migration for the existing files... ?

Those might be practical reasons for putting the burden of the encoding on the callers, since only they would know that it's an email address in the first place.

(Making a lot of assumptions here, I'm not familiar with that codebase.)

* refrain from the idea of using the mail as an id, maybe create a user provider for mail invitees that maps mail-addresses to uuids? (Sound like some bigger effort)

That sounds like it should be the least good option, because it will require having an additional form of storage that will persist those mappings, and we don't know when they will be eligible for removal, etc...
Much much simpler if we can use the email address without requiring that.

If we want to do that without a storage, we need a derivation function that works in both directions, like... an encoding :) and we're back to base64 :)

In any case it should be implemented as a single strategy, in a single set of functions to encode/decode that are then used everywhere, and then we can still change it in the future (albeit requiring migrations).

To not completely stall efforts on the mail-invites, and as I think the above should have no impact on the user-facing (graph) API for now, I'd suggest to do the following with this PR:

* instead of main, target a feature branch for now, as we already did with the original PR

* adjust this PR to make sure that the supplied id (mail address) is actually safe to be used as a directory and/or filename (e.g. by checking that `filepath.Base(mail)` equals `mail` and is not `.` , `..` or  `/`) (AFAICS the names that get through this validation would also be safe for the `user.oc.grant.m:<id>` xattr name.

* before merging the feature branch into main figure out a workable solution

We should definitely pick the appropriate encoding solution before putting it into main as we don't want to have to implement a migration as well.

rhafer · 2026-09-02T14:45:35Z

One point though: are those opaque values always email addresses? or are the some sort of IDs, and in the case of guest shares we use an email address?

The latter, in case of guest shares they're mail addresses, for "normal" user shares they're usually UUIDs in our type of setups.

Point being:

* are we going to always encode those IDs with base64 ?

* or only if it's an email address for a guest share?

* if it's the latter: does only the caller know that it's an email address? or does this run through specific lower-level functions that know that those are email addresses?

There are some parts e.g. in the lower levels of the share manager where we don't know anymore that is is an mail address. It might be possible to change that without much effort, haven't checked in detail yet.

* if we base64 those IDs for everything and not just for guest share IDs, then are we going to have to implement a significant migration for the existing files... ?

Yes, that is something I'd like to avoid.

Those might be practical reasons for putting the burden of the encoding on the callers, since only they would know that it's an email address in the first place.

(Making a lot of assumptions here, I'm not familiar with that codebase.)

Still very helpful for me though 🙏.

* refrain from the idea of using the mail as an id, maybe create a user provider for mail invitees that maps mail-addresses to uuids? (Sound like some bigger effort)
That sounds like it should be the least good option, because it will require having an additional form of storage that will persist those mappings, and we don't know when they will be eligible for removal, etc... Much much simpler if we can use the email address without requiring that.

Agreed. I was just trying to get some ideas of my head.

If we want to do that without a storage, we need a derivation function that works in both directions, like... an encoding :) and we're back to base64 :)

In any case it should be implemented as a single strategy, in a single set of functions to encode/decode that are then used everywhere, and then we can still change it in the future (albeit requiring migrations).
To not completely stall efforts on the mail-invites, and as I think the above should have no impact on the user-facing (graph) API for now, I'd suggest to do the following with this PR:
* instead of main, target a feature branch for now, as we already did with the original PR

* adjust this PR to make sure that the supplied id (mail address) is actually safe to be used as a directory and/or filename (e.g. by checking that `filepath.Base(mail)` equals `mail` and is not `.` , `..` or  `/`) (AFAICS the names that get through this validation would also be safe for the `user.oc.grant.m:<id>` xattr name.

* before merging the feature branch into main figure out a workable solution
We should definitely pick the appropriate encoding solution before putting it into main as we don't want to have to implement a migration as well.

👍

pbleser-oc · 2026-09-02T14:56:41Z

The latter, in case of guest shares they're mail addresses, for "normal" user shares they're usually UUIDs in our type of setups.
There are some parts e.g. in the lower levels of the share manager where we don't know anymore that is is an mail address. It might be possible to change that without much effort, haven't checked in detail yet.

That is a bit unfortunate, because that leaks requirements on those IDs that are passed down to the lowest level that are probably not documented, and can't really be enforced by the compiler either since they're just of type string.

Those requirements being implementation-dependent, like being safe to be used as a filename.
It wouldn't matter in a different implementation, e.g. that uses a different approach for storage, where they are not used as filenames.

Possibly a hellish refactoring to do at this point (not sure how much code would be affected), but one approach would be to

replace the type of those IDs from string to something specific (ShareId ?), which would paint everything red that just passes strings there,
introduce a few functions that convert to such ShareId values, and then we could decide in every calling site whether we use ShareIdFromUuid(uuid string) or ShareIdFromEmail(email string)
and we could even make a quick validation check in the ShareIdFromUuid func with a panic just to make sure it is indeed a UUID and safe to be used as-is as a filename, run that through the tests, and remove that again before the next stable production version

It would still make the "must be usable as a filename" concern bleed out upwards from the low-level storage layer, but at least it would make that requirement a lot clearer, and the compiler will help us enforcing it to some degree.

rhafer · 2026-09-03T10:21:44Z

The latter, in case of guest shares they're mail addresses, for "normal" user shares they're usually UUIDs in our type of setups.
There are some parts e.g. in the lower levels of the share manager where we don't know anymore that is is an mail address. It might be possible to change that without much effort, haven't checked in detail yet.

That is a bit unfortunate, because that leaks requirements on those IDs that are passed down to the lowest level that are probably not documented, and can't really be enforced by the compiler either since they're just of type string.

Those requirements being implementation-dependent, like being safe to be used as a filename. It wouldn't matter in a different implementation, e.g. that uses a different approach for storage, where they are not used as filenames.

Possibly a hellish refactoring to do at this point (not sure how much code would be affected), but one approach would be to
* replace the type of those IDs from `string` to something specific (`ShareId` ?), which would paint everything red that just passes strings there,

* introduce a few functions that convert to such `ShareId` values, and then we could decide in every calling site whether we use `ShareIdFromUuid(uuid string)` or `ShareIdFromEmail(email string)`

* and we could even make a quick validation check in the `ShareIdFromUuid` func with a `panic` just to make sure it is indeed a UUID and safe to be used as-is as a filename, run that through the tests, and remove that again before the next stable production version
It would still make the "must be usable as a filename" concern bleed out upwards from the low-level storage layer, but at least it would make that requirement a lot clearer, and the compiler will help us enforcing it to some degree.

Looking at the current code at least in the ShareManager, it doesn't seem to be too hard to adjust the places where the userid is passed as a generic string with either a concrete type or some Interface. Shouldn't even require any migration for existing deployments. I'll also check the StorageProvider and see how far I can get with that.

rhafer · 2026-09-03T15:33:51Z

Suggested approach, similar to what @pbleser-oc I think:

Introduce a small interface with a single method that returns a filename safe string:

type FilenameEncoder interface {
  Filename() string
}

Create a local type, wrapping userpb.UserId ,that implements the above interface. Dependent on the Type of the UserID it either returns the bare OpaqueId string, or for Type == Guest, the base64 encoded value.

Change the methods in the jsoncs3 sharemanager that now get the user id value passed as a string to use the FileEnoder type.

The new type and interface should also be used by the storage provider to encode the names of the extended attributes oc.user.grant.... and the name of the file used for the by-mail cache.

Implementation is WIP, was distracted by other stuff.

…ke a normal user

Move the ACE prefix constants to the ace package and get rid of hard coded prefixes spread throughout the code.

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.

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.

rhafer · 2026-09-09T08:39:31Z

I've reworked the share manager and the decomposedfs ACE persistence to base64-wrap guest uids. Also rebased to latest main.

@maki5 @pbleser-oc I would appreciate another round of review. (The interesting bits are in the last 2 commits, I'd say)

pbleser-oc

Looks good, thanks for the improvements.

maki5 · 2026-09-09T09:53:48Z

 	} 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, MailAcePrefix) {
+			userID, _ := (utils.FSSafeUserID{ID: &userpb.UserId{Type: userpb.UserType_USER_TYPE_GUEST}}).Decode(id)


we are ignoring err here, might be good to handle it

Hm, you're right about ignoring the error.

Though the method we're in is only called for ACE values that have been created by Unmarshal which does actually error out on invalid base64.

I am going to rework this to avoid the double decoding, which will also get us rid of the ignored error check here.

I am going to rework this to avoid the double decoding, which will also get us rid of the ignored error check here.

Hm, looking at this again. I think this would make the code worse.

ACE are only generate by FromGrant or Unmarshal. FromGrant already ensure valid base64, and Unmarshal rejects invalid base64 (see: line 281).

I'll add a clarifying comment why we ignore the error.

maki5

overall looks good, just a couple of small mentions

maki5 · 2026-09-09T09:55:25Z

+
+// 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 {


this method is pretty similar to func (id FSSafeUserID) SafeFilename() string, maybe it's worth to combine them and to use just one method instead

Hm, I think it's cleaner to keep them separate. CanonicalUserID is mainly for comparison/equality check, while SafeFilename() is about encoding the ID so it can be safely used for filesystem names.

maki5 · 2026-09-09T09:57:21Z


 func UserAce(id *userpb.UserId) string {
-	return "u:" + id.OpaqueId
+	filename := (utils.FSSafeUserID{ID: id}).SafeFilename()


maybe it's better to have a constructor in utils to not initialise the struct here directly, there are some other places where this kind of approach is used

Yep. Makes sense. I'll change that.

Co-authored-by: Pascal Bleser <p.bleser@opencloud.eu>

Also makes the "id" struct field private.

rhafer · 2026-09-09T13:59:55Z

Merging after a final chat with @maki5 about the latest adjustments.

rhafer commented Aug 25, 2026

View reviewed changes

Comment thread internal/grpc/services/usershareprovider/usershareprovider.go Outdated

rhafer force-pushed the persist-guest-shares branch 2 times, most recently from e45f5a2 to 46baf62 Compare August 31, 2026 12:40

pbleser-oc reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/storage/pkg/decomposedfs/node/node.go Outdated

pbleser-oc reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/storage/pkg/decomposedfs/grants.go Outdated

pbleser-oc reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/storage/pkg/decomposedfs/grants.go Outdated

pbleser-oc reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/storage/utils/ace/ace.go Outdated

fschade reviewed Aug 31, 2026

View reviewed changes

Comment thread pkg/storage/pkg/decomposedfs/spaces.go

rhafer force-pushed the persist-guest-shares branch from 9c07130 to d50640b Compare August 31, 2026 16:07

rhafer mentioned this pull request Sep 1, 2026

Add support for creating permission invites (shares) to mail addressses opencloud-eu/opencloud#3440

Draft

4 tasks

rhafer commented Sep 1, 2026

View reviewed changes

Comment thread internal/grpc/services/usershareprovider/usershareprovider.go Outdated

rhafer force-pushed the persist-guest-shares branch from 723c7ce to 77fa410 Compare September 3, 2026 11:01

rhafer force-pushed the persist-guest-shares branch from 77fa410 to 302ad87 Compare September 7, 2026 15:34

aduffeck and others added 9 commits September 9, 2026 10:33

Add support for mail grantees

4450fb1

Fix tests

ec0840e

Do not check the tenant id of guests

677bd46

reverted userprovider check for create share, guest now is handled li…

5c1ae4b

…ke a normal user

guest users can see shared with them spaces

119ffa4

lowercase normalization for grants

47a3804

ability to remove guest grants from index

8d7b86b

refactor(storage): centralize ACE prefix constants

08184dc

Move the ACE prefix constants to the ace package and get rid of hard coded prefixes spread throughout the code.

rhafer added 2 commits September 9, 2026 10:33

fix: only accept bare mail addresses

b3bdf9c

rhafer force-pushed the persist-guest-shares branch from 302ad87 to b3bdf9c Compare September 9, 2026 08:35

rhafer requested review from fschade, maki5 and pbleser-oc September 9, 2026 08:39

pbleser-oc approved these changes Sep 9, 2026

View reviewed changes

maki5 reviewed Sep 9, 2026

View reviewed changes

pbleser-oc reviewed Sep 9, 2026

View reviewed changes

Comment thread pkg/utils/utils.go

pbleser-oc reviewed Sep 9, 2026

View reviewed changes

Comment thread pkg/utils/filename_encoder.go

maki5 approved these changes Sep 9, 2026

View reviewed changes

docs: add clarifying comments about why we're lowercasing guest user ids

0026eef

Co-authored-by: Pascal Bleser <p.bleser@opencloud.eu>

rhafer force-pushed the persist-guest-shares branch from 106d13b to 0026eef Compare September 9, 2026 10:27

rhafer added 2 commits September 9, 2026 10:59

docs: explain ignored guest principal decode error

2e693bb

refactor: use constructor for FSSafeUserID

05b61f2

Also makes the "id" struct field private.

rhafer merged commit 5835058 into opencloud-eu:main Sep 9, 2026
19 checks passed

Uh oh!

Conversation

rhafer commented Aug 25, 2026

Uh oh!

rhafer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhafer commented Aug 31, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhafer commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maki5 commented Sep 2, 2026

Uh oh!

pbleser-oc commented Sep 2, 2026

Uh oh!

rhafer commented Sep 2, 2026

Uh oh!

pbleser-oc commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhafer commented Sep 3, 2026

Uh oh!

rhafer commented Sep 3, 2026

Uh oh!

rhafer commented Sep 9, 2026

Uh oh!

pbleser-oc left a comment

Choose a reason for hiding this comment

Uh oh!

maki5 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rhafer Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rhafer Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maki5 left a comment

Choose a reason for hiding this comment

Uh oh!

maki5 Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhafer Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

maki5 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rhafer Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

rhafer commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhafer commented Sep 2, 2026 •

edited

Loading

pbleser-oc commented Sep 2, 2026 •

edited

Loading

maki5 Sep 9, 2026 •

edited

Loading