[full-ci] Prevent creation of empty files/folders for accounts having no available quota #40567

pako81 · 2023-01-09T10:50:48Z

Description

Prevent creation of empty files/folders for accounts having no available quota

Related Issue

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

Motivation and Context

Until now it was possible for users having 0 quota or who already reached the limit of their assigned quota to still create empty files/folders, which generates confusion. The PR fixes this behaviour.

How Has This Been Tested?

Manually:

Test 1. :

as admin user admin assign quota 0 B to user user1
login as user user1 over WebUI and try to create empty files or folders: an error Could not create.. is returned

Test 2. :

as user1 who has a valid quota assigned create a folder for guests and share it with full permissions with guest user guest@owncloud.com
as admin user admin assign now quota 0 B to user user1
login as guest user guest@owncloud.com, enter the folder for guests and try to create empty files or (sub)folders: an error Could not create.. is returned

Test 3. :

Same steps as Test 1. and 2. but this time with user1 having i.e. 10MB quota assigned and already using 100% of the available space

Test 4. :

as admin user admin assign quota 0 B to user user1
connect user1 via desktop client to his personal storage: sync client returns a 403 Forbidden to MKCOL and PUT operations when trying to respectively create empty folders and files.

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:

pako81 · 2023-01-09T11:13:05Z

Actually using === as per #40505 (review) does not seem to make this work correctly.

ownclouders · 2023-01-09T11:28:07Z

💥 Acceptance tests pipeline apiFilesExt-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/37747/89

jvillafanez · 2023-01-09T11:45:12Z

Actually using === as per #40505 (review) does not seem to make this work correctly.

Double-check the type because the free_space method in

core/lib/private/legacy/helper.php

Line 613 in c4f8cd1

$free = $sourceStorage->free_space('');

could return false.
It's probably better to handle the false case there, before the calculations.

pako81 · 2023-01-11T16:25:58Z

From what I can see, this is rather because https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Local.php#L314 returns float (verified by using gettype on https://github.com/owncloud/core/blob/master/lib/private/legacy/helper.php#L613).

So converting it to int seems to make this work correctly when using ===. Not sure this is the right approach however.

jvillafanez · 2023-01-11T17:17:16Z

The interface says it should return either an int or false, so if it's returning a float then, technically, it's a bug in the implementation.
This means that the local storage is responsible of converting the disk_free_space result to int to be compliant with the interface.

I don't know why the disk_free_space returns a float and not an int, but we might need to take care of the conversion to int. Probably something like:

$space = @\disk_free_space($sourcePath);
if ($space === false || $space === null) {
  return \OCP\Files\FileInfo::SPACE_UNKNOWN;
}
$space = \intval($space);
if ($space === PHP_INT_MAX) {
  return \OCP\Files\FileInfo::SPACE_UNKNOWN;
}
return $space;

The alternative would be to change the interface so it returns a float instead of an int, but we'll have to check that using a float is properly supported in all the places where this method is used. Seems a lot of work.

pako81 · 2023-01-11T22:06:01Z

I was actually wrong, the float (double) type seems rather to come from $this->quota in https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Quota.php#L96.

I've therefore added the conversion to int there, before returning the free space as limited by the quota.

jvillafanez · 2023-01-12T08:32:07Z

The problem might not be really in the wrapper but something inside. When the wrapper is created, the quota is gotten from

core/lib/private/legacy/util.php

Line 375 in e4c8e50

return OC_Helper::computerFileSize($userQuota);

, and the computerFileSize could be a float (the returning type hint of the computerFileSize method is also wrong because it can return float, int and false).

Note that the problem with the disk_free_space function is still present.

At this point, it might be easier to use == instead and write a comment pointing to all these issues we're finding. I don't think we're using strings or other data types around there that could mess up with the behavior.

jvillafanez · 2023-01-23T08:19:51Z

Need to check what happens with the tests

IljaN · 2023-01-24T10:24:04Z

Wonder if this could have potential side-effects/edge-cases:

Creating user home after first login
Creating skeleton files

pako81 · 2023-01-24T12:00:57Z

Wonder if this could have potential side-effects/edge-cases:
Creating user home after first login
Creating skeleton files

User home seems to be correctly created, I guess because it gets initialized at an early stage as to what we are checking now. However, this seems indeed to break the skeleton files creation as folders' structure gets correctly created but with empty files inside, which looks bad.

