diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml index 192a59f9..a79ee15f 100644 --- a/.github/workflows/php.yml +++ b/.github/workflows/php.yml @@ -65,6 +65,14 @@ jobs: working-directory: "solid" # 02.test.php.test-unit.yml php-unittest: + container: + image: ghcr.io/${{ github.repository }}:main-${{ matrix.nextcloud_version }} + env: + NEXTCLOUD_PATH: /usr/src/nextcloud/apps + NEXTCLOUD_UPDATE: 1 + XDEBUG_MODE: coverage + volumes: + - /usr/bin/composer:/usr/bin/composer name: PHP Unit Tests needs: - lint-php-syntax @@ -78,27 +86,24 @@ jobs: - 29 - 30 steps: - - uses: actions/checkout@v4 - - uses: shivammathur/setup-php@v2 - with: - ini-values: error_reporting=E_ALL, display_errors=On - php-version: 8.3 + - uses: actions/checkout@v5 + - name: Setup Test Environment + run: | + git config --global --add safe.directory "${NEXTCLOUD_PATH}" + /entrypoint.sh "echo" + bash "${GITHUB_WORKSPACE}/init.sh" + rm -r "${NEXTCLOUD_PATH}/solid/" + cp --archive --verbose "${GITHUB_WORKSPACE}/." "${NEXTCLOUD_PATH}/" - name: Install and Cache Composer dependencies - uses: "ramsey/composer-install@v2" + uses: ramsey/composer-install@v3 with: - working-directory: "solid" + working-directory: /usr/src/nextcloud/apps/solid env: COMPOSER_AUTH: '{"github-oauth": {"github.com": "${{ secrets.GITHUB_TOKEN }}"}}' - - run: | - docker run \ - --env 'XDEBUG_MODE=coverage' \ - --rm \ - --volume="./solid:/var/www/html/apps/solid" \ - ghcr.io/${{ github.repository }}:main-${{ matrix.nextcloud_version }} \ - bash -c 'NEXTCLOUD_UPDATE=1 /entrypoint.sh "echo" \ - && sudo -u www-data bash /init.sh \ - && cd /var/www/html/apps/solid \ - && bin/phpunit --configuration phpunit.xml' + - name: Run PHPUnit + run: | + "${NEXTCLOUD_PATH}/solid/bin/phpunit" \ + --configuration "${NEXTCLOUD_PATH}/solid/phpunit.xml" # 03.quality.php.scan.dependencies-vulnerabilities.yml scan-dependencies-vulnerabilities: diff --git a/solid/lib/Controller/ServerController.php b/solid/lib/Controller/ServerController.php index b4dc255b..8828f14d 100644 --- a/solid/lib/Controller/ServerController.php +++ b/solid/lib/Controller/ServerController.php @@ -3,6 +3,7 @@ use OCA\Solid\DpopFactoryTrait; use OCA\Solid\ServerConfig; +use OCA\Solid\Service\UserService; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; @@ -47,6 +48,8 @@ class ServerController extends Controller /* @var \Pdsinterop\Solid\Auth\TokenGenerator */ private $tokenGenerator; + private UserService $userService; + public function __construct( $AppName, IRequest $request, @@ -55,7 +58,7 @@ public function __construct( IURLGenerator $urlGenerator, $userId, IConfig $config, - \OCA\Solid\Service\UserService $UserService, + \OCA\Solid\Service\UserService $userService, IDBConnection $connection, ) { parent::__construct($AppName, $request); @@ -66,6 +69,7 @@ public function __construct( $this->request = $request; $this->urlGenerator = $urlGenerator; $this->session = $session; + $this->userService = $userService; $this->setJtiStorage($connection); @@ -150,12 +154,15 @@ public function cors($path) { public function authorize() { // Create a request if (!$this->userManager->userExists($this->userId)) { - $result = new JSONResponse('Authorization required'); - $result->setStatus(401); - return $result; -// return $result->addHeader('Access-Control-Allow-Origin', '*'); + return new JSONResponse('Authorization required', 401); + } + + if (! isset($_GET['client_id'])) { + return new JSONResponse('Bad request, missing client_id', 400); } + $clientId = $_GET['client_id']; + $getVars = $_GET; if (isset($_GET['request'])) { $jwtConfig = Configuration::forSymmetricSigner(new Sha256(), InMemory::plainText($this->config->getPrivateKey())); try { @@ -164,33 +171,32 @@ public function authorize() { } catch(\Exception $e) { $this->session->set("nonce", $_GET['nonce']); } - } - $getVars = $_GET; - if (!isset($getVars['grant_type'])) { - $getVars['grant_type'] = 'implicit'; - } - $getVars['response_type'] = $this->getResponseType(); - $getVars['scope'] = "openid" ; - - if (!isset($getVars['redirect_uri'])) { - if (!isset($token)) { - $result = new JSONResponse('Bad request, does not contain valid token'); - $result->setStatus(400); - return $result; -// return $result->addHeader('Access-Control-Allow-Origin', '*'); + if (!isset($getVars['grant_type'])) { + $getVars['grant_type'] = 'implicit'; } - try { - $getVars['redirect_uri'] = $token->claims()->get("redirect_uri"); - } catch(\Exception $e) { - $result = new JSONResponse('Bad request, missing redirect uri'); - $result->setStatus(400); - return $result; -// return $result->addHeader('Access-Control-Allow-Origin', '*'); + $getVars['response_type'] = $this->getResponseType(); + $getVars['scope'] = "openid"; + + if (!isset($getVars['redirect_uri'])) { + if (!isset($token)) { + return new JSONResponse('Bad request, does not contain valid token', 400); + } + + try { + $getVars['redirect_uri'] = $token->claims()->get("redirect_uri"); + } catch(\Exception $e) { + return new JSONResponse('Bad request, missing redirect uri', 400); + } } } - if (preg_match("/^http(s)?:/", $getVars['client_id'])) { + $request = \Laminas\Diactoros\ServerRequestFactory::fromGlobals($_SERVER, $getVars, $_POST, $_COOKIE, $_FILES); + $response = new \Laminas\Diactoros\Response(); + $authServer = new \Pdsinterop\Solid\Auth\Server($this->authServerFactory, $this->authServerConfig, $response); + + // @FIXME: Check OIDC Spec for rules regarding Client updates + if (preg_match("/^http(s)?:/", $clientId)) { $parsedOrigin = parse_url($getVars['redirect_uri']); $origin = $parsedOrigin['scheme'] . '://' . $parsedOrigin['host']; if (isset($parsedOrigin['port'])) { @@ -198,17 +204,18 @@ public function authorize() { } $clientData = array( "client_id_issued_at" => time(), - "client_name" => $getVars['client_id'], + "client_name" => $clientId, "origin" => $origin, "redirect_uris" => array( $getVars['redirect_uri'] ) ); - $clientId = $this->config->saveClientRegistration($origin, $clientData)['client_id']; - $clientId = $this->config->saveClientRegistration($getVars['client_id'], $clientData)['client_id']; + + $this->config->saveClientRegistration($origin, $clientData); + $clientId = $this->config->saveClientRegistration($clientId, $clientData)['client_id']; + $returnUrl = $getVars['redirect_uri']; } else { - $clientId = $getVars['client_id']; $returnUrl = $_SERVER['REQUEST_URI']; } @@ -225,7 +232,8 @@ public function authorize() { $result->setStatus(302); $approvalUrl = $this->urlGenerator->getAbsoluteURL($this->urlGenerator->linkToRoute("solid.page.approval", array("clientId" => $clientId, "returnUrl" => $returnUrl))); $result->addHeader("Location", $approvalUrl); - return $result; // ->addHeader('Access-Control-Allow-Origin', '*'); + + return $result; } if (isset($getVars['redirect_uri'])) { @@ -260,23 +268,21 @@ public function authorize() { return $result; } + $webId = $this->getProfilePage(); $user = new \Pdsinterop\Solid\Auth\Entity\User(); - $user->setIdentifier($this->getProfilePage()); + $user->setIdentifier($webId); - $request = \Laminas\Diactoros\ServerRequestFactory::fromGlobals($_SERVER, $getVars, $_POST, $_COOKIE, $_FILES); - $response = new \Laminas\Diactoros\Response(); - $server = new \Pdsinterop\Solid\Auth\Server($this->authServerFactory, $this->authServerConfig, $response); + $response = $authServer->respondToAuthorizationRequest($request, $user, $approval); - $response = $server->respondToAuthorizationRequest($request, $user, $approval); $response = $this->tokenGenerator->addIdTokenToResponse( $response, $clientId, - $this->getProfilePage(), + $webId, $this->session->get("nonce"), $this->config->getPrivateKey() ); - return $this->respond($response); // ->addHeader('Access-Control-Allow-Origin', '*'); + return $this->respond($response); } private function checkApproval($clientId) { @@ -389,11 +395,17 @@ public function logout() { * @NoCSRFRequired */ public function register() { - $clientData = file_get_contents('php://input'); - $clientData = json_decode($clientData, true); + $postData = file_get_contents('php://input'); + $clientData = json_decode($postData, true); + + if (! isset($clientData)) { + return new JSONResponse('Missing client data', Http::STATUS_BAD_REQUEST); + } + 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]); $origin = $parsedOrigin['scheme'] . '://' . $parsedOrigin['host']; diff --git a/solid/tests/Unit/Controller/ServerControllerTest.php b/solid/tests/Unit/Controller/ServerControllerTest.php index 2920b0dd..4c113f68 100644 --- a/solid/tests/Unit/Controller/ServerControllerTest.php +++ b/solid/tests/Unit/Controller/ServerControllerTest.php @@ -39,15 +39,13 @@ class ServerControllerTest extends TestCase ////////////////////////////////// FIXTURES \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ private const MOCK_CLIENT_ID = 'mock-client-id'; + private const MOCK_RESPONSE_TYPE = 'mock-response-type'; + private const MOCK_URI = 'mock uri'; private const MOCK_USER_ID = 'mock user id'; public static string $clientData = ''; private static string $privateKey; - private IConfig|MockObject $mockConfig; - private IURLGenerator|MockObject $mockURLGenerator; - private IUserManager|MockObject $mockUserManager; - public static function setUpBeforeClass(): void { $keyPath = __DIR__ . '/../../fixtures/keys'; @@ -85,7 +83,7 @@ public function testInstantiationWithoutRequiredParameter($index) $this->expectExceptionMessageMatches('/^' . $message . '$/'); - new ServerController(...$parameters); + new ServerController(...array_values($parameters)); } /** @@ -97,7 +95,7 @@ public function testInstantiation() { $parameters = $this->createMockConstructorParameters(); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $this->assertInstanceOf(ServerController::class, $controller); } @@ -111,7 +109,7 @@ public function testAuthorizeWithoutUser() { $parameters = $this->createMockConstructorParameters(); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $expected = new JSONResponse('Authorization required', Http::STATUS_UNAUTHORIZED); $actual = $controller->authorize(); @@ -119,6 +117,25 @@ public function testAuthorizeWithoutUser() $this->assertEquals($expected, $actual); } + /** + * @testdox ServerController should return a 400 when asked to authorize with a user but without client_id + * + * @covers ::authorize + */ + public function testAuthorizeWithoutClientId() + { + $parameters = $this->createMockConstructorParameters(); + + $parameters['MockUserManager']->method('userExists')->willReturn(true); + + $controller = new ServerController(...array_values($parameters)); + + $actual = $controller->authorize(); + $expected = new JSONResponse('Bad request, missing client_id', Http::STATUS_BAD_REQUEST); + + $this->assertEquals($expected, $actual); + } + /** * @testdox ServerController should return a 400 when asked to authorize with a user but without valid token * @@ -126,13 +143,18 @@ public function testAuthorizeWithoutUser() */ public function testAuthorizeWithoutValidToken() { - $_GET['response_type'] = 'mock-response-type'; + $_GET['client_id'] = self::MOCK_CLIENT_ID; + $_GET['nonce'] = 'mock-nonce'; + $_GET['request'] = 'mock request'; + $_GET['response_type'] = self::MOCK_RESPONSE_TYPE; + + $_SERVER['REQUEST_URI'] = self::MOCK_URI; $parameters = $this->createMockConstructorParameters(); - $this->mockUserManager->method('userExists')->willReturn(true); + $parameters['MockUserManager']->method('userExists')->willReturn(true); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $actual = $controller->authorize(); $expected = new JSONResponse('Bad request, does not contain valid token', Http::STATUS_BAD_REQUEST); @@ -151,17 +173,17 @@ public function testAuthorizeWithoutApprovedClient() $_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['response_type'] = self::MOCK_RESPONSE_TYPE; - $_SERVER['REQUEST_URI'] = 'mock uri'; + $_SERVER['REQUEST_URI'] = self::MOCK_URI; $parameters = $this->createMockConstructorParameters(); - $this->mockConfig->method('getUserValue')->willReturnArgument(3); + $parameters['MockConfig']->method('getUserValue')->willReturnArgument(3); - $this->mockUserManager->method('userExists')->willReturn(true); + $parameters['MockUserManager']->method('userExists')->willReturn(true); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $actual = $controller->authorize(); $expected = new JSONResponse('Approval required', Http::STATUS_FOUND, ['Location' => '']); @@ -183,13 +205,13 @@ public function testAuthorizeWithInvalidRedirectUri() $parameters = $this->createMockConstructorParameters($clientData); - $this->mockConfig->method('getUserValue') + $parameters['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); + $parameters['MockUserManager']->method('userExists')->willReturn(true); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $response = $controller->authorize(); @@ -228,7 +250,7 @@ public function testAuthorize() $_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['response_type'] = self::MOCK_RESPONSE_TYPE; $_GET['redirect_uri'] = 'https://mock.client/redirect'; $_SERVER['REQUEST_URI'] = 'https://mock.server'; @@ -238,13 +260,13 @@ public function testAuthorize() $parameters = $this->createMockConstructorParameters($clientData); - $this->mockConfig->method('getUserValue') + $parameters['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); + $parameters['MockUserManager']->method('userExists')->willReturn(true); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $response = $controller->authorize(); @@ -296,11 +318,32 @@ public function testAuthorize() * * @covers ::register */ + public function testRegisterWithoutClientData() + { + $parameters = $this->createMockConstructorParameters(); + + $controller = new ServerController(...array_values($parameters)); + + $actual = $controller->register(); + + $this->assertEquals( + new JSONResponse('Missing client data', Http::STATUS_BAD_REQUEST), + $actual + ); + } + + /** + * @testdox ServerController should return a 400 when asked to register without redirect URIs + * + * @covers ::register + */ public function testRegisterWithoutRedirectUris() { $parameters = $this->createMockConstructorParameters(); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); + + self::$clientData = json_encode([]); $actual = $controller->register(); @@ -319,10 +362,10 @@ public function testRegisterWithRedirectUris() { $parameters = $this->createMockConstructorParameters(); - $this->mockURLGenerator->method('getBaseUrl') + $parameters['MockURLGenerator']->method('getBaseUrl') ->willReturn('https://mock.server'); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); self::$clientData = json_encode(['redirect_uris' => ['https://mock.client/redirect']]); @@ -348,6 +391,7 @@ public function testRegisterWithRedirectUris() 'registration_client_uri' => '', 'response_types' => ['id_token token'], 'token_endpoint_auth_method' => 'client_secret_basic', + 'client_secret' => '3b5798fddd49e23662ee6fe801085100' ], 'headers' => [ 'Cache-Control' => 'no-cache, no-store, must-revalidate', @@ -369,6 +413,7 @@ public function testToken() { $_POST['client_id'] = self::MOCK_CLIENT_ID; $_POST['code'] = ''; + $_POST['grant_type'] = 'authorization_code'; $_SERVER['HTTP_DPOP'] = 'mock dpop'; $_SESSION['nonce'] = 'mock nonce'; @@ -391,7 +436,7 @@ public function testToken() 'Content-Type' => 'mock application type' ])); - $controller = new ServerController(...$parameters); + $controller = new ServerController(...array_values($parameters)); $reflectionObject = new \ReflectionObject($controller); $reflectionProperty = $reflectionObject->getProperty('tokenGenerator'); @@ -423,13 +468,52 @@ public function testToken() $this->assertEquals($expected, $actual); } + /** + * @testdox ServerController should return an OK when asked to logout + * + * @covers ::logout + */ + public function testLogout() { + $parameters = $this->createMockConstructorParameters(); + + $controller = new ServerController(...array_values($parameters)); + + $actual = $controller->logout(); + $expected = new JSONResponse('ok', Http::STATUS_OK); + + $this->assertEquals($expected, $actual); + } + + /** + * @testdox ServerController should complain when asked to logout and the logout fails + * + * @covers ::logout + */ + public function testLogoutError() { + $parameters = $this->createMockConstructorParameters(); + + $mockError = 'Mock logout error'; + $expectedException = new \Exception($mockError); + $parameters['MockUserService'] + ->method('logout') + ->willThrowException($expectedException) + ; + + $controller = new ServerController(...array_values($parameters)); + + $this->expectException($expectedException::class); + $this->expectExceptionMessage($mockError); + + $controller->logout(); + } + ////////////////////////////// MOCKS AND STUBS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public function createMockConfig($clientData): IConfig|MockObject { - $this->mockConfig = $this->createMock(IConfig::class); + $mockConfig = $this->createMock(IConfig::class); - $this->mockConfig->method('getAppValue')->willReturnMap([ + $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], @@ -440,40 +524,26 @@ public function createMockConfig($clientData): IConfig|MockObject [Application::APP_ID, 'client-f4a2d00f7602948a97ff409d7a581ec2', '{}', 'return' => $clientData], ]); - return $this->mockConfig; + return $mockConfig; } public function createMockConstructorParameters($clientData = '{}'): array { $parameters = [ 'mock appname', - $this->createMock(IRequest::class), - $this->createMock(ISession::class), - $this->createMockUserManager(), - $this->createMockUrlGenerator(), - self::MOCK_USER_ID, - $this->createMockConfig($clientData), - $this->createMock(UserService::class), - $this->createMock(IDBConnection::class), + 'MockRequest' => $this->createMock(IRequest::class), + 'MockSession' => $this->createMock(ISession::class), + 'MockUserManager' => $this->createMock(IUserManager::class), + 'MockURLGenerator' => $this->createMock(IURLGenerator::class), + 'MOCK_USER_ID' => self::MOCK_USER_ID, + 'MockConfig' => $this->createMockConfig($clientData), + 'MockUserService' => $this->createMock(UserService::class), + 'MockDBConnection' => $this->createMock(IDBConnection::class), ]; 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); - - return $this->mockUserManager; - } - /////////////////////////////// DATAPROVIDERS \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ public static function provideConstructorParameterIndex()