Handle moving calendar to an user who already has the share

Extra:
    * Fix @ChristophWurst style remarks
    * Add a Note that share links have changed when calendars has user shares (see #13603)

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix forgotten test change

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

apps/dav/lib/Command/ListCalendars.php

@@ -59,13 +59,13 @@ class ListCalendars extends Command {
 		$this
 			->setName('dav:list-calendars')
 			->setDescription('List all calendars of a user')
-			->addArgument('user',
+			->addArgument('uid',
 				InputArgument::REQUIRED,
 				'User for whom all calendars will be listed');
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output) {
-		$user = $input->getArgument('user');
+		$user = $input->getArgument('uid');
 		if (!$this->userManager->userExists($user)) {
 			throw new \InvalidArgumentException("User <$user> is unknown.");
 		}

apps/dav/lib/Command/MoveCalendar.php

@@ -93,18 +93,18 @@ class MoveCalendar extends Command {
 			->addArgument('name',
 				InputArgument::REQUIRED,
 				'Name of the calendar to move')
-			->addArgument('userorigin',
+			->addArgument('sourceuid',
 				InputArgument::REQUIRED,
 				'User who currently owns the calendar')
-			->addArgument('userdestination',
+			->addArgument('destinationuid',
 				InputArgument::REQUIRED,
 				'User who will receive the calendar')
-			->addOption('force', 'f', InputOption::VALUE_NONE, "Force the migration by removing shares with groups that the destination user is not in");
+			->addOption('force', 'f', InputOption::VALUE_NONE, "Force the migration by removing existing shares");
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output) {
-		$userOrigin = $input->getArgument('userorigin');
-		$userDestination = $input->getArgument('userdestination');
+		$userOrigin = $input->getArgument('sourceuid');
+		$userDestination = $input->getArgument('destinationuid');
 
 		$this->io = new SymfonyStyle($input, $output);
 
@@ -128,9 +128,7 @@ class MoveCalendar extends Command {
 			throw new \InvalidArgumentException("User <$userDestination> already has a calendar named <$name>.");
 		}
 
-		if ($this->shareManager->shareWithGroupMembersOnly() === true) {
-			$this->checkShares($calendar, $userDestination, $input->getOption('force'));
-		}
+		$this->checkShares($calendar, $userOrigin, $userDestination, $input->getOption('force'));
 
 		$this->calDav->moveCalendar($name, self::URI_USERS . $userOrigin, self::URI_USERS . $userDestination);
 
@@ -138,25 +136,50 @@ class MoveCalendar extends Command {
 	}
 
 	/**
-	 * Check that user destination is member of the groups which whom the calendar was shared
-	 * If we ask to force the migration, the share with the group is dropped
+	 * Check that moving the calendar won't break shares
 	 *
-	 * @param $calendar
-	 * @param $userDestination
+	 * @param array $calendar
+	 * @param string $userOrigin
+	 * @param string $userDestination
 	 * @param bool $force
 	 */
-	private function checkShares($calendar, $userDestination, $force = false)
+	private function checkShares(array $calendar, string $userOrigin, string $userDestination, bool $force = false)
 	{
 		$shares = $this->calDav->getShares($calendar['id']);
 		foreach ($shares as $share) {
-			list(, $prefix, $group) = explode('/', $share['href'], 3);
-			if ('groups' === $prefix && !$this->groupManager->isInGroup($userDestination, $group)) {
+			list(, $prefix, $userOrGroup) = explode('/', $share['href'], 3);
+
+			/**
+			 * Check that user destination is member of the groups which whom the calendar was shared
+			 * If we ask to force the migration, the share with the group is dropped
+			 */
+			if ($this->shareManager->shareWithGroupMembersOnly() === true && 'groups' === $prefix && !$this->groupManager->isInGroup($userDestination, $userOrGroup)) {
+				if ($force) {
+					$this->calDav->updateShares(new Calendar($this->calDav, $calendar, $this->l10n, $this->config), [], ['href' => 'principal:principals/groups/' . $userOrGroup]);
+				} else {
+					throw new \InvalidArgumentException("User <$userDestination> is not part of the group <$userOrGroup> with whom the calendar <" . $calendar['uri'] . "> was shared. You may use -f to move the calendar while deleting this share.");
+				}
+			}
+
+			/**
+			 * Check that calendar isn't already shared with user destination
+			 */
+			if ($userOrGroup === $userDestination) {
 				if ($force) {
-					$this->calDav->updateShares(new Calendar($this->calDav, $calendar, $this->l10n, $this->config), [], ['href' => 'principal:principals/groups/' . $group]);
+					$this->calDav->updateShares(new Calendar($this->calDav, $calendar, $this->l10n, $this->config), [], ['href' => 'principal:principals/users/' . $userOrGroup]);
 				} else {
-					throw new \InvalidArgumentException("User <$userDestination> is not part of the group <$group> with whom the calendar <" . $calendar['uri'] . "> was shared. You may use -f to move the calendar while deleting this share.");
+					throw new \InvalidArgumentException("The calendar <" . $calendar['uri'] . "> is already shared to user <$userDestination>.You may use -f to move the calendar while deleting this share.");
 				}
 			}
 		}
+		/**
+		 * Warn that share links have changed if there are shares
+		 */
+		if (count($shares) > 0) {
+			$this->io->note([
+				"Please note that moving calendar " . $calendar['uri'] . " from user <$userOrigin> to <$userDestination> has caused share links to change.", 
+				"Sharees will need to change \"example.com/remote.php/dav/calendars/uid/" . $calendar['uri'] . "_shared_by_$userOrigin\" to \"example.com/remote.php/dav/calendars/uid/" . $calendar['uri'] . "_shared_by_$userDestination\""
+			]);
+		}
 	}
 }

apps/dav/tests/unit/Command/ListCalendarsTest.php

@@ -36,10 +36,10 @@ use Test\TestCase;
  */
 class ListCalendarsTest extends TestCase {
 
-	/** @var \OCP\IUserManager|\PHPUnit_Framework_MockObject_MockObject $userManager */
+	/** @var \OCP\IUserManager|\PHPUnit\Framework\MockObject\MockObject $userManager */
 	private $userManager;
 
-	/** @var CalDavBackend|\PHPUnit_Framework_MockObject_MockObject $l10n */
+	/** @var CalDavBackend|\PHPUnit\Framework\MockObject\MockObject $l10n */
 	private $calDav;
 
 	/** @var ListCalendars */
@@ -71,7 +71,7 @@ class ListCalendarsTest extends TestCase {
 
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
-			'user' => self::USERNAME,
+			'uid' => self::USERNAME,
 		]);
 		$this->assertContains("User <" . self::USERNAME . "> in unknown", $commandTester->getDisplay());
 	}
@@ -90,7 +90,7 @@ class ListCalendarsTest extends TestCase {
 
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
-			'user' => self::USERNAME,
+			'uid' => self::USERNAME,
 		]);
 		$this->assertContains("User <" . self::USERNAME . "> has no calendars\n", $commandTester->getDisplay());
 	}
@@ -131,7 +131,7 @@ class ListCalendarsTest extends TestCase {
 
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
-			'user' => self::USERNAME,
+			'uid' => self::USERNAME,
 		]);
 		$this->assertContains($output, $commandTester->getDisplay());
 		$this->assertNotContains(BirthdayService::BIRTHDAY_CALENDAR_URI, $commandTester->getDisplay());

apps/dav/tests/unit/Command/MoveCalendarTest.php

@@ -39,10 +39,10 @@ use Test\TestCase;
  */
 class MoveCalendarTest extends TestCase {
 
-	/** @var \OCP\IUserManager|\PHPUnit_Framework_MockObject_MockObject $userManager */
+	/** @var \OCP\IUserManager|\PHPUnit\Framework\MockObject\MockObject $userManager */
 	private $userManager;
 
-	/** @var \OCP\IGroupManager|\PHPUnit_Framework_MockObject_MockObject $groupManager */
+	/** @var \OCP\IGroupManager|\PHPUnit\Framework\MockObject\MockObject $groupManager */
 	private $groupManager;
 
 	/** @var \OCP\Share\IManager|\PHPUnit_Framework_MockObject_MockObject $shareManager */
@@ -111,8 +111,8 @@ class MoveCalendarTest extends TestCase {
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => $this->command->getName(),
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 		]);
 	}
 
@@ -139,8 +139,8 @@ class MoveCalendarTest extends TestCase {
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => 'personal',
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 		]);
 	}
 
