Don't allow file access to non-members in public closed groups

File access was allowed when libraryReading was set to 'all'
VivienDelmon · Oct 21, 2022 · 1c33aee · 1c33aee
1 parent 5265394
commit 1c33aee
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 0 deletions.
diff --git a/controllers/ItemsController.php b/controllers/ItemsController.php
@@ -760,12 +760,26 @@ private function _handleFileRequest($item) {
 				$this->e404();
 			}
 
+			// Return 404 to non-members for files in PublicClosed groups
+			// TODO: Move this into Permissions
+			$type = Zotero_Libraries::getType($this->objectLibraryID);
+			if ($type == 'group') {
+				$groupID = Zotero_Groups::getGroupIDFromLibraryID($this->objectLibraryID);
+				$group = Zotero_Groups::get($groupID);
+				if ($group->type == 'PublicClosed'
+						&& !$this->permissions->canAccess($this->objectLibraryID, 'files')) {
+					$this->e404();
+				}
+			}
+
 			// File viewing
 			if ($this->fileView || $this->fileViewURL) {
+
 				$url = Zotero_Attachments::getTemporaryURL($item);
 				if (!$url) {
 					$this->e500();
 				}
+
 				if ($this->fileViewURL) {
 					header('Content-Type: text/plain');
 					echo $url . "\n";

diff --git a/tests/remote/tests/API/3/FileTest.php b/tests/remote/tests/API/3/FileTest.php
@@ -2312,6 +2312,99 @@ public function test_updating_compressed_attachment_hash_should_clear_associated
 	}
 
 
+	public function test_should_not_allow_anonymous_access_to_file_in_public_closed_group_with_library_reading_for_all() {
+		$file = "work/file";
+		$fileContents = self::getRandomUnicodeString();
+		$contentType = "text/html";
+		$charset = "utf-8";
+		file_put_contents($file, $fileContents);
+		$hash = md5_file($file);
+		$filename = "test_" . $fileContents;
+		$mtime = filemtime($file) * 1000;
+		$size = filesize($file);
+
+		$groupID = API::createGroup([
+			'owner' => self::$config['userID'],
+			'type' => 'PublicClosed',
+			'name' => \Zotero_Utilities::randomString(14),
+			'libraryReading' => 'all',
+			'fileEditing' => 'members'
+		]);
+
+		$parentKey = API::groupCreateItem($groupID, "book", false, $this, 'key');
+		$attachmentKey = API::groupCreateAttachmentItem(
+			$groupID,
+			"imported_file",
+			[
+				'contentType' => "text/plain",
+				'charset' => "utf-8"
+			],
+			$parentKey,
+			$this,
+			'key'
+		);
+
+		// Get authorization
+		$response = API::groupPost(
+			$groupID,
+			"items/$attachmentKey/file",
+			$this->implodeParams([
+				"md5" => $hash,
+				"mtime" => $mtime,
+				"filename" => $filename,
+				"filesize" => $size
+			]),
+			[
+				"Content-Type: application/x-www-form-urlencoded",
+				"If-None-Match: *"
+			]
+		);
+		$this->assert200($response);
+		$json = API::getJSONFromResponse($response);
+
+		self::$toDelete[] = "$hash";
+
+		//
+		// Upload to S3
+		//
+		$response = HTTP::post(
+			$json['url'],
+			$json['prefix'] . $fileContents . $json['suffix'],
+			[
+				"Content-Type: {$json['contentType']}"
+			]
+		);
+		$this->assert201($response);
+
+		// Successful registration
+		$response = API::groupPost(
+			$groupID,
+			"items/$attachmentKey/file",
+			"upload=" . $json['uploadKey'],
+			[
+				"Content-Type: application/x-www-form-urlencoded",
+				"If-None-Match: *"
+			]
+		);
+		$this->assert204($response);
+
+		$response = API::get("groups/$groupID/items/$attachmentKey/file");
+		$this->assert302($response);
+		$response = API::get("groups/$groupID/items/$attachmentKey/file/view");
+		$this->assert302($response);
+		$response = API::get("groups/$groupID/items/$attachmentKey/file/view/url");
+		$this->assert200($response);
+
+		API::useAPIKey("");
+		$response = API::get("groups/$groupID/items/$attachmentKey/file");
+		$this->assert404($response);
+		$response = API::get("groups/$groupID/items/$attachmentKey/file/view");
+		$this->assert404($response);
+		$response = API::get("groups/$groupID/items/$attachmentKey/file/view/url");
+		$this->assert404($response);
+	}
+
+
 	// TODO: Reject for keys not owned by user, even if public library
 	public function testLastStorageSyncNoAuthorization() {
 		API::useAPIKey(false);