Merge pull request #20360 from nextcloud/bugfix/20182/try-to-use-the-displayname-for-file-transfers

Try to use the display name of file transfers

3 files changed, 55 insertions, 26 deletions

diff --git a/apps/files/lib/Notification/Notifier.php b/apps/files/lib/Notification/Notifier.php
index 529eb7cbc77..0ce5287b110 100644
--- a/apps/files/lib/Notification/Notifier.php
+++ b/apps/files/lib/Notification/Notifier.php
@@ -32,6 +32,8 @@ use OCA\Files\Db\TransferOwnershipMapper;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IURLGenerator;
+use OCP\IUser;
+use OCP\IUserManager;
 use OCP\L10N\IFactory;
 use OCP\Notification\IAction;
 use OCP\Notification\IDismissableNotifier;
@@ -50,22 +52,22 @@ class Notifier implements INotifier, IDismissableNotifier {
 	private $mapper;
 	/** @var IManager */
 	private $notificationManager;
+	/** @var IUserManager */
+	private $userManager;
 	/** @var ITimeFactory */
 	private $timeFactory;
 
-	/**
-	 * @param IFactory $l10nFactory
-	 * @param IURLGenerator $urlGenerator
-	 */
 	public function __construct(IFactory $l10nFactory,
 								IURLGenerator $urlGenerator,
 								TransferOwnershipMapper $mapper,
 								IManager $notificationManager,
+								IUserManager $userManager,
 								ITimeFactory $timeFactory) {
 		$this->l10nFactory = $l10nFactory;
 		$this->urlGenerator = $urlGenerator;
 		$this->mapper = $mapper;
 		$this->notificationManager = $notificationManager;
+		$this->userManager = $userManager;
 		$this->timeFactory = $timeFactory;
 	}
 
@@ -138,6 +140,7 @@ class Notifier implements INotifier, IDismissableNotifier {
 				IAction::TYPE_DELETE
 			);
 
+		$sourceUser = $this->getUser($param['sourceUser']);
 		$notification->addParsedAction($approveAction)
 			->addParsedAction($disapproveAction)
 			->setRichSubject(
@@ -145,11 +148,11 @@ class Notifier implements INotifier, IDismissableNotifier {
 				[
 					'user' => [
 						'type' => 'user',
-						'id' => $param['sourceUser'],
-						'name' => $param['sourceUser'],
+						'id' => $sourceUser->getUID(),
+						'name' => $sourceUser->getDisplayName(),
 					],
 				])
-			->setParsedSubject(str_replace('{user}', $param['sourceUser'], $l->t('Incoming ownership transfer from {user}')))
+			->setParsedSubject(str_replace('{user}', $sourceUser->getDisplayName(), $l->t('Incoming ownership transfer from {user}')))
 			->setRichMessage(
 				$l->t("Do you want to accept {path}?\n\nNote: The transfer process after accepting may take up to 1 hour."),
 				[
@@ -168,6 +171,7 @@ class Notifier implements INotifier, IDismissableNotifier {
 		$l = $this->l10nFactory->get('files', $languageCode);
 		$param = $notification->getSubjectParameters();
 
+		$targetUser = $this->getUser($param['targetUser']);
 		$notification->setRichSubject($l->t('Ownership transfer failed'))
 			->setParsedSubject($l->t('Ownership transfer failed'))
 
@@ -181,11 +185,11 @@ class Notifier implements INotifier, IDismissableNotifier {
 					],
 					'user' => [
 						'type' => 'user',
-						'id' => $param['targetUser'],
-						'name' => $param['targetUser'],
+						'id' => $targetUser->getUID(),
+						'name' => $targetUser->getDisplayName(),
 					],
 				])
-			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $param['targetUser']], $l->t('Your ownership transfer of {path} to {user} failed.')));
+			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $targetUser->getDisplayName()], $l->t('Your ownership transfer of {path} to {user} failed.')));
 		return $notification;
 	}
 
@@ -193,6 +197,7 @@ class Notifier implements INotifier, IDismissableNotifier {
 		$l = $this->l10nFactory->get('files', $languageCode);
 		$param = $notification->getSubjectParameters();
 
+		$sourceUser = $this->getUser($param['sourceUser']);
 		$notification->setRichSubject($l->t('Ownership transfer failed'))
 			->setParsedSubject($l->t('Ownership transfer failed'))
 
@@ -206,11 +211,11 @@ class Notifier implements INotifier, IDismissableNotifier {
 					],
 					'user' => [
 						'type' => 'user',
-						'id' => $param['sourceUser'],
-						'name' => $param['sourceUser'],
+						'id' => $sourceUser->getUID(),
+						'name' => $sourceUser->getDisplayName(),
 					],
 				])
-			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $param['sourceUser']], $l->t('The ownership transfer of {path} from {user} failed.')));
+			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $sourceUser->getDisplayName()], $l->t('The ownership transfer of {path} from {user} failed.')));
 
 		return $notification;
 	}
