[avatar] add error handlers for avatar setup #17805

MorrisJobke · 2015-07-22T11:17:08Z

Steps to reproduce:

upload a huge image (i.e. https://commons.wikimedia.org/wiki/File:Bath_from_alexandra_park.jpg )
upload it directly as avatar in the personal settings
before: no error message, afterwards: error message
upload the same file to files app
go to personal settings
click "Choose from files"
choose the image
before: no error message, afterwards: error message

cc @DeepDiver1975 @CaptionF @kawohl @Henni @rullzer @Xenopathic @LukasReschke @oparoz

MorrisJobke · 2015-07-22T11:19:03Z

@oparoz I got following for this huge image:

{"reqId":"slAdfYOL92fOpVODLr6\/","remoteAddr":"127.0.0.1","app":"PHP","message":"Allowed memory size of 536870912 bytes exhausted (tried to allocate 90636805 bytes) at \/home\/mjob\/Projekte\/owncloud\/master\/lib\/private\/encryption\/manager.php#179","level":3,"time":"2015-07-22T09:02:28+00:00","method":"POST","url":"\/master\/index.php\/avatar\/"}

Can we somehow limit the avatar stuff to not accept images that are bigger than 10 MB or so?

karlitschek · 2015-07-22T11:22:49Z

nice 👍
Please also backport

nickvergessen · 2015-07-22T11:30:48Z

settings/js/personal.js

No string concatenation for translations please.
French uses : (space before and after colon)

I just move the colon inside, because on most other translations with a trailing whitespace it was added to the translation as well. And thanks for the hint.

oparoz · 2015-07-22T11:53:39Z

@MorrisJobke - I'm not familiar with the avatar code, but it should be possible to check the size of the image in the controller and send back an error if it's deemed too large. I think a hard limit makes sense as it's only an avatar and lots of systems place limits on dimensions or size.

oparoz · 2015-07-22T11:57:10Z

@MorrisJobke - I think a code 413 would be appropriate and the JS could alter the error message based on the code it receives.

MorrisJobke · 2015-07-22T12:26:02Z

@MorrisJobke - I think a code 413 would be appropriate and the JS could alter the error message based on the code it receives.

Can you make this a separate PR? That would be really nice :)

nickvergessen · 2015-07-22T12:29:14Z

settings/js/personal.js

Also use place holders for the status and statusText? I mean that's why we have them ;)

I always forgot about them 🙈

I changed it.

MorrisJobke · 2015-07-22T12:56:50Z

Jenkins: #17809

MorrisJobke · 2015-07-22T14:12:51Z

@nickvergessen @oparoz Please review :)

rullzer · 2015-07-22T19:28:19Z

@MorrisJobke - I'm not familiar with the avatar code, but it should be possible to check the size of the image in the controller and send back an error if it's deemed too large. I think a hard limit makes sense as it's only an avatar and lots of systems place limits on dimensions or size.

Well we do pose limits on the avatar size. But only if a user requests the avatar. max 2048x2048.
I'm not sure if we have the dimension info available before the whole file is uploaded.

rullzer · 2015-07-22T19:37:26Z

@MorrisJobke - I think a code 413 would be appropriate and the JS could alter the error message based on the code it receives.

Can you make this a separate PR? That would be really nice :)

So the avatar controller can't do this. Because the file needs to be uploaded before we can make this happen. A solution is to use HTML5 (if we can): see https://jsfiddle.net/xnkhN/
That way we could first send a request to the avatarcontroller to see if the files is larger than the maxiumum.

But lets not complicate this more than it has to. This PR shows a nice error message if things go bad. If users decide it is a good idea to upload a 100MB file to create a tiny avatar I do not think it is then our responsibility to make their experience as smooth as possible.

I do wish we had javascript tests for those files.

oparoz · 2015-07-22T21:17:24Z

So the avatar controller can't do this. Because the file needs to be uploaded before we can make this happen

Yes, but uploading the file is only a problem if you encrypt it right away. If it's in /tmp, then you can get its size before processing it further.

If users decide it is a good idea to upload a 100MB file to create a tiny avatar I do not think it is then our responsibility to make their experience as smooth as possible.

