diff --git a/init.sh b/init.sh index e066c923..a7c10b3c 100644 --- a/init.sh +++ b/init.sh @@ -6,7 +6,8 @@ php console.php app:enable solid php console.php config:system:set trusted_domains 1 --value=server php console.php config:system:set trusted_domains 2 --value=nextcloud.local php console.php config:system:set trusted_domains 3 --value=thirdparty -# set 'tester' and 'https://tester' as allowed clients for the test suite to run -php console.php user:setting alice solid allowedClients '["f5d1278e8109edd94e1e4197e04873b9", "2e5cddcf0f663544e98982931e6cc5a6"]' +# set 'tester' and 'https://tester' as tyrusted apps for the test suite to run +php console.php config:app:set solid trustedApps --value='["https://tester", "tester"]' + echo configured mkdir -p /var/www/html/data/files_trashbin/versions \ No newline at end of file diff --git a/solid/lib/BaseServerConfig.php b/solid/lib/BaseServerConfig.php index ab967dea..5e788202 100644 --- a/solid/lib/BaseServerConfig.php +++ b/solid/lib/BaseServerConfig.php @@ -157,25 +157,35 @@ public function removeClientConfig($clientId) { $this->config->setAppValue('solid', 'clientScopes', $scopes); } - public function saveClientRegistration($origin, $clientData) { - $originHash = md5($origin); - $existingRegistration = $this->getClientRegistration($originHash); - if ($existingRegistration && isset($existingRegistration['redirect_uris'])) { - foreach ($existingRegistration['redirect_uris'] as $uri) { - $clientData['redirect_uris'][] = $uri; - } - $clientData['redirect_uris'] = array_unique($clientData['redirect_uris']); - if (isset($existingRegistration['blocked'])) { - $clientData['blocked'] = $existingRegistration['blocked']; + public function saveClientRegistration($clientData) + { + $generatedClientId = bin2hex(random_bytes(16)); // 32 chars for the client Id + + // Avoid collision, give up after 5 tries. + for ($i = 0; $i < 5; $i++) { + $existingRegistration = $this->getClientRegistration($generatedClientId); + if ($existingRegistration['client_id'] ?? false) { + $generatedClientId = bin2hex(random_bytes(16)); + } else { + break; } } - $clientData['client_id'] = $originHash; - $clientData['client_name'] = $origin; - $clientData['client_secret'] = md5(random_bytes(32)); - $this->config->setAppValue('solid', "client-" . $originHash, json_encode($clientData)); + if ($existingRegistration['client_id'] ?? false) { + throw new \Exception("Could not generate unique client ID"); + } + + $generatedClientSecret = bin2hex(random_bytes(32)); // and 64 chars for the client secret + + $clientData['client_id'] = $generatedClientId; + $clientData['client_secret'] = $generatedClientSecret; + + if (!isset($clientData['client_name'])) { + $clientData['client_name'] = "Client $generatedClientId"; + } + + $this->config->setAppValue('solid', "client-" . $generatedClientId, json_encode($clientData)); - $this->config->setAppValue('solid', "client-" . $origin, json_encode($clientData)); return $clientData; } @@ -188,6 +198,13 @@ public function getClientRegistration($clientId) { return json_decode($data, true); } + public function getTrustedApps() + { + $appValue = $this->config->getAppValue('solid', 'trustedApps', '[]'); + + return json_decode($appValue, true, 512, JSON_THROW_ON_ERROR); + } + public function getUserSubDomainsEnabled() { $value = $this->config->getAppValue('solid', 'userSubDomainsEnabled', false); diff --git a/solid/lib/Controller/ServerController.php b/solid/lib/Controller/ServerController.php index 2b558cd1..242565c3 100644 --- a/solid/lib/Controller/ServerController.php +++ b/solid/lib/Controller/ServerController.php @@ -18,6 +18,7 @@ use Lcobucci\JWT\Configuration; use Lcobucci\JWT\Signer\Key\InMemory; use Lcobucci\JWT\Signer\Rsa\Sha256; +use Pdsinterop\Solid\Auth\Enum\Authorization; class ServerController extends Controller { @@ -208,22 +209,25 @@ public function authorize() { $getVars['redirect_uri'] ) ); - $clientId = $this->config->saveClientRegistration($origin, $clientData)['client_id']; - $clientId = $this->config->saveClientRegistration($getVars['client_id'], $clientData)['client_id']; + + $clientId = $this->config->saveClientRegistration($clientData)['client_id']; $returnUrl = $getVars['redirect_uri']; + $clientRegistration = $this->config->getClientRegistration($clientId); + // @FIXME: Implement $clientRegistration = $this->fetchRemoteClientDocument($getVars['client_id']) + // to replace this section of this `if` statement. } else { $clientId = $getVars['client_id']; $returnUrl = $_SERVER['REQUEST_URI']; + $clientRegistration = $this->config->getClientRegistration($clientId); } - $clientRegistration = $this->config->getClientRegistration($clientId); if (isset($clientRegistration['blocked']) && ($clientRegistration['blocked'] === true)) { $result = new JSONResponse('Unauthorized client'); $result->setStatus(403); return $result; } - $approval = $this->checkApproval($clientId); + $approval = $this->checkApproval($clientId, $clientRegistration); if (!$approval) { $result = new JSONResponse('Approval required'); $result->setStatus(302); @@ -283,13 +287,23 @@ public function authorize() { return $this->respond($response); // ->addHeader('Access-Control-Allow-Origin', '*'); } - private function checkApproval($clientId) { + private function checkApproval($clientId, $clientRegistration) + { + $approved = Authorization::DENIED; + $allowedClients = $this->config->getAllowedClients($this->userId); if (in_array($clientId, $allowedClients)) { - return \Pdsinterop\Solid\Auth\Enum\Authorization::APPROVED; - } else { - return \Pdsinterop\Solid\Auth\Enum\Authorization::DENIED; + $approved = Authorization::APPROVED; + } elseif (isset($clientRegistration['origin'])) { + $origin = $clientRegistration['origin']; + $trustedApps = $this->config->getTrustedApps(); + + if (in_array($origin, $trustedApps)) { + $approved = Authorization::APPROVED; + } } + + return $approved; } private function getProfilePage() { @@ -405,20 +419,24 @@ public function register() { 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]); + $parsedOrigin = parse_url($clientData['redirect_uris'][0]); // FIXME: Should we have multiple origins? $origin = $parsedOrigin['scheme'] . '://' . $parsedOrigin['host']; if (isset($parsedOrigin['port'])) { $origin .= ":" . $parsedOrigin['port']; } + $clientData['origin'] = $origin; + + $clientData = $this->config->saveClientRegistration($clientData); - $clientData = $this->config->saveClientRegistration($origin, $clientData); $registration = array( 'client_id' => $clientData['client_id'], 'client_secret' => $clientData['client_secret'], 'registration_client_uri' => $this->urlGenerator->getAbsoluteURL($this->urlGenerator->linkToRoute("solid.server.registeredClient", array("clientId" => $clientData['client_id']))), 'client_id_issued_at' => $clientData['client_id_issued_at'], 'redirect_uris' => $clientData['redirect_uris'], + 'origin' => $clientData['origin'], ); $registration = $this->tokenGenerator->respondToRegistration($registration, $this->config->getPrivateKey()); return (new JSONResponse($registration)); diff --git a/solid/tests/Unit/BaseServerConfigTest.php b/solid/tests/Unit/BaseServerConfigTest.php index cd60af07..d1e07489 100644 --- a/solid/tests/Unit/BaseServerConfigTest.php +++ b/solid/tests/Unit/BaseServerConfigTest.php @@ -149,21 +149,6 @@ public function testGetClientRegistrationForNonExistingClient() $this->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 @@ -176,7 +161,7 @@ public function testSaveClientRegistrationWithoutClientData() $configMock = $this->createMock(IConfig::class); $baseServerConfig = new BaseServerConfig($configMock); - $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN); + $baseServerConfig->saveClientRegistration(); } /** @@ -189,156 +174,61 @@ public function testSaveClientRegistrationForNewClient() $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)) + $configMock->expects($this->exactly(1)) ->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)] + [Application::APP_ID, 'client-' . self::MOCK_ORIGIN, json_encode('{}')] ]); $baseServerConfig = new BaseServerConfig($configMock); - $actual = $baseServerConfig->saveClientRegistration(self::MOCK_ORIGIN, []); + $actual = $baseServerConfig->saveClientRegistration([]); - $this->assertEquals($expected, $actual); + $this->assertArrayHasKey('client_id', $actual); + $this->assertArrayHasKey('client_secret', $actual); + $this->assertArrayHasKey('client_name', $actual); } /** - * @testdox BaseServerConfig should save ClientRegistration when asked to save ClientRegistration for existing client - * @covers ::saveClientRegistration + * @testdox BaseServerConfig should remove ClientRegistration when asked to remove ClientRegistration + * @covers ::removeClientRegistration */ - public function testSaveClientRegistrationForExistingClient() + public function testRemoveClientRegistration() { $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); + ->method('deleteAppValue') + ->with(Application::APP_ID, 'client-' . self::MOCK_CLIENT_ID); - $this->assertEquals($expected, $actual); + $baseServerConfig->removeClientRegistration(self::MOCK_CLIENT_ID); } /** - * @testdox BaseServerConfig should always "blocked" to existing value when asked to save ClientRegistration for blocked client - * @covers ::saveClientRegistration + * @testdox BaseServerConfig should return decoded trusted apps when asked to GetTrustedApps + * @covers ::getTrustedApps */ - public function testSaveClientRegistrationSetsBlocked() + public function testGetTrustedApps() { $configMock = $this->createMock(IConfig::class); + $baseServerConfig = new BaseServerConfig($configMock); - $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, - ]; + $expected = [self::MOCK_ORIGIN]; $configMock->expects($this->once()) ->method('getAppValue') - ->with(Application::APP_ID, 'client-' . md5(self::MOCK_ORIGIN)) + ->with(Application::APP_ID, 'trustedApps', '[]') ->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); + $actual = $baseServerConfig->getTrustedApps(); $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() diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index 32699b40..811fcd06 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -189,6 +189,67 @@ public function testAuthorizeWithoutApprovedClient() $this->assertEquals($expected, $actual); } + /** + * @testdox + * + * @covers ::authorize + */ + public function testAuthorizeWithTrustedApp() + { + $_GET['client_id'] = self::MOCK_CLIENT_ID; + $_GET['redirect_uri'] = 'https://mock.client/redirect'; + + $origin = 'https://mock.client/'; + $clientData = json_encode([ + 'client_name' => 'Mock Client', + 'origin' => $origin, + 'redirect_uris' => ['https://mock.client/redirect'], + ], JSON_THROW_ON_ERROR); + $trustedApps = json_encode([$origin], JSON_THROW_ON_ERROR); + + $parameters = $this->createMockConstructorParameters($clientData, $trustedApps); + + $this->mockConfig->method('getUserValue')->willReturnArgument(3); + + $this->mockUserManager->method('userExists')->willReturn(true); + + $controller = new ServerController(...$parameters); + + $response = $controller->authorize(); + + $expected = $this->createExpectedResponse(); + + $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 authorize a client that sends an incorrect redirect URI * @@ -352,20 +413,29 @@ public function testRegisterWithRedirectUris() $data = [ 'application_type' => 'web', - 'client_id' => 'f4a2d00f7602948a97ff409d7a581ec2', - 'client_secret' => '3b5798fddd49e23662ee6fe801085100', 'grant_types' => ['implicit'], 'id_token_signed_response_alg' => 'RS256', + 'origin' => 'https://mock.client', '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', ]; $expected = $this->createExpectedResponse(Http::STATUS_OK, $data); + $actualData = $response->getData(); + $this->assertArrayHasKey('client_id', $actualData); + $this->assertArrayHasKey('client_secret', $actualData); + $this->assertArrayHasKey('registration_access_token', $actualData); + + unset( + $actualData['client_id'], + $actualData['client_secret'], + $actualData['registration_access_token'], + ); + $actual = [ - 'data' => $response->getData(), + 'data' => $actualData, 'headers' => $response->getHeaders(), 'status' => $response->getStatus(), ]; @@ -452,25 +522,23 @@ public function testToken() ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\ - public function createMockConfig($clientData): IConfig|MockObject + public function createMockConfig($clientData, $trustedApps): 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-6d6f636b2072616e646f6d206279746573', '{}', '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], - // Client ID from register() with https://mock.client - [Application::APP_ID, 'client-f4a2d00f7602948a97ff409d7a581ec2', '{}', 'return' => $clientData], + [Application::APP_ID, 'trustedApps', '[]', 'return' => $trustedApps], ]); return $this->mockConfig; } - public function createMockConstructorParameters($clientData = '{}'): array + public function createMockConstructorParameters($clientData = '{}', $trustedApps = '[]'): array { $parameters = [ 'mock appname', @@ -479,7 +547,7 @@ public function createMockConstructorParameters($clientData = '{}'): array $this->createMockUserManager(), $this->createMockUrlGenerator(), self::MOCK_USER_ID, - $this->createMockConfig($clientData), + $this->createMockConfig($clientData, $trustedApps), $this->createMock(UserService::class), $this->createMock(IDBConnection::class), ];