diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 6c37c964e..738468d02 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -14,6 +14,7 @@ namespace OCA\Richdocuments\AppInfo; use OCA\Richdocuments\AppConfig; +use OCA\Richdocuments\FederationService; use OCP\AppFramework\App; use OC\AppFramework\Utility\SimpleContainer; use OCP\Share; @@ -36,6 +37,24 @@ private function registerServices() { $container->registerService('L10N', function (SimpleContainer $c) use ($server) { return $server->getL10N($c->query('AppName')); }); + + /** + * FederationService — inject TrustedServers only when the federation app is installed. + * When null, FederationService::isServerAllowed() denies all remote servers (secure default). + */ + $container->registerService(FederationService::class, function () use ($server) { + $trustedServers = null; + if ($server->getAppManager()->isInstalled('federation')) { + /* @phan-suppress-next-line PhanUndeclaredClassReference */ + $trustedServers = $server->query(\OCA\Federation\TrustedServers::class); + } + return new FederationService( + $server->getLogger(), + $server->getURLGenerator(), + $server->getHTTPClientService(), + $trustedServers + ); + }); } public function registerScripts() { diff --git a/lib/FederationService.php b/lib/FederationService.php index 3f042f0bf..b2c94bb3c 100644 --- a/lib/FederationService.php +++ b/lib/FederationService.php @@ -21,34 +21,36 @@ */ namespace OCA\Richdocuments; +use OCA\Federation\TrustedServers; use OCP\ILogger; use OCP\Http\Client\IClientService; use OCP\IURLGenerator; class FederationService { - /** - * @var ILogger - */ + /** @var ILogger */ private $logger; - /** - * @var IURLGenerator - */ + /** @var IURLGenerator */ private $urlGenerator; - /** - * @var IClientService - */ + /** @var IClientService */ private $httpClient; + /** @var TrustedServers|null */ + /* @phan-suppress-next-line PhanUndeclaredTypeProperty */ + private $trustedServers; + public function __construct( ILogger $logger, IURLGenerator $urlGenerator, - IClientService $httpClient + IClientService $httpClient, + /* @phan-suppress-next-line PhanUndeclaredTypeParameter */ + ?TrustedServers $trustedServers ) { - $this->logger = $logger; - $this->urlGenerator = $urlGenerator; - $this->httpClient = $httpClient; + $this->logger = $logger; + $this->urlGenerator = $urlGenerator; + $this->httpClient = $httpClient; + $this->trustedServers = $trustedServers; } /** @@ -69,13 +71,12 @@ public function getRemoteFileUrl(string $shareToken, string $shareRelativePath, '&accessToken=' . $accessToken; return $remoteFileUrl; } - + /** - * - * @param string $server addres of a remote server - * @param string $accessToken wopi access token from a remote server - * @return array|null with additional wopi information - */ + * @param string $server address of a remote server + * @param string $accessToken wopi access token from a remote server + * @return array|null with additional wopi information + */ public function getWopiForToken($server, $accessToken) { $remote = $server; @@ -114,15 +115,39 @@ public function getWopiForToken($server, $accessToken) { } /** - * Check if server is allowed + * Check if the given server URL is in ownCloud's trusted-servers list. + * + * Returns false when the federation app is not installed ($trustedServers is null) + * or when the server is not in the trusted list. Checks both the URL as-is (after + * stripping trailing slashes) and the http/https scheme variant to tolerate minor + * mismatches between how the admin stored the URL and how the request arrives. * * @param string $remote a remote url - * @return bool indicating if given remote is allowed server + * @return bool */ - public function isServerAllowed($remote) { - // TODO: implement check for trusted server, for a moment all trusted + public function isServerAllowed(string $remote): bool { + if ($this->trustedServers === null) { + return false; + } + + $normalized = \rtrim($remote, '/'); + + /* @phan-suppress-next-line PhanUndeclaredClassMethod */ + if ($this->trustedServers->isTrustedServer($normalized)) { + return true; + } + + // swap scheme and try again + if (\strpos($normalized, 'https://') === 0) { + $swapped = 'http://' . \substr($normalized, 8); + } elseif (\strpos($normalized, 'http://') === 0) { + $swapped = 'https://' . \substr($normalized, 7); + } else { + return false; + } - return true; + /* @phan-suppress-next-line PhanUndeclaredClassMethod */ + return $this->trustedServers->isTrustedServer($swapped); } /** diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php index 7efb59459..b0e536d3f 100644 --- a/tests/unit/FederationServiceTest.php +++ b/tests/unit/FederationServiceTest.php @@ -21,6 +21,7 @@ */ namespace OCA\Richdocuments\Tests; +use OCA\Federation\TrustedServers; use OCA\Richdocuments\FederationService; use OCP\Http\Client\IClientService; use OCP\ILogger; @@ -29,57 +30,44 @@ use Test\TestCase; class FederationServiceTest extends TestCase { - /** - * The ILogger instance. - * - * @var ILogger - */ + /** @var ILogger|MockObject */ private $logger; - /** - * The IClientService instance. - * - * @var IClientService - */ + /** @var IClientService|MockObject */ private $httpClient; - /** - * The IURLGenerator instance. - * - * @var IURLGenerator - */ + /** @var IURLGenerator|MockObject */ private $urlGenerator; - /** - * @var FederationService|MockObject The discovery service mock object. - */ + /** @var TrustedServers|MockObject */ + private $trustedServers; + + /** @var FederationService */ private $federationService; protected function setUp(): void { parent::setUp(); - $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->logger = $this->createMock(ILogger::class); - $this->httpClient = $this->createMock(IClientService::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->logger = $this->createMock(ILogger::class); + $this->httpClient = $this->createMock(IClientService::class); + $this->trustedServers = $this->createMock(TrustedServers::class); $this->federationService = new FederationService( $this->logger, $this->urlGenerator, - $this->httpClient + $this->httpClient, + $this->trustedServers ); } + // ------------------------------------------------------------------------- + // Existing tests + // ------------------------------------------------------------------------- + public function dataGenerateFederatedCloudID() { - $userPrefix = [ - 'username', - '1234' - ]; - $remotes = [ - 'localhost', - 'local.host', - 'dev.local.host', - '127.0.0.1', - ]; + $userPrefix = ['username', '1234']; + $remotes = ['localhost', 'local.host', 'dev.local.host', '127.0.0.1']; $testCases = []; foreach ($userPrefix as $user) { @@ -92,9 +80,6 @@ public function dataGenerateFederatedCloudID() { /** * @dataProvider dataGenerateFederatedCloudID - * - * @param string $userId - * @param string $expectedFederatedCloudID */ public function testSplitUserRemote($userId, $remote) { $this->urlGenerator->method('getAbsoluteUrl') @@ -105,4 +90,64 @@ public function testSplitUserRemote($userId, $remote) { $this->assertSame("{$userId}@{$remote}", $federatedCloudID); } + + // ------------------------------------------------------------------------- + // isServerAllowed() tests + // ------------------------------------------------------------------------- + + public function testIsServerAllowedReturnsFalseWhenTrustedServersIsNull(): void { + $service = new FederationService( + $this->logger, + $this->urlGenerator, + $this->httpClient, + null + ); + + $this->assertFalse($service->isServerAllowed('https://remote.example.com')); + } + + public function testIsServerAllowedReturnsTrueForExactMatch(): void { + $this->trustedServers->method('isTrustedServer') + ->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com'); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com')); + } + + public function testIsServerAllowedStripsTrailingSlash(): void { + $this->trustedServers->method('isTrustedServer') + ->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com'); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/')); + } + + public function testIsServerAllowedStripsMultipleTrailingSlashes(): void { + $this->trustedServers->method('isTrustedServer') + ->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com'); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///')); + } + + public function testIsServerAllowedSwapsHttpToHttps(): void { + // Admin stored https://, request arrives as http:// + $this->trustedServers->method('isTrustedServer') + ->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com'); + + $this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com')); + } + + public function testIsServerAllowedSwapsHttpsToHttp(): void { + // Admin stored http://, request arrives as https:// + $this->trustedServers->method('isTrustedServer') + ->willReturnCallback(fn ($url) => $url === 'http://trusted.example.com'); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com')); + } + + public function testIsServerAllowedReturnsFalseForUntrustedServer(): void { + $this->trustedServers->method('isTrustedServer') + ->willReturn(false); + + $this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com')); + } + }