Skip to content

Make possible for apps to define their own avatar types #24579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
danxuliu wants to merge 53 commits into from
Closed

Make possible for apps to define their own avatar types #24579

Changes from 1 commit
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
b553b43
Make possible to send requests as anonymous users in integration tests
danxuliu Dec 3, 2020
184742e
Make possible to set body in requesttoken requests in integration tests
danxuliu Dec 3, 2020
2cc22a0
Add integration tests for user avatars
danxuliu Dec 3, 2020
1552add
Add integration tests for resized user avatars
danxuliu Dec 4, 2020
b4b3276
Add integration tests for getting guest avatars
danxuliu Dec 7, 2020
1de8dc3
Add getter for generic avatars to IAvatarManager
danxuliu Dec 7, 2020
bcce5a6
Add OCS endpoint for avatars
danxuliu Dec 7, 2020
2522439
Make possible to set body in OCS requests in integration tests
danxuliu Dec 7, 2020
b190431
Add integration tests for getting and setting generic avatars
danxuliu Dec 7, 2020
63cbd7b
fixup! Add OCS endpoint for avatars
danxuliu Dec 9, 2020
a31a7fd
fixup! Add OCS endpoint for avatars
danxuliu Dec 10, 2020
92c9fc0
Make possible to define custom avatar types
danxuliu Dec 10, 2020
a836608
fixup! Add OCS endpoint for avatars
danxuliu Dec 10, 2020
95e0177
Move registration of IAvatarProviders to IRegistrationContext
danxuliu Dec 10, 2020
f04a16a
fixup! Make possible to define custom avatar types
danxuliu Dec 14, 2020
ebf242a
fixup! Add OCS endpoint for avatars
danxuliu Dec 14, 2020
bf9169e
fixup! Add getter for generic avatars to IAvatarManager
danxuliu Dec 14, 2020
1e13309
fixup! Make possible to define custom avatar types
danxuliu Dec 14, 2020
6e43ce7
fixup! Add OCS endpoint for avatars
danxuliu Dec 14, 2020
90ac35d
fixup! Make possible to define custom avatar types
danxuliu Dec 14, 2020
5d0102e
fixup! Move registration of IAvatarProviders to IRegistrationContext
danxuliu Dec 14, 2020
ab910ec
fixup! Move registration of IAvatarProviders to IRegistrationContext
danxuliu Dec 14, 2020
a2c63ff
fixup! Make possible to define custom avatar types
danxuliu Dec 14, 2020
76e4bd2
Move deprecated ILogger to LoggerInterface in avatar private classes
danxuliu Dec 14, 2020
612dbe6
Add explicit providers for user and guest avatars
danxuliu Dec 14, 2020
0e953eb
Remove no longer needed attributes from AvatarManager
danxuliu Dec 14, 2020
0bb311f
Get the avatar provider rather than the avatar itself from the manager
danxuliu Dec 14, 2020
611881b
Add method to get the cache duration of an avatar to IAvatarProvider
danxuliu Dec 14, 2020
6c99a9a
fixup! Add method to get the cache duration of an avatar to IAvatarPr…
danxuliu Dec 14, 2020
6079793
fixup! Add explicit providers for user and guest avatars
danxuliu Dec 15, 2020
bcd2074
fixup! Move registration of IAvatarProviders to IRegistrationContext
danxuliu Dec 15, 2020
23f4c1d
fixup! Get the avatar provider rather than the avatar itself from the…
danxuliu Dec 15, 2020
e88d3b6
fixup! Make possible to define custom avatar types
danxuliu Dec 15, 2020
4a26943
fixup! Add explicit providers for user and guest avatars
danxuliu Dec 15, 2020
fa0342a
fixup! Add explicit providers for user and guest avatars
danxuliu Dec 15, 2020
ebc3f14
fixup! Get the avatar provider rather than the avatar itself from the…
danxuliu Dec 15, 2020
03eed3e
fixup! Add integration tests for getting and setting generic avatars
danxuliu Dec 15, 2020
1348cab
fixup! Add integration tests for getting and setting generic avatars
danxuliu Dec 15, 2020
8d69a4e
fixup! Add OCS endpoint for avatars
danxuliu Dec 15, 2020
4368ec0
fixup! Make possible to define custom avatar types
danxuliu Dec 15, 2020
6b8f290
fixup! Add integration tests for getting and setting generic avatars
danxuliu Dec 15, 2020
68b298e
fixup! Add method to get the cache duration of an avatar to IAvatarPr…
danxuliu Dec 16, 2020
cc7cd18
Split try/catch blocks to catch only the relevant exceptions
danxuliu Dec 16, 2020
6ed5fde
Add method to check if an avatar can be accessed by the current user
danxuliu Dec 16, 2020
65c8c22
Add method to check if an avatar can be modified by the current user
danxuliu Dec 16, 2020
135d14b
Add integration tests for unauthorized modification of avatars
danxuliu Dec 16, 2020
330fac8
Add method to get the version of an avatar
danxuliu Dec 16, 2020
68fa045
Limit the returned sizes of the avatars to a predefined set
danxuliu Dec 16, 2020
601d741
fixup! Get the avatar provider rather than the avatar itself from the…
danxuliu Dec 16, 2020
6391a2b
fixup! Add explicit providers for user and guest avatars
danxuliu Dec 16, 2020
029d22c
fixup! Add method to check if an avatar can be modified by the curren…
danxuliu Dec 16, 2020
3a4b926
fixup! Add explicit providers for user and guest avatars
danxuliu Dec 16, 2020
6895199
fixup! Make possible to define custom avatar types
danxuliu Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! Add OCS endpoint for avatars 
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
  • Loading branch information
