Improve quota handling #39895

jvillafanez · 2022-03-17T09:46:03Z

In case there are problems writing the contents of the file, try to
figure out the size of the content in several ways in order to provide a
more meaningful message
Ensure the locks are properly released in case an error happens
Handle quota while copying files. In addition, take into account the
space used by the file we're trying to overwrite (in case there is
already a file there)
Part files will also be considered for quota while they're being
uploaded

Description

Copying a received shared file out of the share could go over the quota limit imposed by the admin. This PR fixes that issue and other related ones

Related Issue

https://github.com/owncloud/enterprise/issues/5023

Fixes #40140

Motivation and Context

Total file size should never be over the quota

How Has This Been Tested?

Manually tested

Admin sets the following quotas: user1 = 1KB ; user2 = 1GB
user2 uploads a 6MB file into "/parent/folder/file.txt"
user2 shares "/parent/folder" to user1
user1 copies the "/folder/file.txt" file into "/u1Local"

user1 must receive an "Insufficient Storage" error and the file mustn't have been copied.

Some other variations have been tested also manually:

Assuming user1 has 10MB of quota instead, and the "file.txt" file has been correctly copied, overwriting the file is possible if the new file size still fits within the quota (overwriting a 6MB with 6MB should work, but not overwriting a 6MB with 20MB - always assuming a 10MB of quota)
Moving files have the same restrictions

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

update-docs · 2022-03-17T09:46:06Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

jvillafanez · 2022-03-17T14:45:18Z

Problem with the tests is in https://github.com/owncloud/core/pull/39895/files#diff-b89bc6bd7d5336d68032bb5a5c1f309df16f27173f9f58d9f338ec495d3f951bL145-R160

When overwriting a shared file by the sharee, the part file is written into the sharee's storage and then moved to the share. If there is a limiting quota in the sharee, the part file can't be written even though the sharer has enough free space.
This behavior was patched previously and this PR brought it back. The problem is that, with the previous behavior, the only way to trigger the quota check is going through DAV; uploading a file bypassing the DAV interface might ignore the quota check as what happens with the issue we're trying to fix here.

Challenges for a proper fix are:

Detect whether the part file targets a share or not.
Figure out proper location outside of the enclosed storage. This should allow us to use a View object to check if the target is shared or not

Worst case, we might need to revert that piece. The fix for the original issue should still work since it's applied in the Sabre connector (DAV interface)

phil-davis · 2022-03-21T07:44:57Z

I added test scenarios to check "COPY" of a file from a received share into a user's own storage. Those fail on current master, and pass with this PR, so that is good. There are already scenarios for "MOVE" and they pass, so there is no regression for that.

I also added some more scenarios for the case where the file is zero bytes (empty). Those have always been allowed, and are still allowed. Users with zero quota can create folders and empty files.

jvillafanez · 2022-03-21T08:40:48Z

The list of cases to be checked, assuming user1 has 1KB of quota, user2 1GB of quota and the target file is 10MB:

user2 shares target file with user1, user1 deletes the file -> a copy of the file should be in the user1's trashbin (10MB)
user2 shares target file with user1, user1 uploads a copy of the file -> upload succeed even though user1 has 1KB of quota because user2 has enough quota
user2 shares target file with user1, user1 renames the file -> nothing happens, target file is still inside the user2's storage.
user2 shares target file from an external storage, user1 uploads a copy -> upload succeed. No quota is applied (external storage usually doesn't have quota)
user2 shares folder with user1, user1 uploads file there -> the upload succeed assuming the user2's quota is big enough. user1's quota doesn't matter
user2 shares folder with user1, user1 copies a 10MB from the share to his local storage -> uploads fails because the file is too big for the user1's quota.
user1 has a file with 750B, user2 shares a folder with user1, user1 copies a 800B file from the share to his local storage overwriting the 750B file -> copy succeed because used space is below user1's quota
user1 has a file with 750B, user2 shares a folder with user1, user1 copies a 800B file from the share to his local storage -> copy fails because user1 doesn't have enough quota
user1 (having 1KB of quota) uploads a 750B file. Then the file is overwritten with a 800B -> the 800B upload succeed because the old file is overwritten, so total used space is below the quota.
user1 (having 1KB of quota) uploads a 750B file. Then a new 800B file is uploaded -> upload fails because there is not enough quota
user1 has an external storage, user1 uploads a 10MB file inside the external storage -> upload succeed because external storage (usually) doesn't have quota
user2 shares a file or folder with user1, user1 copy a 10MB file to an external storage -> copy succeed because it doesn't go through the local storage (or goes through transparently)
user1 copies a file from the external storage to his local storage -> user1's quota applies
user2 shares a folder with user1, user1 copies a file from the external storage to the share -> user2's quota applies.

There are some additional variations using a move operation instead of a copy, uploading whole folders (not sure how it will behave if only some of the files in the uploaded folder fit the quota), etc.

Note the behavior is different for shared files and shared folders.
For a shared file, it seems the part file is uploaded to the user1's primary storage and then moved to the share. We need to check that there are no problems even if user1 doesn't have enough quota to fit the file because the file will end up in user2's storage.
For a shared folder, the that behavior doesn't seem to happen and the part file is uploaded into the share, which means there shouldn't be any problem with the quota.

sonarcloud · 2022-03-21T12:35:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

60.3% Coverage
0.0% Duplication

DeepDiver1975 · 2022-03-22T10:42:12Z

apps/dav/lib/Connector/Sabre/File.php

+	 */
+	private function getExpectedContentLength($data) {
+		$expected = -1;
+		if (isset($_SERVER['CONTENT_LENGTH'])) {


working on global var is kind of spooky - isn't there a way to get access to the sabre request object or at least the oc request object?

That piece was moved. I've just added additional ways to try to figure out the content length because I think in case of COPY requests the error message showed a -1 as expected size.
It's only used for the error message, so it's OK.

DeepDiver1975 · 2022-03-22T10:48:35Z

apps/dav/lib/Connector/Sabre/QuotaPlugin.php

+		// We'll assume the "beforeMethod:*" is being processed, so we'll process our "handleBeforeCopy"
+		// right away.
+		if ($server->httpRequest->getMethod() === 'COPY') {
+			$this->handleBeforeCopy($server->httpRequest);


this will be called super early (within ::initialize) - if this is intended I am okay with this

Yes, it's intended until we find a better way to set it up.

apps/dav/lib/Connector/Sabre/QuotaPlugin.php

mmattel · 2022-06-17T13:49:03Z

@jvillafanez could you fix the conflicts?

jvillafanez · 2022-06-20T07:04:17Z

This is waiting for sabre-io/dav#1396 so there will be some code changes.

jvillafanez · 2022-06-21T08:08:08Z

should we wait until sabre/dav has an official release with the changes in sabre-io/dav#1396 ? We'd need to pin the version in the composer.json file

mmattel · 2022-06-23T11:09:19Z

info, I have requested @phil-davis if he could do a new release of sabre-io/dav as he did this last time, just linking to the issue: sabre-io/dav#1406

jvillafanez · 2022-06-27T11:30:09Z

Rebased and code updated to use sabre 4.4 for new events.

Note that there have been conflicts in "tests/acceptance/features/apiMain/quota.feature" which might need a second look in case there is anything missing.

ownclouders · 2022-06-27T12:11:48Z

💥 Acceptance tests pipeline cliDbConversion-sqlite-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/36193/127

* In case there are problems writing the contents of the file, try to figure out the size of the content in several ways in order to provide a more meaningful message * Ensure the locks are properly released in case an error happens * Handle quota while copying files. In addition, take into account the space used by the file we're trying to overwrite (in case there is already a file there) * Part files will also be considered for quota while they're being uploaded

jvillafanez · 2022-06-29T10:46:56Z

@phil-davis could you check the acceptance tests? I think we can remove the ones in "tests/acceptance/features/apiMain/quotaOc10Issue40140.feature" and remove or adjust some "skipOnOcV10" from the "tests/acceptance/features/apiMain/quota.feature"

phil-davis · 2022-06-29T14:57:56Z

Note that there have been conflicts in "tests/acceptance/features/apiMain/quota.feature" which might need a second look in case there is anything missing.

Acceptance tests have been sorted out and are passing.

phil-davis

Tests are all passing, so there is no obvious regression. And the new/changed tests demonstrate that quota checks work better for the COPY case.

Needs final developer review.

sonarcloud · 2022-06-30T07:35:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

61.6% Coverage
0.0% Duplication

mmattel · 2022-07-08T14:20:48Z

@janackermann @JammingBen mind to take a look on for a final approval, would be great to go into 10.11 - many thanks

jvillafanez added the 2 - Developing label Mar 17, 2022

jvillafanez self-assigned this Mar 17, 2022

jvillafanez force-pushed the quota_improvements branch 2 times, most recently from 021d6a3 to c13480e Compare March 17, 2022 10:47

phil-davis mentioned this pull request Mar 21, 2022

[tests-only] [full-ci] Test quota improvements #39903

Closed

jvillafanez added 3 - To Review and removed 2 - Developing labels Mar 21, 2022

jvillafanez requested a review from DeepDiver1975 March 21, 2022 13:20

DeepDiver1975 reviewed Mar 22, 2022

View reviewed changes

apps/dav/lib/Connector/Sabre/QuotaPlugin.php Outdated Show resolved Hide resolved

jvillafanez force-pushed the quota_improvements branch from c71341f to d1f35d5 Compare June 27, 2022 11:27

jvillafanez and others added 7 commits June 29, 2022 11:24

Adjust quota checks for shared files

b06b16f

Move stripPartialFileExtension method to OC_Util

976aba7

Fixes for sonarCloud

af68f41

Add test scenario for copying a file without quota

00c523b

Add more test scenario for actions without quota

05e3418

Add tests for coverage

1f69508

jvillafanez added 2 commits June 29, 2022 11:24

Adjust code for dav 4.4.0

6ef2763

Rise sabre/dav to 4.4 in composer.json (.lock already updated)

064efe7

jvillafanez force-pushed the quota_improvements branch from d1f35d5 to 064efe7 Compare June 29, 2022 09:25

Adjust quota acceptance tests

82b1864

phil-davis requested review from phil-davis and DeepDiver1975 June 29, 2022 14:58

Changloe for 'Improve quota handling when copying files'

4923c29

phil-davis requested review from AlexAndBear and JammingBen June 29, 2022 15:08

phil-davis reviewed Jun 29, 2022

View reviewed changes

DeepDiver1975 approved these changes Jul 18, 2022

View reviewed changes

DeepDiver1975 merged commit 3c3d1b4 into master Jul 18, 2022

delete-merged-branch bot deleted the quota_improvements branch July 18, 2022 13:49

This was referenced Jul 20, 2022

NIghtly fail on quota.feature (bug demonstration) owncloud/encryption#353

Closed

Delete bug demonstration for quota feature from encryption app owncloud/encryption#354

Merged

jnweiger mentioned this pull request Sep 6, 2022

[QA] Part files are not considered for quota while they are uploaded #40343

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve quota handling #39895

Improve quota handling #39895

jvillafanez commented Mar 17, 2022 •

edited by phil-davis

Loading

update-docs bot commented Mar 17, 2022

jvillafanez commented Mar 17, 2022

phil-davis commented Mar 21, 2022 •

edited

Loading

jvillafanez commented Mar 21, 2022

sonarcloud bot commented Mar 21, 2022

DeepDiver1975 Mar 22, 2022

jvillafanez Mar 22, 2022 •

edited

Loading

DeepDiver1975 Mar 22, 2022

jvillafanez Mar 22, 2022

mmattel commented Jun 17, 2022

jvillafanez commented Jun 20, 2022

jvillafanez commented Jun 21, 2022

mmattel commented Jun 23, 2022

jvillafanez commented Jun 27, 2022

ownclouders commented Jun 27, 2022 •

edited

Loading

jvillafanez commented Jun 29, 2022

phil-davis commented Jun 29, 2022

phil-davis left a comment

sonarcloud bot commented Jun 30, 2022

mmattel commented Jul 8, 2022

Improve quota handling #39895

Improve quota handling #39895

Conversation

jvillafanez commented Mar 17, 2022 • edited by phil-davis Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

update-docs bot commented Mar 17, 2022

jvillafanez commented Mar 17, 2022

phil-davis commented Mar 21, 2022 • edited Loading

jvillafanez commented Mar 21, 2022

sonarcloud bot commented Mar 21, 2022

DeepDiver1975 Mar 22, 2022

Choose a reason for hiding this comment

jvillafanez Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

DeepDiver1975 Mar 22, 2022

Choose a reason for hiding this comment

jvillafanez Mar 22, 2022

Choose a reason for hiding this comment

mmattel commented Jun 17, 2022

jvillafanez commented Jun 20, 2022

jvillafanez commented Jun 21, 2022

mmattel commented Jun 23, 2022

jvillafanez commented Jun 27, 2022

ownclouders commented Jun 27, 2022 • edited Loading

jvillafanez commented Jun 29, 2022

phil-davis commented Jun 29, 2022

phil-davis left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 30, 2022

mmattel commented Jul 8, 2022

jvillafanez commented Mar 17, 2022 •

edited by phil-davis

Loading

phil-davis commented Mar 21, 2022 •

edited

Loading

jvillafanez Mar 22, 2022 •

edited

Loading

ownclouders commented Jun 27, 2022 •

edited

Loading