I agree, but what about the case were a user wants to use one of the pictures which are already in his cloud?
If you already cancel the operation when you detect that the image is larger than 2048x2048, then it should be all good.

rullzer · 2015-07-23T05:27:38Z

@oparoz nope then it is not all good. We allow the user to crop an image to create an avatar. So we should allow images larger than 2048x2048.

DeepDiver1975 · 2015-07-23T08:50:05Z

@MorrisJobke for me there is no error on server side - but the image is not shown after upload

I guess some max file size check is reasonable.

oparoz · 2015-07-23T09:41:18Z

We allow the user to crop an image to create an avatar. So we should allow images larger than 2048x2048.

OK, then users will be able to break PHP if their memory limit is set too low, just like for previews, except that it's more annoying when you don't get the avatar picture.

DeepDiver1975 · 2015-07-23T09:47:17Z

To me it would make sense to have a max limit here of 20MB for now - talking about a solution which can be backported as well.

In the long run the avatar code needs to get rid of that cache class - it is not designed to operate with that big data

DeepDiver1975 · 2015-07-23T09:58:06Z

@MorrisJobke @oparoz size limit is in place

oparoz · 2015-07-23T10:15:05Z

size limit is in place

We need the same things for the dimensions (a 4MB JPEG can break PHP if set with a 64MB memory limit), but I think it might be best to keep it in the Preview class

rullzer · 2015-07-23T16:45:03Z

settings/js/personal.js

Currently it displays: 400 Bad request. Which does not tell the user anything. It should display the returned message: File is too big

MorrisJobke · 2015-07-28T12:56:52Z

@rullzer @Xenopathic @LukasReschke @oparoz Please review :)

PVince81 · 2015-07-28T13:07:41Z

settings/js/personal.js

Huh?! Copy-paste fail ?

DeepDiver1975 · 2015-07-28T22:01:36Z

@rullzer @Xenopathic @LukasReschke @oparoz Please review :)

rullzer · 2015-07-29T06:16:18Z

settings/js/personal.js

data is undefined in this function.
Bad copy paste?

👾 😢 me an js 💣

rullzer · 2015-07-29T08:18:14Z

Looking good now! 👍

rullzer · 2015-07-29T08:21:42Z

On tiny last thing.

It now shows:

An error occurred: 400 File is too big

Shouldn't that be something like

An error occurred: File is too big (code 400)

Or even without the code? Since the user does not care about the response code.

@jancborchardt ^^

MorrisJobke · 2015-07-29T18:22:45Z

@owncloud-bot retest this please

DeepDiver1975 · 2015-07-29T20:54:05Z

Shouldn't that be something like

An error occurred: File is too big (code 400)

Or even without the code? Since the user does not care about the response code.

@rullzer branch hijack permission granted - THX

MorrisJobke · 2015-07-29T21:27:05Z

Jenkins is happy: #17968 (comment)

jancborchardt · 2015-07-30T14:20:01Z

@rullzer simply:

Error: File is too big

No error code indeed. ;)

DeepDiver1975 · 2015-07-30T14:25:37Z

No error code indeed. ;)

Already done - 73f025a#diff-a1c52fafc7d1f682c8849c5c62297682R244

status code is only displayed if there is no message communicated to the client

rullzer · 2015-07-30T14:30:19Z

@rullzer branch hijack permission granted - THX

Ah I'm to late ;)
Anyway, looking good! Lets get this in. 👍

Looks simple enough to backport!

MorrisJobke · 2015-07-30T14:58:08Z

settings/js/personal.js

That looks weird. Two times status text?

Okay - I better stop writing JS

I will fix it

o, mmm missed that.
this is why we need javascript tests for those classes as well!

Good catch @MorrisJobke

add colon to translated string use placeholder in t() Adding a size limitation for avatar upload Unit test for file size Fix typo & display server side error message

scrutinizer-notifier · 2015-07-30T16:05:49Z

A new inspection was created.

MorrisJobke · 2015-07-30T16:06:18Z

Now I'm fine with this 👍

MorrisJobke · 2015-07-30T16:12:12Z

@owncloud-bot retest this please

ghost · 2015-07-30T17:38:02Z