@danxuliu
danxuliu committed Dec 10, 2020
commit 63cbd7bb0fa2d65aabe4b1d151edc9deb0a421e5
33 changes: 17 additions & 16 deletions core/Controller/GenericAvatarController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@

use OCP\AppFramework\OCSController;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\FileDisplayResponse;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\Response;
use OCP\IAvatarManager;
use OCP\IL10N;
use OCP\ILogger;
Expand Down Expand Up @@ -64,9 +65,9 @@ public function __construct($appName,
* @param string $avatarType
* @param string $avatarId
* @param int $size
* @return JSONResponse|FileDisplayResponse
* @return DataResponse|FileDisplayResponse
*/
public function getAvatar(string $avatarType, string $avatarId, int $size) {
public function getAvatar(string $avatarType, string $avatarId, int $size): Response {
// min/max size
Copy link
Member

@ChristophWurst ChristophWurst Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@nickvergessen nickvergessen Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@danxuliu danxuliu Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@nickvergessen nickvergessen Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 64, 128, 256, 512 ?

Copy link
Member Author

@danxuliu danxuliu Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ($size > 2048) {
$size = 2048;
Expand All @@ -86,7 +87,7 @@ public function getAvatar(string $avatarType, string $avatarId, int $size) {
]
);
} catch (\Exception $e) {
Copy link
Member

@ChristophWurst ChristophWurst Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 :)

Copy link
Member Author

@danxuliu danxuliu Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

👍

Copy link
Member Author

@danxuliu danxuliu Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-) ).

return new JSONResponse([], Http::STATUS_NOT_FOUND);
return new DataResponse([], Http::STATUS_NOT_FOUND);
}

return $response;
Expand All @@ -96,13 +97,13 @@ public function getAvatar(string $avatarType, string $avatarId, int $size) {
* @PublicPage
*
* @param string $path
* @return JSONResponse
* @return DataResponse
*/
public function setAvatar(string $avatarType, string $avatarId) {
public function setAvatar(string $avatarType, string $avatarId): DataResponse {
$files = $this->request->getUploadedFile('files');

if (is_null($files)) {
return new JSONResponse(
return new DataResponse(
['data' => ['message' => $this->l->t('No file provided')]],
Http::STATUS_BAD_REQUEST
);
Expand All @@ -113,14 +114,14 @@ public function setAvatar(string $avatarType, string $avatarId) {
!is_uploaded_file($files['tmp_name'][0]) ||
\OC\Files\Filesystem::isFileBlacklisted($files['tmp_name'][0])
) {
return new JSONResponse(
return new DataResponse(
['data' => ['message' => $this->l->t('Invalid file provided')]],
Http::STATUS_BAD_REQUEST
);
}

if ($files['size'][0] > 20 * 1024 * 1024) {
return new JSONResponse(
return new DataResponse(
['data' => ['message' => $this->l->t('File is too big')]],
Http::STATUS_BAD_REQUEST
);
Expand All @@ -135,17 +136,17 @@ public function setAvatar(string $avatarType, string $avatarId) {
try {
$avatar = $this->avatarManager->getGenericAvatar($avatarType, $avatarId);
$avatar->set($image);
return new JSONResponse(
return new DataResponse(
['status' => 'success']
);
} catch (\OC\NotSquareException $e) {
return new JSONResponse(
return new DataResponse(
['data' => ['message' => $this->l->t('Crop is not square')]],
Http::STATUS_BAD_REQUEST
);
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'core']);
return new JSONResponse(
return new DataResponse(
['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]],
Http::STATUS_BAD_REQUEST
);
Expand All @@ -155,16 +156,16 @@ public function setAvatar(string $avatarType, string $avatarId) {
/**
* @PublicPage
*
* @return JSONResponse
* @return DataResponse
*/
public function deleteAvatar(string $avatarType, string $avatarId) {
public function deleteAvatar(string $avatarType, string $avatarId): DataResponse {
try {
$avatar = $this->avatarManager->getGenericAvatar($avatarType, $avatarId);
$avatar->remove();
return new JSONResponse();
return new DataResponse();
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'core']);
return new JSONResponse(
return new DataResponse(
['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]],
Http::STATUS_BAD_REQUEST
);
Expand Down