Merge pull request #11431 from nextcloud/backport/11430/stable26

[stable26] fix(attachments): Don't allow selecting shared folders as attachment …
nextcloud · Jan 24, 2024 · a1b141e · a1b141e
2 parents 5d2a47f + cc6bc3a
commit a1b141e
Show file tree

Hide file tree

Showing 13 changed files with 53 additions and 5 deletions.
diff --git a/.github/workflows/integration-mysql.yml b/.github/workflows/integration-mysql.yml
@@ -56,7 +56,7 @@ jobs:
         test-suite: ['callapi', 'chat', 'chat-2', 'command', 'conversation', 'conversation-2', 'federation', 'integration', 'sharing', 'sharing-2']
         php-versions: ['8.2']
         server-versions: ['stable26']
-        guests-versions: ['stable-2.5']
+        guests-versions: ['stable2.5']
         notifications-versions: ['stable26']
 
     services:

diff --git a/.github/workflows/integration-oci.yml b/.github/workflows/integration-oci.yml
@@ -56,7 +56,7 @@ jobs:
         test-suite: ['callapi', 'chat', 'chat-2', 'command', 'conversation', 'conversation-2', 'federation', 'integration', 'sharing', 'sharing-2']
         php-versions: ['8.2']
         server-versions: ['stable26']
-        guests-versions: ['stable-2.5']
+        guests-versions: ['stable2.5']
         notifications-versions: ['stable26']
 
     services:

diff --git a/.github/workflows/integration-pgsql.yml b/.github/workflows/integration-pgsql.yml
@@ -53,7 +53,7 @@ jobs:
         test-suite: ['callapi', 'chat', 'chat-2', 'command', 'conversation', 'conversation-2', 'federation', 'integration', 'sharing', 'sharing-2']
         php-versions: ['8.2']
         server-versions: ['stable26']
-        guests-versions: ['stable-2.5']
+        guests-versions: ['stable2.5']
         notifications-versions: ['stable26']
 
     services:

diff --git a/.github/workflows/integration-sqlite.yml b/.github/workflows/integration-sqlite.yml
@@ -56,7 +56,7 @@ jobs:
         test-suite: ['callapi', 'chat', 'chat-2', 'command', 'conversation', 'conversation-2', 'federation', 'integration', 'sharing', 'sharing-2']
         php-versions: ['8.2']
         server-versions: ['stable26']
-        guests-versions: ['stable-2.5']
+        guests-versions: ['stable2.5']
         notifications-versions: ['stable26']
 
     steps:

diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php
@@ -70,7 +70,6 @@ class PageController extends Controller {
 	private RoomController $api;
 	private TalkSession $talkSession;
 	private IUserSession $userSession;
-	private LoggerInterface $logger;
 	private Manager $manager;
 	private ParticipantService $participantService;
 	private RoomService $roomService;

diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php
@@ -103,6 +103,9 @@ protected function validateUserSetting(string $setting, $value): bool {
 				if (!$node instanceof Folder) {
 					throw new NotPermittedException('Node is not a directory');
 				}
+				if ($node->isShared()) {
+					throw new NotPermittedException('Folder is shared');
+				}
 				return !$node->getStorage()->instanceOfStorage(SharedStorage::class);
 			} catch (NotFoundException $e) {
 				$userFolder->newFolder($value);

diff --git a/lib/Files/TemplateLoader.php b/lib/Files/TemplateLoader.php
@@ -39,6 +39,7 @@
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Util;
+use Psr\Log\LoggerInterface;
 
 /**
  * Helper class to add the Talk UI to the sidebar of the Files app.
@@ -60,6 +61,7 @@ public function __construct(
 		IAppManager $appManager,
 		IRootFolder $rootFolder,
 		IUserSession $userSession,
+		LoggerInterface $logger,
 	) {
 		$this->initialState = $initialState;
 		$this->memcacheFactory = $memcacheFactory;
@@ -68,6 +70,7 @@ public function __construct(
 		$this->appManager = $appManager;
 		$this->rootFolder = $rootFolder;
 		$this->userSession = $userSession;
+		$this->logger = $logger;
 	}
 
 

diff --git a/lib/PublicShare/TemplateLoader.php b/lib/PublicShare/TemplateLoader.php
@@ -36,6 +36,7 @@
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\Util;
+use Psr\Log\LoggerInterface;
 
 /**
  * Helper class to extend the "publicshare" template from the server.
@@ -52,11 +53,13 @@ public function __construct(
 		ICacheFactory $memcacheFactory,
 		Config $talkConfig,
 		IConfig $serverConfig,
+		LoggerInterface $logger,
 	) {
 		$this->initialState = $initialState;
 		$this->talkConfig = $talkConfig;
 		$this->memcacheFactory = $memcacheFactory;
 		$this->serverConfig = $serverConfig;
+		$this->logger = $logger;
 	}
 
 	/**

diff --git a/lib/PublicShareAuth/TemplateLoader.php b/lib/PublicShareAuth/TemplateLoader.php
@@ -35,6 +35,7 @@
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\Util;
+use Psr\Log\LoggerInterface;
 
 /**
  * Helper class to extend the "publicshareauth" template from the server.
@@ -51,11 +52,13 @@ public function __construct(
 		ICacheFactory $memcacheFactory,
 		Config $talkConfig,
 		IConfig $serverConfig,
+		LoggerInterface $logger,
 	) {
 		$this->initialState = $initialState;
 		$this->talkConfig = $talkConfig;
 		$this->memcacheFactory = $memcacheFactory;
 		$this->serverConfig = $serverConfig;
+		$this->logger = $logger;
 	}
 
 	/**

diff --git a/lib/Service/RecordingService.php b/lib/Service/RecordingService.php
@@ -42,6 +42,7 @@
 use OCP\Files\IRootFolder;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
+use OCP\IConfig;
 use OCP\Notification\IManager;
 use OCP\Share\IManager as ShareManager;
 use OCP\Share\IShare;
@@ -72,6 +73,7 @@ public function __construct(
 		protected Manager $roomManager,
 		protected ITimeFactory $timeFactory,
 		protected Config $config,
+		protected IConfig $serverConfig,
 		protected RoomService $roomService,
 		protected ShareManager $shareManager,
 		protected ChatManager $chatManager,
@@ -190,6 +192,13 @@ private function getRecordingFolder(string $owner, string $token): Folder {
 		try {
 			/** @var \OCP\Files\Folder */
 			$recordingRootFolder = $userFolder->get($recordingRootFolderName);
+			if ($recordingRootFolder->isShared()) {
+				$this->logger->error('Talk attachment folder for user {userId} is set to a shared folder. Resetting to their root.', [
+					'userId' => $owner,
+				]);
+
+				$this->serverConfig->setUserValue($owner, 'spreed', 'attachment_folder', '/');
+			}
 		} catch (NotFoundException $e) {
 			/** @var \OCP\Files\Folder */
 			$recordingRootFolder = $userFolder->newFolder($recordingRootFolderName);

diff --git a/lib/TInitialState.php b/lib/TInitialState.php
@@ -34,6 +34,7 @@
 use OCP\IConfig;
 use OCP\IUser;
 use OCP\Util;
+use Psr\Log\LoggerInterface;
 
 trait TInitialState {
 	/** @var Config */
@@ -44,6 +45,8 @@ trait TInitialState {
 	protected $initialState;
 	/** @var ICacheFactory */
 	protected $memcacheFactory;
+	/** @var LoggerInterface */
+	protected $logger;
 
 	protected function publishInitialStateShared(): void {
 		// Needed to enable the screensharing extension in Chromium < 72
@@ -134,6 +137,12 @@ protected function publishInitialStateForUser(IUser $user, IRootFolder $rootFold
 				try {
 					try {
 						$folder = $userFolder->get($attachmentFolder);
+						if ($folder->isShared()) {
+							$this->logger->error('Talk attachment folder for user {userId} is set to a shared folder. Resetting to their root.', [
+								'userId' => $user->getUID(),
+							]);
+							throw new NotPermittedException('Folder is shared');
+						}
 					} catch (NotFoundException $e) {
 						$folder = $userFolder->newFolder($attachmentFolder);
 					}

diff --git a/tests/integration/features/sharing/settings.feature b/tests/integration/features/sharing/settings.feature
@@ -0,0 +1,14 @@
+Feature: sharing/settings
+
+  Background:
+    Given user "participant1" exists
+    Given user "participant2" exists
+
+  Scenario: Do not allow setting a shared folder as attachment_folder
+    Given user "participant1" creates folder "/test"
+    When user "participant1" sets setting "attachment_folder" to "/test" with 200 (v1)
+    Then user "participant1" has capability "spreed=>config=>attachments=>folder" set to "/test"
+    Given user "participant2" creates folder "/test-participant2"
+    Given user "participant2" shares "/test-participant2" with user "participant1" with OCS 100
+    When user "participant1" sets setting "attachment_folder" to "/test-participant2" with 400 (v1)
+    Then user "participant1" has capability "spreed=>config=>attachments=>folder" set to "/test"
diff --git a/tests/php/Service/RecordingServiceTest.php b/tests/php/Service/RecordingServiceTest.php
@@ -48,6 +48,7 @@ function is_uploaded_file($filename) {
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\IMimeTypeDetector;
 use OCP\Files\IRootFolder;
+use OCP\IConfig;
 use OCP\Notification\IManager;
 use OCP\Share\IManager as ShareManager;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -63,6 +64,8 @@ class RecordingServiceTest extends TestCase {
 	private $rootFolder;
 	/** @var Config|MockObject */
 	private $config;
+	/** @var IConfig|MockObject */
+	private $serverConfig;
 	/** @var IManager|MockObject */
 	private $notificationManager;
 	/** @var Manager|MockObject */
@@ -92,6 +95,7 @@ public function setUp(): void {
 		$this->roomManager = $this->createMock(Manager::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->config = $this->createMock(Config::class);
+		$this->serverConfig = $this->createMock(IConfig::class);
 		$this->roomService = $this->createMock(RoomService::class);
 		$this->shareManager = $this->createMock(ShareManager::class);
 		$this->chatManager = $this->createMock(ChatManager::class);
@@ -106,6 +110,7 @@ public function setUp(): void {
 			$this->roomManager,
 			$this->timeFactory,
 			$this->config,
+			$this->serverConfig,
 			$this->roomService,
 			$this->shareManager,
 			$this->chatManager,