From 2bdce3f2c6e5aba82a47bfd30dde29e15343306a Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 10 Jan 2025 14:16:36 +0100 Subject: [PATCH 1/3] fix(api): Make sure user has permissions to update access fields Signed-off-by: Ferdinand Thiessen --- lib/Controller/ApiController.php | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 6fd32c8e8..3f44a8e19 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -287,6 +287,17 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { throw new OCSForbiddenException(); } + // Do not allow changing showToAllUsers if disabled + if (isset($keyValuePairs['access'])) { + $showAll = $keyValuePairs['access']['showToAllUsers'] ?? false; + $permitAll = $keyValuePairs['access']['permitAllUsers'] ?? false; + if (($showAll && !$this->configService->getAllowShowToAll()) + || ($permitAll && !$this->configService->getAllowPermitAll())) { + $this->logger->info('Not allowed to update showToAllUsers or permitAllUsers'); + throw new OCSForbiddenException(); + } + } + // Process file linking if (isset($keyValuePairs['path']) && isset($keyValuePairs['fileFormat'])) { $file = $this->submissionService->writeFileToCloud($form, $keyValuePairs['path'], $keyValuePairs['fileFormat']); @@ -1627,6 +1638,17 @@ public function updateFormLegacy(int $id, array $keyValuePairs): DataResponse { throw new OCSForbiddenException(); } + // Do not allow changing showToAllUsers if disabled + if (isset($keyValuePairs['access'])) { + $showAll = $keyValuePairs['access']['showToAllUsers'] ?? false; + $permitAll = $keyValuePairs['access']['permitAllUsers'] ?? false; + if (($showAll && !$this->configService->getAllowShowToAll()) + || ($permitAll && !$this->configService->getAllowPermitAll())) { + $this->logger->info('Not allowed to update showToAllUsers or permitAllUsers'); + throw new OCSForbiddenException(); + } + } + // Create FormEntity with given Params & Id. foreach ($keyValuePairs as $key => $value) { $method = 'set' . ucfirst($key); From d60ba56e2ad738f1bb44f2f832f58f4cdf76894b Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 10 Jan 2025 14:36:23 +0100 Subject: [PATCH 2/3] fix(db): Only query public shared forms if enabled by admin Signed-off-by: Ferdinand Thiessen --- lib/Db/FormMapper.php | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/lib/Db/FormMapper.php b/lib/Db/FormMapper.php index 2d8a28b9c..efb701fce 100644 --- a/lib/Db/FormMapper.php +++ b/lib/Db/FormMapper.php @@ -27,6 +27,7 @@ namespace OCA\Forms\Db; use OCA\Forms\Constants; +use OCA\Forms\Service\ConfigService; use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -43,10 +44,11 @@ class FormMapper extends QBMapper { * @param IDBConnection $db */ public function __construct( + IDBConnection $db, private QuestionMapper $questionMapper, private ShareMapper $shareMapper, private SubmissionMapper $submissionMapper, - IDBConnection $db, + private ConfigService $configService, ) { parent::__construct($db, 'forms_v2_forms', Form::class); } @@ -151,20 +153,23 @@ public function findSharedForms(string $userId, array $groups = [], array $teams } // build expression for publicy shared forms (default only directly shown) - if ($filterShown) { - // Only shown - $access = $qbShares->expr()->in('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_ARRAY_SHOWN, IQueryBuilder::PARAM_INT_ARRAY)); - } else { - // All - $access = $qbShares->expr()->neq('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_NOPUBLICSHARE, IQueryBuilder::PARAM_INT)); + if ($this->configService->getAllowPermitAll()) { + if ($filterShown && $this->configService->getAllowShowToAll()) { + // Only shown forms + $access = $qbShares->expr()->in('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_ARRAY_SHOWN, IQueryBuilder::PARAM_INT_ARRAY, ':access_shown')); + } elseif ($filterShown === false) { + // All + $access = $qbShares->expr()->neq('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_NOPUBLICSHARE, IQueryBuilder::PARAM_INT, ':access_nopublicshare')); + } } + // Build the where clause for membership or public access + $memberOrPublic = isset($access) ? $qbShares->expr()->orX($memberships, $access) : $memberships; // Select all DISTINCT IDs of shared forms $qbShares->selectDistinct('forms.id') ->from($this->getTableName(), 'forms') ->leftJoin('forms', $this->shareMapper->getTableName(), 'shares', $qbShares->expr()->eq('forms.id', 'shares.form_id')) - ->where($memberships) - ->orWhere($access) + ->where($memberOrPublic) ->andWhere($qbShares->expr()->neq('forms.owner_id', $qbShares->createNamedParameter($userId, IQueryBuilder::PARAM_STR))); // Select the whole forms for the DISTINCT shared forms IDs From 904d9d79b664712d61d9b5e7b338f4f978ac2aa2 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 10 Jan 2025 14:38:02 +0100 Subject: [PATCH 3/3] test: Add test cases for admin settings possibly influencing the API Signed-off-by: Ferdinand Thiessen --- .github/workflows/phpunit-sqlite.yml | 4 +- .../Api/RespectAdminSettingsTest.php | 323 ++++++++++++++++++ tests/Integration/DB/SharedFormsTest.php | 84 +++++ tests/Integration/IntegrationBase.php | 17 +- 4 files changed, 423 insertions(+), 5 deletions(-) create mode 100644 tests/Integration/Api/RespectAdminSettingsTest.php diff --git a/.github/workflows/phpunit-sqlite.yml b/.github/workflows/phpunit-sqlite.yml index 8c4f82c87..a5bd5cff9 100644 --- a/.github/workflows/phpunit-sqlite.yml +++ b/.github/workflows/phpunit-sqlite.yml @@ -175,13 +175,13 @@ jobs: working-directory: apps/${{ env.APP_NAME }} run: composer run test:integration - - name: Upload Unit coverage + - name: Upload integration coverage uses: codecov/codecov-action@e28ff129e5465c2c0dcc6f003fc735cb6ae0c673 # v4.5.0 env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} with: working-directory: apps/${{ env.APP_NAME }} - file: tests/clover.unit.xml + file: tests/clover.integration.xml fail_ci_if_error: true flags: integration diff --git a/tests/Integration/Api/RespectAdminSettingsTest.php b/tests/Integration/Api/RespectAdminSettingsTest.php new file mode 100644 index 000000000..622fe2a38 --- /dev/null +++ b/tests/Integration/Api/RespectAdminSettingsTest.php @@ -0,0 +1,323 @@ + 'Test user', + ]; + + /** + * Store Test Forms Array. + * Necessary as function due to object type-casting. + */ + private function setTestForms() { + $this->testForms = [ + [ + 'hash' => 'abcdefghij123456', + 'title' => 'Title of owned Form', + 'description' => '', + 'owner_id' => 'test', + 'access_enum' => 0, + 'created' => 12345, + 'expires' => 0, + 'state' => 0, + 'is_anonymous' => false, + 'submit_multiple' => false, + 'show_expiration' => false, + 'last_updated' => 123456789, + 'submission_message' => '', + 'file_id' => null, + 'file_format' => null, + 'questions' => [], + 'shares' => [], + 'submissions' => [], + ], + [ + 'hash' => '1234567890abcdef', + 'title' => 'Title of a globally shared Form', + 'description' => '', + 'owner_id' => 'test1', + 'access_enum' => 2, + 'created' => 12345, + 'expires' => 0, + 'state' => 0, + 'is_anonymous' => false, + 'submit_multiple' => false, + 'show_expiration' => false, + 'last_updated' => 123456789, + 'submission_message' => '', + 'file_id' => null, + 'file_format' => null, + 'questions' => [], + 'shares' => [], + 'submissions' => [], + ], + [ + 'hash' => 'bcdf011899881', + 'title' => 'Title of a directly shared Form', + 'description' => '', + 'owner_id' => 'test1', + 'access_enum' => 0, + 'created' => 12345, + 'expires' => 0, + 'state' => 0, + 'is_anonymous' => false, + 'submit_multiple' => false, + 'show_expiration' => false, + 'last_updated' => 123456789, + 'submission_message' => '', + 'file_id' => null, + 'file_format' => null, + 'questions' => [], + 'shares' => [ + [ + 'shareType' => 0, + 'shareWith' => 'test', + 'permissions' => ['submit'], + ], + ], + 'submissions' => [], + ], + ]; + } + + private static function sharedTestForms(): array { + return [ + [ + 'hash' => 'abcdefghij123456', + 'title' => 'Title of owned Form', + 'description' => '', + 'created' => 12345, + 'expires' => 0, + 'state' => 0, + 'questions' => [], + 'shares' => [], + 'ownerId' => 'test', + 'fileId' => null, + 'fileFormat' => null, + 'access' => [ + 'permitAllUsers' => false, + 'showToAllUsers' => false, + ], + 'isAnonymous' => false, + 'submitMultiple' => false, + 'showExpiration' => false, + 'submissionMessage' => '', + 'permissions' => [ + 'edit', + 'results', + 'results_delete', + 'submit', + 'embed', + ], + 'canSubmit' => true, + 'submissionCount' => 0, + ], + ]; + } + + /** + * Set up test environment. + * Writing testforms into db, preparing http request + */ + public function setUp(): void { + $this->setTestForms(); + $this->users = [ + 'test' => 'Test Displayname', + 'user1' => 'User No. 1', + ]; + + parent::setUp(); + + // Set up http Client + $this->http = new Client([ + 'base_uri' => 'http://localhost:8080/ocs/v2.php/apps/forms/', + 'auth' => ['test', 'test'], + 'headers' => [ + 'OCS-ApiRequest' => 'true', + 'Accept' => 'application/json' + ], + ]); + } + + public function tearDown(): void { + parent::tearDown(); + } + + // Small Wrapper for OCS-Response + private function OcsResponse2Data($resp) { + $arr = json_decode($resp->getBody()->getContents(), true); + return $arr['ocs']['data']; + } + + /** + * Allow to update form if there are no admin settings + */ + public function testAllowUpdate(): void { + $resp = $this->http->request( + 'PATCH', + "api/v3/forms/{$this->testForms[0]['id']}", + [ + 'json' => [ + 'keyValuePairs' => ['access' => ['permitAllUsers' => true, 'showToAllUsers' => true]], + ], + ], + ); + $this->assertEquals(200, $resp->getStatusCode()); + + $resp = $this->http->request( + 'GET', + "api/v3/forms/{$this->testForms[0]['id']}", + ); + $data = $this->OcsResponse2Data($resp); + // we do not know the ID and the update time is flaky + unset($data['id']); + unset($data['lastUpdated']); + + $expected = self::sharedTestForms()[0]; + $expected['access'] = ['permitAllUsers' => true, 'showToAllUsers' => true]; + + $this->assertEquals(200, $resp->getStatusCode()); + $this->assertEquals($expected, $data); + } + + /** + * Forbid to update form if there are admin settings + * @dataProvider forbidUpdateAdminSettingsData + */ + public function testForbidUpdate(array $accessValue, array $adminConfigKeys): void { + $config = \OCP\Server::get(IConfig::class); + foreach ($adminConfigKeys as $key => $value) { + $config->setAppValue(Application::APP_ID, $key, $value); + } + + $resp = $this->http->request( + 'PATCH', + "api/v3/forms/{$this->testForms[0]['id']}", + [ + 'json' => [ + 'keyValuePairs' => ['access' => $accessValue], + ], + // do not throw on 403 + 'http_errors' => false, + ], + ); + $this->assertEquals(403, $resp->getStatusCode()); + + $resp = $this->http->request( + 'GET', + "api/v3/forms/{$this->testForms[0]['id']}", + ); + $data = $this->OcsResponse2Data($resp); + // we do not know the ID or the update + unset($data['id']); + unset($data['lastUpdated']); + + $this->assertEquals(200, $resp->getStatusCode()); + $this->assertEquals(self::sharedTestForms()[0], $data); + } + + public static function forbidUpdateAdminSettingsData(): array { + return [ + 'set both without show-to-all permission' => [ + [ + 'permitAllUsers' => true, + 'showToAllUsers' => true, + ], + [ + Constants::CONFIG_KEY_ALLOWSHOWTOALL => 'false', + Constants::CONFIG_KEY_ALLOWPERMITALL => 'true', + ], + ], + 'set both without permit-all permission' => [ + [ + 'permitAllUsers' => true, + 'showToAllUsers' => true, + ], + [ + Constants::CONFIG_KEY_ALLOWSHOWTOALL => 'true', + Constants::CONFIG_KEY_ALLOWPERMITALL => 'false', + ], + ], + 'set show-to-all without permission' => [ + [ + 'showToAllUsers' => true, + ], + [ + Constants::CONFIG_KEY_ALLOWSHOWTOALL => 'false', + Constants::CONFIG_KEY_ALLOWPERMITALL => 'true', + ], + ], + 'set permit-all without permission' => [ + [ + 'permitAllUsers' => true, + ], + [ + Constants::CONFIG_KEY_ALLOWSHOWTOALL => 'true', + Constants::CONFIG_KEY_ALLOWPERMITALL => 'false', + ], + ], + ]; + } + + /** + * Test that forms with public access are listed + */ + public function testListFormsAllowed(): void { + $resp = $this->http->request( + 'GET', + 'api/v3/forms?type=shared', + ); + $this->assertEquals(200, $resp->getStatusCode()); + + $data = $this->OcsResponse2Data($resp); + $this->assertEqualsCanonicalizing( + [ + 'Title of a globally shared Form', + 'Title of a directly shared Form', + ], + array_map(fn ($form) => $form['title'], $data), + ); + } + + /** + * Test that only forms directly shared are listed if the admin setting forbid access to the form. + * Equivalent to creating form with "show to all" permission, but then the admin deactivates the "show all" globally. + */ + public function testListFormsNoAdminPermission(): void { + // Disable global access + \OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, 'false'); + + $resp = $this->http->request( + 'GET', + 'api/v3/forms?type=shared', + ); + $this->assertEquals(200, $resp->getStatusCode()); + + $data = $this->OcsResponse2Data($resp); + $this->assertEqualsCanonicalizing( + ['Title of a directly shared Form'], + array_map(fn ($form) => $form['title'], $data), + ); + } + +}; diff --git a/tests/Integration/DB/SharedFormsTest.php b/tests/Integration/DB/SharedFormsTest.php index 60a43e69e..214773287 100644 --- a/tests/Integration/DB/SharedFormsTest.php +++ b/tests/Integration/DB/SharedFormsTest.php @@ -25,9 +25,11 @@ */ namespace OCA\Forms\Tests\Integration\Api; +use OCA\Forms\AppInfo\Application; use OCA\Forms\Constants; use OCA\Forms\Db\FormMapper; use OCA\Forms\Tests\Integration\IntegrationBase; +use OCP\IConfig; /** * @group DB @@ -189,4 +191,86 @@ public function testPublicSharedForms() { array_map(fn ($form) => $form->read()['hash'], $forms), ); } + + /** + * Test that no public shared forms are shown to user if admin disabled it + * @dataProvider dataForbidPublicShowAccess + */ + public function testShowNoSharedFormsIfDisabled(array $configValues) { + $config = \OCP\Server::get(IConfig::class); + foreach ($configValues as $key => $value) { + $config->setAppValue(Application::APP_ID, $key, json_encode($value)); + } + + $formMapper = \OCP\Server::get(FormMapper::class); + $forms = $formMapper->findSharedForms('user1'); + + $this->assertEquals(2, count($forms)); + $this->assertEqualsCanonicalizing( + ['aaaa', 'dddd'], + array_map(fn ($form) => $form->read()['hash'], $forms), + ); + } + + /** + * Test that a form with public access can be accessed even if show permissions are not granted (can fill out but not see in sidebar) + */ + public function testAllowPublicAccessOnDeniedPublicVisibility(): void { + $config = \OCP\Server::get(IConfig::class); + $config->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, json_encode(false)); + + $formMapper = \OCP\Server::get(FormMapper::class); + $forms = $formMapper->findSharedForms('user1', filterShown: false); + + $this->assertEqualsCanonicalizing( + ['aaaa', 'bbbb', 'cccc', 'dddd'], + array_map(fn ($form) => $form->read()['hash'], $forms), + ); + } + + /** + * Test that no public shared forms are available to user if admin disabled it + * @dataProvider dataForbidPublicAccess + */ + public function testShowNoSharedFormsAccessIfDisabled(array $configValues): void { + $config = \OCP\Server::get(IConfig::class); + foreach ($configValues as $key => $value) { + $config->setAppValue(Application::APP_ID, $key, json_encode($value)); + } + + $formMapper = \OCP\Server::get(FormMapper::class); + $forms = $formMapper->findSharedForms('user1', filterShown: false); + + $this->assertEquals(2, count($forms)); + $this->assertEqualsCanonicalizing( + ['aaaa', 'dddd'], + array_map(fn ($form) => $form->read()['hash'], $forms), + ); + } + + public static function dataForbidPublicAccess(): array { + return [ + 'no-permit' => [ + [ + Constants::CONFIG_KEY_ALLOWPERMITALL => false, + ], + ], + 'non-at-all' => [ + [ + Constants::CONFIG_KEY_ALLOWSHOWTOALL => false, + Constants::CONFIG_KEY_ALLOWPERMITALL => false, + ], + ], + ]; + } + + public static function dataForbidPublicShowAccess(): array { + return array_merge(self::dataForbidPublicAccess(), [ + 'no-show-to-all' => [ + [ + Constants::CONFIG_KEY_ALLOWSHOWTOALL => false, + ], + ], + ]); + } } diff --git a/tests/Integration/IntegrationBase.php b/tests/Integration/IntegrationBase.php index 3ad35615d..9001ac6d9 100644 --- a/tests/Integration/IntegrationBase.php +++ b/tests/Integration/IntegrationBase.php @@ -24,7 +24,12 @@ */ namespace OCA\Forms\Tests\Integration; +use OCA\Forms\AppInfo\Application; +use OCA\Forms\Constants; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\IConfig; +use OCP\IDBConnection; +use OCP\IUserManager; use Test\TestCase; /** @@ -47,7 +52,12 @@ class IntegrationBase extends TestCase { public function setUp(): void { parent::setUp(); - $userManager = \OC::$server->getUserManager(); + $config = \OCP\Server::get(IConfig::class); + foreach (Constants::CONFIG_KEYS as $key) { + $config->deleteAppValue(Application::APP_ID, $key); + } + + $userManager = \OCP\Server::get(IUserManager::class); foreach ($this->users as $userId => $displayName) { $user = $userManager->get($userId); if ($user === null) { @@ -56,7 +66,7 @@ public function setUp(): void { $user->setDisplayName($displayName); } - $qb = \OC::$server->getDatabaseConnection()->getQueryBuilder(); + $qb = \OCP\Server::get(IDBConnection::class)->getQueryBuilder(); // Write our test forms into db foreach ($this->testForms as $index => $form) { @@ -153,7 +163,8 @@ public function setUp(): void { /** Clean up database from testforms */ public function tearDown(): void { - $qb = \OC::$server->getDatabaseConnection()->getQueryBuilder(); + $db = \OCP\Server::get(IDBConnection::class); + $qb = $db->getQueryBuilder(); foreach ($this->testForms as $form) { $qb->delete('forms_v2_forms')