Skip to content

Commit f400835

Browse files
mickenordinmickenordin
committed
test: cover upload authorization for conditional answers
Signed-off-by: Micke Nordin <kano@sunet.se>
1 parent d1a7ba7 commit f400835

2 files changed

Lines changed: 289 additions & 1 deletion

File tree

‎tests/Unit/Controller/ApiControllerTest.php‎

Lines changed: 181 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ function is_uploaded_file(string|bool|null $filename) {
3232
use OCA\Forms\BackgroundJob\SyncSubmissionsWithLinkedFileJob;
3333
use OCA\Forms\Constants;
3434
use OCA\Forms\Controller\ApiController;
35+
use OCA\Forms\Db\Answer;
3536
use OCA\Forms\Db\AnswerMapper;
3637
use OCA\Forms\Db\Form;
3738
use OCA\Forms\Db\FormMapper;
@@ -92,6 +93,8 @@ class ApiControllerTest extends TestCase {
9293
private ISecureRandom|MockObject $secureRandom;
9394

9495
public function setUp(): void {
96+
parent::setUp();
97+
9598
$this->answerMapper = $this->createMock(AnswerMapper::class);
9699
$this->formMapper = $this->createMock(FormMapper::class);
97100
$this->optionMapper = $this->createMock(OptionMapper::class);
@@ -815,7 +818,7 @@ public function testNewSubmission_answers() {
815818

816819
$this->answerMapper->expects($this->exactly(5))
817820
->method('insert')
818-
->with($this->callback(function ($answer) {
821+
->with($this->callback(function (Answer $answer) {
819822
if ($answer->getSubmissionId() !== 12) {
820823
return false;
821824
}
@@ -836,6 +839,11 @@ public function testNewSubmission_answers() {
836839
return false;
837840
}
838841
break;
842+
case 4:
843+
if ($answer->getFileId() !== 100 || $answer->getText() !== 'uploaded-file.txt') {
844+
return false;
845+
}
846+
break;
839847
}
840848

841849
return true;
@@ -854,6 +862,12 @@ public function testNewSubmission_answers() {
854862
->willReturn(true);
855863

856864
$file = $this->createMock(File::class);
865+
$file->expects($this->once())
866+
->method('getName')
867+
->willReturn('uploaded-file.txt');
868+
$file->expects($this->once())
869+
->method('move')
870+
->with('/admin/files/submission-12/uploaded-file.txt');
857871

858872
$uploadedFile = new UploadedFile();
859873
$uploadedFile->setFileId(100);
@@ -864,9 +878,17 @@ public function testNewSubmission_answers() {
864878

865879
$userFolder->expects($this->once())
866880
->method('getById')
881+
->with(100)
867882
->willReturn([$file]);
868883

869884
$folder = $this->createMock(Folder::class);
885+
$folder->expects($this->once())
886+
->method('getNonExistingName')
887+
->with('uploaded-file.txt')
888+
->willReturn('uploaded-file.txt');
889+
$folder->expects($this->once())
890+
->method('getPath')
891+
->willReturn('/admin/files/submission-12');
870892

871893
$userFolder->expects($this->once())
872894
->method('get')
@@ -880,6 +902,164 @@ public function testNewSubmission_answers() {
880902
$this->apiController->newSubmission(1, $answers, '');
881903
}
882904

905+
public static function dataNewSubmission_fileUploadAuthorization(): array {
906+
return [
907+
'ordinary valid token' => [false, 'valid-upload-token', true],
908+
'ordinary foreign token' => [false, 'foreign-upload-token', false],
909+
'ordinary missing token' => [false, null, false],
910+
'ordinary empty token' => [false, '', false],
911+
'conditional valid token' => [true, 'valid-upload-token', true],
912+
'conditional foreign token' => [true, 'foreign-upload-token', false],
913+
'conditional missing token' => [true, null, false],
914+
'conditional empty token' => [true, '', false],
915+
];
916+
}
917+
918+
/**
919+
* @dataProvider dataNewSubmission_fileUploadAuthorization
920+
*/
921+
public function testNewSubmission_fileUploadAuthorization(bool $conditional, ?string $uploadToken, bool $authorized): void {
922+
$form = Form::fromParams(['id' => 7, 'hash' => 'hash', 'ownerId' => 'admin']);
923+
$fileQuestion = [
924+
'id' => 8,
925+
'formId' => 7,
926+
'name' => null,
927+
'text' => 'File question',
928+
'type' => Constants::ANSWER_TYPE_FILE,
929+
'options' => [],
930+
'extraSettings' => ['maxAllowedFilesCount' => 1],
931+
];
932+
$branch = [
933+
'id' => 'file-branch',
934+
'conditions' => [['type' => 'option_selected', 'optionId' => 3]],
935+
'subQuestions' => [$fileQuestion],
936+
];
937+
$question = $conditional ? [
938+
'id' => 9,
939+
'type' => Constants::ANSWER_TYPE_CONDITIONAL,
940+
'text' => 'Conditional question',
941+
'options' => [['id' => 3, 'text' => 'Attach a file']],
942+
'extraSettings' => [
943+
'triggerType' => Constants::ANSWER_TYPE_DROPDOWN,
944+
'branches' => [$branch],
945+
],
946+
] : $fileQuestion;
947+
$fileAnswer = ['uploadedFileId' => '10', 'fileName' => 'client-supplied.txt'];
948+
if ($uploadToken !== null) {
949+
$fileAnswer['uploadToken'] = $uploadToken;
950+
}
951+
$answers = $conditional
952+
? [9 => ['trigger' => ['3'], 'subQuestions' => [8 => [$fileAnswer]]]]
953+
: [8 => [$fileAnswer]];
954+
955+
$this->formsService->expects($this->once())
956+
->method('loadFormForSubmission')
957+
->with(7, '')
958+
->willReturn($form);
959+
$this->formsService->expects($this->once())
960+
->method('getQuestions')
961+
->with(7)
962+
->willReturn([$question]);
963+
$this->formAccess();
964+
965+
// Validation succeeds so storage must independently authorize the upload.
966+
$this->submissionService->expects($this->once())
967+
->method('validateSubmission')
968+
->with([$question], $answers, 'admin', 7);
969+
$this->submissionService->expects($conditional ? $this->once() : $this->never())
970+
->method('getActiveBranches')
971+
->with($question, ['3'])
972+
->willReturn([$branch]);
973+
$this->submissionMapper->expects($this->once())
974+
->method('insert')
975+
->willReturnCallback(function (Submission $submission): Submission {
976+
$this->assertSame(7, $submission->getFormId());
977+
$submission->setId(12);
978+
return $submission;
979+
});
980+
981+
$expectedAnswers = $conditional ? [[9, 12, null, 'Attach a file']] : [];
982+
if ($authorized) {
983+
$expectedAnswers[] = [8, 12, 99, 'real-file.txt'];
984+
}
985+
$this->answerMapper->expects($this->exactly(count($expectedAnswers)))
986+
->method('insert')
987+
->willReturnCallback(function (Answer $answer) use (&$expectedAnswers): Answer {
988+
$this->assertSame(array_shift($expectedAnswers), [
989+
$answer->getQuestionId(),
990+
$answer->getSubmissionId(),
991+
$answer->getFileId(),
992+
$answer->getText(),
993+
]);
994+
return $answer;
995+
});
996+
997+
$uploadedFile = UploadedFile::fromParams([
998+
'id' => 10,
999+
'formId' => 7,
1000+
'questionId' => 8,
1001+
'uploadToken' => 'valid-upload-token',
1002+
'fileId' => 99,
1003+
]);
1004+
$lookup = $this->uploadedFileMapper->expects($this->once())
1005+
->method('getForSubmission')
1006+
->with($this->identicalTo(10), 7, 8, $uploadToken ?? '');
1007+
$this->uploadedFileMapper->expects($this->never())->method('getByUploadedFileId');
1008+
$this->uploadedFileMapper->expects($this->never())->method('findByUploadedFileId');
1009+
$this->uploadedFileMapper->expects($authorized ? $this->once() : $this->never())
1010+
->method('delete')
1011+
->with($uploadedFile);
1012+
1013+
$file = $this->createMock(File::class);
1014+
$file->expects($authorized ? $this->once() : $this->never())
1015+
->method('getName')
1016+
->willReturn('real-file.txt');
1017+
$file->expects($authorized ? $this->once() : $this->never())
1018+
->method('move')
1019+
->with('/admin/files/Forms/submission-12/question-8/real-file.txt');
1020+
$folder = $this->createMock(Folder::class);
1021+
$folder->expects($authorized ? $this->once() : $this->never())
1022+
->method('getNonExistingName')
1023+
->with('real-file.txt')
1024+
->willReturn('real-file.txt');
1025+
$folder->expects($authorized ? $this->once() : $this->never())
1026+
->method('getPath')
1027+
->willReturn('/admin/files/Forms/submission-12/question-8');
1028+
$userFolder = $this->createMock(Folder::class);
1029+
$userFolder->expects($authorized ? $this->once() : $this->never())
1030+
->method('nodeExists')
1031+
->with('Forms/submission-12/question-8')
1032+
->willReturn(true);
1033+
$userFolder->expects($authorized ? $this->once() : $this->never())
1034+
->method('get')
1035+
->with('Forms/submission-12/question-8')
1036+
->willReturn($folder);
1037+
$userFolder->expects($authorized ? $this->once() : $this->never())
1038+
->method('getById')
1039+
->with(99)
1040+
->willReturn([$file]);
1041+
$this->storage->expects($authorized ? $this->once() : $this->never())
1042+
->method('getUserFolder')
1043+
->with('admin')
1044+
->willReturn($userFolder);
1045+
$this->formsService->expects($authorized ? $this->once() : $this->never())
1046+
->method('getUploadedFilePath')
1047+
->with($form, 12, 8, null, 'File question')
1048+
->willReturn('Forms/submission-12/question-8');
1049+
$this->formsService->expects($authorized ? $this->once() : $this->never())
1050+
->method('notifyNewSubmission');
1051+
1052+
if ($authorized) {
1053+
$lookup->willReturn($uploadedFile);
1054+
} else {
1055+
$lookup->willThrowException(new DoesNotExistException('Upload authorization failed'));
1056+
$this->expectException(DoesNotExistException::class);
1057+
$this->expectExceptionMessage('Upload authorization failed');
1058+
}
1059+
1060+
$this->assertEquals(new DataResponse(null, Http::STATUS_CREATED), $this->apiController->newSubmission(7, $answers, ''));
1061+
}
1062+
8831063
public function testNewSubmission_conditionalQuestion() {
8841064
$form = new Form();
8851065
$form->setId(1);

‎tests/Unit/Service/SubmissionServiceTest.php‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCA\Forms\Db\QuestionMapper;
2020
use OCA\Forms\Db\Submission;
2121
use OCA\Forms\Db\SubmissionMapper;
22+
use OCA\Forms\Db\UploadedFile;
2223
use OCA\Forms\Db\UploadedFileMapper;
2324
use OCA\Forms\Service\FormsService;
2425
use OCA\Forms\Service\SubmissionService;
@@ -1792,4 +1793,111 @@ public function testValidateSubmission_rejectsForeignUploadToken(): void {
17921793

17931794
$this->submissionService->validateSubmission($questions, $answers, 'alice', 7);
17941795
}
1796+
1797+
public static function dataFileUploadAuthorization(): array {
1798+
$cases = [];
1799+
foreach ([false, true] as $conditional) {
1800+
foreach ([false, true] as $required) {
1801+
foreach ([
1802+
'valid' => 'valid-upload-token',
1803+
'foreign' => 'attacker-token',
1804+
'missing' => null,
1805+
'empty' => '',
1806+
] as $name => $token) {
1807+
$case = ($conditional ? 'conditional' : 'ordinary') . '-'
1808+
. ($required ? 'required' : 'optional') . '-' . $name;
1809+
$cases[$case] = [$conditional, $required, $token];
1810+
}
1811+
}
1812+
}
1813+
return $cases;
1814+
}
1815+
1816+
/**
1817+
* @dataProvider dataFileUploadAuthorization
1818+
*/
1819+
public function testValidateSubmission_fileUploadAuthorization(bool $conditional, bool $required, ?string $uploadToken): void {
1820+
$fileQuestion = [
1821+
'id' => 8,
1822+
'type' => Constants::ANSWER_TYPE_FILE,
1823+
'text' => 'File question',
1824+
'isRequired' => $required,
1825+
'extraSettings' => [],
1826+
];
1827+
$fileAnswer = [
1828+
'uploadedFileId' => '10',
1829+
'fileName' => 'attachment.rtf',
1830+
];
1831+
if ($uploadToken !== null) {
1832+
$fileAnswer['uploadToken'] = $uploadToken;
1833+
}
1834+
$questions = [$fileQuestion];
1835+
$answers = [8 => [$fileAnswer]];
1836+
1837+
if ($conditional) {
1838+
$questions = [[
1839+
'id' => 9,
1840+
'type' => Constants::ANSWER_TYPE_CONDITIONAL,
1841+
'text' => 'Conditional question',
1842+
'isRequired' => true,
1843+
'options' => [['id' => 11, 'text' => 'Yes']],
1844+
'extraSettings' => [
1845+
'triggerType' => Constants::ANSWER_TYPE_DROPDOWN,
1846+
'branches' => [[
1847+
'id' => 'file-branch',
1848+
'conditions' => [[
1849+
'type' => Constants::CONDITION_TYPE_OPTION_SELECTED,
1850+
'optionId' => 11,
1851+
]],
1852+
'subQuestions' => [$fileQuestion],
1853+
]],
1854+
],
1855+
]];
1856+
$answers = [9 => ['trigger' => ['11'], 'subQuestions' => $answers]];
1857+
}
1858+
1859+
$this->uploadedFileMapper->expects($this->never())
1860+
->method('getByUploadedFileId');
1861+
$this->uploadedFileMapper->expects($this->never())
1862+
->method('findByUploadedFileId');
1863+
1864+
$missingRequiredToken = $required && ($uploadToken === null || $uploadToken === '');
1865+
if ($missingRequiredToken) {
1866+
$this->uploadedFileMapper->expects($this->never())
1867+
->method('getForSubmission');
1868+
} else {
1869+
$lookup = $this->uploadedFileMapper->expects($this->once())
1870+
->method('getForSubmission')
1871+
->with($this->identicalTo(10), $this->identicalTo(7), $this->identicalTo(8), $uploadToken ?? '');
1872+
if ($uploadToken === 'valid-upload-token') {
1873+
$uploadedFile = new UploadedFile();
1874+
$uploadedFile->setId(10);
1875+
$uploadedFile->setFileId(99);
1876+
$lookup->willReturn($uploadedFile);
1877+
} else {
1878+
$lookup->willThrowException(new DoesNotExistException('Upload not found'));
1879+
}
1880+
}
1881+
1882+
if ($uploadToken === 'valid-upload-token') {
1883+
$folder = $this->createMock(Folder::class);
1884+
$folder->expects($this->once())
1885+
->method('getById')
1886+
->with(99)
1887+
->willReturn([$this->createMock(File::class)]);
1888+
$this->storage->expects($this->once())
1889+
->method('getUserFolder')
1890+
->with('alice')
1891+
->willReturn($folder);
1892+
} else {
1893+
$this->storage->expects($this->never())
1894+
->method('getUserFolder');
1895+
$this->expectException(\InvalidArgumentException::class);
1896+
$this->expectExceptionMessage($missingRequiredToken
1897+
? 'Question "File question" is required.'
1898+
: 'File "attachment.rtf" for question "File question" not exists anymore. Please delete and re-upload the file.');
1899+
}
1900+
1901+
$this->submissionService->validateSubmission($questions, $answers, 'alice', 7);
1902+
}
17951903
};

0 commit comments

Comments
 (0)