Skip to content

Commit 814b4f3

Browse files
committed
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 <chris-hartmann@gmx.de>
1 parent 75cebed commit 814b4f3

8 files changed

Lines changed: 191 additions & 22 deletions

File tree

docs/API_v3.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ Returns condensed objects of all Forms beeing owned by the authenticated user.
9090
"submit"
9191
],
9292
"partial": true,
93-
"state": 0
93+
"state": 0,
94+
"lockedBy": null,
95+
"lockedUntil": null
9496
},
9597
{
9698
"id": 3,
@@ -103,7 +105,9 @@ Returns condensed objects of all Forms beeing owned by the authenticated user.
103105
"submit"
104106
],
105107
"partial": true,
106-
"state": 0
108+
"state": 0,
109+
"lockedBy": "someUser"
110+
"lockedUntil": 123456789
107111
}
108112
]
109113
```
@@ -166,6 +170,8 @@ Returns the full-depth object of the requested form (without submissions).
166170
"showExpiration": false,
167171
"canSubmit": true,
168172
"state": 0,
173+
"lockedBy": null,
174+
"lockedUntil": null,
169175
"permissions": [
170176
"edit",
171177
"results",

docs/DataStructure.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ This document describes the Object-Structure, that is used within the Forms App
2626
| expires | unix-timestamp | | When the form should expire. Timestamp `0` indicates _never_ |
2727
| isAnonymous | Boolean | | If Answers will be stored anonymously |
2828
| state | Integer | [Form state](#form-state) | The state of the form |
29+
| lockedBy | String | | The user ID for who has exclusive edit access at the moment |
30+
| lockedUntil | unix timestamp | | When the form lock will expire |
2931
| submitMultiple | Boolean | | If users are allowed to submit multiple times to the form |
3032
| allowEditSubmissions | Boolean | | If users are allowed to edit or delete their response |
3133
| showExpiration | Boolean | | If the expiration date will be shown on the form |
@@ -57,6 +59,8 @@ This document describes the Object-Structure, that is used within the Forms App
5759
],
5860
"questions": [],
5961
"state": 0,
62+
"lockedBy": null,
63+
"lockedUntil": null,
6064
"shares": []
6165
"submissions": [],
6266
}

lib/Controller/ApiController.php

Lines changed: 100 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
use OCP\IUser;
5353
use OCP\IUserManager;
5454
use OCP\IUserSession;
55-
55+
use Psalm\Internal\Scanner\UnresolvedConstant\KeyValuePair;
5656
use Psr\Log\LoggerInterface;
5757

5858
/**
@@ -190,6 +190,8 @@ public function newForm(?int $fromId = null): DataResponse {
190190
unset($formData['state']);
191191
unset($formData['fileId']);
192192
unset($formData['fileFormat']);
193+
unset($formData['locked_by']);
194+
unset($formData['locked_until']);
193195
$formData['hash'] = $this->formsService->generateFormHash();
194196
// TRANSLATORS Appendix to the form Title of a duplicated/copied form.
195197
$formData['title'] .= ' - ' . $this->l10n->t('Copy');
@@ -267,7 +269,9 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse {
267269
'keyValuePairs' => $keyValuePairs
268270
]);
269271

270-
$form = $this->getFormIfAllowed($formId);
272+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
273+
$currentUserId = $this->currentUser->getUID();
274+
$this->obtainFormLock($form);
271275

272276
// Don't allow empty array
273277
if (sizeof($keyValuePairs) === 0) {
@@ -277,6 +281,12 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse {
277281

278282
// Process owner transfer
279283
if (sizeof($keyValuePairs) === 1 && key_exists('ownerId', $keyValuePairs)) {
284+
// Only allow owner transfer if current user is the form owner
285+
if ($currentUserId !== $form->getOwnerId()) {
286+
$this->logger->debug('Only the form owner can transfer ownership');
287+
throw new OCSForbiddenException('Only the form owner can transfer ownership');
288+
}
289+
280290
$this->logger->debug('Updating owner: formId: {formId}, userId: {uid}', [
281291
'formId' => $formId,
282292
'uid' => $keyValuePairs['ownerId']
@@ -288,23 +298,55 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse {
288298
throw new OCSBadRequestException('Could not find new form owner');
289299
}
290300

291-
// update form owner
301+
// update form owner and remove form lock
292302
$form->setOwnerId($keyValuePairs['ownerId']);
303+
$form->setLockedBy(null);
304+
$form->setLockedUntil(null);
293305

294306
// Update changed Columns in Db.
295307
$this->formMapper->update($form);
296308

297309
return new DataResponse($form->getOwnerId());
298310
}
299311

300-
// Don't allow to change params id, hash, ownerId, created, lastUpdated, fileId
312+
// Process form unlocking
301313
if (
302-
key_exists('id', $keyValuePairs) || key_exists('hash', $keyValuePairs) ||
303-
key_exists('ownerId', $keyValuePairs) || key_exists('created', $keyValuePairs) ||
304-
isset($keyValuePairs['fileId']) || key_exists('lastUpdated', $keyValuePairs)
314+
sizeof($keyValuePairs) === 2
315+
&& array_key_exists('locked_by', $keyValuePairs) && is_null($keyValuePairs['locked_by'])
316+
&& array_key_exists('locked_until', $keyValuePairs) && is_null($keyValuePairs['locked_until'])
305317
) {
306-
$this->logger->info('Not allowed to update id, hash, ownerId, created, fileId or lastUpdated');
307-
throw new OCSForbiddenException('Not allowed to update id, hash, ownerId, created, fileId or lastUpdated');
318+
// Only allow form unlocking if for form owner or lock user
319+
if ($currentUserId !== $form->getOwnerId() && $currentUserId !== $form->getLockedBy()) {
320+
$this->logger->debug('Only the form owner or the user who obtained the lock can unlock the form');
321+
throw new OCSForbiddenException('Only the form owner or the user who obtained the lock can unlock the form');
322+
}
323+
324+
// remove form lock
325+
$form->setLockedBy(null);
326+
$form->setLockedUntil(null);
327+
328+
// Update changed Columns in Db.
329+
$this->formMapper->update($form);
330+
331+
return new DataResponse($form->getId());
332+
}
333+
334+
// Don't allow to change the following attributes
335+
$forbiddenKeys = [
336+
'id', 'hash', 'ownerId', 'created', 'lastUpdated', 'lockedBy', 'lockedUntil'
337+
];
338+
339+
foreach ($forbiddenKeys as $key) {
340+
if (array_key_exists($key, $keyValuePairs)) {
341+
$this->logger->info("Not allowed to update {$key}");
342+
throw new OCSForbiddenException("Not allowed to update {$key}");
343+
}
344+
}
345+
346+
// Don't allow to change fileId
347+
if (isset($keyValuePairs['fileId'])) {
348+
$this->logger->info('Not allowed to update fileId');
349+
throw new OCSForbiddenException('Not allowed to update fileId');
308350
}
309351

310352
// Do not allow changing showToAllUsers if disabled
@@ -477,7 +519,9 @@ public function getQuestion(int $formId, int $questionId): DataResponse {
477519
#[BruteForceProtection(action: 'form')]
478520
#[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions')]
479521
public function newQuestion(int $formId, ?string $type = null, string $text = '', ?int $fromId = null): DataResponse {
480-
$form = $this->getFormIfAllowed($formId);
522+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
523+
$this->obtainFormLock($form);
524+
481525
if ($this->formsService->isFormArchived($form)) {
482526
$this->logger->debug('This form is archived and can not be modified');
483527
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -608,7 +652,9 @@ public function updateQuestion(int $formId, int $questionId, array $keyValuePair
608652

609653
// Make sure we query the form first to check the user has permissions
610654
// So the user does not get information about "questions" if they do not even have permissions to the form
611-
$form = $this->getFormIfAllowed($formId);
655+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
656+
$this->obtainFormLock($form);
657+
612658
if ($this->formsService->isFormArchived($form)) {
613659
$this->logger->debug('This form is archived and can not be modified');
614660
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -682,7 +728,9 @@ public function deleteQuestion(int $formId, int $questionId): DataResponse {
682728
]);
683729

684730

685-
$form = $this->getFormIfAllowed($formId);
731+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
732+
$this->obtainFormLock($form);
733+
686734
if ($this->formsService->isFormArchived($form)) {
687735
$this->logger->debug('This form is archived and can not be modified');
688736
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -748,7 +796,9 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse {
748796
'newOrder' => $newOrder
749797
]);
750798

751-
$form = $this->getFormIfAllowed($formId);
799+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
800+
$this->obtainFormLock($form);
801+
752802
if ($this->formsService->isFormArchived($form)) {
753803
$this->logger->debug('This form is archived and can not be modified');
754804
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -847,7 +897,9 @@ public function newOption(int $formId, int $questionId, array $optionTexts): Dat
847897
'text' => $optionTexts,
848898
]);
849899

850-
$form = $this->getFormIfAllowed($formId);
900+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
901+
$this->obtainFormLock($form);
902+
851903
if ($this->formsService->isFormArchived($form)) {
852904
$this->logger->debug('This form is archived and can not be modified');
853905
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -929,7 +981,9 @@ public function updateOption(int $formId, int $questionId, int $optionId, array
929981
'keyValuePairs' => $keyValuePairs
930982
]);
931983

932-
$form = $this->getFormIfAllowed($formId);
984+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
985+
$this->obtainFormLock($form);
986+
933987
if ($this->formsService->isFormArchived($form)) {
934988
$this->logger->debug('This form is archived and can not be modified');
935989
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -996,7 +1050,9 @@ public function deleteOption(int $formId, int $questionId, int $optionId): DataR
9961050
'optionId' => $optionId
9971051
]);
9981052

999-
$form = $this->getFormIfAllowed($formId);
1053+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
1054+
$this->obtainFormLock($form);
1055+
10001056
if ($this->formsService->isFormArchived($form)) {
10011057
$this->logger->debug('This form is archived and can not be modified');
10021058
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -1052,7 +1108,9 @@ public function deleteOption(int $formId, int $questionId, int $optionId): DataR
10521108
#[BruteForceProtection(action: 'form')]
10531109
#[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}/options')]
10541110
public function reorderOptions(int $formId, int $questionId, array $newOrder) {
1055-
$form = $this->getFormIfAllowed($formId);
1111+
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_EDIT);
1112+
$this->obtainFormLock($form);
1113+
10561114
if ($this->formsService->isFormArchived($form)) {
10571115
$this->logger->debug('This form is archived and can not be modified');
10581116
throw new OCSForbiddenException('This form is archived and can not be modified');
@@ -1790,6 +1848,12 @@ private function getFormIfAllowed(int $formId, string $permissions = 'all'): For
17901848
throw new NoSuchFormException('This form is not owned by the current user and user has no `results_delete` permission', Http::STATUS_FORBIDDEN);
17911849
}
17921850
break;
1851+
case Constants::PERMISSION_EDIT:
1852+
if (!$this->formsService->canEditForm($form)) {
1853+
$this->logger->debug('This form is not owned by the current user and user has no `edit` permission');
1854+
throw new NoSuchFormException('This form is not owned by the current user and user has no `edit` permission', Http::STATUS_FORBIDDEN);
1855+
}
1856+
break;
17931857
default:
17941858
// By default we request full permissions
17951859
if ($form->getOwnerId() !== $this->currentUser->getUID()) {
@@ -1800,4 +1864,23 @@ private function getFormIfAllowed(int $formId, string $permissions = 'all'): For
18001864
}
18011865
return $form;
18021866
}
1867+
1868+
/**
1869+
* Locks the given form for the current user for a duration of 15 minutes.
1870+
*
1871+
* @param Form $form The form instance to lock.
1872+
*/
1873+
private function obtainFormLock(Form $form): void {
1874+
// Only lock if not locked or locked by current user, or lock has expired
1875+
if (
1876+
$form->getLockedBy() === null ||
1877+
$form->getLockedBy() === $this->currentUser->getUID() ||
1878+
$form->getLockedUntil() < time()
1879+
) {
1880+
$form->setLockedBy($this->currentUser->getUID());
1881+
$form->setLockedUntil(time() + 15 * 60);
1882+
} else {
1883+
throw new OCSForbiddenException('Form is currently locked by another user.');
1884+
}
1885+
}
18031886
}

lib/Db/Form.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@
4747
* @psalm-method 0|1|2 getState()
4848
* @method void setState(int|null $value)
4949
* @psalm-method void setState(0|1|2|null $value)
50+
* @method string getLockedBy()
51+
* @method void setLockedBy(string $value)
52+
* @method int getLockedUntil()
53+
* @method void setLockedUntil(int $value)
5054
*/
5155
class Form extends Entity {
5256
protected $hash;
@@ -65,6 +69,8 @@ class Form extends Entity {
6569
protected $submissionMessage;
6670
protected $lastUpdated;
6771
protected $state;
72+
protected $lockedBy;
73+
protected $lockedUntil;
6874

6975
/**
7076
* Form constructor.
@@ -78,6 +84,8 @@ public function __construct() {
7884
$this->addType('showExpiration', 'boolean');
7985
$this->addType('lastUpdated', 'integer');
8086
$this->addType('state', 'integer');
87+
$this->addType('locked_by', 'string');
88+
$this->addType('locked_until', 'integer');
8189
}
8290

8391
// JSON-Decoding of access-column.
@@ -149,6 +157,8 @@ public function setAccess(array $access): void {
149157
* lastUpdated: int,
150158
* submissionMessage: ?string,
151159
* state: 0|1|2,
160+
* locked_by: ?string,
161+
* locked_until: ?int,
152162
* }
153163
*/
154164
public function read() {
@@ -170,6 +180,8 @@ public function read() {
170180
'lastUpdated' => (int)$this->getLastUpdated(),
171181
'submissionMessage' => $this->getSubmissionMessage(),
172182
'state' => $this->getState(),
183+
'locked_by' => $this->getLockedBy(),
184+
'locked_until' => $this->getLockedUntil(),
173185
];
174186
}
175187
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Forms\Migration;
11+
12+
use Closure;
13+
use OCP\DB\ISchemaWrapper;
14+
use OCP\DB\Types;
15+
use OCP\Migration\IOutput;
16+
use OCP\Migration\SimpleMigrationStep;
17+
18+
class Version050200Date20250512004000 extends SimpleMigrationStep {
19+
20+
/**
21+
* @param IOutput $output
22+
* @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
23+
* @param array $options
24+
* @return null|ISchemaWrapper
25+
*/
26+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
27+
/** @var ISchemaWrapper $schema */
28+
$schema = $schemaClosure();
29+
$table = $schema->getTable('forms_v2_forms');
30+
31+
if (!$table->hasColumn('locked_by')) {
32+
$table->addColumn('locked_by', Types::STRING, [
33+
'notnull' => false,
34+
'default' => null,
35+
]);
36+
}
37+
38+
if (!$table->hascolumn('locked_until')) {
39+
$table->addColumn('locked_until', Types::INTEGER, [
40+
'notnull' => false,
41+
'default' => null,
42+
'comment' => 'unix-timestamp',
43+
]);
44+
}
45+
46+
return $schema;
47+
}
48+
}

lib/ResponseDefinitions.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@
117117
* expires: int,
118118
* fileFormat: ?string,
119119
* fileId: ?int,
120-
* filePath?: ?string,
120+
* filePath?: string,
121121
* isAnonymous: bool,
122122
* lastUpdated: int,
123123
* submitMultiple: bool,
@@ -127,6 +127,8 @@
127127
* permissions: list<FormsPermission>,
128128
* questions: list<FormsQuestion>,
129129
* state: 0|1|2,
130+
* lockedBy: ?string,
131+
* lockedUntil: ?int,
130132
* shares: list<FormsShare>,
131133
* submissionCount?: int,
132134
* submissionMessage: ?string,

0 commit comments

Comments
 (0)