upstream/nextcloud
1
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2026-02-19 02:38:40 -05:00
Code Issues Projects Releases Packages Wiki Activity Actions 11

fix(files): more conversion tests and translate error messages

Browse source 
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
This commit is contained in:
skjnldsv 2025-01-17 17:28:38 +01:00
parent 6afe12593e
commit abd3cb60fc
3 changed files with 66 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
apps/files/lib/Controller/ConversionApiController.php
View file

@ -10,17 +10,20 @@ declare(strict_types=1);
namespace OCA\Files\Controller;
use OC\Files\Utils\PathHelper;
use OC\ForbiddenException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\ApiRoute;
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\Attribute\UserRateLimit;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCS\OCSBadRequestException;
use OCP\AppFramework\OCS\OCSException;
use OCP\AppFramework\OCS\OCSForbiddenException;
use OCP\AppFramework\OCS\OCSNotFoundException;
use OCP\AppFramework\OCSController;
use OCP\Files\Conversion\IConversionManager;
use OCP\Files\File;
use OCP\Files\GenericFileException;
use OCP\Files\IRootFolder;
use OCP\IL10N;
use OCP\IRequest;
@ -59,7 +62,8 @@ class ConversionApiController extends OCSController {
$userFolder = $this->rootFolder->getUserFolder($this->userId);
$file = $userFolder->getFirstNodeById($fileId);
if (!($file instanceof File)) {
// Also throw a 404 if the file is not readable to not leak information
if (!($file instanceof File) || $file->isReadable() === false) {
throw new OCSNotFoundException($this->l10n->t('The file cannot be found'));
}
@ -72,7 +76,7 @@ class ConversionApiController extends OCSController {
}
if (!$userFolder->get($parentDir)->isCreatable()) {
throw new OCSForbiddenException();
throw new OCSForbiddenException($this->l10n->t('You do not have permission to create a file at the specified location'));
}
$destination = $userFolder->getFullPath($destination);
@ -80,6 +84,10 @@ class ConversionApiController extends OCSController {
try {
$convertedFile = $this->fileConversionManager->convert($file, $targetMimeType, $destination);
} catch (ForbiddenException $e) {
throw new OCSForbiddenException($e->getMessage());
} catch (GenericFileException $e) {
throw new OCSBadRequestException($e->getMessage());
} catch (\Exception $e) {
logger('files')->error($e->getMessage(), ['exception' => $e]);
throw new OCSException($this->l10n->t('The file could not be converted.'));

21
build/integration/file_conversions/file_conversions.feature
View file

@ -76,6 +76,27 @@ Feature: conversions
Then as "user0" the file "/image.jpg" exists
Then as "user0" the file "/image.png" does not exist
Scenario: Converting a file to a given path without extension fails
Given user "user0" uploads file "data/clouds.jpg" to "/image.jpg"
And User "user0" created a folder "/folder"
Then as "user0" the file "/image.jpg" exists
Then as "user0" the folder "/folder" exists
When user "user0" converts file "/image.jpg" to "image/png" and saves it to "/folder/image"
Then the HTTP status code should be "400"
Then the OCS status code should be "400"
Then as "user0" the file "/folder/image.png" does not exist
Then as "user0" the file "/image.png" does not exist
@local_storage
Scenario: Converting a file bigger than 100 MiB fails
Given file "/image.jpg" of size 108003328 is created in local storage
Then as "user0" the folder "/local_storage" exists
Then as "user0" the file "/local_storage/image.jpg" exists
When user "user0" converts file "/local_storage/image.jpg" to "image/png" and saves it to "/image.png"
Then the HTTP status code should be "400"
Then the OCS status code should be "400"
Then as "user0" the file "/image.png" does not exist
Scenario: Forbid conversion to a destination without create permission
Given user "user1" exists
# Share the folder with user1

44
lib/private/Files/Conversion/ConversionManager.php
View file

@ -10,13 +10,16 @@ declare(strict_types=1);
namespace OC\Files\Conversion;
use OC\AppFramework\Bootstrap\Coordinator;
use OC\ForbiddenException;
use OC\SystemConfig;
use OCP\Files\Conversion\IConversionManager;
use OCP\Files\Conversion\IConversionProvider;
use OCP\Files\File;
use OCP\Files\GenericFileException;
use OCP\Files\IRootFolder;
use OCP\IL10N;
use OCP\ITempManager;
use OCP\L10N\IFactory;
use OCP\PreConditionNotMetException;
use Psr\Container\ContainerExceptionInterface;
use Psr\Container\ContainerInterface;
@ -37,6 +40,7 @@ class ConversionManager implements IConversionManager {
/** @var list<IConversionProvider> */
private array $providers = [];
private IL10N $l10n;
public function __construct(
private Coordinator $coordinator,
private ContainerInterface $serverContainer,
@ -44,7 +48,9 @@ class ConversionManager implements IConversionManager {
private ITempManager $tempManager,
private LoggerInterface $logger,
private SystemConfig $config,
IFactory $l10nFactory,
) {
$this->l10n = $l10nFactory->get('files');
}
public function hasProviders(): bool {
@ -62,22 +68,21 @@ class ConversionManager implements IConversionManager {
public function convert(File $file, string $targetMimeType, ?string $destination = null): string {
if (!$this->hasProviders()) {
throw new PreConditionNotMetException('No file conversion providers available');
throw new PreConditionNotMetException($this->l10n->t('No file conversion providers available'));
}
// Operate in mebibytes
$fileSize = $file->getSize() / (1024 * 1024);
$threshold = $this->config->getValue('max_file_conversion_filesize', 100);
if ($fileSize > $threshold) {
throw new GenericFileException('File is too large to convert');
throw new GenericFileException($this->l10n->t('File is too large to convert'));
}
$fileMimeType = $file->getMimetype();
$validProvider = $this->getValidProvider($fileMimeType, $targetMimeType);
if ($validProvider !== null) {
$convertedFile = $validProvider->convertFile($file, $targetMimeType);
// Get the target extension given by the provider
$targetExtension = '';
foreach ($validProvider->getSupportedMimeTypes() as $mimeProvider) {
if ($mimeProvider->getTo() === $targetMimeType) {
@ -85,7 +90,7 @@ class ConversionManager implements IConversionManager {
break;
}
}
// If destination not provided, we use the same path
// as the original file, but with the new extension
if ($destination === null) {
@ -94,11 +99,21 @@ class ConversionManager implements IConversionManager {
$destination = $parent->getFullPath($basename . '.' . $targetExtension);
}
// If destination doesn't match the target extension, we throw an error
if (pathinfo($destination, PATHINFO_EXTENSION) !== $targetExtension) {
throw new GenericFileException($this->l10n->t('Destination does not match conversion extension'));
}
// Check destination before converting
$this->checkDestination($destination);
// Convert the file and write it to the destination
$convertedFile = $validProvider->convertFile($file, $targetMimeType);
$convertedFile = $this->writeToDestination($destination, $convertedFile);
return $convertedFile->getPath();
}
throw new RuntimeException('Could not convert file');
throw new RuntimeException($this->l10n->t('Could not convert file'));
}
/**
@ -127,14 +142,25 @@ class ConversionManager implements IConversionManager {
return array_values(array_merge([], $this->preferredProviders, $this->providers));
}
private function checkDestination(string $destination): void {
if (!$this->rootFolder->nodeExists(dirname($destination))) {
throw new ForbiddenException($this->l10n->t('Destination does not exist'));
}
$folder = $this->rootFolder->get(dirname($destination));
if (!$folder->isCreatable()) {
throw new ForbiddenException($this->l10n->t('Destination is not creatable'));
}
}
private function writeToDestination(string $destination, mixed $content): File {
$this->checkDestination($destination);
if ($this->rootFolder->nodeExists($destination)) {
$file = $this->rootFolder->get($destination);
$parent = $file->getParent();
if (!$parent->isCreatable()) {
throw new GenericFileException('Destination is not creatable');
}
// Folder permissions is already checked in checkDestination method
$newName = $parent->getNonExistingName(basename($destination));
$destination = $parent->getFullPath($newName);
}