@@ -175,8 +175,8 @@ class MoveCalendarTest extends TestCase {
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => 'personal',
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 		]);
 	}
 
@@ -206,14 +206,11 @@ class MoveCalendarTest extends TestCase {
 			->with(1234)
 			->willReturn([]);
 
-		$this->shareManager->expects($this->once())->method('shareWithGroupMembersOnly')
-			->willReturn(true);
-
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => 'personal',
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 		]);
 
 		$this->assertContains("[OK] Calendar <personal> was moved from user <user> to <user2>", $commandTester->getDisplay());
@@ -256,13 +253,12 @@ class MoveCalendarTest extends TestCase {
 		$this->shareManager->expects($this->once())->method('shareWithGroupMembersOnly')
 			->willReturn($shareWithGroupMembersOnly);
 
+		$this->calDav->expects($this->once())->method('getShares')
+			->with(1234)
+			->willReturn([
+				['href' => 'principal:principals/groups/nextclouders']
+		]);
 		if ($shareWithGroupMembersOnly === true) {
-			$this->calDav->expects($this->once())->method('getShares')
-				->with(1234)
-				->willReturn([
-					['href' => 'principal:principals/groups/nextclouders']
-			]);
-
 			$this->expectException(InvalidArgumentException::class);
 			$this->expectExceptionMessage("User <user2> is not part of the group <nextclouders> with whom the calendar <personal> was shared. You may use -f to move the calendar while deleting this share.");
 		}
@@ -270,8 +266,8 @@ class MoveCalendarTest extends TestCase {
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => 'personal',
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 		]);
 	}
 
