Make possible for apps to define their own avatar types

[danxuliu]

Requires #24578

I am really sorry to come with this change so late in the cycle. It is perfectly fine if it is deferred to Nextcloud 22 :-) (or if it is a bad idea and it is never merged ;-) ). However, while working on the Group conversation picture feature for Talk I thought that it would be good to generalize the avatars and use them for that. This would make possible, for example, to show the avatar of a room in the list of shares without needing to use a Talk endpoint.

I guess it could be used too for unifying the avatars between clients or differentiating user and group avatars (although that would not be something to be done for Nextcloud 21).

Before this pull request the avatars were limited to users and guests, and there was no OCS endpoint for getting and setting the avatars. Now, besides the default user and guest types, an app can register its own avatar type(s), and any avatar can be got or set using a OCS endpoint (and the old ones are still available, of course).

In order to set an avatar with the old endpoints a temporary image needs to be posted, and then a request to crop that temporary image is needed to finally set the avatar. When using the OCS endpoint, on the other hand, the image needs to be uploaded already with the desired size (and it can not be selected from the internal files, the client setting the avatar needs to download it first, crop it as needed and then upload it again). If the avatar does not support the aspect ratio (for example, the user avatar must be squared) then the request fails.

Pending:

• Add IAvatarProvider (or something like that) and registration of providers to IAvatarManager
• Prevent user avatars from being modified by other users
  • Should it be done in UserAvatar? Or should be prevented only in the controller (for example, to allow an admin to modify the avatar from an OCC command, or something like that)? In case it needs to be done in the controller, how? Add something like canAvatarBeModifiedByUser($userId) to IAvatar and check it from the controller?
• Use different cache values in the controller depending on the avatar type
  • Where to specify the cache value to use for each type? With "getCache()" in the AvatarProvider? With a more generic "getAvatarOptions()" (or something similar) that returns a map of keys and values so IAvatarProvider does not need to be extended? In the IAvatar itself so it could even change depending on the avatar instance?
• Add a method to get the avatar version to IAvatarProvider, similar to getCacheTimeToLive(IAvatar)

Future:

• Create a UserAvatarProvider and move code specific to user avatars from AvatarManager to UserAvatarProvider (not done yet to minimize the changes). - UserAvatarProvider has been created already, as getting the cache will be done from IAvatarProvider, although the code specific to user avatars was not moved yet to the provider.

[ChristophWurst]

I like this a lot because we can also use this to unify/replace https://github.com/nextcloud/mail/blob/master/lib/Controller/AvatarsController.php

[ChristophWurst]

you can even use the min/max functions for this 😉

[nickvergessen]

but not for <= 0 use 64.

On the other side @rullzer always wanted to limit the formats anyway.
I would suggest to only allow: 64, 128, 256 and 512 as sizes.

[danxuliu]

Actually I have realized that this (which is a shameless copy/paste from the old controller :-P ) does not allow to use -1 to get an avatar with the original size (which is documented in IAvatar). Should we allow that in the new controller?

[nickvergessen]

Only 64, 128, 256, 512 ?

[danxuliu]

I have no real preference on the set of sizes, although limiting it to only a specific set sounds like a good idea to prevent an unneeded use of disk space if a "funny" user does something like for i in {64..512}; do curl .../avatar/user0/$i; done. Thus I have added a commit for that.

I still wonder about the "-1" to get the original size; it could be used for example if a user wants to check her current avatar exactly like it was uploaded.

[ChristophWurst]

• This is a bit generic – is it possible to catch a more specific exception?
• If catching that generic I would highly recommend logging the exception to help our futures selves debug any issues with this code :)

[danxuliu]

This is a bit generic – is it possible to catch a more specific exception?

We could catch OCP\Files\NotFoundException as it is thrown by getFile(), InvalidArgumentException as it is thrown by IAvatarManager and whatever is thrown by IAvatarProvider::getAvatar(). I wonder whether other exceptions should be catched or just left to bubble up and generate an error 500 (maybe even the exception thrown by IAvatarProvider::getAvatar() if it ends being just a generic Exception).

If catching that generic I would highly recommend logging the exception to help our futures selves debug any issues with this code :)

👍

[danxuliu]

Instead of AvatarException I named it AvatarProviderException, as that is more specific and gives room to add in the future an AvatarException for avatar specific errors if needed.

In the end I removed the log, as now a generic exception is no longer caught, but an AvatarProviderException, which would be thrown if the provider can not get an avatar for the given id. I think that belongs to the realm of expected errors that do not need to be logged (but if I am wrong it can be added back, of course ;-) ).

[ChristophWurst]

I'd argue that we are lying here. The argument isn't invalid, but the code can't handle the request. Another exception would be more appropriate IMO.

Of course this is highly unlikely to happen because we do the registration before anything else. Hence failing hard here could be an option.

[danxuliu]

I'd argue that we are lying here. The argument isn't invalid, but the code can't handle the request. Another exception would be more appropriate IMO.

I agree, although I used InvalidArgumentException because I did not find any suitable exception and adding a new exception just for this seemed unnecessary (given that in practice it will not happen). So exception suggestions welcome :-)

Of course this is highly unlikely to happen because we do the registration before anything else. Hence failing hard here could be an option.

Failing hard as in exit()? Having an exit there would feel very out of place in my opinion. Or do you mean throw new Error()? Or something else? :-)

[ChristophWurst]

