Skip to content

Fix/57375 zero quota external storage folder delete #59986

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.

Sign up for GitHub

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

Open
huntervcx wants to merge 1 commit into
base: master
Choose a base branch
from
+67 −9
Open

Fix/57375 zero quota external storage folder delete #59986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions lib/private/Files/Storage/Wrapper/Quota.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ public function file_put_contents(string $path, mixed $data): int|float|false {
return $this->getWrapperStorage()->file_put_contents($path, $data);
}
$free = $this->free_space($path);
if ($free < 0 || strlen($data) < $free) {
// Only apply quota for files under the user's "files/" tree.
// Writes to metadata locations (files_trashbin/, files_versions/, ...)
// must not be blocked, otherwise features like the trashbin break
// for users whose quota happens to be exhausted (notably quota=0).
if ($free < 0 || !$this->shouldApplyQuota($path) || strlen($data) < $free) {
return $this->getWrapperStorage()->file_put_contents($path, $data);
} else {
return false;
Expand All @@ -113,7 +117,7 @@ public function copy(string $source, string $target): bool {
return $this->getWrapperStorage()->copy($source, $target);
}
$free = $this->free_space($target);
if ($free < 0 || $this->getSize($source) < $free) {
if ($free < 0 || !$this->shouldApplyQuota($target) || $this->getSize($source) < $free) {
return $this->getWrapperStorage()->copy($source, $target);
} else {
return false;
Expand Down Expand Up @@ -167,7 +171,12 @@ public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalP
return $this->getWrapperStorage()->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}
$free = $this->free_space($targetInternalPath);
if ($free < 0 || $this->getSize($sourceInternalPath, $sourceStorage) < $free) {
// Skip the quota check when the target lives outside of "files/"
// (e.g. files_trashbin/, files_versions/). This is essential so that
// the trashbin can store deleted items even when the user's quota is
// fully consumed: otherwise DELETE operations on external mounts fail
// with HTTP 403 because the move-to-trash copy returns false.
if ($free < 0 || !$this->shouldApplyQuota($targetInternalPath) || $this->getSize($sourceInternalPath, $sourceStorage) < $free) {
return $this->getWrapperStorage()->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
} else {
return false;
Expand All @@ -180,7 +189,7 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP
return $this->getWrapperStorage()->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
}
$free = $this->free_space($targetInternalPath);
if ($free < 0 || $this->getSize($sourceInternalPath, $sourceStorage) < $free) {
if ($free < 0 || !$this->shouldApplyQuota($targetInternalPath) || $this->getSize($sourceInternalPath, $sourceStorage) < $free) {
return $this->getWrapperStorage()->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
} else {
return false;
Expand All @@ -206,7 +215,9 @@ public function touch(string $path, ?int $mtime = null): bool {
return $this->getWrapperStorage()->touch($path, $mtime);
}
$free = $this->free_space($path);
if ($free == 0) {
// Same rule as the other write paths: only block when the target is
// actually under the user-quota controlled "files/" tree.
if ($free == 0 && $this->shouldApplyQuota($path)) {
return false;
}

Expand All @@ -219,12 +230,12 @@ public function enableQuota(bool $enabled): void {

#[\Override]
public function writeStream(string $path, $stream, ?int $size = null): int {
if (!$this->hasQuota()) {
if (!$this->hasQuota() || !$this->shouldApplyQuota($path)) {
return parent::writeStream($path, $stream, $size);
}

$free = $this->free_space($path);
if ($this->shouldApplyQuota($path) && $free == 0) {
if ($free == 0) {
throw new NotEnoughSpaceException();
}

Expand Down
51 changes: 49 additions & 2 deletions tests/lib/Files/Storage/Wrapper/QuotaTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,13 @@ public function testMkdirQuotaZeroTrashbin(): void {
$this->assertTrue($instance->mkdir('cache'));
}

public function testNoTouchQuotaZero(): void {
public function testTouchBlockedUnderFilesWhenQuotaIsZero(): void {
$instance = $this->getLimitedStorage(0.0);
$this->assertFalse($instance->touch('foobar'));
// touch is blocked only for paths under files/ (user quota area)
$this->assertFalse($instance->touch('files/foobar'));
// touch outside files/ (trashbin, versions, ...) must remain allowed
$this->assertTrue($instance->mkdir('files_trashbin'));
$this->assertTrue($instance->touch('files_trashbin/foobar'));
}

public function testNoFopenQuotaZero(): void {
Expand Down Expand Up @@ -256,4 +260,47 @@ public function testNoWriteStreamQuotaZero(): void {
$this->expectException(Files\NotEnoughSpaceException::class);
$instance->writeStream('files/test.txt', $stream);
}

/**
* writeStream outside of files/ (trashbin, versions, ...) must succeed
* even when the user quota is exhausted.
*/
public function testWriteStreamAllowedOutsideFilesWhenQuotaIsZero(): void {
$instance = $this->getLimitedStorage(0.0);
$this->assertTrue($instance->mkdir('files_trashbin'));
$stream = fopen('php://temp', 'w+');
fwrite($stream, 'foo');
rewind($stream);
$this->assertEquals(3, $instance->writeStream('files_trashbin/test.txt', $stream));
}

/**
* Writes under "files/" must still be blocked when quota is 0, but writes
* outside that prefix (trashbin metadata, versions, ...) must not be
* blocked, otherwise features like the trashbin break for users whose
* quota happens to be exhausted (notably quota=0).
*/
public function testFilePutContentsBlockedUnderFilesWhenQuotaIsZero(): void {
$instance = $this->getLimitedStorage(0.0);
$this->assertFalse($instance->file_put_contents('files/foo', 'x'));
$this->assertTrue($instance->mkdir('files_trashbin'));
$this->assertNotFalse($instance->file_put_contents('files_trashbin/foo.json', '{}'));
}

/**
* Copying from another storage (e.g. an external SMB mount) into the
* trashbin must succeed even when the quota is exhausted: this is what
* happens on every DELETE when files_trashbin is enabled. Conversely,
* copying into "files/" must still be blocked.
*/
public function testCopyFromStorageAllowedToTrashbinWhenQuotaIsZero(): void {
$source = new Local(['datadir' => $this->tmpDir]);
$source->file_put_contents('source.txt', 'hello');

$instance = $this->getLimitedStorage(0.0);
$this->assertTrue($instance->mkdir('files_trashbin'));
$this->assertTrue($instance->copyFromStorage($source, 'source.txt', 'files_trashbin/source.txt'));

$this->assertFalse($instance->copyFromStorage($source, 'source.txt', 'files/source.txt'));
}
}