Merge pull request #993 from nextcloud/port/fixes

Port/fixes

Author: saw-jan

CHANGELOG.md

@@ -16,10 +16,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 - Fix: Handle projects whose parent project is unknown [#985](https://github.com/nextcloud/integration_openproject/pull/985)
+- Fix: Force HTTPS on Nextcloud base URL for OpenProject API requests [#992](https://github.com/nextcloud/integration_openproject/pull/992)
 
 ### Added
 
 - [BREAKING] Support Nextcloud 33 [#952](https://github.com/nextcloud/integration_openproject/pull/952)
+- Include OpenProject API request as debug log [#991](https://github.com/nextcloud/integration_openproject/pull/991)
 
 ### Changed
 

lib/Service/OpenProjectAPIService.php

@@ -246,10 +246,25 @@ public function getNotifications(string $userId): array {
 	}
 
 	/**
-	 * wrapper around IURLGenerator::getBaseUrl() to make it easier to mock in tests
+	 * @return string
 	 */
-	public function getBaseUrl(): string {
-		return $this->config->getSystemValueString('overwrite.cli.url');
+	public function getNCBaseUrl(): string {
+		$message = 'Invalid or missing "overwrite.cli.url" system configuration.';
+		$ncUrl = $this->config->getSystemValueString('overwrite.cli.url');
+		if (!$ncUrl) {
+			$this->logger->error($message, ['app' => $this->appName]);
+			return '';
+		}
+
+		$parsedUrl = parse_url($ncUrl);
+		if (!$parsedUrl || !isset($parsedUrl['scheme']) || !isset($parsedUrl['host'])) {
+			$this->logger->error($message, ['app' => $this->appName]);
+			return '';
+		}
+		if ($parsedUrl['scheme'] !== 'https') {
+			$ncUrl = str_replace('http://', 'https://', $ncUrl);
+		}
+		return $ncUrl;
 	}
 
 	/**
@@ -303,7 +318,7 @@ private function searchRequest(string $userId, array $filters, bool $onlyLinkabl
 		if ($onlyLinkableWorkPackages) {
 			$filters[] = [
 				'linkable_to_storage_url' =>
-					['operator' => '=', 'values' => [urlencode($this->getBaseUrl())]]
+					['operator' => '=', 'values' => [urlencode($this->getNCBaseUrl())]]
 			];
 		}
 
@@ -439,6 +454,15 @@ public function rawRequest(
 			}
 		}
 
+		$sanitizedOptions = $this->sanitizeReqOptionsForLog($options);
+		$requestLog = json_encode([
+			'method' => $method,
+			'url' => $url,
+			'headers' => $sanitizedOptions['headers'],
+			'body' => $sanitizedOptions['body'],
+		]);
+		$this->logger->debug('OpenProject API request: ' . $requestLog, ['app' => $this->appName]);
+
 		if ($method === 'GET') {
 			$response = $this->client->get($url, $options);
 		} elseif ($method === 'POST') {
@@ -757,7 +781,7 @@ public function linkWorkPackageToFile(
 				],
 				'_links' => [
 					'storageUrl' => [
-						'href' => $this->getBaseUrl()
+						'href' => $this->getNCBaseUrl()
 					]
 				]
 			];
@@ -1511,7 +1535,7 @@ public function getAvailableOpenProjectProjects(string $userId, ?string $searchQ
 		}
 		$filters[] = [
 			'storageUrl' =>
-				['operator' => '=', 'values' => [$this->getBaseUrl()]],
+				['operator' => '=', 'values' => [$this->getNCBaseUrl()]],
 			'userAction' =>
 				['operator' => '&=', 'values' => ["file_links/manage", "work_packages/create"]]
 		];
@@ -1867,4 +1891,42 @@ public function getAppsName(string $appId): string {
 
 		return $appInfo['name'];
 	}
+
+	/**
+	 * @param array $options
+	 *
+	 * @return array
+	 */
+	public function sanitizeReqOptionsForLog(array $options): array {
+		$sanitizedOptions = [
+			'headers' => [],
+			'body' => null,
+		];
+		$headers = ['authorization', 'cookie', 'set-cookie'];
+		if (isset($options['headers'])) {
+			foreach ($options['headers'] as $key => $value) {
+				if (in_array(strtolower($key), $headers)) {
+					$sanitizedOptions['headers'][$key] = '[REDACTED]';
+				} else {
+					$sanitizedOptions['headers'][$key] = $value;
+				}
+			}
+		}
+
+		if (isset($options['body'])) {
+			try {
+				$body = json_decode($options['body'], true, 512, JSON_THROW_ON_ERROR);
+				$fields = ['password', 'client_secret', 'refresh_token', 'access_token', 'token'];
+				foreach ($fields as $field) {
+					if (isset($body[$field])) {
+						$body[$field] = '[REDACTED]';
+					}
+				}
+				$sanitizedOptions['body'] = $body;
+			} catch (\JsonException) {
+				$sanitizedOptions['body'] = '[DATA]';
+			}
+		}
+		return $sanitizedOptions;
+	}
 }

tests/lib/Service/OpenProjectAPIServiceTest.php

@@ -803,10 +803,6 @@ private function getOpenProjectAPIService(
 			$appManagerMock->method('isInstalled')->willReturn(true);
 		}
 
-		$urlGeneratorMock = $this->getMockBuilder(IURLGenerator::class)->getMock();
-		$urlGeneratorMock
-			->method('getBaseUrl')
-			->willReturn($baseUrl);
 		$this->defaultConfigMock
 			->method('getSystemValueString')
 			->with($this->equalTo('overwrite.cli.url'))
@@ -817,7 +813,6 @@ private function getOpenProjectAPIService(
 			'config' => $this->defaultConfigMock,
 			'clientService' => $clientService,
 			'rootFolder' => $storageMock,
-			'urlGenerator' => $urlGeneratorMock,
 			'appManager' => $appManagerMock,
 			'tokenEventFactory' => $exchangeTokenMock,
 		]);
@@ -831,21 +826,27 @@ private function getOpenProjectAPIService(
 	 *
 	 * @param array<string> $mockMethods
 	 * @param array<string, object> $constructParams
+	 * @param bool $mockBaseUrl
 	 *
 	 * @return OpenProjectAPIService|MockObject
 	 */
 	private function getOpenProjectAPIServiceMock(
 		array $mockMethods = ['request'],
 		array $constructParams = [],
+		bool $mockBaseUrl = true
 	): OpenProjectAPIService|MockObject {
-		$mockMethods[] = 'getBaseUrl';
+		if ($mockBaseUrl) {
+			$mockMethods[] = 'getNCBaseUrl';
+		}
 		$constructArgs = $this->getOpenProjectAPIServiceConstructArgs($constructParams);
 
 		$mock = $this->getMockBuilder(OpenProjectAPIService::class)
 			->setConstructorArgs($constructArgs)
 			->onlyMethods($mockMethods)
 			->getMock();
-		$mock->method('getBaseUrl')->willReturn('https://nc.my-server.org');
+		if ($mockBaseUrl) {
+			$mock->method('getNCBaseUrl')->willReturn('https://nc.my-server.org');
+		}
 		return $mock;
 	}
 
@@ -3906,14 +3907,7 @@ public function testGetAvailableOpenProjectProjectsErrorResponse(): void {
 	 * @return void
 	 */
 	public function testGetAvailableOpenProjectProjectsQueryOnly() {
-		$iUrlGeneratorMock = $this->getMockBuilder(IURLGenerator::class)->disableOriginalConstructor()->getMock();
-		$iUrlGeneratorMock->method('getBaseUrl')->willReturn('https%3A%2F%2Fnc.my-server.org');
-		$service = $this->getOpenProjectAPIServiceMock(
-			['request'],
-			[
-				'urlGenerator' => $iUrlGeneratorMock,
-			],
-		);
+		$service = $this->getOpenProjectAPIServiceMock(['request']);
 		$service->method('request')
 			->with(
 				'user', 'work_packages/available_projects',
@@ -5197,4 +5191,52 @@ public function testGetAppsName(string $appId, ?array $appInfo, string $expected
 		$this->assertSame($expected, $result);
 	}
 
+	/**
+	 * @return array<mixed>
+	 */
+	public function getNCBaseUrlDataProvider() {
+		return [
+			'HTTP url' => [
+				'url' => 'http://test.nc.local',
+				'expected' => 'https://test.nc.local',
+			],
+			'HTTPS url' => [
+				'url' => 'https://test.nc.local',
+				'expected' => 'https://test.nc.local',
+			],
+			'invalid url' => [
+				'url' => 'invalid_url',
+				'expected' => '',
+			],
+			'empty url' => [
+				'url' => '',
+				'expected' => '',
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider getNCBaseUrlDataProvider
+	 * @param string $url
+	 * @param string $expected
+	 * @return void
+	 */
+	public function testGetNCBaseUrl(string $url, string $expected): void {
+		$configMock = $this->createMock(IConfig::class);
+		$configMock
+			->method('getSystemValueString')
+			->with($this->equalTo('overwrite.cli.url'))
+			->willReturn($url);
+
+		$service = $this->getOpenProjectAPIServiceMock(
+			[],
+			[
+				'config' => $configMock,
+			],
+			false,
+		);
+
+		$baseUrl = $service->getNCBaseUrl();
+		$this->assertEquals($expected, $baseUrl);
+	}
 }