From 08de2eb956baf38e4f066a70ce8c1f00cc0e435a Mon Sep 17 00:00:00 2001 From: Christian Hartmann Date: Mon, 12 May 2025 00:34:16 +0200 Subject: [PATCH 1/7] feat: add form locking mechanism - Implemented form locking functionality to prevent concurrent edits. - Updated API responses to include `lockedBy` and `lockedUntil` fields. - Enhanced permission checks to ensure only the form owner can transfer ownership. - Revised documentation to reflect changes in form data structure and API behavior. Signed-off-by: Christian Hartmann --- docs/API_v3.md | 10 +- docs/DataStructure.md | 4 + lib/Controller/ApiController.php | 228 +++++++++++------- lib/Controller/ShareApiController.php | 56 ++--- lib/Db/Form.php | 12 + .../Version050200Date20250512004000.php | 48 ++++ lib/ResponseDefinitions.php | 6 +- lib/Service/FormsService.php | 73 +++++- openapi.json | 24 +- 9 files changed, 339 insertions(+), 122 deletions(-) create mode 100644 lib/Migration/Version050200Date20250512004000.php diff --git a/docs/API_v3.md b/docs/API_v3.md index a3b127b07..4797da48a 100644 --- a/docs/API_v3.md +++ b/docs/API_v3.md @@ -92,7 +92,9 @@ Returns condensed objects of all Forms beeing owned by the authenticated user. "submit" ], "partial": true, - "state": 0 + "state": 0, + "lockedBy": null, + "lockedUntil": null }, { "id": 3, @@ -105,7 +107,9 @@ Returns condensed objects of all Forms beeing owned by the authenticated user. "submit" ], "partial": true, - "state": 0 + "state": 0, + "lockedBy": "someUser" + "lockedUntil": 123456789 } ] ``` @@ -169,6 +173,8 @@ Returns the full-depth object of the requested form (without submissions). "showExpiration": false, "canSubmit": true, "state": 0, + "lockedBy": null, + "lockedUntil": null, "permissions": [ "edit", "results", diff --git a/docs/DataStructure.md b/docs/DataStructure.md index bd9490585..ffd0cf42f 100644 --- a/docs/DataStructure.md +++ b/docs/DataStructure.md @@ -26,6 +26,8 @@ This document describes the Object-Structure, that is used within the Forms App | expires | unix-timestamp | | When the form should expire. Timestamp `0` indicates _never_ | | isAnonymous | Boolean | | If Answers will be stored anonymously | | state | Integer | [Form state](#form-state) | The state of the form | +| lockedBy | String | | The user ID for who has exclusive edit access at the moment | +| lockedUntil | unix timestamp | | When the form lock will expire | | submitMultiple | Boolean | | If users are allowed to submit multiple times to the form | | allowEditSubmissions | Boolean | | If users are allowed to edit or delete their response | | showExpiration | Boolean | | If the expiration date will be shown on the form | @@ -59,6 +61,8 @@ This document describes the Object-Structure, that is used within the Forms App ], "questions": [], "state": 0, + "lockedBy": null, + "lockedUntil": null, "shares": [] "submissions": [], "submissionCount": 0, diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index dc0208340..438f7c19d 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -180,7 +180,7 @@ public function newForm(?int $fromId = null): DataResponse { $this->formMapper->insert($form); } else { - $oldForm = $this->getFormIfAllowed($fromId); + $oldForm = $this->formsService->getFormIfAllowed($fromId, Constants::PERMISSION_EDIT); // Read old form, (un)set new form specific data, extend title $formData = $oldForm->read(); @@ -190,6 +190,8 @@ public function newForm(?int $fromId = null): DataResponse { unset($formData['state']); unset($formData['fileId']); unset($formData['fileFormat']); + unset($formData['lockedBy']); + unset($formData['lockedUntil']); $formData['hash'] = $this->formsService->generateFormHash(); // TRANSLATORS Appendix to the form Title of a duplicated/copied form. $formData['title'] .= ' - ' . $this->l10n->t('Copy'); @@ -238,7 +240,7 @@ public function newForm(?int $fromId = null): DataResponse { #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}')] public function getForm(int $formId): DataResponse { - $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_SUBMIT); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_SUBMIT); return new DataResponse($this->formsService->getForm($form)); } @@ -267,18 +269,8 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { 'keyValuePairs' => $keyValuePairs ]); - $form = $this->getFormIfAllowed($formId); - if ( - $this->formsService->isFormArchived($form) - && !( - sizeof($keyValuePairs) === 1 - && key_exists('state', $keyValuePairs) - && $keyValuePairs['state'] === Constants::FORM_STATE_CLOSED - ) - ) { - $this->logger->debug('This form is archived and can not be modified except to change state to closed.'); - throw new OCSForbiddenException('This form is archived and can not be modified except to change state to closed.'); - } + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $currentUserId = $this->currentUser->getUID(); // Don't allow empty array if (sizeof($keyValuePairs) === 0) { @@ -286,8 +278,100 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { throw new OCSForbiddenException('Empty keyValuePairs, will not update.'); } + // Only allow the form owner to set/unset the "archived" state + if ( + ( + $this->formsService->isFormArchived($form) + && !( + $currentUserId === $form->getOwnerId() + && sizeof($keyValuePairs) === 1 + && key_exists('state', $keyValuePairs) + && $keyValuePairs['state'] === Constants::FORM_STATE_CLOSED + ) + ) + || ( + !$this->formsService->isFormArchived($form) + && !( + $currentUserId === $form->getOwnerId() + && sizeof($keyValuePairs) === 1 + && key_exists('state', $keyValuePairs) + && $keyValuePairs['state'] === Constants::FORM_STATE_ARCHIVED + ) + ) + ) { + $this->logger->debug('Only the form owner can set/unset the \`archived\' state'); + throw new OCSForbiddenException('Only the form owner can set/unset the \`archived\' state'); + } + + // Process complete form locking (lockedUntil: 0) + if ( + sizeof($keyValuePairs) === 1 + && array_key_exists('lockedUntil', $keyValuePairs) + && $keyValuePairs['lockedUntil'] === 0 + ) { + // Only allow form locking for form owner + if ($currentUserId !== $form->getOwnerId() || ($form->getLockedBy() !== null && $currentUserId !== $form->getLockedBy())) { + $this->logger->debug('Only the form owner can lock the form permanently'); + throw new OCSForbiddenException('Only the form owner can lock the form permanently'); + } + + // Only allow if the form is not currently locked by another user + if ( + $form->getLockedBy() !== null + && $form->getLockedBy() !== $currentUserId + && $form->getLockedUntil() >= time() + ) { + $this->logger->debug('Form is currently locked by another user.'); + throw new OCSForbiddenException('Form is currently locked by another user.'); + } + + // Abort if form is already completely locked + if ($form->getLockedUntil() === 0) { + $this->logger->debug('Form is already locked completely.'); + throw new OCSBadRequestException('Form is already locked completely.'); + } + + $form->setLockedBy($form->getOwnerId()); + $form->setLockedUntil(0); + + // Update changed Columns in Db. + $this->formMapper->update($form); + + return new DataResponse($form->getId()); + } + + // Process form unlocking + if ( + sizeof($keyValuePairs) === 1 + && array_key_exists('lockedUntil', $keyValuePairs) && is_null($keyValuePairs['lockedUntil']) + ) { + // Only allow form unlocking if for form owner or lock user + if ($currentUserId !== $form->getOwnerId() && $currentUserId !== $form->getLockedBy() && $form->getLockedUntil() !== 0) { + $this->logger->debug('Only the form owner or the user who obtained the lock can unlock the form'); + throw new OCSForbiddenException('Only the form owner or the user who obtained the lock can unlock the form'); + } + + // remove form lock + $form->setLockedBy(null); + $form->setLockedUntil(null); + + // Update changed Columns in Db. + $this->formMapper->update($form); + + return new DataResponse($form->getId()); + } + + // Lock form temporary + $this->formsService->obtainFormLock($form); + // Process owner transfer if (sizeof($keyValuePairs) === 1 && key_exists('ownerId', $keyValuePairs)) { + // Only allow owner transfer if current user is the form owner + if ($currentUserId !== $form->getOwnerId()) { + $this->logger->debug('Only the form owner can transfer ownership'); + throw new OCSForbiddenException('Only the form owner can transfer ownership'); + } + $this->logger->debug('Updating owner: formId: {formId}, userId: {uid}', [ 'formId' => $formId, 'uid' => $keyValuePairs['ownerId'] @@ -299,8 +383,10 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { throw new OCSBadRequestException('Could not find new form owner'); } - // update form owner + // update form owner and remove form lock $form->setOwnerId($keyValuePairs['ownerId']); + $form->setLockedBy(null); + $form->setLockedUntil(null); // Update changed Columns in Db. $this->formMapper->update($form); @@ -308,14 +394,22 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { return new DataResponse($form->getOwnerId()); } - // Don't allow to change params id, hash, ownerId, created, lastUpdated, fileId - if ( - key_exists('id', $keyValuePairs) || key_exists('hash', $keyValuePairs) - || key_exists('ownerId', $keyValuePairs) || key_exists('created', $keyValuePairs) - || isset($keyValuePairs['fileId']) || key_exists('lastUpdated', $keyValuePairs) - ) { - $this->logger->info('Not allowed to update id, hash, ownerId, created, fileId or lastUpdated'); - throw new OCSForbiddenException('Not allowed to update id, hash, ownerId, created, fileId or lastUpdated'); + // Don't allow to change the following attributes + $forbiddenKeys = [ + 'id', 'hash', 'ownerId', 'created', 'lastUpdated', 'lockedBy', 'lockedUntil' + ]; + + foreach ($forbiddenKeys as $key) { + if (array_key_exists($key, $keyValuePairs)) { + $this->logger->info("Not allowed to update {$key}"); + throw new OCSForbiddenException("Not allowed to update {$key}"); + } + } + + // Don't allow to change fileId + if (isset($keyValuePairs['fileId'])) { + $this->logger->info('Not allowed to update fileId'); + throw new OCSForbiddenException('Not allowed to update fileId'); } // Do not allow changing showToAllUsers if disabled @@ -378,7 +472,7 @@ public function deleteForm(int $formId): DataResponse { 'formId' => $formId, ]); - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId); $this->formMapper->deleteForm($form); return new DataResponse($formId); @@ -488,7 +582,9 @@ public function getQuestion(int $formId, int $questionId): DataResponse { #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions')] public function newQuestion(int $formId, ?string $type = null, string $text = '', ?int $fromId = null): DataResponse { - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -619,7 +715,9 @@ public function updateQuestion(int $formId, int $questionId, array $keyValuePair // Make sure we query the form first to check the user has permissions // So the user does not get information about "questions" if they do not even have permissions to the form - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -693,7 +791,9 @@ public function deleteQuestion(int $formId, int $questionId): DataResponse { ]); - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -759,7 +859,9 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse { 'newOrder' => $newOrder ]); - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -858,7 +960,9 @@ public function newOption(int $formId, int $questionId, array $optionTexts): Dat 'text' => $optionTexts, ]); - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -940,7 +1044,9 @@ public function updateOption(int $formId, int $questionId, int $optionId, array 'keyValuePairs' => $keyValuePairs ]); - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -1007,7 +1113,9 @@ public function deleteOption(int $formId, int $questionId, int $optionId): DataR 'optionId' => $optionId ]); - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -1063,7 +1171,9 @@ public function deleteOption(int $formId, int $questionId, int $optionId): DataR #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}/options')] public function reorderOptions(int $formId, int $questionId, array $newOrder) { - $form = $this->getFormIfAllowed($formId); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_EDIT); + $this->formsService->obtainFormLock($form); + if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); @@ -1164,7 +1274,7 @@ public function reorderOptions(int $formId, int $questionId, array $newOrder) { #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}/submissions')] public function getSubmissions(int $formId, ?string $query = null, ?int $limit = null, int $offset = 0, ?string $fileFormat = null): DataResponse|DataDownloadResponse { - $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); if ($fileFormat !== null) { $submissionsData = $this->submissionService->getSubmissionsData($form, $fileFormat); @@ -1237,7 +1347,7 @@ public function getSubmissions(int $formId, ?string $query = null, ?int $limit = #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}/submissions/{submissionId}')] public function getSubmission(int $formId, int $submissionId): DataResponse|DataDownloadResponse { - $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); $submission = $this->submissionService->getSubmission($submissionId); if ($submission === null) { @@ -1282,7 +1392,7 @@ public function getSubmission(int $formId, int $submissionId): DataResponse|Data #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/submissions')] public function deleteAllSubmissions(int $formId): DataResponse { - $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); // Delete all submissions (incl. Answers) $this->submissionMapper->deleteByForm($formId); @@ -1480,7 +1590,7 @@ public function deleteSubmission(int $formId, int $submissionId): DataResponse { 'submissionId' => $submissionId, ]); - $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE); try { $submission = $this->submissionMapper->findById($submissionId); } catch (IMapperException $e) { @@ -1524,7 +1634,7 @@ public function deleteSubmission(int $formId, int $submissionId): DataResponse { #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/submissions/export')] public function exportSubmissionsToCloud(int $formId, string $path, string $fileFormat = Constants::DEFAULT_FILE_FORMAT) { - $form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); + $form = $this->formsService->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS); $file = $this->submissionService->writeFileToCloud($form, $path, $fileFormat); return new DataResponse($file->getName()); @@ -1774,48 +1884,4 @@ private function loadFormForSubmission(int $formId, string $shareHash): Form { return $form; } - - /** - * Helper that retrieves a form if the current user is allowed to edit it - * This throws an exception in case either the form is not found or permissions are missing. - * @param int $formId The form ID to retrieve - * @throws NoSuchFormException If the form was not found or the current user has no permission to edit - */ - private function getFormIfAllowed(int $formId, string $permissions = 'all'): Form { - try { - $form = $this->formMapper->findById($formId); - } catch (IMapperException $e) { - $this->logger->debug('Could not find form'); - throw new NoSuchFormException('Could not find form'); - } - - switch ($permissions) { - case Constants::PERMISSION_SUBMIT: - if (!$this->formsService->hasUserAccess($form)) { - $this->logger->debug('User has no permissions to get this form'); - throw new NoSuchFormException('User has no permissions to get this form', Http::STATUS_FORBIDDEN); - } - break; - case Constants::PERMISSION_RESULTS: - if (!$this->formsService->canSeeResults($form)) { - $this->logger->debug('The current user has no permission to get the results for this form'); - throw new NoSuchFormException('The current user has no permission to get the results for this form', Http::STATUS_FORBIDDEN); - } - break; - case Constants::PERMISSION_RESULTS_DELETE: - if (!$this->formsService->canDeleteResults($form)) { - $this->logger->debug('This form is not owned by the current user and user has no `results_delete` permission'); - throw new NoSuchFormException('This form is not owned by the current user and user has no `results_delete` permission', Http::STATUS_FORBIDDEN); - } - break; - default: - // By default we request full permissions - if ($form->getOwnerId() !== $this->currentUser->getUID()) { - $this->logger->debug('This form is not owned by the current user'); - throw new NoSuchFormException('This form is not owned by the current user', Http::STATUS_FORBIDDEN); - } - break; - } - return $form; - } } diff --git a/lib/Controller/ShareApiController.php b/lib/Controller/ShareApiController.php index 067c171a1..e1ad6b14f 100644 --- a/lib/Controller/ShareApiController.php +++ b/lib/Controller/ShareApiController.php @@ -116,28 +116,16 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar throw new OCSForbiddenException('Link share not allowed.'); } - try { - $form = $this->formMapper->findById($formId); - } catch (IMapperException $e) { - $this->logger->debug('Could not find form', ['exception' => $e]); - throw new OCSNotFoundException('Could not find form'); + if (!$this->validatePermissions($permissions, $shareType)) { + throw new OCSBadRequestException('Invalid permission given'); } + $form = $this->formsService->getFormIfAllowed($formId); if ($this->formsService->isFormArchived($form)) { $this->logger->debug('This form is archived and can not be modified'); throw new OCSForbiddenException('This form is archived and can not be modified'); } - // Check for permission to share form - if ($form->getOwnerId() !== $this->currentUser->getUID()) { - $this->logger->debug('This form is not owned by the current user'); - throw new OCSForbiddenException('This form is not owned by the current user'); - } - - if (!$this->validatePermissions($permissions, $shareType)) { - throw new OCSBadRequestException('Invalid permission given'); - } - // Create public-share hash, if necessary. if ($shareType === IShare::TYPE_LINK) { $shareWith = $this->secureRandom->generate( @@ -194,6 +182,8 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar throw new OCSBadRequestException('Unknown shareType.'); } + $this->formsService->obtainFormLock($form); + $share = new Share(); $share->setFormId($formId); $share->setShareType($shareType); @@ -240,29 +230,24 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da 'keyValuePairs' => $keyValuePairs ]); + $form = $this->formsService->getFormIfAllowed($formId); + if ($this->formsService->isFormArchived($form)) { + $this->logger->debug('This form is archived and can not be modified'); + throw new OCSForbiddenException('This form is archived and can not be modified'); + } + try { $formShare = $this->shareMapper->findById($shareId); - $form = $this->formMapper->findById($formId); } catch (IMapperException $e) { $this->logger->debug('Could not find share', ['exception' => $e]); throw new OCSNotFoundException('Could not find share'); } - if ($this->formsService->isFormArchived($form)) { - $this->logger->debug('This form is archived and can not be modified'); - throw new OCSForbiddenException('This form is archived and can not be modified'); - } - if ($formId !== $formShare->getFormId()) { $this->logger->debug('This share doesn\'t belong to the given Form'); throw new OCSBadRequestException('Share doesn\'t belong to given Form'); } - if ($form->getOwnerId() !== $this->currentUser->getUID()) { - $this->logger->debug('This form is not owned by the current user'); - throw new OCSForbiddenException('This form is not owned by the current user'); - } - // Don't allow empty array if (sizeof($keyValuePairs) === 0) { $this->logger->info('Empty keyValuePairs, will not update.'); @@ -279,6 +264,8 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da throw new OCSBadRequestException('Invalid permission given'); } + $this->formsService->obtainFormLock($form); + $formShare->setPermissions($keyValuePairs['permissions']); $formShare = $this->shareMapper->update($formShare); @@ -338,28 +325,25 @@ public function deleteShare(int $formId, int $shareId): DataResponse { 'shareId' => $shareId, ]); + $form = $this->formsService->getFormIfAllowed($formId); + if ($this->formsService->isFormArchived($form)) { + $this->logger->debug('This form is archived and can not be modified'); + throw new OCSForbiddenException('This form is archived and can not be modified'); + } + try { $share = $this->shareMapper->findById($shareId); - $form = $this->formMapper->findById($formId); } catch (IMapperException $e) { $this->logger->debug('Could not find share', ['exception' => $e]); throw new OCSNotFoundException('Could not find share'); } - if ($this->formsService->isFormArchived($form)) { - $this->logger->debug('This form is archived and can not be modified'); - throw new OCSForbiddenException('This form is archived and can not be modified'); - } - if ($formId !== $share->getFormId()) { $this->logger->debug('This share doesn\'t belong to the given Form'); throw new OCSBadRequestException('Share doesn\'t belong to given Form'); } - if ($form->getOwnerId() !== $this->currentUser->getUID()) { - $this->logger->debug('This form is not owned by the current user'); - throw new OCSForbiddenException('This form is not owned by the current user'); - } + $this->formsService->obtainFormLock($form); $this->shareMapper->delete($share); $this->formMapper->update($form); diff --git a/lib/Db/Form.php b/lib/Db/Form.php index a49181be6..fe1637eda 100644 --- a/lib/Db/Form.php +++ b/lib/Db/Form.php @@ -47,6 +47,10 @@ * @psalm-method 0|1|2 getState() * @method void setState(int|null $value) * @psalm-method void setState(0|1|2|null $value) + * @method string getLockedBy() + * @method void setLockedBy(string|null $value) + * @method int getLockedUntil() + * @method void setLockedUntil(int|null $value) */ class Form extends Entity { protected $hash; @@ -65,6 +69,8 @@ class Form extends Entity { protected $submissionMessage; protected $lastUpdated; protected $state; + protected $lockedBy; + protected $lockedUntil; /** * Form constructor. @@ -78,6 +84,8 @@ public function __construct() { $this->addType('showExpiration', 'boolean'); $this->addType('lastUpdated', 'integer'); $this->addType('state', 'integer'); + $this->addType('lockedBy', 'string'); + $this->addType('lockedUntil', 'integer'); } // JSON-Decoding of access-column. @@ -149,6 +157,8 @@ public function setAccess(array $access): void { * lastUpdated: int, * submissionMessage: ?string, * state: 0|1|2, + * lockedBy: ?string, + * lockedUntil: ?int, * } */ public function read() { @@ -170,6 +180,8 @@ public function read() { 'lastUpdated' => (int)$this->getLastUpdated(), 'submissionMessage' => $this->getSubmissionMessage(), 'state' => $this->getState(), + 'lockedBy' => $this->getLockedBy(), + 'lockedUntil' => $this->getLockedUntil(), ]; } } diff --git a/lib/Migration/Version050200Date20250512004000.php b/lib/Migration/Version050200Date20250512004000.php new file mode 100644 index 000000000..d8cbf762c --- /dev/null +++ b/lib/Migration/Version050200Date20250512004000.php @@ -0,0 +1,48 @@ +getTable('forms_v2_forms'); + + if (!$table->hasColumn('locked_by')) { + $table->addColumn('locked_by', Types::STRING, [ + 'notnull' => false, + 'default' => null, + ]); + } + + if (!$table->hascolumn('locked_until')) { + $table->addColumn('locked_until', Types::INTEGER, [ + 'notnull' => false, + 'default' => null, + 'comment' => 'unix-timestamp', + ]); + } + + return $schema; + } +} diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 05e8e08cc..b4fc22e70 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -104,7 +104,9 @@ * expires: int, * permissions: list, * partial: true, - * state: int + * state: int, + * lockedBy: ?string, + * lockedUntil: ?int, * } * * @psalm-type FormsForm = array{ @@ -128,6 +130,8 @@ * permissions: list, * questions: list, * state: 0|1|2, + * lockedBy: ?string, + * lockedUntil: ?int, * shares: list, * submissionCount?: int, * submissionMessage: ?string, diff --git a/lib/Service/FormsService.php b/lib/Service/FormsService.php index f25837ad2..3c07935ba 100644 --- a/lib/Service/FormsService.php +++ b/lib/Service/FormsService.php @@ -62,8 +62,8 @@ public function __construct( private CirclesService $circlesService, private IRootFolder $rootFolder, private IL10N $l10n, - private IEventDispatcher $eventDispatcher, private LoggerInterface $logger, + private IEventDispatcher $eventDispatcher, ) { $this->currentUser = $userSession->getUser(); } @@ -243,6 +243,8 @@ public function getPartialFormArray(Form $form): array { 'permissions' => $this->getPermissions($form), 'partial' => true, 'state' => $form->getState(), + 'lockedBy' => $form->getLockedBy(), + 'lockedUntil' => $form->getLockedUntil(), ]; // Append submissionCount if currentUser has permissions to see results @@ -281,6 +283,75 @@ public function getPublicForm(Form $form): array { return $formData; } + /** + * Helper that retrieves a form if the current user is allowed to edit it + * This throws an exception in case either the form is not found or permissions are missing. + * @param int $formId The form ID to retrieve + * @throws NoSuchFormException If the form was not found or the current user has no permission to edit + */ + public function getFormIfAllowed(int $formId, string $permissions = 'all'): Form { + try { + $form = $this->formMapper->findById($formId); + } catch (IMapperException $e) { + $this->logger->debug('Could not find form'); + throw new NoSuchFormException('Could not find form'); + } + + switch ($permissions) { + case Constants::PERMISSION_SUBMIT: + if (!$this->hasUserAccess($form)) { + $this->logger->debug('User has no permissions to get this form'); + throw new NoSuchFormException('User has no permissions to get this form', Http::STATUS_FORBIDDEN); + } + break; + case Constants::PERMISSION_RESULTS: + if (!$this->canSeeResults($form)) { + $this->logger->debug('The current user has no permission to get the results for this form'); + throw new NoSuchFormException('The current user has no permission to get the results for this form', Http::STATUS_FORBIDDEN); + } + break; + case Constants::PERMISSION_RESULTS_DELETE: + if (!$this->canDeleteResults($form)) { + $this->logger->debug('This form is not owned by the current user and user has no `results_delete` permission'); + throw new NoSuchFormException('This form is not owned by the current user and user has no `results_delete` permission', Http::STATUS_FORBIDDEN); + } + break; + case Constants::PERMISSION_EDIT: + if (!$this->canEditForm($form)) { + $this->logger->debug('This form is not owned by the current user and user has no `edit` permission'); + throw new NoSuchFormException('This form is not owned by the current user and user has no `edit` permission', Http::STATUS_FORBIDDEN); + } + break; + default: + // By default we request full permissions + if ($form->getOwnerId() !== $this->currentUser->getUID()) { + $this->logger->debug('This form is not owned by the current user'); + throw new NoSuchFormException('This form is not owned by the current user', Http::STATUS_FORBIDDEN); + } + break; + } + return $form; + } + + /** + * Locks the given form for the current user for a duration of 15 minutes. + * + * @param Form $form The form instance to lock. + */ + public function obtainFormLock(Form $form): void { + // Only lock if not locked or locked by current user, or lock has expired + if ( + $form->getLockedBy() !== null + && $form->getLockedBy() !== $this->currentUser->getUID() + && ($form->getLockedUntil() >= time() || $form->getLockedUntil() === 0) + ) { + throw new OCSForbiddenException('Form is currently locked by another user.'); + } + + $form->setLockedBy($this->currentUser->getUID()); + $form->setLockedUntil(time() + 15 * 60); + } + /** * Get current users permissions on a form * diff --git a/openapi.json b/openapi.json index be4bcbb84..3ed422c89 100644 --- a/openapi.json +++ b/openapi.json @@ -111,6 +111,8 @@ "permissions", "questions", "state", + "lockedBy", + "lockedUntil", "shares", "submissionMessage" ], @@ -195,6 +197,15 @@ 2 ] }, + "lockedBy": { + "type": "string", + "nullable": true + }, + "lockedUntil": { + "type": "integer", + "format": "int64", + "nullable": true + }, "shares": { "type": "array", "items": { @@ -291,7 +302,9 @@ "expires", "permissions", "partial", - "state" + "state", + "lockedBy", + "lockedUntil" ], "properties": { "id": { @@ -323,6 +336,15 @@ "state": { "type": "integer", "format": "int64" + }, + "lockedBy": { + "type": "string", + "nullable": true + }, + "lockedUntil": { + "type": "integer", + "format": "int64", + "nullable": true } } }, From 551969b0a9eebcaeb347282a0fe66cae2e8cebcb Mon Sep 17 00:00:00 2001 From: Christian Hartmann Date: Mon, 12 May 2025 23:17:55 +0200 Subject: [PATCH 2/7] add `edit` permission to sharing Signed-off-by: Christian Hartmann --- .../SidebarTabs/SharingShareDiv.vue | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/components/SidebarTabs/SharingShareDiv.vue b/src/components/SidebarTabs/SharingShareDiv.vue index abad47654..2edca1cc1 100644 --- a/src/components/SidebarTabs/SharingShareDiv.vue +++ b/src/components/SidebarTabs/SharingShareDiv.vue @@ -17,14 +17,19 @@