Skip to content

fix: Check admin settings when fetching shared forms #2485

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

Merged
Chartman123 merged 3 commits into from
Jan 11, 2025
Merged

fix: Check admin settings when fetching shared forms #2485

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
2dbbee2
fix(api): Make sure user has permissions to update access fields
susnux Jan 10, 2025
5ca6231
fix(db): Only query public shared forms if enabled by admin
susnux Jan 10, 2025
a467120
test: Add test cases for admin settings possibly influencing the API
susnux Jan 10, 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
11 changes: 11 additions & 0 deletions lib/Controller/ApiController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,17 @@
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();

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

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L282-L288 

Added lines #L282 - L288 were not covered by tests
}
}

// Process file linking
if (isset($keyValuePairs['path']) && isset($keyValuePairs['fileFormat'])) {
$file = $this->submissionService->writeFileToCloud($form, $keyValuePairs['path'], $keyValuePairs['fileFormat']);
Expand Down
24 changes: 15 additions & 9 deletions lib/Db/FormMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,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;
Expand All @@ -23,10 +24,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);
}
Expand Down Expand Up @@ -132,20 +134,24 @@ public function findSharedForms(string $userId, array $groups = [], array $teams
}

// build expression for publicly 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, ':access_shown'));
} else {
// All
$access = $qbShares->expr()->neq('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_NOPUBLICSHARE, IQueryBuilder::PARAM_INT, ':access_nopublicshare'));
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, ':owner_id')));

// Select the whole forms for the DISTINCT shared forms IDs
Expand Down
323 changes: 323 additions & 0 deletions tests/Integration/Api/RespectAdminSettingsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,323 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OCA\Forms\Tests\Integration\Api;

use GuzzleHttp\Client;
use OCA\Forms\AppInfo\Application;
use OCA\Forms\Constants;
use OCA\Forms\Tests\Integration\IntegrationBase;
use OCP\IConfig;

/**
* This tests that the API respects all admin settings
* @group DB
*/
class RespectAdminSettingsTest extends IntegrationBase {
/** @var GuzzleHttp\Client */
private $http;

protected array $users = [
'test' => '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),
);
}

};
Loading
Loading