I wonder however which use case would bite us here..I guess only in case the Admin creates DB user having 0 quota assigned or in case of newly created LDAP users where the default quota / quota attribute is set to 0. Both seem unlikely to happen.

IljaN · 2023-01-24T12:52:00Z

However, this seems indeed to break the skeleton files creation as folders' structure gets correctly created but with empty files inside, which looks bad.

This would be okay as this is also the current behaviour. I was fearing that the exception might interrupt some process, but I guess it's fine if it is implemented in the DAV-Layer instead of in the Quota Wrapper.

jvillafanez · 2023-01-24T13:01:37Z

I think the "block" only happens when accessing through webdav because the check are placed in our dav connector. Access through other channels (view, nodes, storage, etc) would still allow to create folders. However, the file quota applies at stream level.

Basically, when the skeleton files are copied, the folders are created because we aren't accessing through webdav, but the files can't be written due to the quota being applied in the stream.

I assume this is an acceptable behavior. Note that apps could still create folders by themselves in the FS following the same mechanism that core is using.

pako81 · 2023-01-24T16:02:22Z

Changed it to return Sabre\DAV\Exception\InsufficientStorage to be consistent with what clients are expecting.

pako81 · 2023-01-25T10:11:10Z

@jvillafanez do you maybe have an idea why https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiMain/external-storage.feature#L89 is currently failing? AFAICT uploading to external storages when no quota is set should still work (and it still does with this PR), so not sure why the test expects now a 507 instead of a 201.

jvillafanez · 2023-01-25T13:02:01Z

I think it's because of the new check you've set for the file.
I assume that it worked before because the quota was controlled in the stream, and for external storages maybe the quota wrapper isn't set. With your new check, you might be uploading a 5-bytes file having a quota of 1 byte, and your check applies to all storages, including external ones.

phil-davis · 2023-01-26T09:36:50Z

This branch is 10 commits ahead, 100 commits behind owncloud:master.

https://github.com/pako81/core/tree/prevent_empty_folders

This is now a long way behind master - it would be good to rebase and get fresh CI results. I put full-ci in the PR title so that a full set of results will come when you rebase.

phil-davis · 2023-01-27T04:38:44Z

https://drone.owncloud.com/owncloud/core/37767/41/13

  Scenario: user cannot create file on shared folder by a user with zero quota                                                   # /drone/src/tests/acceptance/features/apiMain/quota.feature:225
    Given the administrator has set the default folder for received shares to "Shares"                                           # OccContext::theAdministratorHasSetTheDefaultFolderForReceivedSharesTo()
    And auto-accept shares has been disabled                                                                                     # FeatureContext::autoAcceptSharesHasBeenDisabled()
    And user "Brian" has been created with default attributes and without skeleton files                                         # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
    And the quota of user "Brian" has been set to "0 B"                                                                          # FeatureContext::theQuotaOfUserHasBeenSetTo()
    And the quota of user "Alice" has been set to "10 MB"                                                                        # FeatureContext::theQuotaOfUserHasBeenSetTo()
    And user "Brian" has created folder "shareFolder"                                                                            # FeatureContext::userHasCreatedFolder()
      HTTP status code was not 201 or 204 while trying to create folder 'shareFolder' for user 'Brian'
      Failed asserting that an array contains 507.

https://drone.owncloud.com/owncloud/core/37767/90/13

  Scenario: Upload a file to external storage while quota is set on home storage                                                                               # /drone/src/tests/acceptance/features/apiMain/external-storage.feature:89
    Given user "Alice" has been created with default attributes and small skeleton files                                                                       # FeatureContext::userHasBeenCreatedWithDefaultAttributes()
    And the quota of user "Alice" has been set to "1 B"                                                                                                        # FeatureContext::theQuotaOfUserHasBeenSetTo()
    When user "Alice" uploads file "filesForUpload/textfile.txt" to filenames based on "/local_storage/testquota.txt" with all mechanisms using the WebDAV API # FeatureContext::userUploadsAFileToWithAllMechanisms()
    Then the HTTP status code of all upload responses should be "201"                                                                                          # FeatureContext::theHTTPStatusCodeOfAllUploadResponsesShouldBe()
      Response did not return expected status code
      Failed asserting that 507 matches expected 201.

I will adjust those tests so that they expect the new behavior...

phil-davis · 2023-01-27T06:07:44Z

