Skip to content
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.

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

abort infinite upload for guest users without storage #4138

Closed
call-me-matt opened this issue Sep 11, 2020 · 14 comments · Fixed by #5036
Closed

abort infinite upload for guest users without storage #4138

call-me-matt opened this issue Sep 11, 2020 · 14 comments · Fixed by #5036
Assignees
Labels
1. to develop bug feature: upload & shares & voice 📤🎙️ Sharing files into a chat and audio recordings
Milestone

Comments

@call-me-matt
Copy link
Member

call-me-matt commented Sep 11, 2020

Steps to reproduce

  1. connect as a guest user
  2. upload a picture to the chatroom

Expected behaviour

The picture should be shown to everybody or alternatively the upload should be cancelled with an error message. Or, even better, the attachment icon should be hidden if no write permission is granted to the guest.

Actual behaviour

An error message is displayed (briefly), but the uploaded picture is also shown in the chat. To the guest it is not obvious that the shared picture is only visible to her/him and why it disappears with a refresh of the page.

Talk app

Talk app version: (see apps admin page: /index.php/settings/apps) I'm only a guest, but should be the latest and greatest.

@PVince81
Copy link
Member

ok, I was confused about the term "guest". so this is not a conversation accessed by public link but as user from the guest app

@PVince81
Copy link
Member

I just tried it locally and can confirm the error.

So what we could do is have an initial Webdav call to the configured attachment folder (see my note in #3343 (comment) about allowing to configure it, will default to "/" for now), and if that one is writable (and has space left, more than 0), then enable all the upload operations.

@PVince81
Copy link
Member

PVince81 commented Oct 27, 2020

with Webdav I was mostly thinking about frontend only.

maybe there's also a way to bundle that information through any of the other calls that might come from the Talk API

Tasks:

  • add error verification in case the storage became unavailable/blocked/forbidden while uploading, display an error message and discard the temporary message: Catch upload 507 errors #5031
  • get and store quota and perms of attachments folder upon loading the app
  • if perms does not allow uploading, remove the upload components from view (don't render)
  • if quota does not allow uploading, display the upload components disabled with a tooltip saying "not enough space" or alike
  • after uploading a bunch of files, re-query the attachment folder's quota and perms
  • if the attachment folder was changed in the settings, re-query as well

@nickvergessen nickvergessen added this to the 💖 Next Major (22) milestone Jan 13, 2021
@PVince81 PVince81 self-assigned this Jan 25, 2021
@PVince81
Copy link
Member

I thought about a simpler approach: detect the guest user in the capabilities manager and return "attachments.allowed = false" in case of a guest user.

But then it came to my mind that while a guest app user cannot upload into their "/Talk" folder, they could always reconfigure the attachment folder to point at a writable received share. A friendly user could share some of their personal space for them to be able to post attachments.

Due to the latter condition, the capabilities approach is not suitable. So as stated before, will need it to PROPFIND the attachment folder to check for available quota.

@PVince81
Copy link
Member

While locally testing the capabilities approach I discovered that the frontend isn't reading it, so I fixed it here #5033

@PVince81
Copy link
Member

So it seems the webdav library we use is not flexible enough:

The only solution (besides fixing it upstream) would be to build the XML query locally in Talk just to query the quota.

Maybe for now I just query for the root and add a TODO for later...

@nickvergessen
Copy link
Member

Due to the latter condition, the capabilities approach is not suitable. So as stated before, will need it to PROPFIND the attachment folder to check for available quota.

Just check the quota on php side in TInitialState?

@PVince81
Copy link
Member

Due to the latter condition, the capabilities approach is not suitable. So as stated before, will need it to PROPFIND the attachment folder to check for available quota.

Just check the quota on php side in TInitialState?

I thought of this as well but then it won't update if the user changes the attachments folder to one with a bigger quota. Would require a page refresh. If we consider this a corner case anyway I guess that will do for now.

@nickvergessen
Copy link
Member

I thought of this as well but then it won't update if the user changes the attachments folder to one with a bigger quota. Would require a page refresh. If we consider this a corner case anyway I guess that will do for now.

Return the quota on the API that changes the folder and update accordingly?

@PVince81
Copy link
Member

I thought of this as well but then it won't update if the user changes the attachments folder to one with a bigger quota. Would require a page refresh. If we consider this a corner case anyway I guess that will do for now.

Return the quota on the API that changes the folder and update accordingly?

Returning the quota on a setter. Didn't feel right.
But ok, that might do.

@PVince81
Copy link
Member

it seems selecting a received shared for attachments is forbidden: https://github.com/nextcloud/spreed/blob/master/lib/Controller/SettingsController.php#L106

So the use case of "a generous other user giving some storage for guests" is not fulfillable.

I'm not sure if we should allow that or not. At least it would open up the possibility for guest users to post attachments in chats.
Thoughts ? @nickvergessen

@PVince81
Copy link
Member

PVince81 commented Jan 27, 2021

using shared folders for attachments (or group folders) would likely lead to collisions/duplications if all users are using the same... so probably wise to prevent it I guess

@PVince81
Copy link
Member

I hacked the code to allow me to upload attachments to shared folders but some other parts of the sharing code disallows this for guests "Access to this resource is forbidden for guests.". Smells like a can of worms.

Abandoning this idea for now.

I'll push what I have for the quota check, which will fulfil the requirement of "removing the upload button in case the quota/free space is 0"

@PVince81
Copy link
Member

PR here: #5036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop bug feature: upload & shares & voice 📤🎙️ Sharing files into a chat and audio recordings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants