richdocuments icon indicating copy to clipboard operation
richdocuments copied to clipboard

File gets truncated on save when quota is exceeded

Open thebearon opened this issue 2 years ago • 3 comments

  • Set up a certain quota for a user,
  • Add files to almost fill the quota,
  • Inside a document opened in Collabora Online, embed a large image that gets the user beyond the quota limit,
  • Save document.

-> Error message: Document cannot be saved, please check your permissions, and Document has been changed popup right afterwards. More importantly, the file itself is truncated in storage.

Expectation:

  • the file should be untouched in storage (Nextcloud),
  • the user should be informed the document cannot be saved due to exceeded quota, offering them the option to download instead (COOL).

thebearon avatar Nov 10 '23 07:11 thebearon

Thanks for reporting. I would not expect the Nextcloud storage layers to truncated the file when the storage throws an out of quota exception but I might be wrong there. I'll check that on our side.

For the error, we can definitely report back such a case on the WOPI request. It actually would be good to know the size upfront to we do not even attempt to write larger files if they don't fit.

  • [ ] Check if Content-Length is already passed by Collabora and if we can validate it against the free space
  • [ ] Check why

Regarding reporting back we might need to agree on a response status and format to give back a custom error message as WOPI itself only defines some basic status codes and no format for a response body https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/putfile#status-codes However I think error message hand over to also show something to the user would be nice in any failure case

juliusknorr avatar Nov 10 '23 11:11 juliusknorr

We can check for the quota with something like this:

Server storage layer does not check the quota upfront as is just does a stream copy without being aware of the size.

diff --git a/lib/Controller/WopiController.php b/lib/Controller/WopiController.php
index a9cf99ac..5e31e318 100644
--- a/lib/Controller/WopiController.php
+++ b/lib/Controller/WopiController.php
@@ -70,6 +70,7 @@ use OCP\Share\IShare;
 use Psr\Container\ContainerExceptionInterface;
 use Psr\Container\NotFoundExceptionInterface;
 use Psr\Log\LoggerInterface;
+use Sabre\DAV\Exception\InsufficientStorage;

 class WopiController extends Controller {
        /** @var IRootFolder */
@@ -107,6 +108,7 @@ class WopiController extends Controller {

        // Signifies LOOL that document has been changed externally in this storage
        public const LOOL_STATUS_DOC_CHANGED = 1010;
+       public const LOOL_STATUS_NO_SPACE = 3000;

        public const WOPI_AVATAR_SIZE = 64;

@@ -506,6 +508,13 @@ class WopiController extends Controller {
                                }
                        }

+                       $size = $this->request->getHeader('Content-Length');
+
+                       if ($file->getParent()->getFreeSpace() < $size) {
+                               return new JSONResponse([
+                                       'LOOLStatusCode' => self::LOOL_STATUS_NO_SPACE,
+                               ]);
+                       }
                        $content = fopen('php://input', 'rb');

                        try {

juliusknorr avatar Nov 15 '23 09:11 juliusknorr

@thebearon What do you think about adding a new status code for this in https://github.com/CollaboraOnline/online/blob/db31a486dfbf42669662b2b2d8db74bf99f881a0/wsd/Storage.hpp#L311 ?

We could actually extend this even further with a custom error message that Nextcloud could return, e.g. to indicate more clearly what to check in other error cases like of https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/putfile#status-codes

juliusknorr avatar Nov 15 '23 12:11 juliusknorr

@pedropintosilva Something we probably can discuss next week in our call, I think having a way to report the proper root cause back to the user within Collabora Online would be really benificial here to not just show a generic error if the save has failed.

juliusknorr avatar Mar 27 '24 12:03 juliusknorr

Filed https://github.com/CollaboraOnline/online/issues/8713 upstream for improved error reporting

juliusknorr avatar Apr 05 '24 13:04 juliusknorr