https://drone.owncloud.com/owncloud/core/37769/90/13

  Scenario: Cannot upload a file to external storage while quota is too low                                                                                    # /drone/src/tests/acceptance/features/apiMain/external-storage.feature:89
    Given user "Alice" has been created with default attributes and small skeleton files                                                                       # FeatureContext::userHasBeenCreatedWithDefaultAttributes()
    And the quota of user "Alice" has been set to "1 B"                                                                                                        # FeatureContext::theQuotaOfUserHasBeenSetTo()
    When user "Alice" uploads file "filesForUpload/textfile.txt" to filenames based on "/local_storage/testquota.txt" with all mechanisms using the WebDAV API # FeatureContext::userUploadsAFileTothAllMechanisms()
    Then the HTTP status code of all upload responses should be "507"                                                                                          # FeatureContext::theHTTPStatusCodeOfAllUploadResponsesShouldBe()
      Response did not return expected status code
      Failed asserting that 201 matches expected 507.

I need to look again at exactly what happens when a user with not enough quota uploads a file to a destination that is in an "external storage" (rather than in their own storage, or into a received share that is someone else's storage)

pako81 · 2023-01-27T07:15:31Z

I need to look again at exactly what happens when a user with not enough quota uploads a file to a destination that is in an "external storage" (rather than in their own storage, or into a received share that is someone else's storage)

Upload should succeed as "external storages" are not counted against user's quota.

decouples test from implementation details

pako81 · 2023-02-14T15:24:33Z

@jvillafanez would be fine like this? Apparently, we now have some acceptance tests related to the new chunking algorithm which are failing.

jvillafanez · 2023-02-14T16:11:06Z

Technically yes. We need to check if there is any side effect. Not sure if the failing tests are related to that change or not.

phil-davis · 2023-02-15T10:01:38Z

https://drone.owncloud.com/owncloud/core/37947/41/13

runsh: Total unexpected failed scenarios throughout the test run:
apiMain/checksums.feature:192
apiMain/checksums.feature:208
apiMain/checksums.feature:226
apiMain/checksums.feature:355
apiMain/checksums.feature:366

  Scenario: Upload new DAV chunked file using async MOVE where checksum matches                                                                                                           # /drone/src/tests/acceptance/features/apiMain/checksums.feature:192
    Given using new DAV path                                                                                                                                                              # FeatureContext::usingOldOrNewDavPath()
    And the administrator has enabled async operations                                                                                                                                    # FeatureContext::triggerAsyncUpload()
    And user "Alice" has created a new chunking upload with id "chunking-42"                                                                                                              # FeatureContext::userHasCreatedANewChunkingUploadWithId()
      The HTTP status code 405 is not between 200 and 299
      Failed asserting that 405 is equal to 299 or is less than 299.

The various failing test scenarios are when Alice starts a new chunking upload.

Similar for https://drone.owncloud.com/owncloud/core/37947/88/13 and https://drone.owncloud.com/owncloud/core/37947/89/13

It looks like the code changes are now rejecting some chunked uploads with 405 "method not allowed"

IljaN · 2023-02-15T20:28:43Z

Debugged this a bit w @pako81. Somehow the server thinks that the chunking directory does already exist. But the folder seems not to be created in the storage.

jvillafanez · 2023-02-16T08:30:58Z

You'll probably need to ensure the mounted folder exists before returning the mount, the same way it's done with a custom dav.chunk_base_dir

pako81 · 2023-02-22T10:08:44Z

We are back to the case where only the Upload a file to external storage while quota is set on home storage (/drone/src/tests/acceptance/features/apiMain/external-storage.feature:89) acceptance test is failing --> https://drone.owncloud.com/owncloud/core/37995/90/13

@jvillafanez we still have the issue that something seems to be not correct when returning the datadir mountpoint in ChunkLocationProvider.php in case dav.chunk_base_dir is not set.

jvillafanez · 2023-02-22T11:38:28Z

apps/dav/lib/Upload/ChunkLocationProvider.php

+				\mkdir($cacheDir, 0770, true);
+			}
+			return [
+				new MountPoint(Local::class, $cacheDir, ['datadir' => $dataDir, $loader])


new MountPoint(Local::class, $cacheDir, ['datadir' => $cacheDir, $loader])

It should be 'datadir' => $cacheDir instead of 'datadir' => $dataDir. I don't know if this is causing the error in the tests

Yes, initially tried with 'datadir' => $cacheDir as well - no difference. The test still fails locally for me with:

Scenario: Upload a file to external storage while quota is set on home storage # /home/core/tests/acceptance/features/apiMain/external-storage.feature:89 Given user "Alice" has been created with default attributes and small skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributes() And the quota of user "Alice" has been set to "1 B" # FeatureContext::theQuotaOfUserHasBeenSetTo() When user "Alice" uploads file "filesForUpload/textfile.txt" to filenames based on "/local_storage/testquota.txt" with all mechanisms using the WebDAV API # FeatureContext::userUploadsAFileToWithAllMechanisms() Then the HTTP status code of all upload responses should be "201" # FeatureContext::theHTTPStatusCodeOfAllUploadResponsesShouldBe() Response did not return expected status code Failed asserting that 507 matches expected 201. And as "Alice" the files uploaded to "/local_storage/testquota.txt" with all mechanisms should exist # FeatureContext::filesUploadedToWithAllMechanismsShouldExist()

Which is because we are still getting through

core/apps/dav/lib/Connector/Sabre/Directory.php

Line 225 in 4f60253

throw new SabreInsufficientStorage('Creation of empty directories is forbidden in case of no available quota');

so apparently the target storage is still seen of type Home.

The other, simplified, approach which had all tests passing was b8e3458, where ChunkLocationProvider.php was not touched.

Isn't this because the class Home is extending the Local one

https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Home.php#L32

so basically there is no way to differentiate in this case and we always end up at not passing this check

core/apps/dav/lib/Connector/Sabre/Directory.php

Line 224 in 4f60253

if ($free == 0 && ($targetStorage->instanceOfStorage('\OCP\Files\IHomeStorage') === true)) {

I don't think it's like that. A local storage isn't a home storage.

It might be that we're pointing exactly at the root of the mount, so maybe the target storage is the home storage with a relative path of /user/files/uploads instead of what we're expecting (local storage with relative path of '' - basically the root of the mount)

sonarcloud · 2023-02-23T23:21:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

60.4% Coverage
2.4% Duplication

pako81 · 2023-02-24T08:28:07Z

@jvillafanez @phil-davis should we go ahead and merge this?

jvillafanez

I assume it works, haven't tested it

pako81 · 2023-02-24T10:14:40Z

I have again manually tested all the steps from the top post with the new code in place and did not observe any unexpected behaviour.

individual-it · 2023-03-06T11:00:37Z

Tests with desktop client 3.2.0 on Windows 10:

create folder for a user that has 0 quota:
1. create a user with 0 quota
2. join the desktop client
3. create a folder on the desktop
  => message that it was not possible to sync 👍
  
  => red cross in the desktop client settings UI and taskbar 👍
  
  => Windows Explorer does not indicate a problem 👎
increase quota of a user that had no quota before
1. use the user of first test and change the quota to 100KB
2. force-sync the desktop client
  => new folder is created on the server 👍
overfill quota
1. use the user from test no 2
2. copy a couple of files that individually fit into the quota but in sum don't
  => some files are uploaded, others are not with the message that there is not sufficient quota 👍
create a folder when quota is almost full
1. use the user from test no 3
2. create a folder while the quota is nearly full (see test no 2)
  => folder is created and synced to the server 👍
reduce quota, so that its over 100% full and try to create a folder
1. use the user from test no 4
2. as admin change the quota of the user to 90kb
3. try to create a folder
  => same result as test no 1
upload to a share without quota
1. create a new user without quota
2. from this user share a folder to the user from test 5, who has limited quota and already exceeded it
3. as share receiver upload a file to the share
4. as share receiver create a folder in the share
  => both works and is synced to the server 👍
create a folder inside of a share that belongs to a user who has exceeded its quota
1. use both users of test no 6
2. limit the sharer, so that its quota is exceeded
3. as share receiver try to create a folder
  => same result at test no 1 👍

pako81 added the 3 - To Review label Jan 9, 2023

pako81 added this to the development milestone Jan 9, 2023

pako81 self-assigned this Jan 9, 2023

pako81 mentioned this pull request Jan 9, 2023

Prevent creation of empty files/folders for accounts having no available quota #40505

Closed

11 tasks

pako81 requested a review from jvillafanez January 9, 2023 10:57

owncloud deleted a comment from update-docs bot Jan 9, 2023

jvillafanez approved these changes Jan 23, 2023

View reviewed changes

IljaN self-requested a review January 24, 2023 10:24

IljaN approved these changes Jan 24, 2023

View reviewed changes

phil-davis changed the title ~~Prevent creation of empty files/folders for accounts having no available quota~~ [full-ci] Prevent creation of empty files/folders for accounts having no available quota Jan 26, 2023

phil-davis approved these changes Jan 27, 2023

View reviewed changes

Pasquale Tripodi and others added 4 commits February 13, 2023 20:39

fix style

ebbce26

adjust unit test

6f7bb53

Remove number of calls assertion

e4fa93c

decouples test from implementation details

fix test

3cbf767

Pasquale Tripodi added 5 commits February 21, 2023 15:57

Merge branch 'owncloud:master' into prevent_empty_folders

bca5178

fix getMountsForUser

a44d1be

fix getMountsForUser

093a9d6

fix getMountsForUser

8f8962a

fix getMountsForUser

4f60253

jvillafanez reviewed Feb 22, 2023

View reviewed changes

Pasquale Tripodi added 3 commits February 23, 2023 23:27

Merge branch 'owncloud:master' into prevent_empty_folders

42cc65e

adjust ChunkLocationProvider

8043a90

fix style

c903ea1

jvillafanez approved these changes Feb 24, 2023

View reviewed changes

pako81 merged commit 531f1a5 into owncloud:master Feb 24, 2023

pako81 deleted the prevent_empty_folders branch February 24, 2023 10:33

jesmrec mentioned this pull request Mar 1, 2023

[FEATURE REQUEST] Improve error when folder is created with quota over owncloud/android#3944

Closed

9 tasks

HanaGemela mentioned this pull request Mar 6, 2023

[QA] Windows Explorer does not indicate a problem when syncing a folder with 0 quota owncloud/client#10571

Closed

2 tasks

pako81 mentioned this pull request Mar 21, 2023

Use getHome in ChunkLocationProvider.php #40693

Merged

11 tasks

pako81 mentioned this pull request Aug 18, 2023

[QA] skeleton files are added as corrupt files, when the user was created with 0 B quota #40931

Closed

[full-ci] Prevent creation of empty files/folders for accounts having no available quota #40567

[full-ci] Prevent creation of empty files/folders for accounts having no available quota #40567

Conversation

pako81 commented Jan 9, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

pako81 commented Jan 9, 2023 • edited Loading

ownclouders commented Jan 9, 2023 • edited Loading

jvillafanez commented Jan 9, 2023

pako81 commented Jan 11, 2023 • edited Loading

jvillafanez commented Jan 11, 2023

pako81 commented Jan 11, 2023 • edited Loading

jvillafanez commented Jan 12, 2023

jvillafanez commented Jan 23, 2023

IljaN commented Jan 24, 2023 • edited Loading

pako81 commented Jan 24, 2023 • edited Loading

IljaN commented Jan 24, 2023 • edited Loading

jvillafanez commented Jan 24, 2023

pako81 commented Jan 24, 2023 • edited Loading

pako81 commented Jan 25, 2023 • edited Loading

jvillafanez commented Jan 25, 2023

phil-davis commented Jan 26, 2023

phil-davis commented Jan 27, 2023

phil-davis commented Jan 27, 2023

pako81 commented Jan 27, 2023

pako81 commented Feb 14, 2023

jvillafanez commented Feb 14, 2023

phil-davis commented Feb 15, 2023

IljaN commented Feb 15, 2023

jvillafanez commented Feb 16, 2023

pako81 commented Feb 22, 2023

jvillafanez Feb 22, 2023

Choose a reason for hiding this comment

pako81 Feb 22, 2023

Choose a reason for hiding this comment

pako81 Feb 22, 2023

Choose a reason for hiding this comment

pako81 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

jvillafanez Feb 23, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Feb 23, 2023

pako81 commented Feb 24, 2023

jvillafanez left a comment

Choose a reason for hiding this comment

pako81 commented Feb 24, 2023 • edited Loading

individual-it commented Mar 6, 2023

pako81 commented Jan 9, 2023 •

edited

Loading

pako81 commented Jan 9, 2023 •

edited

Loading

ownclouders commented Jan 9, 2023 •

edited

Loading

pako81 commented Jan 11, 2023 •

edited

Loading

pako81 commented Jan 11, 2023 •

edited

Loading

IljaN commented Jan 24, 2023 •

edited

Loading

pako81 commented Jan 24, 2023 •

edited

Loading

IljaN commented Jan 24, 2023 •

edited

Loading

pako81 commented Jan 24, 2023 •

edited

Loading

pako81 commented Jan 25, 2023 •

edited

Loading

pako81 Feb 23, 2023 •

edited

Loading

pako81 commented Feb 24, 2023 •

edited

Loading