From d99bedddce156bdab82e06ecf602df40ac27c360 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Sat, 7 Jun 2025 16:35:12 +0200 Subject: [PATCH 01/12] Add unit tests for BaseServerConfig::getClientRegistration(). --- solid/tests/Unit/BaseServerConfigTest.php | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/solid/tests/Unit/BaseServerConfigTest.php b/solid/tests/Unit/BaseServerConfigTest.php index aeb70bad..c9687226 100644 --- a/solid/tests/Unit/BaseServerConfigTest.php +++ b/solid/tests/Unit/BaseServerConfigTest.php @@ -16,6 +16,8 @@ class BaseServerConfigTest extends TestCase { /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ + private const MOCK_CLIENT_ID = 'mock-client-id'; + /** * @testdox BaseServerConfig should complain when called before given a Configuration * @covers ::__construct @@ -93,6 +95,48 @@ public function testSetUserSubDomainsEnabled($expected) $baseServerConfig->setUserSubDomainsEnabled($expected); } + /** + * @testdox BaseServerConfig should retrieve client ID AppValue when asked to GetClientRegistration for existing client + * @covers ::getClientRegistration + */ + public function testGetClientRegistrationForExistingClient() + { + $configMock = $this->createMock(IConfig::class); + $baseServerConfig = new BaseServerConfig($configMock); + + $expected = ['mock' => 'client']; + + $configMock->expects($this->once()) + ->method('getAppValue') + ->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID) + ->willReturn(json_encode($expected)); + + $actual = $baseServerConfig->getClientRegistration(self::MOCK_CLIENT_ID); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox BaseServerConfig should return empty array when asked to GetClientRegistration for non-existing client + * @covers ::getClientRegistration + */ + public function testGetClientRegistrationForNonExistingClient() + { + $configMock = $this->createMock(IConfig::class); + $baseServerConfig = new BaseServerConfig($configMock); + + $expected = []; + + $configMock->expects($this->once()) + ->method('getAppValue') + ->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID) + ->willReturnArgument(2); + + $actual = $baseServerConfig->getClientRegistration(self::MOCK_CLIENT_ID); + + $this->assertEquals($expected, $actual); + } + /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public function provideBooleans() From 0ff3cf66f3e44c33ef5872db08155080f0715229 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 09:50:55 +0200 Subject: [PATCH 02/12] Add better test coverage for BaseServerConfig::castToBool(). --- solid/lib/BaseServerConfig.php | 2 +- solid/tests/Unit/BaseServerConfigTest.php | 38 +++++++++++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/solid/lib/BaseServerConfig.php b/solid/lib/BaseServerConfig.php index 7c391893..ab967dea 100644 --- a/solid/lib/BaseServerConfig.php +++ b/solid/lib/BaseServerConfig.php @@ -202,7 +202,7 @@ public function setUserSubDomainsEnabled($enabled) { ////////////////////////////// UTILITY METHODS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\ - private function castToBool(string $mixedValue): bool + private function castToBool(?string $mixedValue): bool { $type = gettype($mixedValue); diff --git a/solid/tests/Unit/BaseServerConfigTest.php b/solid/tests/Unit/BaseServerConfigTest.php index c9687226..fb5e99f1 100644 --- a/solid/tests/Unit/BaseServerConfigTest.php +++ b/solid/tests/Unit/BaseServerConfigTest.php @@ -46,12 +46,13 @@ public function testConstructorWithValidConfig() /** * @testdox BaseServerConfig should return a boolean when asked whether UserSubDomains are Enabled * @covers ::getUserSubDomainsEnabled + * @covers ::castToBool * @dataProvider provideBooleans */ - public function testGetUserSubDomainsEnabled($expected) + public function testGetUserSubDomainsEnabled($value, $expected) { $configMock = $this->createMock(IConfig::class); - $configMock->method('getAppValue')->willReturn($expected); + $configMock->method('getAppValue')->willReturn($value); $baseServerConfig = new BaseServerConfig($configMock); $actual = $baseServerConfig->getUserSubDomainsEnabled(); @@ -80,10 +81,11 @@ public function testGetUserSubDomainsEnabledFromAppConfig() /** * @testdox BaseServerConfig should set value in AppConfig when asked to set UserSubDomainsEnabled * @covers ::setUserSubDomainsEnabled + * @covers ::castToBool * * @dataProvider provideBooleans */ - public function testSetUserSubDomainsEnabled($expected) + public function testSetUserSubDomainsEnabled($value, $expected) { $configMock = $this->createMock(IConfig::class); $configMock->expects($this->atLeast(1)) @@ -92,7 +94,7 @@ public function testSetUserSubDomainsEnabled($expected) ; $baseServerConfig = new BaseServerConfig($configMock); - $baseServerConfig->setUserSubDomainsEnabled($expected); + $baseServerConfig->setUserSubDomainsEnabled($value); } /** @@ -142,8 +144,32 @@ public function testGetClientRegistrationForNonExistingClient() public function provideBooleans() { return [ - 'false' => [false], - 'true' => [true], + // Only 'boolean', 'NULL', 'integer', 'string' are allowed + // @TODO: Add test for type that trigger a TypeError: + // - array + // - callable + // - float + // - object + // - resource + // @TODO: Add test for values that trigger a TypeError + // 'integer:-1' => ['value'=> -1], + // 'integer:2' => ['value'=> 2], + // 'string:-1' => ['value'=> '-1'], + // 'string:2' => ['value'=> '2'], + // 'string:foo' => ['value'=> 'foo'], + // 'string:NULL' => ['value'=> 'NULL'], + 'boolean:false' => ['value'=> false, 'expected' => false], + 'boolean:true' => ['value'=> true, 'expected' => true], + 'integer:0' => ['value'=> 0, 'expected' => false], + 'integer:1' => ['value'=> 1, 'expected' => true], + 'NULL' => ['value'=> null, 'expected' => false], + 'string:0' => ['value'=> '0', 'expected' => false], + 'string:1' => ['value'=> '1', 'expected' => true], + 'string:empty' => ['value'=> '', 'expected' => false], + 'string:false' => ['value'=> 'false', 'expected' => false], + 'string:FALSE' => ['value'=> 'FALSE', 'expected' => false], + 'string:true' => ['value'=> 'true', 'expected' => true], + 'string:TRUE' => ['value'=> 'TRUE', 'expected' => true], ]; } } From c5f6e6120c4f3df245de33d261ac614517fcef2c Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 09:52:12 +0200 Subject: [PATCH 03/12] Add unit test for BaseServerConfig::saveClientRegistration() and BaseServerConfig::removeClientRegistration() --- solid/tests/Unit/BaseServerConfigTest.php | 201 +++++++++++++++++++++- 1 file changed, 200 insertions(+), 1 deletion(-) diff --git a/solid/tests/Unit/BaseServerConfigTest.php b/solid/tests/Unit/BaseServerConfigTest.php index fb5e99f1..a4b3d271 100644 --- a/solid/tests/Unit/BaseServerConfigTest.php +++ b/solid/tests/Unit/BaseServerConfigTest.php @@ -1,6 +1,6 @@ assertEquals($expected, $actual); } + /** + * @testdox BaseServerConfig should complain when asked to save ClientRegistration without origin + * @covers ::saveClientRegistration + */ + public function testSaveClientRegistrationWithoutOrigin() + { + $this->expectException(TypeError::class); + $this->expectExceptionMessage('Too few arguments to function'); + + $configMock = $this->createMock(IConfig::class); + $baseServerConfig = new BaseServerConfig($configMock); + + $baseServerConfig->saveClientRegistration(); + } + + /** + * @testdox BaseServerConfig should complain when asked to save ClientRegistration without client data + * @covers ::saveClientRegistration + */ + public function testSaveClientRegistrationWithoutClientData() + { + $this->expectException(TypeError::class); + $this->expectExceptionMessage('Too few arguments to function'); + + $configMock = $this->createMock(IConfig::class); + $baseServerConfig = new BaseServerConfig($configMock); + + $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN); + } + + /** + * @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for new client + * @covers ::saveClientRegistration + */ + public function testSaveClientRegistrationForNewClient() + { + $configMock = $this->createMock(IConfig::class); + + $configMock->expects($this->once()) + ->method('getAppValue') + ->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN)) + ->willReturnArgument(2); + + $expected = [ + 'client_id' => md5(self::MOCK_ORIGIN), + 'client_name' => self::MOCK_ORIGIN, + 'client_secret' => md5(self::MOCK_RANDOM_BYTES), + ]; + + $configMock->expects($this->exactly(2)) + ->method('setAppValue') + ->willReturnMap([ + // Using willReturnMap as withConsecutive is removed since PHPUnit 10 + [Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)], + [Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)] + ]); + + $baseServerConfig = new BaseServerConfig($configMock); + + $actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for existing client + * @covers ::saveClientRegistration + */ + public function testSaveClientRegistrationForExistingClient() + { + $configMock = $this->createMock(IConfig::class); + + $expected = [ + 'client_id' => md5(self::MOCK_ORIGIN), + 'client_name' => self::MOCK_ORIGIN, + 'client_secret' => md5(self::MOCK_RANDOM_BYTES), + 'redirect_uris' => [self::MOCK_REDIRECT_URI], + ]; + + $configMock->expects($this->once()) + ->method('getAppValue') + ->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN)) + ->willReturn(json_encode($expected)); + + $configMock->expects($this->exactly(2)) + ->method('setAppValue') + ->willReturnMap([ + // Using willReturnMap as withConsecutive is deprecated since PHPUnit 10 + [Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)], + [Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)] + ]); + + $baseServerConfig = new BaseServerConfig($configMock); + + $actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for blocked client + * @covers ::saveClientRegistration + */ + public function testSaveClientRegistrationForBlockedClient() + { + $configMock = $this->createMock(IConfig::class); + + $expected = [ + 'client_id' => md5(self::MOCK_ORIGIN), + 'client_name' => self::MOCK_ORIGIN, + 'client_secret' => md5(self::MOCK_RANDOM_BYTES), + 'redirect_uris' => [self::MOCK_REDIRECT_URI], + 'blocked' => true, + ]; + + $configMock->expects($this->once()) + ->method('getAppValue') + ->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN)) + ->willReturn(json_encode($expected)); + + $configMock->expects($this->exactly(2)) + ->method('setAppValue') + ->willReturnMap([ + // Using willReturnMap as withConsecutive is deprecated since PHPUnit 10 + [Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)], + [Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)] + ]); + + $baseServerConfig = new BaseServerConfig($configMock); + + $actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $expected); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox BaseServerConfig should always "blocked" to existing value when asked to save ClientRegistration for blocked client + * @covers ::saveClientRegistration + */ + public function testSaveClientRegistrationSetsBlocked() + { + $configMock = $this->createMock(IConfig::class); + + $expected = [ + 'client_id' => md5(self::MOCK_ORIGIN), + 'client_name' => self::MOCK_ORIGIN, + 'client_secret' => md5(self::MOCK_RANDOM_BYTES), + 'redirect_uris' => [self::MOCK_REDIRECT_URI], + 'blocked' => true, + ]; + + $configMock->expects($this->once()) + ->method('getAppValue') + ->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN)) + ->willReturn(json_encode($expected)); + + $clientData = $expected; + $clientData['blocked'] = false; + + $configMock->expects($this->exactly(2)) + ->method('setAppValue') + ->willReturnMap([ + // Using willReturnMap as withConsecutive is deprecated since PHPUnit 10 + [Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN), json_encode($expected)], + [Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode($expected)] + ]); + + $baseServerConfig = new BaseServerConfig($configMock); + + $actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, $clientData); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox BaseServerConfig should remove ClientRegistration when asked to remove ClientRegistration + * @covers ::removeClientRegistration + */ + public function testRemoveClientRegistration() + { + $configMock = $this->createMock(IConfig::class); + $baseServerConfig = new BaseServerConfig($configMock); + + $configMock->expects($this->once()) + ->method('deleteAppValue') + ->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID); + + $baseServerConfig->removeClientRegistration(self::MOCK_CLIENT_ID); + } + /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public function provideBooleans() From f728f3fe36e752dc02b7a3a5e1a056e06b459c71 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 11:18:02 +0200 Subject: [PATCH 04/12] Add unit tests for ServerController constructor. --- .../Unit/Controller/ServerControllerTest.php | 142 ++++++++++++++++++ solid/tests/fixtures/keys/private.key | 27 ++++ 2 files changed, 169 insertions(+) create mode 100644 solid/tests/Unit/Controller/ServerControllerTest.php create mode 100644 solid/tests/fixtures/keys/private.key diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php new file mode 100644 index 00000000..92fa27e7 --- /dev/null +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -0,0 +1,142 @@ +createMock(IRequest::class), + $this->createMock(ISession::class), + $this->createMock(IUserManager::class), + $this->createMock(IURLGenerator::class), + 'mock user id', + $this->createMock(IConfig::class), + $this->createMock(UserService::class), + $this->createMock(IDBConnection::class), + ]; + + $parameters = array_slice($parameters, 0, $index); + + $this->expectException(\ArgumentCountError::class); + $message = vsprintf( + 'Too few arguments to function %s::%s, %s passed in %s on line %s and exactly %s expected', + [ + 'class' => preg_quote('OCA\Solid\Controller\ServerController', '/'), + 'method' => preg_quote('__construct()', '/'), + 'index' => $index, + 'file' => '.*', + 'line' => '\d+', + 'count' => 9, + + ] + ); + + $this->expectExceptionMessageMatches('/^' . $message . '$/'); + + new ServerController(...$parameters); + } + + /** + * @testdox ServerController should be instantiable with all required parameters + * + * @covers ::__construct + * + * @uses \League\OAuth2\Server\AuthorizationServer + * @uses \OCA\Solid\BaseServerConfig + * @uses \OCA\Solid\JtiReplayDetector + * @uses \OCA\Solid\ServerConfig + * @uses \Pdsinterop\Solid\Auth\Factory\AuthorizationServerFactory + * @uses \Pdsinterop\Solid\Auth\Factory\GrantTypeFactory + * @uses \Pdsinterop\Solid\Auth\Factory\RepositoryFactory + * @uses \Pdsinterop\Solid\Auth\TokenGenerator + */ + public function testInstatiation() + { + $configMock = $this->createMock(IConfig::class); + + $configMock->method('getAppValue')->willReturnMap([ + [Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID, '{}', 'return' => '{}'], + [Application::APP_ID, 'client-d6d7896757f61ac4c397d914053180ff', '{}', 'return' => '{}'], + [Application::APP_ID, 'client-', '{}', 'return' => '{}'], + [Application::APP_ID, 'profileData', '', 'return' => ''], + [Application::APP_ID, 'encryptionKey', '', 'return' => 'mock encryption key'], + [Application::APP_ID, 'privateKey', '', 'return' => self::$privateKey], + ]); + + $parameters = [ + 'mock appname', + $this->createMock(IRequest::class), + $this->createMock(ISession::class), + $this->createMock(IUserManager::class), + $this->createMock(IURLGenerator::class), + 'mock user id', + $configMock, + $this->createMock(UserService::class), + $this->createMock(IDBConnection::class), + ]; + + $controller = new ServerController(...$parameters); + + $this->assertInstanceOf(ServerController::class, $controller); + } + + /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ + + public static function provideConstructorParameterIndex() + { + return [ + 'appName' => [0], + 'request' => [1], + 'session' => [2], + 'userManager' => [3], + 'urlGenerator' => [4], + 'userId' => [5], + 'config' => [6], + 'userService' => [7], + 'connection' => [8], + ]; + } +} diff --git a/solid/tests/fixtures/keys/private.key b/solid/tests/fixtures/keys/private.key new file mode 100644 index 00000000..94793713 --- /dev/null +++ b/solid/tests/fixtures/keys/private.key @@ -0,0 +1,27 @@ +-----BEGIN RSA PRIVATE KEY----- +MIIEpAIBAAKCAQEAvqb0htUFZaZ+z5rn7cHWg0VzsSoVnusbtJvwWtHfD0T0s6Hb +OqzE5h2fgdGbB49HRtc21SNHx6jeEStGv03yyqYkLUKrJJSg+ksrL+pT3Nd0h25q +sx7YUoPPxnm6sbd3XTg5efCb2yyV2dOoAegUPjK46Ra6PqUvmICQWDsjnv0VJIx+ +TdDWmKY2xElk0T6CVNMD08OZVTHPwJgpGdRZyCK/SSmrvmAZ6K3ocKySJdKgYriR +bVMdx9NsczRkYU9b7tUpPmLu3IvsLboTbfRN23Y70Gx3Z8fuI1FRn23sEuQSIRW+ +NsAi7l+AEdJ7MdYn0xSY6YMNJ0/aGXi55gagQwIDAQABAoIBAQCz8CNNtnPXkqKR +EmTfk1kAoGYmyc+KI+AMQDlDnlzmrnA9sf+Vi0Zy4XaQMeId6m6dP7Yyx4+Rs6GT +lsK4/7qs5M20If4hEl40nQlvubvY7UjAIch2sh/9EQbjDjTUUpJH2y70FdEjtRrh +cdBZrE6evYSkCZ1STtlzF7QkcfyWqilTHEntrHRaM3N+B6F74Yi5g6VyGE9uqKEM +EuGDHVSXizdUjauTTVEa4o7pxTh+eTIdQsfRewer7iuxFPo2vBNOTU2O/obNUsVK +mgmGM4QDjurgXLL2XPr0dVVo3eiFvIdmtZgGVyLfL/vUXH7bwUIfkV6qWyRmdBiY +Dfsm8BJBAoGBAOGebDUVnP3NgFacWVYrtvBXcH2Q6X1W6JEAxctDDsnjchTdyG9E +zcsMVM/gFKXIDF5VeNoSt2pwCTBL6K0oPC31c01clActbHStaJWOOCuifzrvmu4n +X51TNGoKggbbSVx1UTifKte2t6SPRaZ26EqVrmO44fGkA3ip6TRYnSFzAoGBANhT +J47EieRWiNflq9XqDAZ1fZzo3AHB+b+pO4r8GZr3Dw0ShCAnQXv7Gb2JAJvE3UrC +Aq5r3yZMM7nI+n/OT06+UcJ3/vDGAPx9trNrpWkwmcWBmoBfp86vDRhT0kEIiKbO +wLYMmSNLHNkmQQdBX2ytnsRxRyCWtQmm09bzOJHxAoGBAKEB/nSPnP5elfS5FOPy +xFWWANgK/yWMTOGV7JFWpIocvz/22d/V+QqrHSdP4UxBi9oSIvF1I+FYXKZTtZNE +wFWH8SXHKHhKyTgmvBjmal1xVFyJu0WzYX+TbjcykoI0IZFSw4ilxdw1L67G88yM +1M7NLKtLuCpKgpOspZjOmCvTAoGAGji6KswYCt2SaNkmIx/jpUTInSR8xpnEtD7H +QOmeEPKxmFwON/eKMIUXcaoRsNAEIvOxb4MT4YiLHJIIC0XuxxS6xF/XP0hBBloW +s1jxC/cgLJixKa5uoNcHN1OxwMBQECgvo+GTDnwkWw4QA9kgwAOroxQ4EvMxrqHS +O9Pvn4ECgYA7xr/3Sz8n+BhgOdABW0m91P144rK9QDYiaClSxAha1KiFunmAy3pB +Uxdl4yTCTA9yKIH7X3bShDXnj+RmEZ+SkwzpPuKvAE8ZkZQuXv41anFrZYkR2PZy +oYiERqXgH5yS/mkDeXRFx1nWsVxjoLWfd/Vi7Lr43cuYFy4UjqXZdg== +-----END RSA PRIVATE KEY----- From 3500fcff79433e4f3b1cf8d5ebb96f157ec24e49 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 13:21:48 +0200 Subject: [PATCH 05/12] Add unit tests for ServerController::auth(). --- .../Unit/Controller/ServerControllerTest.php | 217 +++++++++++++++--- 1 file changed, 183 insertions(+), 34 deletions(-) diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index 92fa27e7..4be9a1d3 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -3,15 +3,16 @@ namespace OCA\Solid\Controller\ServerController; use OCA\Solid\AppInfo\Application; -use OCA\Solid\BaseServerConfig; use OCA\Solid\Controller\ServerController; use OCA\Solid\Service\UserService; +use OCP\AppFramework\Http\JSONResponse; use OCP\IConfig; use OCP\IDBConnection; use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; /** @@ -19,15 +20,21 @@ * @covers ::__construct * * @uses \OCA\Solid\Controller\ServerController + * @uses \OCA\Solid\BaseServerConfig + * @uses \OCA\Solid\JtiReplayDetector + * @uses \OCA\Solid\ServerConfig */ class ServerControllerTest extends TestCase { ////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ private const MOCK_CLIENT_ID = 'mock-client-id'; - private static string $encryptionKey; + private const MOCK_USER_ID = 'mock user id'; + private static string $privateKey; - private static string $publicKey; + + private IConfig|MockObject $mockConfig; + private IUserManager|MockObject $mockUserManager; public static function setUpBeforeClass(): void @@ -35,6 +42,7 @@ public static function setUpBeforeClass(): void $keyPath = __DIR__ . '/../../fixtures/keys'; self::$privateKey = file_get_contents($keyPath . '/private.key'); } + /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ /** @@ -44,19 +52,9 @@ public static function setUpBeforeClass(): void * * @dataProvider provideConstructorParameterIndex */ - public function testInstatiationWithoutRequiredParameter($index) + public function testInstantiationWithoutRequiredParameter($index) { - $parameters = [ - 'mock appname', - $this->createMock(IRequest::class), - $this->createMock(ISession::class), - $this->createMock(IUserManager::class), - $this->createMock(IURLGenerator::class), - 'mock user id', - $this->createMock(IConfig::class), - $this->createMock(UserService::class), - $this->createMock(IDBConnection::class), - ]; + $parameters = $this->createMockConstructorParameters(); $parameters = array_slice($parameters, 0, $index); @@ -83,44 +81,195 @@ public function testInstatiationWithoutRequiredParameter($index) * @testdox ServerController should be instantiable with all required parameters * * @covers ::__construct + */ + public function testInstantiation() + { + $parameters = $this->createMockConstructorParameters(); + + $controller = new ServerController(...$parameters); + + $this->assertInstanceOf(ServerController::class, $controller); + } + + /** + * @testdox ServerController should return a 401 when asked to authorize without signed-in user + * + * @covers ::authorize + */ + public function testAuthorizeWithoutUser() + { + $parameters = $this->createMockConstructorParameters(); + + $controller = new ServerController(...$parameters); + + $expected = new JSONResponse('Authorization required', 401); + $actual = $controller->authorize(); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox ServerController should return a 400 when asked to authorize with a user but without valid token + * + * @covers ::authorize + */ + public function testAuthorizeWithoutValidToken() + { + $_GET['response_type'] = 'mock-response-type'; + + $parameters = $this->createMockConstructorParameters(); + + $this->mockUserManager->method('userExists')->willReturn(true); + + $controller = new ServerController(...$parameters); + + $actual = $controller->authorize(); + $expected = new JSONResponse('Bad request, does not contain valid token', 400); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox ServerController should return a 302 redirect when asked to authorize client that has not been approved + * + * @covers ::authorize + */ + public function testAuthorizeWithoutApprovedClient() + { + $_GET['client_id'] = self::MOCK_CLIENT_ID; + $_GET['nonce'] = 'mock-nonce'; + // JWT with empty payload, HS256 encoded, created with `private.key` from fixtures + $_GET['request'] = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.8VKCTiBegJPuPIZlp0wbV0Sbdn5BS6TE5DCx6oYNc5o'; + $_GET['response_type'] = 'mock-response-type'; + + $_SERVER['REQUEST_URI'] = 'mock uri'; + + $parameters = $this->createMockConstructorParameters(); + + $this->mockConfig->method('getUserValue')->willReturnArgument(3); + + $this->mockUserManager->method('userExists')->willReturn(true); + + $controller = new ServerController(...$parameters); + + $actual = $controller->authorize(); + $expected = new JSONResponse('Approval required', 302, ['Location' => '']); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox ServerController should return a 302 redirect when asked to authorize client that has been approved * - * @uses \League\OAuth2\Server\AuthorizationServer - * @uses \OCA\Solid\BaseServerConfig - * @uses \OCA\Solid\JtiReplayDetector - * @uses \OCA\Solid\ServerConfig - * @uses \Pdsinterop\Solid\Auth\Factory\AuthorizationServerFactory - * @uses \Pdsinterop\Solid\Auth\Factory\GrantTypeFactory - * @uses \Pdsinterop\Solid\Auth\Factory\RepositoryFactory - * @uses \Pdsinterop\Solid\Auth\TokenGenerator + * @covers ::authorize */ - public function testInstatiation() + public function testAuthorizeWithApprovedClient() { - $configMock = $this->createMock(IConfig::class); + $_GET['client_id'] = self::MOCK_CLIENT_ID; + $_GET['nonce'] = 'mock-nonce'; + // JWT with empty payload, HS256 encoded, created with `private.key` from fixtures + $_GET['request'] = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.8VKCTiBegJPuPIZlp0wbV0Sbdn5BS6TE5DCx6oYNc5o'; + $_GET['response_type'] = 'mock-response-type'; + + $_SERVER['REQUEST_URI'] = 'https://mock.server'; + + $clientData = json_encode(['client_name' => 'Mock Client', 'redirect_uris' => ['https://mock.client/redirect']]); + + $parameters = $this->createMockConstructorParameters($clientData); + + $this->mockConfig->method('getUserValue') + ->with(self::MOCK_USER_ID, Application::APP_ID, 'allowedClients', '[]') + ->willReturn(json_encode([self::MOCK_CLIENT_ID])); + + $this->mockUserManager->method('userExists')->willReturn(true); + + $controller = new ServerController(...$parameters); + + $response = $controller->authorize(); + + $expected = [ + 'data' => 'ok', + 'headers' => [ + 'Cache-Control' => 'no-cache, no-store, must-revalidate', + 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", + 'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'", + 'X-Robots-Tag' => 'noindex, nofollow', + 'Content-Type' => 'application/json; charset=utf-8', - $configMock->method('getAppValue')->willReturnMap([ - [Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID, '{}', 'return' => '{}'], - [Application::APP_ID, 'client-d6d7896757f61ac4c397d914053180ff', '{}', 'return' => '{}'], - [Application::APP_ID, 'client-', '{}', 'return' => '{}'], + ], + 'status' => 302, + ]; + + $headers = $response->getHeaders(); + $location = $headers['Location']; + // Not comparing time-sensitive data + unset($headers['X-Request-Id'], $headers['Location']); + + $actual = [ + 'data' => $response->getData(), + 'headers' => $headers, + 'status' => $response->getStatus(), + ]; + + $this->assertEquals($expected, $actual); + + // @TODO: Move $location assert to a separate test + $url = parse_url($location); + + parse_str($url['fragment'], $url['fragment']); + + unset($url['fragment']['access_token'], $url['fragment']['id_token']); + + $this->assertEquals([ + 'scheme' => 'https', + 'host' => 'mock.client', + 'path' => '/redirect', + 'fragment' => [ + 'token_type'=>'Bearer', + 'expires_in'=>'3600', + ], + ], $url); + } + + ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\ + + public function createMockConfig($clientData): IConfig|MockObject + { + $this->mockConfig = $this->createMock(IConfig::class); + + $this->mockConfig->method('getAppValue')->willReturnMap([ + [Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID, '{}', 'return' => $clientData], + [Application::APP_ID, 'client-d6d7896757f61ac4c397d914053180ff', '{}', 'return' => $clientData], + [Application::APP_ID, 'client-', '{}', 'return' => $clientData], [Application::APP_ID, 'profileData', '', 'return' => ''], [Application::APP_ID, 'encryptionKey', '', 'return' => 'mock encryption key'], [Application::APP_ID, 'privateKey', '', 'return' => self::$privateKey], ]); + return $this->mockConfig; + } + public function createMockConstructorParameters($clientData = '{}'): array + { $parameters = [ 'mock appname', $this->createMock(IRequest::class), $this->createMock(ISession::class), - $this->createMock(IUserManager::class), + $this->createMockUserManager(), $this->createMock(IURLGenerator::class), - 'mock user id', - $configMock, + self::MOCK_USER_ID, + $this->createMockConfig($clientData), $this->createMock(UserService::class), $this->createMock(IDBConnection::class), ]; - $controller = new ServerController(...$parameters); + return $parameters; + } - $this->assertInstanceOf(ServerController::class, $controller); + public function createMockUserManager(): IUserManager|MockObject + { + $this->mockUserManager = $this->createMock(IUserManager::class); + + return $this->mockUserManager; } /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ From 3ea77d65ca66fc05cca4b42751b034ee1d227e05 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 14:25:16 +0200 Subject: [PATCH 06/12] Add unit tests for ServerController::register(). --- .../Unit/Controller/ServerControllerTest.php | 98 ++++++++++++++++++- 1 file changed, 94 insertions(+), 4 deletions(-) diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index 4be9a1d3..759aba19 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -1,9 +1,9 @@ createMockConstructorParameters(); + + $controller = new ServerController(...$parameters); + + $actual = $controller->register(); + + $this->assertEquals( + new JSONResponse('Missing redirect URIs', Http::STATUS_BAD_REQUEST), + $actual + ); + } + + /** + * @testdox ServerController should return a 200 with client data when asked to register with valid redirect URIs + * + * @covers ::register + */ + public function testRegisterWithRedirectUris() + { + $parameters = $this->createMockConstructorParameters(); + + $this->mockURLGenerator->method('getBaseUrl') + ->willReturn('https://mock.server'); + + $controller = new ServerController(...$parameters); + + self::$clientData = json_encode(['redirect_uris' => ['https://mock.client/redirect']]); + + $response = $controller->register(); + + $actual = [ + 'data' => $response->getData(), + 'headers' => $response->getHeaders(), + 'status' => $response->getStatus(), + ]; + + // Not comparing time-sensitive data + unset($actual['data']['client_id_issued_at'], $actual['headers']['X-Request-Id']); + + $this->assertEquals([ + 'data' => [ + 'application_type' => 'web', + 'client_id' => 'f4a2d00f7602948a97ff409d7a581ec2', + 'grant_types' => ['implicit'], + 'id_token_signed_response_alg' => 'RS256', + 'redirect_uris' => ['https://mock.client/redirect'], + 'registration_access_token' => 'eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9.eyJpc3MiOiJodHRwczovL21vY2suc2VydmVyIiwiYXVkIjoiZjRhMmQwMGY3NjAyOTQ4YTk3ZmY0MDlkN2E1ODFlYzIiLCJzdWIiOiJmNGEyZDAwZjc2MDI5NDhhOTdmZjQwOWQ3YTU4MWVjMiJ9.AfOi9YW70rL0EKn4_dvhkyu02iI4yGYV-Xh8hQ9RbHBUnvcXROFfQzn-OL-R3kV3nn8tknmpG-r_8Ouoo7O_Sjo8Hx1QSFfeqjJGOgB8HbXV7WN2spOMicSB-68EyftqfTGH0ksyPyJaNSTbkdIqtawsDaSKUVqTmziEo4IrE5anwDLZrtSUcS0A4KVrOAkJmgYGiC4MC0NMYXeBRxgkr1_h7GN4hekAXs9-5XwRH1mwswUVRL-6prx0IYpPNURFNqkS2NU83xNf-vONThOdLVkADVy-l3PCHT3E1sRdkklCHLjhWiZo7NcMlB0WdS-APnZYCi5hLEr5-jwNI2sxoA', + 'registration_client_uri' => '', + 'response_types' => ['id_token token'], + 'token_endpoint_auth_method' => 'client_secret_basic', + ], + 'headers' => [ + 'Cache-Control' => 'no-cache, no-store, must-revalidate', + 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", + 'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'", + 'X-Robots-Tag' => 'noindex, nofollow', + 'Content-Type' => 'application/json; charset=utf-8', + ], + 'status' => Http::STATUS_OK, + ], $actual); + } + ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public function createMockConfig($clientData): IConfig|MockObject @@ -244,10 +324,13 @@ public function createMockConfig($clientData): IConfig|MockObject [Application::APP_ID, 'profileData', '', 'return' => ''], [Application::APP_ID, 'encryptionKey', '', 'return' => 'mock encryption key'], [Application::APP_ID, 'privateKey', '', 'return' => self::$privateKey], + // Client ID from register() with https://mock.client + [Application::APP_ID, 'client-f4a2d00f7602948a97ff409d7a581ec2', '{}', 'return' => $clientData], ]); return $this->mockConfig; } + public function createMockConstructorParameters($clientData = '{}'): array { $parameters = [ @@ -255,7 +338,7 @@ public function createMockConstructorParameters($clientData = '{}'): array $this->createMock(IRequest::class), $this->createMock(ISession::class), $this->createMockUserManager(), - $this->createMock(IURLGenerator::class), + $this->createMockUrlGenerator(), self::MOCK_USER_ID, $this->createMockConfig($clientData), $this->createMock(UserService::class), @@ -265,6 +348,13 @@ public function createMockConstructorParameters($clientData = '{}'): array return $parameters; } + public function createMockUrlGenerator(): IURLGenerator|MockObject + { + $this->mockURLGenerator = $this->createMock(IURLGenerator::class); + + return $this->mockURLGenerator; + } + public function createMockUserManager(): IUserManager|MockObject { $this->mockUserManager = $this->createMock(IUserManager::class); From 59d8f236b73fc7eb0155c35fb2e4f368330e01fc Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 14:26:28 +0200 Subject: [PATCH 07/12] Change ServerController and BaseServerConfig unit test code to be cleaner. --- solid/tests/Unit/BaseServerConfigTest.php | 7 ++--- .../Unit/Controller/ServerControllerTest.php | 27 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/solid/tests/Unit/BaseServerConfigTest.php b/solid/tests/Unit/BaseServerConfigTest.php index a4b3d271..cd60af07 100644 --- a/solid/tests/Unit/BaseServerConfigTest.php +++ b/solid/tests/Unit/BaseServerConfigTest.php @@ -3,7 +3,6 @@ namespace OCA\Solid; use OCA\Solid\AppInfo\Application; -use OCA\Solid\BaseServerConfig; use OCP\IConfig; use PHPUnit\Framework\TestCase; use TypeError; @@ -20,13 +19,15 @@ function random_bytes() */ class BaseServerConfigTest extends TestCase { - /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ + ////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public const MOCK_RANDOM_BYTES = 'mock random bytes'; - const MOCK_REDIRECT_URI = 'mock redirect uri'; + private const MOCK_REDIRECT_URI = 'mock redirect uri'; private const MOCK_CLIENT_ID = 'mock-client-id'; private const MOCK_ORIGIN = 'mock origin'; + /////////////////////////////////// TESTS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ + /** * @testdox BaseServerConfig should complain when called before given a Configuration * @covers ::__construct diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index 759aba19..bd4f6d15 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -113,7 +113,7 @@ public function testAuthorizeWithoutUser() $controller = new ServerController(...$parameters); - $expected = new JSONResponse('Authorization required', 401); + $expected = new JSONResponse('Authorization required', Http::STATUS_UNAUTHORIZED); $actual = $controller->authorize(); $this->assertEquals($expected, $actual); @@ -135,7 +135,7 @@ public function testAuthorizeWithoutValidToken() $controller = new ServerController(...$parameters); $actual = $controller->authorize(); - $expected = new JSONResponse('Bad request, does not contain valid token', 400); + $expected = new JSONResponse('Bad request, does not contain valid token', Http::STATUS_BAD_REQUEST); $this->assertEquals($expected, $actual); } @@ -164,7 +164,7 @@ public function testAuthorizeWithoutApprovedClient() $controller = new ServerController(...$parameters); $actual = $controller->authorize(); - $expected = new JSONResponse('Approval required', 302, ['Location' => '']); + $expected = new JSONResponse('Approval required', Http::STATUS_FOUND, ['Location' => '']); $this->assertEquals($expected, $actual); } @@ -203,25 +203,24 @@ public function testAuthorizeWithApprovedClient() 'headers' => [ 'Cache-Control' => 'no-cache, no-store, must-revalidate', 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", + 'Content-Type' => 'application/json; charset=utf-8', 'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'", 'X-Robots-Tag' => 'noindex, nofollow', - 'Content-Type' => 'application/json; charset=utf-8', - ], - 'status' => 302, + 'status' => Http::STATUS_FOUND, ]; - $headers = $response->getHeaders(); - $location = $headers['Location']; - // Not comparing time-sensitive data - unset($headers['X-Request-Id'], $headers['Location']); - $actual = [ 'data' => $response->getData(), - 'headers' => $headers, + 'headers' => $response->getHeaders(), 'status' => $response->getStatus(), ]; + $location = $actual['headers']['Location']; + + // Not comparing time-sensitive data + unset($actual['headers']['X-Request-Id'], $actual['headers']['Location']); + $this->assertEquals($expected, $actual); // @TODO: Move $location assert to a separate test @@ -236,8 +235,8 @@ public function testAuthorizeWithApprovedClient() 'host' => 'mock.client', 'path' => '/redirect', 'fragment' => [ - 'token_type'=>'Bearer', - 'expires_in'=>'3600', + 'token_type' => 'Bearer', + 'expires_in' => '3600', ], ], $url); } From 54c843fe87489bb77f787eb549c0e77c6bfa4802 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 14:27:47 +0200 Subject: [PATCH 08/12] Fix bug in ServerController caused by incorrect check and missing status code. --- solid/lib/Controller/ServerController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/solid/lib/Controller/ServerController.php b/solid/lib/Controller/ServerController.php index 06f18c9f..f9dbfd5a 100644 --- a/solid/lib/Controller/ServerController.php +++ b/solid/lib/Controller/ServerController.php @@ -348,8 +348,8 @@ public function logout() { public function register() { $clientData = file_get_contents('php://input'); $clientData = json_decode($clientData, true); - if (!$clientData['redirect_uris']) { - return new JSONResponse("Missing redirect URIs"); + if (! isset($clientData['redirect_uris'])) { + return new JSONResponse("Missing redirect URIs", Http::STATUS_BAD_REQUEST); } $clientData['client_id_issued_at'] = time(); $parsedOrigin = parse_url($clientData['redirect_uris'][0]); From 03d58e55c298d7f3f3d673e583047463d5524177 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 17:20:34 +0200 Subject: [PATCH 09/12] Add very basic unit test for ServerController::token(). --- .../Unit/Controller/ServerControllerTest.php | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index bd4f6d15..b3f18bb3 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -2,6 +2,7 @@ namespace OCA\Solid\Controller; +use Laminas\Diactoros\Response; use OC\AppFramework\Http; use OCA\Solid\AppInfo\Application; use OCA\Solid\Service\UserService; @@ -310,6 +311,69 @@ public function testRegisterWithRedirectUris() ], $actual); } + /** + * @testdox ServerController should consume Post, Server, and Session variables when generating a token + * + * @covers ::token + */ + public function testToken() + { + $_POST['client_id'] = self::MOCK_CLIENT_ID; + $_POST['code'] = ''; + $_SERVER['HTTP_DPOP'] = 'mock dpop'; + $_SESSION['nonce'] = 'mock nonce'; + + $parameters = $this->createMockConstructorParameters(); + + // @FIXME: Use actual TokenGenerator when we know how to make a valid 'code' for the test + $mockTokenGenerator = $this->createMock(\Pdsinterop\Solid\Auth\TokenGenerator::class); + $mockTokenGenerator->method('getCodeInfo')->willReturn(['user_id' => self::MOCK_USER_ID]); + $mockTokenGenerator->expects($this->once()) + ->method('addIdTokenToResponse') + ->with( + $this->isInstanceOf(Response::class), + $_POST['client_id'], + self::MOCK_USER_ID, + $_SESSION['nonce'], + self::$privateKey, + $_SERVER['HTTP_DPOP'], + ) + ->willReturn(new Response('php://memory', Http::STATUS_IM_A_TEAPOT, [ + 'Content-Type' => 'mock application type' + ])); + + $controller = new ServerController(...$parameters); + + $reflectionObject = new \ReflectionObject($controller); + $reflectionProperty = $reflectionObject->getProperty('tokenGenerator'); + $reflectionProperty->setAccessible(true); + $reflectionProperty->setValue($controller, $mockTokenGenerator); + + $tokenResponse = $controller->token(); + + $expected = [ + 'data' => "I'm a teapot", + 'headers' => [ + 'Cache-Control' => 'no-cache, no-store, must-revalidate', + 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", + 'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'", + 'X-Robots-Tag' => 'noindex, nofollow', + 'Content-Type' => 'application/json; charset=utf-8', + ], + 'status' => Http::STATUS_IM_A_TEAPOT, + ]; + + $actual = [ + 'data' => $tokenResponse->getData(), + 'headers' => $tokenResponse->getHeaders(), + 'status' => $tokenResponse->getStatus(), + ]; + unset($actual['headers']['X-Request-Id']); + + + $this->assertEquals($expected, $actual); + } + ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public function createMockConfig($clientData): IConfig|MockObject From 4bc8355f9b7dfe377c6c03a108c0e9bf6f02acbf Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Mon, 9 Jun 2025 18:26:54 +0200 Subject: [PATCH 10/12] Fix for CLN-006 in ServerController Resolves CWE-601 - URL Redirection to Untrusted Site ('Open Redirect') --- solid/lib/Controller/ServerController.php | 20 ++++ .../Unit/Controller/ServerControllerTest.php | 112 +++++++++++++----- 2 files changed, 101 insertions(+), 31 deletions(-) diff --git a/solid/lib/Controller/ServerController.php b/solid/lib/Controller/ServerController.php index f9dbfd5a..2382444b 100644 --- a/solid/lib/Controller/ServerController.php +++ b/solid/lib/Controller/ServerController.php @@ -220,6 +220,26 @@ public function authorize() { return $result; // ->addHeader('Access-Control-Allow-Origin', '*'); } + if (isset($getVars['redirect_uri'])) { + $redirectUri = $getVars['redirect_uri']; + if (! isset($clientRegistration['redirect_uris']) || ! is_array($clientRegistration['redirect_uris'])) { + return new JSONResponse('Invalid client registration, no redirect URIs found', Http::STATUS_BAD_REQUEST); + } + + $redirectUris = $clientRegistration['redirect_uris']; + + $validRedirectUris = array_filter($redirectUris, function ($uri) use ($redirectUri) { + // @CHECKME: Does either URI need to be normalized when it is a URL? + // For instance, anchors `#` or query parameters `?` + return $uri === $redirectUri; + }); + + if (count($validRedirectUris) === 0) { + return new JSONResponse('Provided redirect URI does not match any registered URIs', Http::STATUS_BAD_REQUEST); + } + } + + // @CHECKME: Can more than one redirect_uri could be provided for custom schemes? $parsedOrigin = parse_url($clientRegistration['redirect_uris'][0]); if ( $parsedOrigin['scheme'] != "https" && diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index b3f18bb3..1a965db8 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -171,19 +171,14 @@ public function testAuthorizeWithoutApprovedClient() } /** - * @testdox ServerController should return a 302 redirect when asked to authorize client that has been approved + * @testdox ServerController should return a 400 when asked to authorize a client that sends an incorrect redirect URI * * @covers ::authorize */ - public function testAuthorizeWithApprovedClient() + public function testAuthorizeWithInvalidRedirectUri() { $_GET['client_id'] = self::MOCK_CLIENT_ID; - $_GET['nonce'] = 'mock-nonce'; - // JWT with empty payload, HS256 encoded, created with `private.key` from fixtures - $_GET['request'] = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.8VKCTiBegJPuPIZlp0wbV0Sbdn5BS6TE5DCx6oYNc5o'; - $_GET['response_type'] = 'mock-response-type'; - - $_SERVER['REQUEST_URI'] = 'https://mock.server'; + $_GET['redirect_uri'] = 'https://some.other.client/redirect'; $clientData = json_encode(['client_name' => 'Mock Client', 'redirect_uris' => ['https://mock.client/redirect']]); @@ -200,7 +195,7 @@ public function testAuthorizeWithApprovedClient() $response = $controller->authorize(); $expected = [ - 'data' => 'ok', + 'data' => 'Provided redirect URI does not match any registered URIs', 'headers' => [ 'Cache-Control' => 'no-cache, no-store, must-revalidate', 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", @@ -208,7 +203,7 @@ public function testAuthorizeWithApprovedClient() 'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'", 'X-Robots-Tag' => 'noindex, nofollow', ], - 'status' => Http::STATUS_FOUND, + 'status' => Http::STATUS_BAD_REQUEST, ]; $actual = [ @@ -217,32 +212,87 @@ public function testAuthorizeWithApprovedClient() 'status' => $response->getStatus(), ]; - $location = $actual['headers']['Location']; - // Not comparing time-sensitive data - unset($actual['headers']['X-Request-Id'], $actual['headers']['Location']); + unset($actual['headers']['X-Request-Id']); $this->assertEquals($expected, $actual); - - // @TODO: Move $location assert to a separate test - $url = parse_url($location); - - parse_str($url['fragment'], $url['fragment']); - - unset($url['fragment']['access_token'], $url['fragment']['id_token']); - - $this->assertEquals([ - 'scheme' => 'https', - 'host' => 'mock.client', - 'path' => '/redirect', - 'fragment' => [ - 'token_type' => 'Bearer', - 'expires_in' => '3600', - ], - ], $url); } - /** + /** + * @testdox ServerController should return a 302 redirect when asked to authorize client that has been approved + * + * @covers ::authorize + */ + public function testAuthorize() + { + $_GET['client_id'] = self::MOCK_CLIENT_ID; + $_GET['nonce'] = 'mock-nonce'; + // JWT with empty payload, HS256 encoded, created with `private.key` from fixtures + $_GET['request'] = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.8VKCTiBegJPuPIZlp0wbV0Sbdn5BS6TE5DCx6oYNc5o'; + $_GET['response_type'] = 'mock-response-type'; + $_GET['redirect_uri'] = 'https://mock.client/redirect'; + + $_SERVER['REQUEST_URI'] = 'https://mock.server'; + $_SERVER['REQUEST_URI']; + + $clientData = json_encode(['client_name' => 'Mock Client', 'redirect_uris' => ['https://mock.client/redirect']]); + + $parameters = $this->createMockConstructorParameters($clientData); + + $this->mockConfig->method('getUserValue') + ->with(self::MOCK_USER_ID, Application::APP_ID, 'allowedClients', '[]') + ->willReturn(json_encode([self::MOCK_CLIENT_ID])); + + $this->mockUserManager->method('userExists')->willReturn(true); + + $controller = new ServerController(...$parameters); + + $response = $controller->authorize(); + + $expected = [ + 'data' => 'ok', + 'headers' => [ + 'Cache-Control' => 'no-cache, no-store, must-revalidate', + 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", + 'Content-Type' => 'application/json; charset=utf-8', + 'Feature-Policy' => "autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none'", + 'X-Robots-Tag' => 'noindex, nofollow', + ], + 'status' => Http::STATUS_FOUND, + ]; + + $actual = [ + 'data' => $response->getData(), + 'headers' => $response->getHeaders(), + 'status' => $response->getStatus(), + ]; + + $location = $actual['headers']['Location'] ?? ''; + + // Not comparing time-sensitive data + unset($actual['headers']['X-Request-Id'], $actual['headers']['Location']); + + $this->assertEquals($expected, $actual); + + // @TODO: Move $location assert to a separate test + $url = parse_url($location); + + parse_str($url['fragment'], $url['fragment']); + + unset($url['fragment']['access_token'], $url['fragment']['id_token']); + + $this->assertEquals([ + 'scheme' => 'https', + 'host' => 'mock.client', + 'path' => '/redirect', + 'fragment' => [ + 'token_type' => 'Bearer', + 'expires_in' => '3600', + ], + ], $url); + } + + /** * @testdox ServerController should return a 400 when asked to register without valid client data * * @covers ::register From 01ab5ebc359650b1b6fff893ab54cbe9a4e66e13 Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Thu, 26 Jun 2025 17:35:24 +0200 Subject: [PATCH 11/12] Change error message for unregistered redirect URIs to specify provided URI. --- solid/lib/Controller/ServerController.php | 6 +++--- solid/tests/Unit/Controller/ServerControllerTest.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/solid/lib/Controller/ServerController.php b/solid/lib/Controller/ServerController.php index 2382444b..f753b8d7 100644 --- a/solid/lib/Controller/ServerController.php +++ b/solid/lib/Controller/ServerController.php @@ -23,6 +23,7 @@ class ServerController extends Controller { use DpopFactoryTrait; + public const ERROR_UNREGISTERED_URI = 'Provided redirect URI "%s" does not match any registered URIs'; private $userId; /* @var IUserManager */ @@ -229,13 +230,12 @@ public function authorize() { $redirectUris = $clientRegistration['redirect_uris']; $validRedirectUris = array_filter($redirectUris, function ($uri) use ($redirectUri) { - // @CHECKME: Does either URI need to be normalized when it is a URL? - // For instance, anchors `#` or query parameters `?` return $uri === $redirectUri; }); if (count($validRedirectUris) === 0) { - return new JSONResponse('Provided redirect URI does not match any registered URIs', Http::STATUS_BAD_REQUEST); + $message = vsprintf(self::ERROR_UNREGISTERED_URI, [$redirectUri]); + return new JSONResponse($message, Http::STATUS_BAD_REQUEST); } } diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index 1a965db8..b1104cbd 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -195,7 +195,7 @@ public function testAuthorizeWithInvalidRedirectUri() $response = $controller->authorize(); $expected = [ - 'data' => 'Provided redirect URI does not match any registered URIs', + 'data' => vsprintf($controller::ERROR_UNREGISTERED_URI, [$_GET['redirect_uri']]), 'headers' => [ 'Cache-Control' => 'no-cache, no-store, must-revalidate', 'Content-Security-Policy' => "default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none'", From 71e1fdcd714fac608a7bd9fb23dfd444d4e1157a Mon Sep 17 00:00:00 2001 From: Ben Peachey Date: Fri, 27 Jun 2025 11:08:25 +0200 Subject: [PATCH 12/12] Code cleanup. --- solid/lib/Controller/ServerController.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/solid/lib/Controller/ServerController.php b/solid/lib/Controller/ServerController.php index f753b8d7..cbef27b7 100644 --- a/solid/lib/Controller/ServerController.php +++ b/solid/lib/Controller/ServerController.php @@ -239,11 +239,10 @@ public function authorize() { } } - // @CHECKME: Can more than one redirect_uri could be provided for custom schemes? - $parsedOrigin = parse_url($clientRegistration['redirect_uris'][0]); + $parsedOrigin = parse_url($redirectUri); if ( - $parsedOrigin['scheme'] != "https" && - $parsedOrigin['scheme'] != "http" && + $parsedOrigin['scheme'] !== "https" && + $parsedOrigin['scheme'] !== "http" && !isset($_GET['customscheme']) ) { $result = new JSONResponse('Custom schema');