Merge pull request #2069 from nextcloud/fix/shared-forms

fix(DB): Correctly fetch shared forms
nextcloud · Apr 15, 2024 · f4feede · f4feede
2 parents 56c7e8f + 4346e51
commit f4feede
Show file tree

Hide file tree

Showing 5 changed files with 425 additions and 186 deletions.
diff --git a/lib/Db/FormMapper.php b/lib/Db/FormMapper.php
@@ -97,67 +97,65 @@ public function findByHash(string $hash): Form {
 	 * @return Form[]
 	 */
 	public function findSharedForms(string $userId, array $groups = [], array $teams = [], bool $filterShown = true): array {
-		$qbForms = $this->db->getQueryBuilder();
 		$qbShares = $this->db->getQueryBuilder();
+		$qbForms = $this->db->getQueryBuilder();
 
 		$memberships = $qbShares->expr()->orX();
 		// share type user and share with current user
 		$memberships->add(
 			$qbShares->expr()->andX(
-				$qbShares->expr()->eq('share_type', $qbShares->createNamedParameter(IShare::TYPE_USER)),
-				$qbShares->expr()->eq('share_with', $qbShares->createNamedParameter($userId, IQueryBuilder::PARAM_STR)),
+				$qbShares->expr()->eq('shares.share_type', $qbShares->createNamedParameter(IShare::TYPE_USER)),
+				$qbShares->expr()->eq('shares.share_with', $qbShares->createNamedParameter($userId, IQueryBuilder::PARAM_STR)),
 			),
 		);
 		// share type group and one of the user groups
 		if (!empty($groups)) {
 			$memberships->add(
 				$qbShares->expr()->andX(
-					$qbShares->expr()->eq('share_type', $qbShares->createNamedParameter(IShare::TYPE_GROUP)),
-					$qbShares->expr()->in('share_with', $qbShares->createNamedParameter($groups, IQueryBuilder::PARAM_STR_ARRAY)),
+					$qbShares->expr()->eq('shares.share_type', $qbShares->createNamedParameter(IShare::TYPE_GROUP)),
+					$qbShares->expr()->in('shares.share_with', $qbShares->createNamedParameter($groups, IQueryBuilder::PARAM_STR_ARRAY)),
 				),
 			);
 		}
 		// share type team and one of the user teams
 		if (!empty($teams)) {
 			$memberships->add(
 				$qbShares->expr()->andX(
-					$qbShares->expr()->eq('share_type', $qbShares->createNamedParameter(IShare::TYPE_CIRCLE)),
-					$qbShares->expr()->in('share_with', $qbShares->createNamedParameter($teams, IQueryBuilder::PARAM_STR_ARRAY)),
+					$qbShares->expr()->eq('shares.share_type', $qbShares->createNamedParameter(IShare::TYPE_CIRCLE)),
+					$qbShares->expr()->in('shares.share_with', $qbShares->createNamedParameter($teams, IQueryBuilder::PARAM_STR_ARRAY)),
 				),
 			);
 		}
 
-		// get form_id's that are shared to user
-		$qbShares->selectDistinct('form_id')
-			->from('forms_v2_shares')
-			->where($memberships);
-		$sharedFormIdsResult = $qbShares->executeQuery();
-		$sharedFormIds = [];
-		for ($i = 0; $i < $sharedFormIdsResult->rowCount(); $i++) {
-			$sharedFormIds[] = $sharedFormIdsResult->fetchOne();
-		}
-
 		// build expression for publicy shared forms (default only directly shown)
 		if ($filterShown) {
-			$access = $qbForms->expr()->in('access_enum', $qbForms->createNamedParameter(Constants::FORM_ACCESS_ARRAY_SHOWN, IQueryBuilder::PARAM_INT_ARRAY));
+			// Only shown
+			$access = $qbShares->expr()->in('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_ARRAY_SHOWN, IQueryBuilder::PARAM_INT_ARRAY));
 		} else {
-			$access = $qbForms->expr()->in('access_enum', $qbForms->createNamedParameter(Constants::FORM_ACCESS_ARRAY_PERMIT, IQueryBuilder::PARAM_INT_ARRAY));
+			// All
+			$access = $qbShares->expr()->neq('access_enum', $qbShares->createNamedParameter(Constants::FORM_ACCESS_NOPUBLICSHARE, IQueryBuilder::PARAM_INT));
 		}
 
-		$whereTerm = $qbForms->expr()->orX();
-		$whereTerm->add($qbForms->expr()->in('id', $qbForms->createNamedParameter($sharedFormIds, IQueryBuilder::PARAM_INT_ARRAY)));
-		$whereTerm->add($access);
+		// 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)
+			->andWhere($qbShares->expr()->neq('forms.owner_id', $qbShares->createNamedParameter($userId, IQueryBuilder::PARAM_STR)));
 
+		// Select the whole forms for the DISTINCT shared forms IDs
 		$qbForms->select('*')
 			->from($this->getTableName())
-			// user is member of or form is publicly shared
-			->where($whereTerm)
-			// ensure not to include owned forms
-			->andWhere($qbForms->expr()->neq('owner_id', $qbForms->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
-			//Last updated forms first, then newest forms first
+			->where(
+				$qbForms->expr()->in('id', $qbForms->createFunction($qbShares->getSQL())),
+			)
 			->addOrderBy('last_updated', 'DESC')
 			->addOrderBy('created', 'DESC');
 
+		// We need to add the parameters from the shared forms IDs select to the final select query
+		$qbForms->setParameters($qbShares->getParameters(), $qbShares->getParameterTypes());
+
 		return $this->findEntities($qbForms);
 	}
 

diff --git a/lib/Service/FormsService.php b/lib/Service/FormsService.php
@@ -384,14 +384,17 @@ public function hasUserAccess(Form $form): bool {
 
 	/**
 	 * Get all forms shared to the user
+	 * @param IUser $user User to query shared forms for
+	 * @param bool $filterShown Set to false to also include forms shared but not visible on sidebar
 	 */
-	public function getSharedForms(IUser $user): array {
+	public function getSharedForms(IUser $user, bool $filterShown = true): array {
 		$groups = $this->groupManager->getUserGroupIds($user);
 		$teams = $this->circlesService->getUserTeamIds($user->getUID());
 		$forms = $this->formMapper->findSharedForms(
 			$user->getUID(),
 			$groups,
 			$teams,
+			$filterShown,
 		);
 
 		// filter expired forms

diff --git a/tests/Integration/Api/ApiV2Test.php b/tests/Integration/Api/ApiV2Test.php
@@ -28,23 +28,15 @@
 use GuzzleHttp\Exception\ClientException;
 
 use OCA\Forms\Constants;
-use OCA\Forms\Db\FormMapper;
-use OCP\DB\QueryBuilder\IQueryBuilder;
-use Test\TestCase;
+use OCA\Forms\Tests\Integration\IntegrationBase;
 
 /**
  * @group DB
  */
-class ApiV2Test extends TestCase {
+class ApiV2Test extends IntegrationBase {
 	/** @var GuzzleHttp\Client */
 	private $http;
 
-	/** @var FormMapper */
-	private $formMapper;
-
-	/** @var Array */
-	private $testForms;
-
 	/**
 	 * Store Test Forms Array.
 	 * Necessary as function due to object type-casting.
@@ -234,115 +226,13 @@ private function setTestForms() {
 	 * Writing testforms into db, preparing http request
 	 */
 	public function setUp(): void {
-		parent::setUp();
-		$userManager = \OC::$server->getUserManager();
-		$user = $userManager->get('test');
-		if ($user === null) {
-			$user = $userManager->createUser('test', 'test');
-		}
-		$user->setDisplayName('Test Displayname');
-
-		// We also have user2 and user3 but those accounts are "deleted"
-		$user = $userManager->get("user1");
-		if ($user === null) {
-			$user = $userManager->createUser("user1", "user1");
-		}
-		$user->setDisplayName("User No. 1");
-
 		$this->setTestForms();
+		$this->users = [
+			'test' => 'Test Displayname',
+			'user1' => 'User No. 1',
+		];
 
-		$qb = \OC::$server->getDatabaseConnection()->getQueryBuilder();
-
-		// Write our test forms into db
-		foreach ($this->testForms as $index => $form) {
-			$qb->insert('forms_v2_forms')
-				->values([
-					'hash' => $qb->createNamedParameter($form['hash'], IQueryBuilder::PARAM_STR),
-					'title' => $qb->createNamedParameter($form['title'], IQueryBuilder::PARAM_STR),
-					'description' => $qb->createNamedParameter($form['description'], IQueryBuilder::PARAM_STR),
-					'owner_id' => $qb->createNamedParameter($form['owner_id'], IQueryBuilder::PARAM_STR),
-					'access_enum' => $qb->createNamedParameter($form['access_enum'], IQueryBuilder::PARAM_INT),
-					'created' => $qb->createNamedParameter($form['created'], IQueryBuilder::PARAM_INT),
-					'expires' => $qb->createNamedParameter($form['expires'], IQueryBuilder::PARAM_INT),
-					'state' => $qb->createNamedParameter($form['state'], IQueryBuilder::PARAM_INT),
-					'is_anonymous' => $qb->createNamedParameter($form['is_anonymous'], IQueryBuilder::PARAM_BOOL),
-					'submit_multiple' => $qb->createNamedParameter($form['submit_multiple'], IQueryBuilder::PARAM_BOOL),
-					'show_expiration' => $qb->createNamedParameter($form['show_expiration'], IQueryBuilder::PARAM_BOOL),
-					'last_updated' => $qb->createNamedParameter($form['last_updated'], IQueryBuilder::PARAM_INT),
-					'submission_message' => $qb->createNamedParameter($form['submission_message'], IQueryBuilder::PARAM_STR),
-					'file_id' => $qb->createNamedParameter($form['file_id'], IQueryBuilder::PARAM_INT),
-					'file_format' => $qb->createNamedParameter($form['file_format'], IQueryBuilder::PARAM_STR),
-				]);
-			$qb->executeStatement();
-			$formId = $qb->getLastInsertId();
-			$this->testForms[$index]['id'] = $formId;
-
-			// Insert Questions into DB
-			foreach ($form['questions'] as $qIndex => $question) {
-				$qb->insert('forms_v2_questions')
-					->values([
-						'form_id' => $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT),
-						'order' => $qb->createNamedParameter($question['order'], IQueryBuilder::PARAM_INT),
-						'type' => $qb->createNamedParameter($question['type'], IQueryBuilder::PARAM_STR),
-						'is_required' => $qb->createNamedParameter($question['isRequired'], IQueryBuilder::PARAM_BOOL),
-						'text' => $qb->createNamedParameter($question['text'], IQueryBuilder::PARAM_STR),
-						'name' => $qb->createNamedParameter($question['name'], IQueryBuilder::PARAM_STR),
-						'description' => $qb->createNamedParameter($question['description'], IQueryBuilder::PARAM_STR),
-						'extra_settings_json' => $qb->createNamedParameter(json_encode($question['extraSettings']), IQueryBuilder::PARAM_STR),
-					]);
-				$qb->executeStatement();
-				$questionId = $qb->getLastInsertId();
-				$this->testForms[$index]['questions'][$qIndex]['id'] = $questionId;
-
-				// Insert Options into DB
-				foreach ($question['options'] as $oIndex => $option) {
-					$qb->insert('forms_v2_options')
-						->values([
-							'question_id' => $qb->createNamedParameter($questionId, IQueryBuilder::PARAM_INT),
-							'text' => $qb->createNamedParameter($option['text'], IQueryBuilder::PARAM_STR)
-						]);
-					$qb->executeStatement();
-					$this->testForms[$index]['questions'][$qIndex]['options'][$oIndex]['id'] = $qb->getLastInsertId();
-				}
-			}
-
-			// Insert Shares into DB
-			foreach ($form['shares'] as $sIndex => $share) {
-				$qb->insert('forms_v2_shares')
-					->values([
-						'form_id' => $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT),
-						'share_type' => $qb->createNamedParameter($share['shareType'], IQueryBuilder::PARAM_STR),
-						'share_with' => $qb->createNamedParameter($share['shareWith'], IQueryBuilder::PARAM_STR),
-						'permissions_json' => $qb->createNamedParameter(json_encode($share['permissions'] ?? null), IQueryBuilder::PARAM_STR),
-					]);
-				$qb->executeStatement();
-				$this->testForms[$index]['shares'][$sIndex]['id'] = $qb->getLastInsertId();
-			}
-
-			// Insert Submissions into DB
-			foreach ($form['submissions'] as $suIndex => $submission) {
-				$qb->insert('forms_v2_submissions')
-					->values([
-						'form_id' => $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT),
-						'user_id' => $qb->createNamedParameter($submission['userId'], IQueryBuilder::PARAM_STR),
-						'timestamp' => $qb->createNamedParameter($submission['timestamp'], IQueryBuilder::PARAM_INT)
-					]);
-				$qb->executeStatement();
-				$submissionId = $qb->getLastInsertId();
-				$this->testForms[$index]['submissions'][$suIndex]['id'] = $submissionId;
-
-				foreach ($submission['answers'] as $aIndex => $answer) {
-					$qb->insert('forms_v2_answers')
-						->values([
-							'submission_id' => $qb->createNamedParameter($submissionId, IQueryBuilder::PARAM_INT),
-							'question_id' => $qb->createNamedParameter($this->testForms[$index]['questions'][$answer['questionIndex']]['id'], IQueryBuilder::PARAM_INT),
-							'text' => $qb->createNamedParameter($answer['text'], IQueryBuilder::PARAM_STR)
-						]);
-					$qb->executeStatement();
-					$this->testForms[$index]['submissions'][$suIndex]['answers'][$aIndex]['id'] = $qb->getLastInsertId();
-				}
-			}
-		}
+		parent::setUp();
 
 		// Set up http Client
 		$this->http = new Client([
@@ -355,48 +245,7 @@ public function setUp(): void {
 		]);
 	}
 
-	/** Clean up database from testforms */
 	public function tearDown(): void {
-		$qb = \OC::$server->getDatabaseConnection()->getQueryBuilder();
-
-		foreach ($this->testForms as $form) {
-			$qb->delete('forms_v2_forms')
-				->where($qb->expr()->eq('id', $qb->createNamedParameter($form['id'], IQueryBuilder::PARAM_INT)));
-			$qb->executeStatement();
-
-			foreach ($form['questions'] as $question) {
-				$qb->delete('forms_v2_questions')
-					->where($qb->expr()->eq('id', $qb->createNamedParameter($question['id'], IQueryBuilder::PARAM_INT)));
-				$qb->executeStatement();
-
-				foreach ($question['options'] as $option) {
-					$qb->delete('forms_v2_options')
-						->where($qb->expr()->eq('id', $qb->createNamedParameter($option['id'], IQueryBuilder::PARAM_INT)));
-					$qb->executeStatement();
-				}
-			}
-
-			foreach ($form['shares'] as $share) {
-				$qb->delete('forms_v2_shares')
-					->where($qb->expr()->eq('id', $qb->createNamedParameter($share['id'], IQueryBuilder::PARAM_INT)));
-				$qb->executeStatement();
-			}
-
-			if (isset($form['submissions'])) {
-				foreach ($form['submissions'] as $submission) {
-					$qb->delete('forms_v2_submissions')
-						->where($qb->expr()->eq('id', $qb->createNamedParameter($submission['id'], IQueryBuilder::PARAM_INT)));
-					$qb->executeStatement();
-
-					foreach ($submission['answers'] as $answer) {
-						$qb->delete('forms_v2_answers')
-							->where($qb->expr()->eq('id', $qb->createNamedParameter($answer['id'], IQueryBuilder::PARAM_INT)));
-						$qb->executeStatement();
-					}
-				}
-			}
-		}
-
 		parent::tearDown();
 	}