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

fix: Add brute force protection to form endpoints #2269

Merged
merged 3 commits into from
Jan 23, 2025
Merged

fix: Add brute force protection to form endpoints #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13870cd
fix: Add brute force protection to form endpoints
susnux Aug 7, 2024
0b7f1df
adjust integration test to new return code
Chartman123 Jan 20, 2025
4203396
fixup
susnux Jan 20, 2025
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
2 changes: 2 additions & 0 deletions lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OCA\Forms\FormsMigrator;
use OCA\Forms\Listener\AnalyticsDatasourceListener;
use OCA\Forms\Listener\UserDeletedListener;
use OCA\Forms\Middleware\ThrottleFormAccessMiddleware;
use OCA\Forms\Search\SearchProvider;
use OCP\AppFramework\App;
use OCP\AppFramework\Bootstrap\IBootContext;
Expand Down Expand Up @@ -43,6 +44,7 @@
$context->registerCapability(Capabilities::class);
$context->registerEventListener(UserDeletedEvent::class, UserDeletedListener::class);
$context->registerEventListener(DatasourceEvent::class, AnalyticsDatasourceListener::class);
$context->registerMiddleware(ThrottleFormAccessMiddleware::class);

Check warning on line 47 in lib/AppInfo/Application.php

View check run for this annotation

Codecov / codecov/patch

lib/AppInfo/Application.php#L47 

Added line #L47 was not covered by tests
$context->registerSearchProvider(SearchProvider::class);
$context->registerUserMigrator(FormsMigrator::class);
}
Expand Down
154 changes: 73 additions & 81 deletions lib/Controller/ApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use OCA\Forms\Db\SubmissionMapper;
use OCA\Forms\Db\UploadedFile;
use OCA\Forms\Db\UploadedFileMapper;
use OCA\Forms\Exception\NoSuchFormException;
use OCA\Forms\ResponseDefinitions;
use OCA\Forms\Service\ConfigService;
use OCA\Forms\Service\FormsService;
Expand All @@ -31,6 +32,7 @@
use OCP\AppFramework\Db\IMapperException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\ApiRoute;
use OCP\AppFramework\Http\Attribute\BruteForceProtection;
use OCP\AppFramework\Http\Attribute\CORS;
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
Expand Down Expand Up @@ -147,6 +149,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'POST', url: '/api/v3/forms')]
public function newForm(?int $fromId = null): DataResponse {
// Check if user is allowed
Expand Down Expand Up @@ -226,19 +229,10 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}')]
public function getForm(int $formId): DataResponse {
try {
$form = $this->formMapper->findById($formId);
} catch (IMapperException $e) {
$this->logger->debug('Could not find form');
throw new OCSBadRequestException('Could not find form');
}

if (!$this->formsService->hasUserAccess($form)) {
$this->logger->debug('User has no permissions to get this form');
throw new OCSForbiddenException('User has no permissions to get this form');
}
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_SUBMIT);

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

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L235 

Added line #L235 was not covered by tests

