Skip to content

Commit aaff8fa

Browse files
Chartman123Chartman123
committed
refactor: streamline form update logic and enhance permission checks for archiving, locking, and ownership transfer
Signed-off-by: Christian Hartmann <[email protected]>
1 parent 3890788 commit aaff8fa

File tree

7 files changed

+390
-494
lines changed

7 files changed

+390
-494
lines changed

‎lib/Controller/ApiController.php

Lines changed: 152 additions & 164 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
use OCA\Forms\Db\SubmissionMapper;
2323
use OCA\Forms\Db\UploadedFile;
2424
use OCA\Forms\Db\UploadedFileMapper;
25-
use OCA\Forms\Exception\NoSuchFormException;
2625
use OCA\Forms\ResponseDefinitions;
2726
use OCA\Forms\Service\ConfigService;
2827
use OCA\Forms\Service\FormsService;
@@ -272,161 +271,42 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse {
272271
$form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
273272
$currentUserId = $this->currentUser->getUID();
274273

275-
// Don't allow empty array
276-
if (sizeof($keyValuePairs) === 0) {
274+
if (empty($keyValuePairs)) {
277275
$this->logger->info('Empty keyValuePairs, will not update.');
278276
throw new OCSForbiddenException('Empty keyValuePairs, will not update.');
279277
}
280278

281279
// Only allow the form owner to set/unset the "archived" state
282-
if (
283-
(
284-
$this->formsService->isFormArchived($form)
285-
&& !(
286-
$currentUserId === $form->getOwnerId()
287-
&& sizeof($keyValuePairs) === 1
288-
&& key_exists('state', $keyValuePairs)
289-
&& $keyValuePairs['state'] === Constants::FORM_STATE_CLOSED
290-
)
291-
)
292-
|| (
293-
!$this->formsService->isFormArchived($form)
294-
&& !(
295-
$currentUserId === $form->getOwnerId()
296-
&& sizeof($keyValuePairs) === 1
297-
&& key_exists('state', $keyValuePairs)
298-
&& $keyValuePairs['state'] === Constants::FORM_STATE_ARCHIVED
299-
)
300-
)
301-
) {
302-
$this->logger->debug('Only the form owner can set/unset the \`archived\' state');
303-
throw new OCSForbiddenException('Only the form owner can set/unset the \`archived\' state');
304-
}
280+
$this->checkArchivePermission($form, $currentUserId, $keyValuePairs);
305281

306-
// Process complete form locking (lockedUntil: 0)
307-
if (
308-
sizeof($keyValuePairs) === 1
309-
&& array_key_exists('lockedUntil', $keyValuePairs)
310-
&& $keyValuePairs['lockedUntil'] === 0
311-
) {
312-
// Only allow form locking for form owner
313-
if ($currentUserId !== $form->getOwnerId() || ($form->getLockedBy() !== null && $currentUserId !== $form->getLockedBy())) {
314-
$this->logger->debug('Only the form owner can lock the form permanently');
315-
throw new OCSForbiddenException('Only the form owner can lock the form permanently');
316-
}
317-
318-
// Only allow if the form is not currently locked by another user
319-
if (
320-
$form->getLockedBy() !== null
321-
&& $form->getLockedBy() !== $currentUserId
322-
&& $form->getLockedUntil() >= time()
323-
) {
324-
$this->logger->debug('Form is currently locked by another user.');
325-
throw new OCSForbiddenException('Form is currently locked by another user.');
326-
}
327-
328-
// Abort if form is already completely locked
329-
if ($form->getLockedUntil() === 0) {
330-
$this->logger->debug('Form is already locked completely.');
331-
throw new OCSBadRequestException('Form is already locked completely.');
332-
}
333-
334-
$form->setLockedBy($form->getOwnerId());
335-
$form->setLockedUntil(0);
336-
337-
// Update changed Columns in Db.
338-
$this->formMapper->update($form);
339-
340-
return new DataResponse($form->getId());
282+
// Handle form locking/unlocking
283+
if ($this->isLockingRequest($keyValuePairs)) {
284+
return $this->handleFormLocking($form, $currentUserId);
341285
}
342-
343-
// Process form unlocking
344-
if (
345-
sizeof($keyValuePairs) === 1
346-
&& array_key_exists('lockedUntil', $keyValuePairs) && is_null($keyValuePairs['lockedUntil'])
347-
) {
348-
// Only allow form unlocking if for form owner or lock user
349-
if ($currentUserId !== $form->getOwnerId() && $currentUserId !== $form->getLockedBy() && $form->getLockedUntil() !== 0) {
350-
$this->logger->debug('Only the form owner or the user who obtained the lock can unlock the form');
351-
throw new OCSForbiddenException('Only the form owner or the user who obtained the lock can unlock the form');
352-
}
353-
354-
// remove form lock
355-
$form->setLockedBy(null);
356-
$form->setLockedUntil(null);
357-
358-
// Update changed Columns in Db.
359-
$this->formMapper->update($form);
360-
361-
return new DataResponse($form->getId());
286+
if ($this->isUnlockingRequest($keyValuePairs)) {
287+
return $this->handleFormUnlocking($form, $currentUserId);
362288
}
363289

364290
// Lock form temporary
365291
$this->formsService->obtainFormLock($form);
366-
367-
// Process owner transfer
368-
if (sizeof($keyValuePairs) === 1 && key_exists('ownerId', $keyValuePairs)) {
369-
// Only allow owner transfer if current user is the form owner
370-
if ($currentUserId !== $form->getOwnerId()) {
371-
$this->logger->debug('Only the form owner can transfer ownership');
372-
throw new OCSForbiddenException('Only the form owner can transfer ownership');
373-
}
374-
375-
$this->logger->debug('Updating owner: formId: {formId}, userId: {uid}', [
376-
'formId' => $formId,
377-
'uid' => $keyValuePairs['ownerId']
378-
]);
379-
380-
$user = $this->userManager->get($keyValuePairs['ownerId']);
381-
if ($user == null) {
382-
$this->logger->debug('Could not find new form owner');
383-
throw new OCSBadRequestException('Could not find new form owner');
384-
}
385292

386-
// update form owner and remove form lock
387-
$form->setOwnerId($keyValuePairs['ownerId']);
388-
$form->setLockedBy(null);
389-
$form->setLockedUntil(null);
390-
391-
// Update changed Columns in Db.
392-
$this->formMapper->update($form);
393-
394-
return new DataResponse($form->getOwnerId());
293+
// Handle owner transfer
294+
if ($this->isOwnerTransferRequest($keyValuePairs)) {
295+
return $this->handleOwnerTransfer($form, $formId, $currentUserId, $keyValuePairs);
395296
}
396297

397298
// Don't allow to change the following attributes
398-
$forbiddenKeys = [
399-
'id', 'hash', 'ownerId', 'created', 'lastUpdated', 'lockedBy', 'lockedUntil'
400-
];
401-
402-
foreach ($forbiddenKeys as $key) {
403-
if (array_key_exists($key, $keyValuePairs)) {
404-
$this->logger->info("Not allowed to update {$key}");
405-
throw new OCSForbiddenException("Not allowed to update {$key}");
406-
}
407-
}
299+
$this->checkForbiddenKeys($keyValuePairs);
408300

409301
// Don't allow to change fileId
410-
if (isset($keyValuePairs['fileId'])) {
411-
$this->logger->info('Not allowed to update fileId');
412-
throw new OCSForbiddenException('Not allowed to update fileId');
413-
}
302+
$this->checkFileIdUpdate($keyValuePairs);
414303

415-
// Do not allow changing showToAllUsers if disabled
416-
if (isset($keyValuePairs['access'])) {
417-
$showAll = $keyValuePairs['access']['showToAllUsers'] ?? false;
418-
$permitAll = $keyValuePairs['access']['permitAllUsers'] ?? false;
419-
if (($showAll && !$this->configService->getAllowShowToAll())
420-
|| ($permitAll && !$this->configService->getAllowPermitAll())) {
421-
$this->logger->info('Not allowed to update showToAllUsers or permitAllUsers');
422-
throw new OCSForbiddenException();
423-
}
424-
}
304+
// Do not allow changing showToAllUsers or permitAllUsers if disabled
305+
$this->checkAccessUpdate($keyValuePairs);
425306

426307
// Process file linking
427308
if (isset($keyValuePairs['path']) && isset($keyValuePairs['fileFormat'])) {
428309
$file = $this->submissionService->writeFileToCloud($form, $keyValuePairs['path'], $keyValuePairs['fileFormat']);
429-
430310
$form->setFileId($file->getId());
431311
$form->setFileFormat($keyValuePairs['fileFormat']);
432312
}
@@ -1429,7 +1309,7 @@ public function newSubmission(int $formId, array $answers, string $shareHash = '
14291309
'shareHash' => $shareHash,
14301310
]);
14311311

1432-
$form = $this->loadFormForSubmission($formId, $shareHash);
1312+
$form = $this->formsService->loadFormForSubmission($formId, $shareHash);
14331313

14341314
$questions = $this->formsService->getQuestions($formId);
14351315
try {
@@ -1515,7 +1395,7 @@ public function updateSubmission(int $formId, int $submissionId, array $answers)
15151395
]);
15161396

15171397
// submissions can't be updated on public shares, so passing empty shareHash
1518-
$form = $this->loadFormForSubmission($formId, '');
1398+
$form = $this->formsService->loadFormForSubmission($formId, '');
15191399

15201400
if (!$form->getAllowEditSubmissions()) {
15211401
throw new OCSBadRequestException('Can only update if allowEditSubmissions is set');
@@ -1680,7 +1560,7 @@ public function uploadFiles(int $formId, int $questionId, string $shareHash = ''
16801560
throw new OCSBadRequestException('No files provided');
16811561
}
16821562

1683-
$form = $this->loadFormForSubmission($formId, $shareHash);
1563+
$form = $this->formsService->loadFormForSubmission($formId, $shareHash);
16841564

16851565
if (!$this->formsService->canSubmit($form)) {
16861566
throw new OCSForbiddenException('Already submitted');
@@ -1848,40 +1728,148 @@ private function storeAnswersForQuestion(Form $form, $submissionId, array $quest
18481728
}
18491729
}
18501730

1851-
private function loadFormForSubmission(int $formId, string $shareHash): Form {
1852-
try {
1853-
$form = $this->formMapper->findById($formId);
1854-
} catch (IMapperException $e) {
1855-
$this->logger->debug('Could not find form');
1856-
throw new NoSuchFormException('Could not find form');
1731+
/**
1732+
* Throws if forbidden keys are present in update
1733+
*/
1734+
private function checkForbiddenKeys(array $keyValuePairs): void {
1735+
$forbiddenKeys = [
1736+
'id', 'hash', 'ownerId', 'created', 'lastUpdated', 'lockedBy', 'lockedUntil'
1737+
];
1738+
foreach ($forbiddenKeys as $key) {
1739+
if (array_key_exists($key, $keyValuePairs)) {
1740+
$this->logger->info("Not allowed to update {$key}");
1741+
throw new OCSForbiddenException("Not allowed to update {$key}");
1742+
}
18571743
}
1744+
}
18581745

1859-
// Does the user have access to the form (Either by logged-in user, or by providing public share-hash.)
1860-
try {
1861-
$isPublicShare = false;
1862-
1863-
// If hash given, find the corresponding share & check if hash corresponds to given formId.
1864-
if ($shareHash !== '') {
1865-
// Public link share
1866-
$share = $this->shareMapper->findPublicShareByHash($shareHash);
1867-
if ($share->getFormId() === $formId) {
1868-
$isPublicShare = true;
1869-
}
1746+
/**
1747+
* Throws if fileId is present in update
1748+
*/
1749+
private function checkFileIdUpdate(array $keyValuePairs): void {
1750+
if (isset($keyValuePairs['fileId'])) {
1751+
$this->logger->info('Not allowed to update fileId');
1752+
throw new OCSForbiddenException('Not allowed to update fileId');
1753+
}
1754+
}
1755+
1756+
/**
1757+
* Throws if access keys are being updated when not allowed
1758+
*/
1759+
private function checkAccessUpdate(array $keyValuePairs): void {
1760+
if (isset($keyValuePairs['access'])) {
1761+
$showAll = $keyValuePairs['access']['showToAllUsers'] ?? false;
1762+
$permitAll = $keyValuePairs['access']['permitAllUsers'] ?? false;
1763+
if (($showAll && !$this->configService->getAllowShowToAll())
1764+
|| ($permitAll && !$this->configService->getAllowPermitAll())) {
1765+
$this->logger->info('Not allowed to update showToAllUsers or permitAllUsers');
1766+
throw new OCSForbiddenException();
18701767
}
1871-
} catch (DoesNotExistException $e) {
1872-
// $isPublicShare already false.
1873-
} finally {
1874-
// Now forbid, if no public share and no direct share.
1875-
if (!$isPublicShare && !$this->formsService->hasUserAccess($form)) {
1876-
throw new NoSuchFormException('Not allowed to access this form', Http::STATUS_FORBIDDEN);
1768+
}
1769+
}
1770+
1771+
/**
1772+
* Checks if the current user is allowed to archive/unarchive the form
1773+
*/
1774+
private function checkArchivePermission(Form $form, string $currentUserId, array $keyValuePairs): void {
1775+
$isArchived = $this->formsService->isFormArchived($form);
1776+
$owner = $currentUserId === $form->getOwnerId();
1777+
$onlyState = sizeof($keyValuePairs) === 1 && key_exists('state', $keyValuePairs);
1778+
1779+
// Only check if the request is trying to change the archived state
1780+
if ($onlyState && $keyValuePairs['state'] === Constants::FORM_STATE_ARCHIVED) {
1781+
// Trying to archive
1782+
if (!$owner || $isArchived) {
1783+
$this->logger->debug('Only the form owner can archive the form, and only if it is not already archived');
1784+
throw new OCSForbiddenException('Only the form owner can archive the form, and only if it is not already archived');
1785+
}
1786+
} elseif ($onlyState && $keyValuePairs['state'] === Constants::FORM_STATE_CLOSED) {
1787+
// Trying to unarchive
1788+
if (!$owner || !$isArchived) {
1789+
$this->logger->debug('Only the form owner can unarchive the form, and only if it is currently archived');
1790+
throw new OCSForbiddenException('Only the form owner can unarchive the form, and only if it is currently archived');
18771791
}
18781792
}
1793+
// All other updates are allowed (including updates that do not touch the state)
1794+
}
18791795

1880-
// Not allowed if form has expired.
1881-
if ($this->formsService->hasFormExpired($form)) {
1882-
throw new OCSForbiddenException('This form is no longer taking answers');
1796+
private function isLockingRequest(array $keyValuePairs): bool {
1797+
return sizeof($keyValuePairs) === 1
1798+
&& array_key_exists('lockedUntil', $keyValuePairs)
1799+
&& $keyValuePairs['lockedUntil'] === 0;
1800+
}
1801+
1802+
private function isUnlockingRequest(array $keyValuePairs): bool {
1803+
return sizeof($keyValuePairs) === 1
1804+
&& array_key_exists('lockedUntil', $keyValuePairs)
1805+
&& is_null($keyValuePairs['lockedUntil']);
1806+
}
1807+
1808+
private function isOwnerTransferRequest(array $keyValuePairs): bool {
1809+
return sizeof($keyValuePairs) === 1 && key_exists('ownerId', $keyValuePairs);
1810+
}
1811+
1812+
/**
1813+
* @return DataResponse<Http::STATUS_OK, int, array{}>
1814+
*/
1815+
private function handleFormLocking(Form $form, string $currentUserId): DataResponse {
1816+
if ($currentUserId !== $form->getOwnerId() || ($form->getLockedBy() !== null && $currentUserId !== $form->getLockedBy())) {
1817+
$this->logger->debug('Only the form owner can lock the form permanently');
1818+
throw new OCSForbiddenException('Only the form owner can lock the form permanently');
1819+
}
1820+
if (
1821+
$form->getLockedBy() !== null
1822+
&& $form->getLockedBy() !== $currentUserId
1823+
&& $form->getLockedUntil() >= time()
1824+
) {
1825+
$this->logger->debug('Form is currently locked by another user.');
1826+
throw new OCSForbiddenException('Form is currently locked by another user.');
1827+
}
1828+
if ($form->getLockedUntil() === 0) {
1829+
$this->logger->debug('Form is already locked completely.');
1830+
throw new OCSBadRequestException('Form is already locked completely.');
1831+
}
1832+
$form->setLockedBy($form->getOwnerId());
1833+
$form->setLockedUntil(0);
1834+
$this->formMapper->update($form);
1835+
return new DataResponse($form->getId());
1836+
}
1837+
1838+
/**
1839+
* @return DataResponse<Http::STATUS_OK, int, array{}>
1840+
*/
1841+
private function handleFormUnlocking(Form $form, string $currentUserId): DataResponse {
1842+
if ($currentUserId !== $form->getOwnerId() && $currentUserId !== $form->getLockedBy() && $form->getLockedUntil() !== 0) {
1843+
$this->logger->debug('Only the form owner or the user who obtained the lock can unlock the form');
1844+
throw new OCSForbiddenException('Only the form owner or the user who obtained the lock can unlock the form');
18831845
}
1846+
$form->setLockedBy(null);
1847+
$form->setLockedUntil(null);
1848+
$this->formMapper->update($form);
1849+
return new DataResponse($form->getId());
1850+
}
18841851

1885-
return $form;
1852+
/**
1853+
* @return DataResponse<Http::STATUS_OK, string, array{}>
1854+
*/
1855+
private function handleOwnerTransfer(Form $form, int $formId, string $currentUserId, array $keyValuePairs): DataResponse {
1856+
if ($currentUserId !== $form->getOwnerId()) {
1857+
$this->logger->debug('Only the form owner can transfer ownership');
1858+
throw new OCSForbiddenException('Only the form owner can transfer ownership');
1859+
}
1860+
$this->logger->debug('Updating owner: formId: {formId}, userId: {uid}', [
1861+
'formId' => $formId,
1862+
'uid' => $keyValuePairs['ownerId']
1863+
]);
1864+
$user = $this->userManager->get($keyValuePairs['ownerId']);
1865+
if ($user == null) {
1866+
$this->logger->debug('Could not find new form owner');
1867+
throw new OCSBadRequestException('Could not find new form owner');
1868+
}
1869+
$form->setOwnerId($keyValuePairs['ownerId']);
1870+
$form->setLockedBy(null);
1871+
$form->setLockedUntil(null);
1872+
$this->formMapper->update($form);
1873+
return new DataResponse($form->getOwnerId());
18861874
}
18871875
}

0 commit comments

Comments
 (0)