diff --git a/.gitignore b/.gitignore index 014e07349..108b313d8 100644 --- a/.gitignore +++ b/.gitignore @@ -23,3 +23,6 @@ node_modules .idea/ .pnpm-store/ + +# Superpowers planning artifacts +docs/superpowers/ diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 6c37c964e..83a105e26 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -23,16 +23,9 @@ class Application extends App { public function __construct(array $urlParams = []) { parent::__construct('richdocuments', $urlParams); - $this->registerServices(); - } - - private function registerServices() { $container = $this->getContainer(); $server = $container->getServer(); - /** - * Core - */ $container->registerService('L10N', function (SimpleContainer $c) use ($server) { return $server->getL10N($c->query('AppName')); }); diff --git a/lib/FederationService.php b/lib/FederationService.php index 3f042f0bf..82880da0f 100644 --- a/lib/FederationService.php +++ b/lib/FederationService.php @@ -21,34 +21,34 @@ */ namespace OCA\Richdocuments; +use OCP\IConfig; 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 IConfig */ + private $config; + public function __construct( ILogger $logger, IURLGenerator $urlGenerator, - IClientService $httpClient + IClientService $httpClient, + IConfig $config ) { - $this->logger = $logger; + $this->logger = $logger; $this->urlGenerator = $urlGenerator; - $this->httpClient = $httpClient; + $this->httpClient = $httpClient; + $this->config = $config; } /** @@ -69,13 +69,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 +113,33 @@ public function getWopiForToken($server, $accessToken) { } /** - * Check if server is allowed + * Check if the given server URL matches an entry in the richdocuments.federation_allowlist + * system config. Allowlist entries are plain domain names (no scheme). + * + * Returns false when the key is absent, empty, or not an array. + * + * Known limitation: path-prefixed installs (e.g. cloud.example.com/owncloud) are not + * supported — the path component will prevent matching a bare domain entry. * * @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 { + $allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', []); + + if (!\is_array($allowlist) || empty($allowlist)) { + return false; + } + + $domain = \rtrim((string)\preg_replace('|^https?://|', '', $remote), '/'); + + foreach ($allowlist as $entry) { + if ($domain === \rtrim((string)$entry, '/')) { + return true; + } + } - return true; + return false; } /** diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php index 7efb59459..cb7a4ba98 100644 --- a/tests/unit/FederationServiceTest.php +++ b/tests/unit/FederationServiceTest.php @@ -23,63 +23,51 @@ use OCA\Richdocuments\FederationService; use OCP\Http\Client\IClientService; +use OCP\IConfig; use OCP\ILogger; use OCP\IURLGenerator; use PHPUnit\Framework\MockObject\MockObject; 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 IConfig|MockObject */ + private $config; + + /** @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->logger = $this->createMock(ILogger::class); + $this->httpClient = $this->createMock(IClientService::class); + $this->config = $this->createMock(IConfig::class); $this->federationService = new FederationService( $this->logger, $this->urlGenerator, - $this->httpClient + $this->httpClient, + $this->config ); } + // ------------------------------------------------------------------------- + // 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,72 @@ public function testSplitUserRemote($userId, $remote) { $this->assertSame("{$userId}@{$remote}", $federatedCloudID); } + + // ------------------------------------------------------------------------- + // isServerAllowed() tests + // ------------------------------------------------------------------------- + + public function testIsServerAllowedReturnsFalseWhenKeyIsAbsent(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn([]); + + $this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com')); + } + + public function testIsServerAllowedReturnsFalseForNonArrayConfig(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn('not-an-array'); + + $this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com')); + } + + public function testIsServerAllowedReturnsTrueForExactMatch(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com')); + } + + public function testIsServerAllowedStripsTrailingSlash(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/')); + } + + public function testIsServerAllowedStripsMultipleTrailingSlashes(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///')); + } + + public function testIsServerAllowedMatchesHttps(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com')); + } + + public function testIsServerAllowedMatchesHttp(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com')); + } + + public function testIsServerAllowedReturnsFalseForUntrustedServer(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com')); + } }