Merge pull request #46133 from nextcloud/bugfix/noid/fix-missing-prot…

…ocol-on-remotes

fix(federation): Fix missing protocol on CloudID remote

apps/federatedfilesharing/tests/AddressHandlerTest.php

@@ -74,6 +74,11 @@ public function dataTestSplitUserRemote() {
 				foreach ($protocols as $protocol) {
 					$baseUrl = $user . '@' . $protocol . $remote;
 
+					if ($protocol === '') {
+						// https:// protocol is expected in the final result
+						$protocol = 'https://';
+					}
+
 					$testCases[] = [$baseUrl, $user, $protocol . $remote];
 					$testCases[] = [$baseUrl . '/', $user, $protocol . $remote];
 					$testCases[] = [$baseUrl . '/index.php', $user, $protocol . $remote];

lib/private/Collaboration/Collaborators/RemotePlugin.php

@@ -157,7 +157,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b
 	public function splitUserRemote(string $address): array {
 		try {
 			$cloudId = $this->cloudIdManager->resolveCloudId($address);
-			return [$cloudId->getUser(), $cloudId->getRemote()];
+			return [$cloudId->getUser(), $this->cloudIdManager->removeProtocolFromUrl($cloudId->getRemote(), true)];
 		} catch (\InvalidArgumentException $e) {
 			throw new \InvalidArgumentException('Invalid Federated Cloud ID', 0, $e);
 		}

lib/private/Federation/CloudIdManager.php

@@ -84,7 +84,7 @@ public function resolveCloudId(string $cloudId): ICloudId {
 		}
 
 		// Find the first character that is not allowed in user names
-		$id = $this->fixRemoteURL($cloudId);
+		$id = $this->stripShareLinkFragments($cloudId);
 		$posSlash = strpos($id, '/');
 		$posColon = strpos($id, ':');
 
@@ -107,6 +107,7 @@ public function resolveCloudId(string $cloudId): ICloudId {
 			$this->userManager->validateUserId($user);
 
 			if (!empty($user) && !empty($remote)) {
+				$remote = $this->ensureDefaultProtocol($remote);
 				return new CloudId($id, $user, $remote, $this->getDisplayNameFromContact($id));
 			}
 		}
@@ -152,8 +153,9 @@ public function getCloudId(string $user, ?string $remote): ICloudId {
 		// 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);
+		$remote = $this->stripShareLinkFragments($remote);
 		$host = $this->removeProtocolFromUrl($remote);
+		$remote = $this->ensureDefaultProtocol($remote);
 
 		$key = $user . '@' . ($isLocal ? 'local' : $host);
 		$cached = $this->cache[$key] ?? $this->memCache->get($key);
@@ -198,6 +200,14 @@ public function removeProtocolFromUrl(string $url, bool $httpsOnly = false): str
 		return $url;
 	}
 
+	protected function ensureDefaultProtocol(string $remote): string {
+		if (!str_contains($remote, '://')) {
+			$remote = 'https://' . $remote;
+		}
+
+		return $remote;
+	}
+
 	/**
 	 * Strips away a potential file names and trailing slashes:
 	 * - http://localhost
@@ -210,7 +220,7 @@ public function removeProtocolFromUrl(string $url, bool $httpsOnly = false): str
 	 * @param string $remote
 	 * @return string
 	 */
-	protected function fixRemoteURL(string $remote): string {
+	protected function stripShareLinkFragments(string $remote): string {
 		$remote = str_replace('\\', '/', $remote);
 		if ($fileNamePosition = strpos($remote, '/index.php')) {
 			$remote = substr($remote, 0, $fileNamePosition);

lib/public/Federation/ICloudIdManager.php

@@ -48,9 +48,11 @@ public function isValidCloudId(string $cloudId): bool;
 	 * remove scheme/protocol from an url
 	 *
 	 * @param string $url
+	 * @param bool $httpsOnly
 	 *
 	 * @return string
 	 * @since 28.0.0
+	 * @since 30.0.0 - Optional parameter $httpsOnly was added
 	 */
-	public function removeProtocolFromUrl(string $url): string;
+	public function removeProtocolFromUrl(string $url, bool $httpsOnly = false): string;
 }

tests/lib/Collaboration/Collaborators/RemotePluginTest.php

@@ -395,6 +395,11 @@ public function dataTestSplitUserRemote() {
 				foreach ($protocols as $protocol) {
 					$baseUrl = $user . '@' . $protocol . $remote;
 
+					if ($protocol === 'https://') {
+						// https:// protocol is not expected in the final result
+						$protocol = '';
+					}
+
 					$testCases[] = [$baseUrl, $user, $protocol . $remote];
 					$testCases[] = [$baseUrl . '/', $user, $protocol . $remote];
 					$testCases[] = [$baseUrl . '/index.php', $user, $protocol . $remote];

tests/lib/Federation/CloudIdManagerTest.php

@@ -48,7 +48,7 @@ protected function setUp(): void {
 		);
 	}
 
-	public function cloudIdProvider() {
+	public function cloudIdProvider(): array {
 		return [
 			['test@example.com', 'test', 'example.com', 'test@example.com'],
 			['test@example.com/cloud', 'test', 'example.com/cloud', 'test@example.com/cloud'],
@@ -60,12 +60,8 @@ public function cloudIdProvider() {
 
 	/**
 	 * @dataProvider cloudIdProvider
-	 *
-	 * @param string $cloudId
-	 * @param string $user
-	 * @param string $remote
 	 */
-	public function testResolveCloudId($cloudId, $user, $remote, $cleanId) {
+	public function testResolveCloudId(string $cloudId, string $user, string $noProtocolRemote, string $cleanId): void {
 		$displayName = 'Ample Ex';
 
 		$this->contactsManager->expects($this->any())
@@ -81,12 +77,12 @@ public function testResolveCloudId($cloudId, $user, $remote, $cleanId) {
 		$cloudId = $this->cloudIdManager->resolveCloudId($cloudId);
 
 		$this->assertEquals($user, $cloudId->getUser());
-		$this->assertEquals($remote, $cloudId->getRemote());
+		$this->assertEquals('https://' . $noProtocolRemote, $cloudId->getRemote());
 		$this->assertEquals($cleanId, $cloudId->getId());
-		$this->assertEquals($displayName . '@' . $remote, $cloudId->getDisplayId());
+		$this->assertEquals($displayName . '@' . $noProtocolRemote, $cloudId->getDisplayId());
 	}
 
-	public function invalidCloudIdProvider() {
+	public function invalidCloudIdProvider(): array {
 		return [
 			['example.com'],
 			['test:foo@example.com'],
@@ -100,7 +96,7 @@ public function invalidCloudIdProvider() {
 	 * @param string $cloudId
 	 *
 	 */
-	public function testInvalidCloudId($cloudId) {
+	public function testInvalidCloudId(string $cloudId): void {
 		$this->expectException(\InvalidArgumentException::class);
 
 		$this->contactsManager->expects($this->never())
@@ -111,10 +107,10 @@ public function testInvalidCloudId($cloudId) {
 
 	public function getCloudIdProvider(): array {
 		return [
-			['test', 'example.com', 'test@example.com'],
+			['test', 'example.com', 'test@example.com', null, 'https://example.com', 'https://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', 'http://example.com'],
-			['test@example.com', 'example.com', 'test@example.com@example.com'],
+			['test@example.com', 'example.com', 'test@example.com@example.com', null, 'https://example.com', 'https://example.com'],
 			['test@example.com', 'https://example.com', 'test@example.com@example.com'],
 			['test@example.com', null, 'test@example.com@example.com', null, 'https://example.com', 'https://example.com'],
 			['test@example.com', 'https://example.com/index.php/s/shareToken', 'test@example.com@example.com', null, 'https://example.com', 'https://example.com'],
@@ -123,10 +119,6 @@ public function getCloudIdProvider(): array {
 
 	/**
 	 * @dataProvider getCloudIdProvider
-	 *
-	 * @param string $user
-	 * @param null|string $remote
-	 * @param string $id
 	 */
 	public function testGetCloudId(string $user, ?string $remote, string $id, ?string $searchCloudId = null, ?string $localHost = 'https://example.com', ?string $expectedRemoteId = null): void {
 		if ($remote !== null) {