@@ -219,6 +224,7 @@ class Notifier implements INotifier, IDismissableNotifier {
 		$l = $this->l10nFactory->get('files', $languageCode);
 		$param = $notification->getSubjectParameters();
 
+		$targetUser = $this->getUser($param['targetUser']);
 		$notification->setRichSubject($l->t('Ownership transfer done'))
 			->setParsedSubject($l->t('Ownership transfer done'))
 
@@ -232,11 +238,11 @@ class Notifier implements INotifier, IDismissableNotifier {
 					],
 					'user' => [
 						'type' => 'user',
-						'id' => $param['targetUser'],
-						'name' => $param['targetUser'],
+						'id' => $targetUser->getUID(),
+						'name' => $targetUser->getDisplayName(),
 					],
 				])
-			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $param['targetUser']], $l->t('Your ownership transfer of {path} to {user} has completed.')));
+			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $targetUser->getDisplayName()], $l->t('Your ownership transfer of {path} to {user} has completed.')));
 
 		return $notification;
 	}
@@ -245,6 +251,7 @@ class Notifier implements INotifier, IDismissableNotifier {
 		$l = $this->l10nFactory->get('files', $languageCode);
 		$param = $notification->getSubjectParameters();
 
+		$sourceUser = $this->getUser($param['sourceUser']);
 		$notification->setRichSubject($l->t('Ownership transfer done'))
 			->setParsedSubject($l->t('Ownership transfer done'))
 
@@ -258,11 +265,11 @@ class Notifier implements INotifier, IDismissableNotifier {
 					],
 					'user' => [
 						'type' => 'user',
-						'id' => $param['sourceUser'],
-						'name' => $param['sourceUser'],
+						'id' => $sourceUser->getUID(),
+						'name' => $sourceUser->getDisplayName(),
 					],
 				])
-			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $param['sourceUser']], $l->t('The ownership transfer of {path} from {user} has completed.')));
+			->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $sourceUser->getDisplayName()], $l->t('The ownership transfer of {path} from {user} has completed.')));
 
 		return $notification;
 	}
@@ -293,4 +300,12 @@ class Notifier implements INotifier, IDismissableNotifier {
 
 		$this->mapper->delete($transferOwnership);
 	}
+
+	protected function getUser(string $userId): IUser {
+		$user = $this->userManager->get($userId);
+		if ($user instanceof IUser) {
+			return $user;
+		}
+		throw new \InvalidArgumentException('User not found');
+	}
 }
diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php
index e1b26c6449e..3415a2fd9e7 100644
--- a/apps/files/lib/Service/OwnershipTransferService.php
+++ b/apps/files/lib/Service/OwnershipTransferService.php
@@ -36,6 +36,7 @@ use OCA\Files\Exception\TransferOwnershipException;
 use OCP\Encryption\IManager as IEncryptionManager;
 use OCP\Files\FileInfo;
 use OCP\Files\IHomeStorage;
+use OCP\Files\InvalidPathException;
 use OCP\Files\Mount\IMountManager;
 use OCP\IUser;
 use OCP\Share\IManager as IShareManager;
@@ -94,18 +95,31 @@ class OwnershipTransferService {
 			throw new TransferOwnershipException("The target user is not ready to accept files. The user has at least to have logged in once.", 2);
 		}
 
+		// setup filesystem
+		Filesystem::initMountPoints($sourceUid);
+		Filesystem::initMountPoints($destinationUid);
+
+		$view = new View();
+
 		if ($move) {
 			$finalTarget = "$destinationUid/files/";
 		} else {
 			$date = date('Y-m-d H-i-s');
-			$finalTarget = "$destinationUid/files/transferred from $sourceUid on $date";
-		}
 
-		// setup filesystem
-		Filesystem::initMountPoints($sourceUid);
-		Filesystem::initMountPoints($destinationUid);
+			// Remove some characters which are prone to cause errors
+			$cleanUserName = str_replace(['\\', '/', ':', '.', '?', '#', '\'', '"'], '-', $sourceUser->getDisplayName());
+			// Replace multiple dashes with one dash
+			$cleanUserName = preg_replace('/-{2,}/s', '-', $cleanUserName);
+			$cleanUserName = $cleanUserName ?: $sourceUid;
+
+			$finalTarget = "$destinationUid/files/transferred from $cleanUserName on $date";
+			try {
+				$view->verifyPath(dirname($finalTarget), basename($finalTarget));
+			} catch (InvalidPathException $e) {
+				$finalTarget = "$destinationUid/files/transferred from $sourceUid on $date";
+			}
+		}
 
-		$view = new View();
 		if (!($view->is_dir($sourcePath) || $view->is_file($sourcePath))) {
 			throw new TransferOwnershipException("Unknown path provided: $path", 1);
 		}
diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php
index 8fddbf71200..44da2e6f20f 100644
--- a/lib/private/Files/Filesystem.php
+++ b/lib/private/Files/Filesystem.php
@@ -828,7 +828,7 @@ class Filesystem {
 		$patterns = [
 			'/\\\\/s',          // no windows style slashes
 			'/\/\.(\/\.)?\//s', // remove '/./'
-			'/\/{2,}/s',        // remove squence of slashes
+			'/\/{2,}/s',        // remove sequence of slashes
 			'/\/\.$/s',         // remove trailing /.
 		];