Skip to content
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

Jump to bottom

fix: simplify mime-type checks to support jpg and other image formats #2401

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: simplify mime-type checks to support jpg and other image formats #2401

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
14 changes: 7 additions & 7 deletions lib/Controller/ApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,7 @@
$this->logger->debug('This form is archived and can not be modified');
throw new OCSForbiddenException();
}

try {
$question = $this->questionMapper->findById($questionId);
} catch (IMapperException $e) {
Expand Down Expand Up @@ -899,7 +899,7 @@
$this->logger->debug('This form is archived and can not be modified');
throw new OCSForbiddenException();
}

try {
$option = $this->optionMapper->findById($optionId);
$question = $this->questionMapper->findById($questionId);
Expand Down Expand Up @@ -961,14 +961,14 @@
$this->logger->debug('The given array contains duplicates');
throw new OCSBadRequestException('The given array contains duplicates');
}

$options = $this->optionMapper->findByQuestion($questionId);

if (sizeof($options) !== sizeof($newOrder)) {
$this->logger->debug('The length of the given array does not match the number of stored options');
throw new OCSBadRequestException('The length of the given array does not match the number of stored options');
}

$options = []; // Clear Array of Entities
$response = []; // Array of ['optionId' => ['order' => newOrder]]

Expand Down Expand Up @@ -1011,7 +1011,7 @@
}

$this->formMapper->update($form);

return new DataResponse($response);
}

Expand Down Expand Up @@ -1365,7 +1365,7 @@

$valid = false;
foreach ($extraSettings['allowedFileTypes'] ?? [] as $allowedFileType) {
if (str_starts_with($aliases[$mimeType] ?? '', $allowedFileType)) {
if (str_starts_with($mimeType, $allowedFileType) || str_starts_with($aliases[$mimeType] ?? '', $allowedFileType)) {

Check warning on line 1368 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1368 

Added line #L1368 was not covered by tests
$valid = true;
break;
}
Expand Down
16 changes: 6 additions & 10 deletions lib/Service/FormsService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\IMapperException;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\IMimeTypeDetector;
use OCP\Files\IRootFolder;
use OCP\Files\NotFoundException;
use OCP\IGroup;
Expand Down Expand Up @@ -72,7 +71,6 @@
private CirclesService $circlesService,
private IRootFolder $rootFolder,
private IL10N $l10n,
private IMimeTypeDetector $mimeTypeDetector,
private IEventDispatcher $eventDispatcher,
) {
$this->currentUser = $userSession->getUser();
Expand Down Expand Up @@ -124,10 +122,9 @@
$question['accept'] = [];
if ($question['type'] === Constants::ANSWER_TYPE_FILE) {
if ($question['extraSettings']['allowedFileTypes'] ?? null) {
$question['accept'] = array_keys(array_intersect(
$this->mimeTypeDetector->getAllAliases(),
$question['extraSettings']['allowedFileTypes']
));
$question['accept'] = array_map(function (string $fileType) {
return str_contains($fileType, '/') ? $fileType : $fileType . '/*';
}, $question['extraSettings']['allowedFileTypes']);
}

if ($question['extraSettings']['allowedFileExtensions'] ?? null) {
Expand Down Expand Up @@ -161,10 +158,9 @@
$question['accept'] = [];
if ($question['type'] === Constants::ANSWER_TYPE_FILE) {
if ($question['extraSettings']['allowedFileTypes'] ?? null) {
$question['accept'] = array_keys(array_intersect(
$this->mimeTypeDetector->getAllAliases(),
$question['extraSettings']['allowedFileTypes']
));
$question['accept'] = array_map(function (string $fileType) {
return str_contains($fileType, '/') ? $fileType : $fileType . '/*';
}, $question['extraSettings']['allowedFileTypes']);

Check warning on line 163 in lib/Service/FormsService.php

View check run for this annotation

Codecov / codecov/patch

lib/Service/FormsService.php#L161-L163 

Added lines #L161 - L163 were not covered by tests
}

if ($question['extraSettings']['allowedFileExtensions'] ?? null) {
Expand Down
19 changes: 2 additions & 17 deletions tests/Unit/Service/FormsServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ function microtime(bool|float $asFloat = false) {
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Folder;
use OCP\Files\IMimeTypeDetector;
use OCP\Files\IRootFolder;
use OCP\Files\NotFoundException;
use OCP\IGroup;
Expand Down Expand Up @@ -124,9 +123,6 @@ class FormsServiceTest extends TestCase {
/** @var IL10N|MockObject */
private $l10n;

/** @var IMimeTypeDetector|MockObject */
private $mimeTypeDetector;

public function setUp(): void {
parent::setUp();
$this->activityManager = $this->createMock(ActivityManager::class);
Expand Down Expand Up @@ -160,8 +156,6 @@ public function setUp(): void {
return $identity;
}));

$this->mimeTypeDetector = $this->createMock(IMimeTypeDetector::class);

$this->formsService = new FormsService(
$userSession,
$this->activityManager,
Expand All @@ -177,7 +171,6 @@ public function setUp(): void {
$this->circlesService,
$this->storage,
$this->l10n,
$this->mimeTypeDetector,
\OCP\Server::get(IEventDispatcher::class),
);
}
Expand Down Expand Up @@ -653,7 +646,6 @@ public function testGetPermissions_NotLoggedIn() {
$this->circlesService,
$this->storage,
$this->l10n,
$this->mimeTypeDetector,
\OCP\Server::get(IEventDispatcher::class),
);

Expand Down Expand Up @@ -893,7 +885,6 @@ public function testPublicCanSubmit() {
$this->circlesService,
$this->storage,
$this->l10n,
$this->mimeTypeDetector,
\OCP\Server::get(IEventDispatcher::class),
);

Expand Down Expand Up @@ -1005,7 +996,6 @@ public function testHasUserAccess_NotLoggedIn() {
$this->circlesService,
$this->storage,
$this->l10n,
$this->mimeTypeDetector,
\OCP\Server::get(IEventDispatcher::class),
);

Expand Down Expand Up @@ -1237,7 +1227,6 @@ public function testNotifyNewSubmission($shares, $shareNotifications) {
$this->circlesService,
$this->storage,
$this->l10n,
$this->mimeTypeDetector,
$eventDispatcher,
])
->getMock();
Expand Down Expand Up @@ -1496,22 +1485,18 @@ public function testGetQuestionsWithVariousQuestionTypes(): void {
$this->createQuestionEntity(['id' => 1, 'type' => 'text']),
$this->createQuestionEntity(['id' => 2, 'type' => Constants::ANSWER_TYPE_FILE, 'extraSettings' => [
'allowedFileTypes' => ['image', 'x-office/document'],
'allowedFileExtensions' => ['jpg']
'allowedFileExtensions' => ['pdf']
]])
];

$this->questionMapper->method('findByForm')->willReturn($questionEntities);
$this->mimeTypeDetector->method('getAllAliases')->willReturn([
'application/coreldraw' => 'image',
'application/msonenote' => 'x-office/document',
]);

$result = $this->formsService->getQuestions(1);

$this->assertCount(2, $result);
$this->assertEquals('text', $result[0]['type']);
$this->assertEquals(Constants::ANSWER_TYPE_FILE, $result[1]['type']);
$this->assertEquals(['application/coreldraw', 'application/msonenote', '.jpg'], $result[1]['accept']);
$this->assertEquals(['image/*', 'x-office/document', '.pdf'], $result[1]['accept']);
}

public function testGetQuestionsHandlesDoesNotExistException(): void {
Expand Down
Loading