Merge pull request #44453 from nextcloud/bugfix/noid/fix-cloud-id-generation-with-http

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

3 files changed, 32 insertions, 29 deletions

diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php
index 6f0cb93c4a9..40b8b944507 100644
--- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php
+++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php
@@ -195,9 +195,9 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				$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 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				$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 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				$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 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				$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 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				$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
index 22b06af386f..70431ce2289 100644
--- a/lib/private/Federation/CloudIdManager.php
+++ b/lib/private/Federation/CloudIdManager.php
@@ -168,17 +168,16 @@ class CloudIdManager implements ICloudIdManager {
 	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('/'), '/');
 		}
 
+		// 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->removeProtocolFromUrl($remote, true);
+		$remote = $this->fixRemoteURL($remote);
+		$host = $this->removeProtocolFromUrl($remote);
+
 		$key = $user . '@' . ($isLocal ? 'local' : $host);
 		$cached = $this->cache[$key] ?? $this->memCache->get($key);
 		if ($cached) {
@@ -197,23 +196,23 @@ class CloudIdManager implements ICloudIdManager {
 		$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
index 0db36b0524a..748d292e977 100644
--- a/tests/lib/Federation/CloudIdManagerTest.php
+++ b/tests/lib/Federation/CloudIdManagerTest.php
@@ -124,11 +124,15 @@ class CloudIdManagerTest extends TestCase {
 		$this->cloudIdManager->resolveCloudId($cloudId);
 	}
 
-	public function getCloudIdProvider() {
+	public function getCloudIdProvider(): array {
 		return [
 			['test', 'example.com', 'test@example.com'],
+			['test', 'http://example.com', 'test@http://example.com', 'test@example.com'],
+			['test', null, 'test@http://example.com', 'test@example.com', 'http://example.com'],
 			['test@example.com', 'example.com', 'test@example.com@example.com'],
+			['test@example.com', 'https://example.com', 'test@example.com@example.com'],
 			['test@example.com', null, 'test@example.com@example.com'],
+			['test@example.com', 'https://example.com/index.php/s/shareToken', 'test@example.com@example.com'],
 		];
 	}
 
@@ -136,24 +140,24 @@ class CloudIdManagerTest extends TestCase {
 	 * @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 $searchCloudId = null, ?string $localHost = 'https://example.com'): void {
 		if ($remote !== null) {
 			$this->contactsManager->expects($this->any())
 				->method('search')
-				->with($id, ['CLOUD'])
+				->with($searchCloudId ?? $id, ['CLOUD'])
 				->willReturn([
 					[
-						'CLOUD' => [$id],
+						'CLOUD' => [$searchCloudId ?? $id],
 						'FN' => 'Ample Ex',
 					]
 				]);
 		} else {
 			$this->urlGenerator->expects(self::once())
 				->method('getAbsoluteUrl')
-				->willReturn('https://example.com');
+				->willReturn($localHost);
 		}
 
 		$cloudId = $this->cloudIdManager->getCloudId($user, $remote);