diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b6ea02ba..c69877543 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php index e3a262381..ae5116110 100644 --- a/lib/Service/OpenProjectAPIService.php +++ b/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; + } } diff --git a/tests/lib/Service/OpenProjectAPIServiceTest.php b/tests/lib/Service/OpenProjectAPIServiceTest.php index 118c0275b..867649aaa 100644 --- a/tests/lib/Service/OpenProjectAPIServiceTest.php +++ b/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 $mockMethods * @param array $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 + */ + 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); + } }