@@ -314,8 +310,8 @@ class MoveCalendarTest extends TestCase {
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => 'personal',
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 		]);
 
 		$this->assertContains("[OK] Calendar <personal> was moved from user <user> to <user2>", $commandTester->getDisplay());
@@ -338,7 +334,7 @@ class MoveCalendarTest extends TestCase {
 			->willReturn([
 				'id' => 1234,
 				'uri' => 'personal',
-				'{DAV:}displayname' => 'personal'
+				'{DAV:}displayname' => 'Personal'
 			]);
 
 		$this->calDav->expects($this->at(1))->method('getCalendarByUri')
@@ -351,17 +347,81 @@ class MoveCalendarTest extends TestCase {
 		$this->calDav->expects($this->once())->method('getShares')
 			->with(1234)
 			->willReturn([
-				['href' => 'principal:principals/groups/nextclouders']
+				[
+					'href' => 'principal:principals/groups/nextclouders',
+					'{DAV:}displayname' => 'Personal'
+				]
 			]);
+		$this->calDav->expects($this->once())->method('updateShares');
 
 		$commandTester = new CommandTester($this->command);
 		$commandTester->execute([
 			'name' => 'personal',
-			'userorigin' => 'user',
-			'userdestination' => 'user2',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
 			'--force' => true
 		]);
 
 		$this->assertContains("[OK] Calendar <personal> was moved from user <user> to <user2>", $commandTester->getDisplay());
 	}
+
+	public function dataTestMoveWithCalendarAlreadySharedToDestination(): array
+	{
+		return [
+			[true],
+			[false]
+		];
+	}
+
+	/**
+	 * @dataProvider dataTestMoveWithCalendarAlreadySharedToDestination
+	 */
+	public function testMoveWithCalendarAlreadySharedToDestination(bool $force)
+	{
+		$this->userManager->expects($this->at(0))
+			->method('userExists')
+			->with('user')
+			->willReturn(true);
+
+		$this->userManager->expects($this->at(1))
+			->method('userExists')
+			->with('user2')
+			->willReturn(true);
+
+		$this->calDav->expects($this->at(0))->method('getCalendarByUri')
+			->with('principals/users/user', 'personal')
+			->willReturn([
+				'id' => 1234,
+				'uri' => 'personal',
+				'{DAV:}displayname' => 'Personal',
+			]);
+
+		$this->calDav->expects($this->at(1))->method('getCalendarByUri')
+			->with('principals/users/user2', 'personal')
+			->willReturn(null);
+
+		$this->calDav->expects($this->once())->method('getShares')
+				->with(1234)
+				->willReturn([
+					[
+						'href' => 'principal:principals/users/user2',
+						'{DAV:}displayname' => 'Personal'
+					]
+			]);
+
+		if ($force === false) {
+			$this->expectException(InvalidArgumentException::class);
+			$this->expectExceptionMessage("The calendar <personal> is already shared to user <user2>.You may use -f to move the calendar while deleting this share.");
+		} else {
+			$this->calDav->expects($this->once())->method('updateShares');
+		}
+
+		$commandTester = new CommandTester($this->command);
+		$commandTester->execute([
+			'name' => 'personal',
+			'sourceuid' => 'user',
+			'destinationuid' => 'user2',
+			'--force' => $force,
+		]);
+	}
 }