return new DataResponse($this->formsService->getForm($form));
}
Expand All @@ -259,6 +253,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}')]
public function updateForm(int $formId, array $keyValuePairs): DataResponse {
$this->logger->debug('Updating form: formId: {formId}, values: {keyValuePairs}', [
Expand Down Expand Up @@ -359,6 +354,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}')]
public function deleteForm(int $formId): DataResponse {
$this->logger->debug('Delete Form: {formId}', [
Expand Down Expand Up @@ -472,6 +468,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[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);
Expand Down Expand Up @@ -594,6 +591,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}')]
public function updateQuestion(int $formId, int $questionId, array $keyValuePairs): DataResponse {
$this->logger->debug('Updating question: formId: {formId}, questionId: {questionId}, values: {keyValuePairs}', [
Expand All @@ -602,6 +600,14 @@
'keyValuePairs' => $keyValuePairs
]);

// 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);
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 warning on line 608 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L605-L608 

Added lines #L605 - L608 were not covered by tests
}

try {
$question = $this->questionMapper->findById($questionId);
} catch (IMapperException $e) {
Expand All @@ -613,12 +619,6 @@
throw new OCSBadRequestException('Question doesn\'t belong to given Form');
}

$form = $this->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');
}

// Don't allow empty array
if (sizeof($keyValuePairs) === 0) {
$this->logger->info('Empty keyValuePairs, will not update.');
Expand Down Expand Up @@ -668,12 +668,20 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/questions/{questionId}')]
public function deleteQuestion(int $formId, int $questionId): DataResponse {
$this->logger->debug('Mark question as deleted: {questionId}', [
'questionId' => $questionId,
]);


$form = $this->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 warning on line 682 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L679-L682 

Added lines #L679 - L682 were not covered by tests
}

try {
$question = $this->questionMapper->findById($questionId);
} catch (IMapperException $e) {
Expand All @@ -685,12 +693,6 @@
throw new OCSBadRequestException('Question doesn\'t belong to given Form');
}

$form = $this->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');
}

// Store Order of deleted Question
$deletedOrder = $question->getOrder();

Expand Down Expand Up @@ -732,6 +734,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions')]
public function reorderQuestions(int $formId, array $newOrder): DataResponse {
$this->logger->debug('Reordering Questions on Form {formId} as Question-Ids {newOrder}', [
Expand Down Expand Up @@ -829,6 +832,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions/{questionId}/options')]
public function newOption(int $formId, int $questionId, array $optionTexts): DataResponse {
$this->logger->debug('Adding new options: formId: {formId}, questionId: {questionId}, text: {text}', [
Expand Down Expand Up @@ -909,6 +913,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/questions/{questionId}/options/{optionId}')]
public function updateOption(int $formId, int $questionId, int $optionId, array $keyValuePairs): DataResponse {
$this->logger->debug('Updating option: form: {formId}, question: {questionId}, option: {optionId}, values: {keyValuePairs}', [
Expand Down Expand Up @@ -978,6 +983,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/questions/{questionId}/options/{optionId}')]
public function deleteOption(int $formId, int $questionId, int $optionId): DataResponse {
$this->logger->debug('Deleting option: {optionId}', [
Expand Down Expand Up @@ -1037,6 +1043,7 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[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);
Expand Down Expand Up @@ -1134,19 +1141,10 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'GET', url: '/api/v3/forms/{formId}/submissions')]
public function getSubmissions(int $formId, ?string $fileFormat = null): DataResponse|DataDownloadResponse {
try {
$form = $this->formMapper->findById($formId);
} catch (IMapperException $e) {
$this->logger->debug('Could not find form');
throw new OCSNotFoundException('Could not find form');
}

if (!$this->formsService->canSeeResults($form)) {
$this->logger->debug('The current user has no permission to get the results for this form');
throw new OCSForbiddenException('The current user has no permission to get the results for this form');
}
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS);

if ($fileFormat !== null) {
$submissionsData = $this->submissionService->getSubmissionsData($form, $fileFormat);
Expand Down Expand Up @@ -1205,24 +1203,10 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/submissions')]
public function deleteAllSubmissions(int $formId): DataResponse {
$this->logger->debug('Delete all submissions to form: {formId}', [
'formId' => $formId,
]);

try {
$form = $this->formMapper->findById($formId);
} catch (IMapperException $e) {
$this->logger->debug('Could not find form');
throw new OCSNotFoundException('Could not find form');
}

// The current user has permissions to remove submissions
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 OCSForbiddenException('This form is not owned by the current user and user has no `results_delete` permission');
}
$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE);

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

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1209 

Added line #L1209 was not covered by tests

// Delete all submissions (incl. Answers)
$this->submissionMapper->deleteByForm($formId);
Expand Down Expand Up @@ -1337,31 +1321,26 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'DELETE', url: '/api/v3/forms/{formId}/submissions/{submissionId}')]
public function deleteSubmission(int $formId, int $submissionId): DataResponse {
$this->logger->debug('Delete Submission: {submissionId}', [
'submissionId' => $submissionId,
]);

$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS_DELETE);
try {
$submission = $this->submissionMapper->findById($submissionId);
$form = $this->formMapper->findById($formId);
} catch (IMapperException $e) {
$this->logger->debug('Could not find form or submission');
throw new OCSNotFoundException('Could not find form or submission');
$this->logger->debug('Could not find submission');
throw new OCSNotFoundException('Could not find submission');
}

if ($formId !== $submission->getFormId()) {
$this->logger->debug('Submission doesn\'t belong to given form');
throw new OCSBadRequestException('Submission doesn\'t belong to given form');
}

// The current user has permissions to remove submissions
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 OCSForbiddenException('This form is not owned by the current user and user has no `results_delete` permission');
}

// Delete submission (incl. Answers)
$this->submissionMapper->deleteById($submissionId);
$this->formMapper->update($form);
Expand All @@ -1383,20 +1362,10 @@
*/
#[CORS()]
#[NoAdminRequired()]
#[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) {
try {
$form = $this->formMapper->findById($formId);
} catch (IMapperException $e) {
$this->logger->debug('Could not find form');
throw new OCSNotFoundException('Could not find form');
}

if (!$this->formsService->canSeeResults($form)) {
$this->logger->debug('The current user has no permission to get the results for this form');
throw new OCSForbiddenException('The current user has no permission to get the results for this form');
}

$form = $this->getFormIfAllowed($formId, Constants::PERMISSION_RESULTS);
$file = $this->submissionService->writeFileToCloud($form, $path, $fileFormat);

return new DataResponse($file->getName());
Expand All @@ -1421,8 +1390,9 @@
* 200: the file id and name of the uploaded file
*/
#[CORS()]
#[NoAdminRequired()]
#[PublicPage()]
#[NoAdminRequired()]
#[BruteForceProtection(action: 'form')]
#[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/submissions/files/{questionId}')]
public function uploadFiles(int $formId, int $questionId, string $shareHash = ''): DataResponse {
$this->logger->debug('Uploading files for formId: {formId}, questionId: {questionId}', [
Expand Down Expand Up @@ -1614,7 +1584,7 @@
$form = $this->formMapper->findById($formId);
} catch (IMapperException $e) {
$this->logger->debug('Could not find form');
throw new OCSNotFoundException('Could not find form');
throw new NoSuchFormException('Could not find form');
}

// Does the user have access to the form (Either by logged-in user, or by providing public share-hash.)
Expand All @@ -1634,7 +1604,7 @@
} finally {
// Now forbid, if no public share and no direct share.
if (!$isPublicShare && !$this->formsService->hasUserAccess($form)) {
throw new OCSForbiddenException('Not allowed to access this form');
throw new NoSuchFormException('Not allowed to access this form', Http::STATUS_FORBIDDEN);
}
}

Expand All @@ -1650,20 +1620,42 @@
* 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 OCSNotFoundException If the form was not found
* @throws OCSForbiddenException If the current user has no permission to edit
* @throws NoSuchFormException If the form was not found or the current user has no permission to edit
*/
private function getFormIfAllowed(int $formId): Form {
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 OCSNotFoundException('Could not find form');
throw new NoSuchFormException('Could not find 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');
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);

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

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1634-L1637 

Added lines #L1634 - L1637 were not covered by tests
}
break;
case Constants::PERMISSION_RESULTS:

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

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1639-L1640 

Added lines #L1639 - L1640 were not covered by tests
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:

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

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1646 

Added line #L1646 was not covered by tests
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;
}
Expand Down
Loading
Loading