fix(sharing): configure which shares of disabled accounts are hidden

apps/dav/lib/CalDAV/PublicCalendarRoot.php

@@ -67,12 +67,12 @@ public function getChildren() {
 
 	/**
 	 * Checks if the public calendar should be visible or not, based on
-	 * the configuration of the `hide_disabled_user_shares` setting within
+	 * the configuration of the `hide_disabled_user_external_shares` setting within
 	 * `files_sharing` and the status of the owning user (disabled or not).
 	 */
 	private function validateVisibility(string $principalUri): bool {
 		$hideCalendarsOfDisabledUsers = $this->appConfig->getValueBool(
-			'files_sharing', 'hide_disabled_user_shares', true
+			'files_sharing', 'hide_disabled_user_external_shares', true
 		);
 
 		if (!$hideCalendarsOfDisabledUsers) {

apps/dav/tests/unit/CalDAV/PublicCalendarRootTest.php

@@ -150,7 +150,7 @@ public function testGetChildHidesCalendarOfDisabledUser(): void {
 		$publicUri = $calendar->getPublishStatus();
 
 		$this->mockDisabledOwner();
-		$this->setHideDisabledUserShares(true);
+		$this->setHideDisabledUserExternalShares(true);
 
 		$this->expectException(NotFound::class);
 		$this->publicCalendarRoot->getChild($publicUri);
@@ -161,7 +161,7 @@ public function testGetChildServesCalendarOfDisabledUserWhenHidingIsDisabled():
 		$publicUri = $calendar->getPublishStatus();
 
 		$this->mockDisabledOwner();
-		$this->setHideDisabledUserShares(false);
+		$this->setHideDisabledUserExternalShares(false);
 
 		$calendarResult = $this->publicCalendarRoot->getChild($publicUri);
 		$this->assertEquals($calendar, $calendarResult);
@@ -188,9 +188,9 @@ private function mockDisabledOwner(): void {
 			->willReturn($disabledUser);
 	}
 
-	private function setHideDisabledUserShares(bool $hide): void {
+	private function setHideDisabledUserExternalShares(bool $hide): void {
 		$this->appConfig->method('getValueBool')
-			->with('files_sharing', 'hide_disabled_user_shares', 'yes')
+			->with('files_sharing', 'hide_disabled_user_external_shares', true)
 			->willReturn($hide);
 	}
 }

apps/files_sharing/lib/Config/ConfigLexicon.php

@@ -25,6 +25,8 @@ class ConfigLexicon implements ILexicon {
 	public const SHOW_FEDERATED_TO_TRUSTED_AS_INTERNAL = 'show_federated_shares_to_trusted_servers_as_internal';
 	public const EXCLUDE_RESHARE_FROM_EDIT = 'shareapi_exclude_reshare_from_edit';
 	public const UPDATE_CUTOFF_TIME = 'update_cutoff_time';
+	public const HIDE_DISABLED_USER_SHARES = 'hide_disabled_user_shares';
+	public const HIDE_DISABLED_USER_EXTERNAL_SHARES = 'hide_disabled_user_external_shares';
 	public const USER_NEEDS_SHARE_REFRESH = 'user_needs_share_refresh';
 
 	#[\Override]
@@ -40,6 +42,8 @@ public function getAppConfigs(): array {
 			new Entry(self::EXCLUDE_RESHARE_FROM_EDIT, ValueType::BOOL, false, 'Exclude reshare permission from "Allow editing" bundled permissions'),
 
 			new Entry(self::UPDATE_CUTOFF_TIME, ValueType::FLOAT, 3.0, 'Maximum time in second during which we update the share data immediately before switching to only marking the user'),
+			new Entry(self::HIDE_DISABLED_USER_SHARES, ValueType::BOOL, true, 'Hide internal shares (to accounts, groups, teams, …) owned or created by a disabled account'),
+			new Entry(self::HIDE_DISABLED_USER_EXTERNAL_SHARES, ValueType::BOOL, true, 'Hide public, mail and federated shares owned or created by a disabled account'),
 		];
 	}
 

lib/private/Share20/Manager.php

@@ -1173,7 +1173,7 @@ public function getSharesBy(string $userId, int $shareType, ?Node $path = null,
 			return [];
 		}
 
-		if ($onlyValid && $this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'no') === 'yes') {
+		if ($onlyValid && $this->hidesDisabledUserSharesOfType($shareType)) {
 			/*
 			 * If shares from disabled users are hidden, check user status first to avoid useless work.
 			 * Otherwise all shares would’ve been filtered out by checkShare anyway.
@@ -1425,6 +1425,27 @@ public function getShareByToken(string $token): IShare {
 		return $share;
 	}
 
+	/**
+	 * Whether shares of the given type are hidden when an account involved in them
+	 * is disabled. Public, mail and federated shares are external, any other type
+	 * is internal.
+	 */
+	private function hidesDisabledUserSharesOfType(?int $shareType): bool {
+		$external = in_array($shareType, [
+			IShare::TYPE_LINK,
+			IShare::TYPE_EMAIL,
+			IShare::TYPE_REMOTE,
+			IShare::TYPE_REMOTE_GROUP,
+			IShare::TYPE_SCIENCEMESH,
+		], true);
+
+		return $this->appConfig->getValueBool(
+			'files_sharing',
+			$external ? 'hide_disabled_user_external_shares' : 'hide_disabled_user_shares',
+			true,
+		);
+	}
+
 	/**
 	 * Whether a share has to be hidden because one of the accounts involved in it
 	 * is disabled.
@@ -1435,7 +1456,7 @@ public function getShareByToken(string $token): IShare {
 	 * @param ?string $viewer Account the share is checked for, if known
 	 */
 	private function isHiddenDisabledUserShare(IShare $share, ?string $viewer = null): bool {
-		if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'yes') !== 'yes') {
+		if (!$this->hidesDisabledUserSharesOfType($share->getShareType())) {
 			return false;
 		}
 

lib/private/Sharing/SharingBackend.php

@@ -31,6 +31,8 @@
 use NCU\Sharing\Source\IShareSourceMetadata;
 use NCU\Sharing\Source\IShareSourceType;
 use NCU\Sharing\Source\ShareSource;
+use OC\Core\Sharing\Recipient\EmailShareRecipientType;
+use OC\Core\Sharing\Recipient\TokenShareRecipientType;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IAppConfig;
@@ -634,8 +636,26 @@ public function setLastUpdated(array $ids, \DateTimeImmutable $lastUpdated): voi
 		}
 	}
 
-	private function hideDisabledUserShares(): bool {
-		return $this->appConfig->getValueString('files_sharing', 'hide_disabled_user_shares', 'yes') === 'yes';
+	/**
+	 * Whether recipients of the given kind are hidden when the account that owns the
+	 * share or added the recipient is disabled.
+	 * Public, mail and federated recipients are external, any other recipient is internal.
+	 */
+	private function hideDisabledUserShares(bool $external): bool {
+		return $this->appConfig->getValueBool(
+			'files_sharing',
+			$external ? 'hide_disabled_user_external_shares' : 'hide_disabled_user_shares',
+			true,
+		);
+	}
+
+	/**
+	 * @param class-string<IShareRecipientType> $class
+	 */
+	private function isExternalRecipient(string $class, ?string $instance): bool {
+		return $instance !== null
+			|| $class === TokenShareRecipientType::class
+			|| $class === EmailShareRecipientType::class;
 	}
 
 	// TODO: Split up the method
@@ -731,6 +751,13 @@ private function list(
 			}
 		}
 
+		// Whether internal and external recipients are hidden when their share owner or initiator is disabled
+		$hideInternal = $this->hideDisabledUserShares(false);
+		$hideExternal = $this->hideDisabledUserShares(true);
+		// Shares of a disabled owner that are only partly hidden, their recipients are filtered one by one
+		/** @var array<array-key, true> $disabledOwnerShareIds */
+		$disabledOwnerShareIds = [];
+
 		// The key type is array-key, because PHP will automatically cast the value. We can't type it as integer though, because we need to also support 32 bit systems and there the autocasting doesn't happen, if the value is too large.
 		/** @var array<array-key, array{id: non-empty-string, owner: ShareUser, last_updated: numeric-string, state: ShareState, user_status: ShareUserStatus, sources: list<ShareSource>, recipients: array<array-key, ShareRecipient>, properties: array<class-string<ISharePropertyType>, ShareProperty>, permissions: array<class-string<ISharePermissionType>, SharePermission>}> $shares */
 		$shares = [];
@@ -785,15 +812,22 @@ private function list(
 				/** @var ?non-empty-string $ownerInstance */
 				$ownerInstance = $row['owner_instance'];
 
+				/** @var non-empty-string $shareId */
+				$shareId = (string)$row['id'];
+
 				/** @psalm-suppress PossiblyNullReference The share is automatically deleted, when the owner is deleted. */
-				if ($ownerInstance === null && !$accessContext->overrideChecks && $this->hideDisabledUserShares() && !$this->userManager->get(
+				if ($ownerInstance === null && !$accessContext->overrideChecks && ($hideInternal || $hideExternal) && !$this->userManager->get(
 					$ownerUserId
 				)->isEnabled()) {
-					continue;
+					// Every recipient would be hidden, so skip the whole share
+					if ($hideInternal && $hideExternal) {
+						continue;
+					}
+
+					// Only the recipients of the hidden kind are skipped, see below.
+					$disabledOwnerShareIds[$shareId] = true;
 				}
 
-				/** @var non-empty-string $shareId */
-				$shareId = (string)$row['id'];
 				/** @var numeric-string $lastUpdated */
 				$lastUpdated = (string)$row['last_updated'];
 				$state = (int)$row['state_id'];
@@ -963,18 +997,27 @@ private function list(
 				$initiatorUserId = $row['initiator_user_id'];
 				/** @var ?non-empty-string $initiatorInstance */
 				$initiatorInstance = $row['initiator_instance'];
+				/** @var ?non-empty-string $instance */
+				$instance = $row['recipient_instance'];
 
-				/** @psalm-suppress PossiblyNullReference The initiator is automatically promoted to the owner, when the initiator is deleted. */
-				if ($initiatorInstance === null && !$accessContext->overrideChecks && !$shares[$shareId]['owner']->isCurrentUser(
-					$accessContext
-				) && $this->hideDisabledUserShares() && !$this->userManager->get($initiatorUserId)->isEnabled()) {
-					continue;
+				// Recipients of a kind that is hidden are skipped when the share owner or the account that added them is disabled
+				$hideIfDisabledUser = $this->isExternalRecipient($typeClass, $instance) ? $hideExternal : $hideInternal;
+				if ($hideIfDisabledUser && !$accessContext->overrideChecks) {
+					if (isset($disabledOwnerShareIds[$shareId])) {
+						continue;
+					}
+
+					// The owner still sees the recipients added by a disabled initiator, to manage them
+					/** @psalm-suppress PossiblyNullReference The initiator is automatically promoted to the owner, when the initiator is deleted. */
+					if ($initiatorInstance === null && !$shares[$shareId]['owner']->isCurrentUser(
+						$accessContext
+					) && !$this->userManager->get($initiatorUserId)->isEnabled()) {
+						continue;
+					}
 				}
 
 				/** @var non-empty-string $value */
 				$value = $row['recipient_value'];
-				/** @var ?non-empty-string $instance */
-				$instance = $row['recipient_instance'];
 				// The secret is only removed in the next step, because we still need it to check if the current access context still has access to the share, after the recipients of disabled initiators have been skipped.
 				/** @var non-empty-string $secret */
 				$secret = $row['recipient_secret'];
@@ -1221,7 +1264,8 @@ private function list(
 		if (!$accessContext->overrideChecks) {
 			$shares = array_filter(
 				$shares,
-				fn (Share $share): bool => $share->owner->isCurrentUser($accessContext) || $hasRecipientAccess[$share->id] || $hasSourceAccess[$share->id]
+				// Source access is only checked for a logged-in user, not for an access by secret
+				fn (Share $share): bool => $share->owner->isCurrentUser($accessContext) || $hasRecipientAccess[$share->id] || ($hasSourceAccess[$share->id] ?? false)
 			);
 		}