🚀 Test PASSed.🚀

[avatar] add error handlers for avatar setup

MorrisJobke added 3 - To Review feature:avatars labels Jul 22, 2015

MorrisJobke added this to the 8.2-current milestone Jul 22, 2015

nickvergessen reviewed Jul 22, 2015
View reviewed changes

MorrisJobke mentioned this pull request Jul 22, 2015

Jenkins avatar handle errors #17809

Closed

MorrisJobke added the backport-request label Jul 22, 2015

DeepDiver1975 mentioned this pull request Jul 23, 2015

[Jenkins] Avatar handle errors #17839

Closed

rullzer reviewed Jul 23, 2015
View reviewed changes

PVince81 reviewed Jul 28, 2015
View reviewed changes

DeepDiver1975 mentioned this pull request Jul 28, 2015

[Jenkins] Avatar handle errors #17931

Closed

DeepDiver1975 force-pushed the avatar-handle-errors branch from b5ff7c2 to 4c54086 Compare July 28, 2015 13:21

DeepDiver1975 force-pushed the avatar-handle-errors branch from 4c54086 to 356ed4c Compare July 28, 2015 13:35

DeepDiver1975 mentioned this pull request Jul 28, 2015

[Jenkins] Avatar handle errors #17934

Closed

rullzer reviewed Jul 29, 2015
View reviewed changes

DeepDiver1975 force-pushed the avatar-handle-errors branch from 356ed4c to ca173be Compare July 29, 2015 07:45

MorrisJobke mentioned this pull request Jul 29, 2015

Jenkins avatar handle errors #17968

Closed

DeepDiver1975 force-pushed the avatar-handle-errors branch from ca173be to 73f025a Compare July 30, 2015 12:40

MorrisJobke reviewed Jul 30, 2015
View reviewed changes

[avatar] add error handlers for avatar setup

e184157

add colon to translated string use placeholder in t() Adding a size limitation for avatar upload Unit test for file size Fix typo & display server side error message

MorrisJobke force-pushed the avatar-handle-errors branch from 73f025a to e184157 Compare July 30, 2015 16:05

rullzer added a commit that referenced this pull request Jul 31, 2015

Merge pull request #17805 from owncloud/avatar-handle-errors

db91b45

[avatar] add error handlers for avatar setup

rullzer merged commit db91b45 into master Jul 31, 2015

rullzer deleted the avatar-handle-errors branch July 31, 2015 05:31

rullzer mentioned this pull request Jul 31, 2015

[stable8.1][avatar] add error handlers for avatar setup #18003

Merged

MorrisJobke added 8.1.1-current and removed backport-request labels Aug 2, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019

[avatar] add error handlers for avatar setup #17805

[avatar] add error handlers for avatar setup #17805

Uh oh!

Conversation

MorrisJobke commented Jul 22, 2015

Uh oh!

MorrisJobke commented Jul 22, 2015

Uh oh!

karlitschek commented Jul 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oparoz commented Jul 22, 2015

Uh oh!

oparoz commented Jul 22, 2015

Uh oh!

MorrisJobke commented Jul 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Jul 22, 2015

Uh oh!

MorrisJobke commented Jul 22, 2015

Uh oh!

rullzer commented Jul 22, 2015

Uh oh!

rullzer commented Jul 22, 2015

Uh oh!

oparoz commented Jul 22, 2015

Uh oh!

rullzer commented Jul 23, 2015

Uh oh!

DeepDiver1975 commented Jul 23, 2015

Uh oh!

oparoz commented Jul 23, 2015

Uh oh!

DeepDiver1975 commented Jul 23, 2015

Uh oh!

DeepDiver1975 commented Jul 23, 2015

Uh oh!

oparoz commented Jul 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Jul 29, 2015

Uh oh!

rullzer commented Jul 29, 2015

Uh oh!

MorrisJobke commented Jul 29, 2015

Uh oh!

DeepDiver1975 commented Jul 29, 2015

Uh oh!

MorrisJobke commented Jul 29, 2015

Uh oh!

jancborchardt commented Jul 30, 2015

Uh oh!

DeepDiver1975 commented Jul 30, 2015

Uh oh!

rullzer commented Jul 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!