fix(federation): Fix creating local cloudIds with http:// protocol

Signed-off-by: Joas Schilling <[email protected]>
nextcloud · Mar 25, 2024 · 09af86d · 09af86d
1 parent 46906b7
commit 09af86d
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 29 deletions.
diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php
@@ -195,9 +195,9 @@ public function testCreate($expirationDate, $expectedDataDate) {
 				$this->equalTo('myFile'),
 				$this->anything(),
 				'shareOwner',
-				'shareOwner@http://localhost/',
+				'shareOwner@http://localhost',
 				'sharedBy',
-				'sharedBy@http://localhost/'
+				'sharedBy@http://localhost'
 			)
 			->willReturn(true);
 
@@ -276,9 +276,9 @@ public function testCreateCouldNotFindServer() {
 				$this->equalTo('myFile'),
 				$this->anything(),
 				'shareOwner',
-				'shareOwner@http://localhost/',
+				'shareOwner@http://localhost',
 				'sharedBy',
-				'sharedBy@http://localhost/'
+				'sharedBy@http://localhost'
 			)->willReturn(false);
 
 		$this->rootFolder->method('getById')
@@ -337,9 +337,9 @@ public function testCreateException() {
 				$this->equalTo('myFile'),
 				$this->anything(),
 				'shareOwner',
-				'shareOwner@http://localhost/',
+				'shareOwner@http://localhost',
 				'sharedBy',
-				'sharedBy@http://localhost/'
+				'sharedBy@http://localhost'
 			)->willThrowException(new \Exception('dummy'));
 
 		$this->rootFolder->method('getById')
@@ -443,9 +443,9 @@ public function testCreateAlreadyShared() {
 				$this->equalTo('myFile'),
 				$this->anything(),
 				'shareOwner',
-				'shareOwner@http://localhost/',
+				'shareOwner@http://localhost',
 				'sharedBy',
-				'sharedBy@http://localhost/'
+				'sharedBy@http://localhost'
 			)->willReturn(true);
 
 		$this->rootFolder->expects($this->never())->method($this->anything());
@@ -514,9 +514,9 @@ public function testUpdate($owner, $sharedBy, $expirationDate) {
 				$this->equalTo('myFile'),
 				$this->anything(),
 				$owner,
-				$owner . '@http://localhost/',
+				$owner . '@http://localhost',
 				$sharedBy,
-				$sharedBy . '@http://localhost/'
+				$sharedBy . '@http://localhost'
 			)->willReturn(true);
 
 		if ($owner === $sharedBy) {

diff --git a/lib/private/Federation/CloudIdManager.php b/lib/private/Federation/CloudIdManager.php
@@ -168,52 +168,51 @@ protected function getDisplayNameFromContact(string $cloudId): ?string {
 	public function getCloudId(string $user, ?string $remote): ICloudId {
 		$isLocal = $remote === null;
 		if ($isLocal) {
-			$remote = rtrim($this->removeProtocolFromUrl($this->urlGenerator->getAbsoluteURL('/')), '/');
-			$fixedRemote = $this->fixRemoteURL($remote);
-			$host = $fixedRemote;
-		} else {
-			// note that for remote id's we don't strip the protocol for the remote we use to construct the CloudId
-			// this way if a user has an explicit non-https cloud id this will be preserved
-			// we do still use the version without protocol for looking up the display name
-			$fixedRemote = $this->fixRemoteURL($remote);
-			$host = $this->removeProtocolFromUrl($fixedRemote);
+			$remote = rtrim($this->urlGenerator->getAbsoluteURL('/'), '/');
+			$remote = $this->removeProtocolFromUrl($remote, true);
 		}
 
+		// note that for remote id's we don't strip the protocol for the remote we use to construct the CloudId
+		// this way if a user has an explicit non-https cloud id this will be preserved
+		// we do still use the version without protocol for looking up the display name
+		$remote = $this->fixRemoteURL($remote);
+		$host = $this->removeProtocolFromUrl($remote);
+
 		$key = $user . '@' . ($isLocal ? 'local' : $host);
 		$cached = $this->cache[$key] ?? $this->memCache->get($key);
 		if ($cached) {
 			$this->cache[$key] = $cached; // put items from memcache into local cache
 			return new CloudId($cached['id'], $cached['user'], $cached['remote'], $cached['displayName']);
 		}
 
+		$id = $user . '@' . $remote;
 		if ($isLocal) {
 			$localUser = $this->userManager->get($user);
 			$displayName = $localUser ? $localUser->getDisplayName() : '';
 		} else {
-			$displayName = $this->getDisplayNameFromContact($user . '@' . $host);
+			$displayName = $this->getDisplayNameFromContact($id);
 		}
-		$id = $user . '@' . $remote;
 
 		$data = [
 			'id' => $id,
 			'user' => $user,
-			'remote' => $fixedRemote,
+			'remote' => $remote,
 			'displayName' => $displayName,
 		];
 		$this->cache[$key] = $data;
 		$this->memCache->set($key, $data, 15 * 60);
-		return new CloudId($id, $user, $fixedRemote, $displayName);
+		return new CloudId($id, $user, $remote, $displayName);
 	}
 
 	/**
 	 * @param string $url
 	 * @return string
 	 */
-	public function removeProtocolFromUrl(string $url): string {
+	public function removeProtocolFromUrl(string $url, bool $httpsOnly = false): string {
 		if (str_starts_with($url, 'https://')) {
 			return substr($url, 8);
 		}
-		if (str_starts_with($url, 'http://')) {
+		if (!$httpsOnly && str_starts_with($url, 'http://')) {
 			return substr($url, 7);
 		}
 

diff --git a/tests/lib/Federation/CloudIdManagerTest.php b/tests/lib/Federation/CloudIdManagerTest.php
@@ -124,9 +124,11 @@ public function testInvalidCloudId($cloudId) {
 		$this->cloudIdManager->resolveCloudId($cloudId);
 	}
 
-	public function getCloudIdProvider() {
+	public function getCloudIdProvider(): array {
 		return [
 			['test', 'example.com', '[email protected]'],
+			['test', 'http://example.com', 'test@http://example.com'],
+			['test', null, 'test@http://example.com', 'http://example.com'],
 			['[email protected]', 'example.com', '[email protected]@example.com'],
 			['[email protected]', null, '[email protected]@example.com'],
 		];
@@ -136,10 +138,10 @@ public function getCloudIdProvider() {
 	 * @dataProvider getCloudIdProvider
 	 *
 	 * @param string $user
-	 * @param string $remote
+	 * @param null|string $remote
 	 * @param string $id
 	 */
-	public function testGetCloudId($user, $remote, $id) {
+	public function testGetCloudId(string $user, ?string $remote, string $id, ?string $localHost = 'https://example.com'): void {
 		if ($remote !== null) {
 			$this->contactsManager->expects($this->any())
 				->method('search')
@@ -153,7 +155,7 @@ public function testGetCloudId($user, $remote, $id) {
 		} else {
 			$this->urlGenerator->expects(self::once())
 				->method('getAbsoluteUrl')
-				->willReturn('https://example.com');
+				->willReturn($localHost);
 		}
 
 		$cloudId = $this->cloudIdManager->getCloudId($user, $remote);