I agree, although I used InvalidArgumentException because I did not find any suitable exception and adding a new exception just for this seemed unnecessary (given that in practice it will not happen). So exception suggestions welcome :-)

In https://github.com/nextcloud/server/pull/24702/files#diff-7bfcce99ae84563547086798084d75003c7099fedeabb795d2c01962293b33fcR88 I went with \RuntimeException.

Failing hard as in exit()? Having an exit there would feel very out of place in my opinion. Or do you mean throw new Error()? Or something else? :-)

I meant letting the exception bubble up, without any explicit handling :)

[danxuliu]

In https://github.com/nextcloud/server/pull/24702/files#diff-7bfcce99ae84563547086798084d75003c7099fedeabb795d2c01962293b33fcR88 I went with \RuntimeException.

\RuntimeException it is then.

Failing hard as in exit()? Having an exit there would feel very out of place in my opinion. Or do you mean throw new Error()? Or something else? :-)

I meant letting the exception bubble up, without any explicit handling :)

Ok, that makes a lot more sense :-P

[ChristophWurst]

I'd like to see a more specific exception thrown here. If this is just about any possible error that we didn't expect then I wouldn't document the exception

[danxuliu]

It should be the same exception thrown by IAvatarProvider::getAvatar(). Should AvatarException be introduced to wrap other specific exceptions that could be thrown by providers? For example, in the case of Talk, the avatar provider could throw RoomNotFoundException if the avatar id does not match any room token, but of course that exception makes little sense in a generic context.

[ChristophWurst]

Should AvatarException be introduced to wrap other specific exceptions

Sounds good to me! Then we can transport any more specific exception as previous.

[ChristophWurst]

no . at the end of phpdoc lines :)

[danxuliu]

I will remove all them, except at the end of It is allowed to register more than one provider per app. (IRegistrationContext::registerAvatarProvider), as:

• I guess the rule does not apply to the "body" of a documentation block (but if it does please let me know ;-) )
• it is consistent with the rest of the documentation in IRegistrationContext

But if that should be removed too let me know ;-)

[ChristophWurst]

There is no hard rule, I guess. For the body it seems fine. The first line (title?) is a bit strange.

But this is just nitpicking. You may also just ignore my remark ;)

[danxuliu]

You may also just ignore my remark ;)

Too late ;-)

[ChristophWurst]

Should it be done in UserAvatar? Or should be prevented only in the controller (for example, to allow an admin to modify the avatar from an OCC command, or something like that)? In case it needs to be done in the controller, how? Add something like canAvatarBeModifiedByUser($userId) to IAvatar and check it from the controller?

We could also make this part of the new avatar provider, right? And then the controller asks the avatar service, which delegates to the provider (or falls back to false or whatever)

But now I'm also thinking about whether a avatar is just identified by (type, id) or if there should be an optional third param for uid. Then we only let users update their avatars? Or the provider internally checks whether the "current" user is allowed to change that avatar? Like if a user updates an avatar of a talk conversation, the Talk provider could know if that UID is moderator and only then it tells the service/provider that modification is fine.

Alternatively we don't even ask for permission but just let the providers throw a dedicated exception when the operation isn't allowed. But again we will likely need the UID as context.

Where to specify the cache value to use for each type? With "getCache()" in the AvatarProvider? With a more generic "getAvatarOptions()" (or something similar) that returns a map of keys and values so IAvatarProvider does not need to be extended? In the IAvatar itself so it could even change depending on the avatar instance?

A IAvatar::getAvatarOptions could give us lots of flexibility with the price of no type guarantees. Also if this interface is implemented by any app, the app will break with this change.

Therefore I'm more in favor of the IAvatarProvider::getCache. Maybe like a IAvatarProvider::getCacheTtl(IAvatar): ?int so the provider can have the depending on the avatar instance flexibility or ignore the parameter. Returning null could signal that there shouldn't be any caching.

[danxuliu]

We could also make this part of the new avatar provider, right? And then the controller asks the avatar service, which delegates to the provider (or falls back to false or whatever)

But now I'm also thinking about whether a avatar is just identified by (type, id) or if there should be an optional third param for uid. Then we only let users update their avatars? Or the provider internally checks whether the "current" user is allowed to change that avatar? Like if a user updates an avatar of a talk conversation, the Talk provider could know if that UID is moderator and only then it tells the service/provider that modification is fine.

Alternatively we don't even ask for permission but just let the providers throw a dedicated exception when the operation isn't allowed. But again we will likely need the UID as context.

Wouldn't it be possible to inject the current user in the avatar provider constructor? Or that is possible only for controllers? In that case the extra parameter would be unneeded, as the IAvatarProvider subclass would define everything it needs to know if the current user can modify (or even get) an avatar or not. Even if the current user is not a real user, but for example a guest (in Talk currently only moderators would be able to set the avatar of a conversation, but technically that could be possible also for a guest moderator).

Therefore I'm more in favor of the IAvatarProvider::getCache. Maybe like a IAvatarProvider::getCacheTtl(IAvatar): ?int so the provider can have the depending on the avatar instance flexibility or ignore the parameter. Returning null could signal that there shouldn't be any caching.

Fully agree, except maybe in the name. Personally I prefer to avoid abbreviatures in API names (or in code names in general), so I would call it getCacheTimeToLive(IAvatar) instead.

[skjnldsv]

As this sounds like a nice feature, the requests for this are quite low. Currently there a no plans to implement such a feature. Thus I will close this ticket for now. This does not mean we don't want this feature, but it is simply not on our roadmap for the near future.