Merge pull request #44453 from nextcloud/bugfix/noid/fix-cloud-id-gen…

…eration-with-http fix(federation): Fix creating local cloudIds with http:// protocol
nextcloud · Mar 25, 2024 · ce059fe · ce059fe
2 parents 16e4a93 + dababa5
commit ce059fe
Show file tree

Hide file tree

Showing 3 changed files with 32 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,17 +168,16 @@ 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('/'), '/');
 		}
 
+		// 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 @@ public function getCloudId(string $user, ?string $remote): ICloudId {
 		$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,36 +124,40 @@ 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', '[email protected]'],
+			['test', null, 'test@http://example.com', '[email protected]', 'http://example.com'],
 			['[email protected]', 'example.com', '[email protected]@example.com'],
+			['[email protected]', 'https://example.com', '[email protected]@example.com'],
 			['[email protected]', null, '[email protected]@example.com'],
+			['[email protected]', 'https://example.com/index.php/s/shareToken', '[email protected]@example.com'],
 		];
 	}
 
 	/**
 	 * @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);