Improve corrupt share permission situation

apps/files_sharing/lib/Cache.php

@@ -15,6 +15,7 @@
 use OC\Files\Search\SearchComparison;
 use OC\Files\Storage\Wrapper\Jail;
 use OC\User\DisplayNameCache;
+use OCP\Constants;
 use OCP\Files\Cache\ICache;
 use OCP\Files\Cache\ICacheEntry;
 use OCP\Files\Search\ISearchBinaryOperator;
@@ -154,6 +155,14 @@ protected function formatCacheEntry($entry, $path = null) {
 				$entry['permissions'] = $this->storage->getPermissions($entry['path']);
 			}
 
+			if (!SharedStorage::hasValidSourcePermissions($this->sourceRootInfo)) {
+				// SharedStorage disables a share whose source lost the share permission:
+				// every write is refused while reading still works. Report the same here,
+				// otherwise the file list advertises write permissions that every
+				// operation then denies with a 403.
+				$entry['permissions'] &= Constants::PERMISSION_READ;
+			}
+
 			if ($this->share->getNodeId() === $entry['fileid']) {
 				$entry['name'] = basename($this->share->getTarget());
 			}

apps/files_sharing/lib/SharedStorage.php

@@ -63,6 +63,8 @@ class SharedStorage extends Jail implements LegacyISharedStorage, ISharedStorage
 
 	private $initialized = false;
 
+	private bool $invalidSourceLogged = false;
+
 	/**
 	 * @var ICacheEntry
 	 */
@@ -236,8 +238,58 @@ public function getShareId() {
 		return $this->superShare->getId();
 	}
 
+	/**
+	 * A share only works as long as its source still carries the share permission.
+	 * Both this storage and its cache have to agree on that, so keep the check in
+	 * one place.
+	 *
+	 * @param ICacheEntry|false|null $sourceRootInfo
+	 */
+	public static function hasValidSourcePermissions($sourceRootInfo): bool {
+		return $sourceRootInfo instanceof ICacheEntry
+			&& ($sourceRootInfo->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE;
+	}
+
 	private function isValid(): bool {
-		return $this->getSourceRootInfo() && ($this->getSourceRootInfo()->getPermissions() & Constants::PERMISSION_SHARE) === Constants::PERMISSION_SHARE;
+		$sourceRootInfo = $this->getSourceRootInfo();
+		if (self::hasValidSourcePermissions($sourceRootInfo)) {
+			return true;
+		}
+
+		$this->logInvalidSource($sourceRootInfo);
+
+		return false;
+	}
+
+	/**
+	 * An invalid share is disabled entirely: every permission check returns 0 while
+	 * reading keeps working, which is easy to mistake for arbitrary breakage such as
+	 * failing uploads or renames that revert. Leave a trace naming the share and the
+	 * source that has to be repaired.
+	 *
+	 * @param ICacheEntry|false|null $sourceRootInfo
+	 */
+	private function logInvalidSource($sourceRootInfo): void {
+		if ($this->invalidSourceLogged) {
+			return;
+		}
+		$this->invalidSourceLogged = true;
+
+		if (!($sourceRootInfo instanceof ICacheEntry) || $sourceRootInfo->getId() < 0) {
+			// The source could not be resolved at all, in which case `init()` has
+			// already swapped in a `FailedStorage` and `FailedCache` and the root info
+			// is only a read-only placeholder. That is a different problem from a
+			// source whose cached permissions are wrong, so don't report it as one.
+			return;
+		}
+
+		$this->logger->warning('Share {shareId} is disabled because its source is missing the share permission. Repair the cached permissions of {shareOwner} with "occ files:scan", or check whether the source is masked by a read-only mount or access control rule.', [
+			'app' => 'files_sharing',
+			'shareId' => $this->superShare->getId(),
+			'shareOwner' => $this->superShare->getShareOwner(),
+			'sourceFileId' => $sourceRootInfo->getId(),
+			'sourcePermissions' => $sourceRootInfo->getPermissions(),
+		]);
 	}
 
 	#[\Override]

apps/files_sharing/tests/SharedStorageTest.php

@@ -443,6 +443,53 @@ public function testOwnerPermissions(): void {
 		$this->shareManager->deleteShare($share);
 	}
 
+	public function testInvalidSourcePermissionsAreReportedAsReadOnly(): void {
+		self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
+
+		$share = $this->share(
+			IShare::TYPE_USER,
+			$this->folder,
+			self::TEST_FILES_SHARING_API_USER1,
+			self::TEST_FILES_SHARING_API_USER2,
+			Constants::PERMISSION_ALL
+		);
+
+		// Drop the share permission of the source in the file cache, the way a scan
+		// through a permission mask used to persist masked permissions. This disables
+		// the share in `SharedStorage::isValid()`.
+		$sourceInfo = $this->view->getFileInfo($this->folder);
+		$sourceCache = $sourceInfo->getStorage()->getCache();
+		$sourceCache->update($sourceInfo->getId(), [
+			'permissions' => Constants::PERMISSION_ALL & ~Constants::PERMISSION_SHARE,
+		]);
+
+		self::loginHelper(self::TEST_FILES_SHARING_API_USER2);
+		$user2View = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
+
+		// the storage refuses every write, so the cache must not advertise more.
+		// The mount point keeps its delete permission: `View::getFileInfo()` adds that
+		// for every movable mount, so the recipient can always remove the share from
+		// their own view.
+		$folderInfo = $user2View->getFileInfo($this->folder);
+		$this->assertNotFalse($folderInfo);
+		$this->assertTrue($folderInfo->isReadable());
+		$this->assertFalse($folderInfo->isCreatable());
+		$this->assertFalse($folderInfo->isUpdateable());
+		$this->assertFalse($folderInfo->isShareable());
+
+		$fileInfo = $user2View->getFileInfo($this->folder . $this->filename);
+		$this->assertNotFalse($fileInfo);
+		$this->assertSame(Constants::PERMISSION_READ, $fileInfo->getPermissions());
+
+		// and what it does advertise still holds
+		$this->assertSame('file in subfolder', $user2View->file_get_contents($this->folder . $this->filename));
+		$this->assertFalse($user2View->fopen($this->folder . '/blocked.txt', 'w'));
+
+		self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
+		$sourceCache->update($sourceInfo->getId(), ['permissions' => Constants::PERMISSION_ALL]);
+		$this->shareManager->deleteShare($share);
+	}
+
 	public function testInitWithNonExistingUser(): void {
 		$share = $this->createMock(IShare::class);
 		$share->method('getShareOwner')->willReturn('unexist');