From 494e5219a5e5c0455c2b118eaa3d721f48ede7d0 Mon Sep 17 00:00:00 2001 From: Thijs Van der Schaeghe Date: Thu, 27 Aug 2026 14:23:34 +0200 Subject: [PATCH] Add GitHub Actions CI and expand unit test coverage CI: .github/workflows/tests.yml runs vendor/bin/phpunit on push to master and on pull requests, on a PHP 8.1-8.5 matrix (composer.lock is not tracked, so each job resolves fresh; every dependency accepts the whole range). gettext is enabled explicitly because catlabinteractive/neuron requires ext-gettext. Tests (no database, no network; HTTP goes through Guzzle's MockHandler): - BasicFlowTest: array scope/response_type joining, fresh state per request, state is single-use, callback without pending request, missing/non-string state (no TypeError), $_GET fallback, error description in the message, token response without access_token, error wins over access_token, non-object JSON bodies rejected (userinfo), userinfo HTTP error, Guzzle transport exception kept as previous, client secret only in the POST body. - UserModelTest: mergeFromInput only takes the username, shouldPing first/stale/recent/custom-interval behaviour and its updateLastPing persistence, anonymize strips email+display name and persists, Guest never exposes an id or personal data. Uses Tests\Fakes\ RecordingUserMapper registered once in Neuron's MapperFactory. - ModuleTest: login stores user id + access token in the session and fires user:login, post-login-redirect is honoured once and cleared, fallback to app root, logout clears the session and fires user:logout. .phpunit.result.cache is now ignored. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Qz9QvEe8uJ65wx8aVKwk7o --- .github/workflows/tests.yml | 35 ++++++ .gitignore | 3 +- tests/BasicFlowTest.php | 175 ++++++++++++++++++++++++++++ tests/Fakes/RecordingUserMapper.php | 75 ++++++++++++ tests/ModuleTest.php | 102 ++++++++++++++++ tests/UserModelTest.php | 135 +++++++++++++++++++++ 6 files changed, 524 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/tests.yml create mode 100644 tests/Fakes/RecordingUserMapper.php create mode 100644 tests/ModuleTest.php create mode 100644 tests/UserModelTest.php diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..614de57 --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,35 @@ +name: Tests + +on: + push: + branches: [ master ] + pull_request: + +jobs: + unit-tests: + name: PHPUnit (PHP ${{ matrix.php }}) + runs-on: ubuntu-latest + permissions: + contents: read + + strategy: + fail-fast: false + matrix: + php: [ '8.1', '8.2', '8.3', '8.4', '8.5' ] + + steps: + - uses: actions/checkout@v4 + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + # gettext: required by catlabinteractive/neuron + extensions: gettext, mbstring, curl + coverage: none + + - name: Install Composer dependencies + run: composer install --prefer-dist --no-progress --no-interaction + + - name: Run tests + run: vendor/bin/phpunit diff --git a/.gitignore b/.gitignore index cdb3d20..97a6a13 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ .idea/ vendor/ -composer.lock \ No newline at end of file +composer.lock +.phpunit.result.cache diff --git a/tests/BasicFlowTest.php b/tests/BasicFlowTest.php index b81686e..b103750 100644 --- a/tests/BasicFlowTest.php +++ b/tests/BasicFlowTest.php @@ -5,9 +5,12 @@ use CatLab\OpenIDClient\BasicFlow; use CatLab\OpenIDClient\Exceptions\OpenIDConnectException; use GuzzleHttp\Client; +use GuzzleHttp\Exception\ConnectException; +use GuzzleHttp\Exception\GuzzleException; use GuzzleHttp\Handler\MockHandler; use GuzzleHttp\HandlerStack; use GuzzleHttp\Middleware; +use GuzzleHttp\Psr7\Request; use GuzzleHttp\Psr7\Response; use PHPUnit\Framework\TestCase; @@ -154,4 +157,176 @@ public function testHttpErrorsBecomeOpenIDConnectExceptions() $this->expectException(OpenIDConnectException::class); $flow->getAccessToken('auth-code-123'); } + + public function testArrayScopeAndResponseTypeAreSpaceSeparated() + { + $flow = new BasicFlow($this->config()); + $uri = $flow->getAuthorizationRequestUri([ 'openid', 'email' ], [ 'code', 'id_token' ]); + + parse_str(parse_url($uri, PHP_URL_QUERY), $query); + $this->assertEquals('openid email', $query['scope']); + $this->assertEquals('code id_token', $query['response_type']); + } + + public function testEachAuthorizationRequestGetsAFreshState() + { + $flow = new BasicFlow($this->config()); + + parse_str(parse_url($flow->getAuthorizationRequestUri(), PHP_URL_QUERY), $first); + parse_str(parse_url($flow->getAuthorizationRequestUri(), PHP_URL_QUERY), $second); + + $this->assertNotEquals($first['state'], $second['state']); + $this->assertEquals(32, strlen($second['state'])); + + // Only the most recent state is valid. + $this->expectException(OpenIDConnectException::class); + $flow->getAuthorizationCode([ 'code' => 'c', 'state' => $first['state'] ]); + } + + public function testStateIsSingleUse() + { + $flow = new BasicFlow($this->config()); + parse_str(parse_url($flow->getAuthorizationRequestUri(), PHP_URL_QUERY), $query); + $callback = [ 'code' => 'auth-code-123', 'state' => $query['state'] ]; + + $this->assertEquals('auth-code-123', $flow->getAuthorizationCode($callback)); + $this->assertArrayNotHasKey(BasicFlow::SESSION_STATE_KEY, $_SESSION); + + $this->expectException(OpenIDConnectException::class); + $flow->getAuthorizationCode($callback); + } + + public function testAuthorizationCodeRejectsCallbackWithoutPendingRequest() + { + $flow = new BasicFlow($this->config()); + + $this->expectException(OpenIDConnectException::class); + $flow->getAuthorizationCode([ 'code' => 'auth-code-123', 'state' => 'anything' ]); + } + + public function testAuthorizationCodeRejectsMissingState() + { + $flow = new BasicFlow($this->config()); + $flow->getAuthorizationRequestUri(); + + $this->expectException(OpenIDConnectException::class); + $flow->getAuthorizationCode([ 'code' => 'auth-code-123' ]); + } + + public function testAuthorizationCodeRejectsNonStringStateWithoutTypeError() + { + $flow = new BasicFlow($this->config()); + $flow->getAuthorizationRequestUri(); + + $this->expectException(OpenIDConnectException::class); + $flow->getAuthorizationCode([ 'code' => 'auth-code-123', 'state' => [ 'injected' ] ]); + } + + public function testAuthorizationCodeReadsGetByDefault() + { + $flow = new BasicFlow($this->config()); + parse_str(parse_url($flow->getAuthorizationRequestUri(), PHP_URL_QUERY), $query); + + $_GET = [ 'code' => 'from-get', 'state' => $query['state'] ]; + try { + $this->assertEquals('from-get', $flow->getAuthorizationCode()); + } finally { + $_GET = []; + } + } + + public function testAuthorizationErrorMessageIncludesDescription() + { + $flow = new BasicFlow($this->config()); + $flow->getAuthorizationRequestUri(); + + try { + $flow->getAuthorizationCode([ 'error' => 'access_denied', 'error_description' => 'User said no' ]); + $this->fail('Expected an OpenIDConnectException'); + } catch (OpenIDConnectException $e) { + $this->assertStringContainsString('access_denied', $e->getMessage()); + $this->assertStringContainsString('User said no', $e->getMessage()); + } + } + + public function testGetAccessTokenThrowsWhenTokenIsMissing() + { + $flow = $this->makeFlow([ + new Response(200, [], json_encode([ 'token_type' => 'Bearer', 'expires_in' => 3600 ])) + ]); + + $this->expectException(OpenIDConnectException::class); + $this->expectExceptionMessage('No access_token'); + $flow->getAccessToken('auth-code-123'); + } + + public function testGetAccessTokenPrefersErrorOverToken() + { + $flow = $this->makeFlow([ + new Response(200, [], json_encode([ 'error' => 'invalid_client', 'access_token' => 'should-not-be-used' ])) + ]); + + $this->expectException(OpenIDConnectException::class); + $this->expectExceptionMessage('invalid_client'); + $flow->getAccessToken('auth-code-123'); + } + + /** + * @dataProvider nonObjectBodies + */ + public function testNonObjectBodiesAreRejected(string $body) + { + $flow = $this->makeFlow([ new Response(200, [], $body) ]); + + $this->expectException(OpenIDConnectException::class); + $this->expectExceptionMessage('invalid JSON'); + $flow->getUserInfo('token-abc'); + } + + public function nonObjectBodies(): array + { + return [ + 'html' => [ 'Sign in' ], + 'empty' => [ '' ], + 'json null' => [ 'null' ], + 'json string' => [ '"token-abc"' ], + 'json number' => [ '42' ], + ]; + } + + public function testGetUserInfoHttpErrorBecomesOpenIDConnectException() + { + $flow = $this->makeFlow([ new Response(401, [], json_encode([ 'error' => 'invalid_token' ])) ]); + + $this->expectException(OpenIDConnectException::class); + $flow->getUserInfo('expired-token'); + } + + public function testTransportFailuresKeepTheGuzzleCause() + { + $flow = $this->makeFlow([ + new ConnectException('Connection refused', new Request('POST', 'https://accounts.example.com/oauth2/token')) + ]); + + try { + $flow->getAccessToken('auth-code-123'); + $this->fail('Expected an OpenIDConnectException'); + } catch (OpenIDConnectException $e) { + $this->assertInstanceOf(GuzzleException::class, $e->getPrevious()); + $this->assertStringContainsString('Connection refused', $e->getMessage()); + } + } + + public function testClientSecretIsOnlySentInTheBody() + { + $flow = $this->makeFlow([ + new Response(200, [], json_encode([ 'access_token' => 'token-abc' ])) + ]); + $flow->getAccessToken('auth-code-123'); + + $request = $this->history[0]['request']; + $this->assertFalse($request->hasHeader('Authorization')); + $this->assertStringNotContainsString('test-secret', (string) $request->getUri()); + $this->assertStringContainsString('client_secret=test-secret', (string) $request->getBody()); + } } diff --git a/tests/Fakes/RecordingUserMapper.php b/tests/Fakes/RecordingUserMapper.php new file mode 100644 index 0000000..8d0a818 --- /dev/null +++ b/tests/Fakes/RecordingUserMapper.php @@ -0,0 +1,75 @@ +setMapper('user', $mapper); + } + + if (!$mapper instanceof self) { + throw new \LogicException('A different user mapper is already registered in this process.'); + } + + return $mapper; + } + + public function reset(): void + { + $this->updated = []; + $this->pinged = []; + $this->created = []; + } + + public function create(User $user) + { + $this->created[] = $user; + return $user; + } + + public function update(User $user) + { + $this->updated[] = $user; + return $user; + } + + public function updateLastPing(User $user) + { + $this->pinged[] = $user; + return $user; + } + + public function getFromEmail($email) + { + return null; + } +} diff --git a/tests/ModuleTest.php b/tests/ModuleTest.php new file mode 100644 index 0000000..52bf025 --- /dev/null +++ b/tests/ModuleTest.php @@ -0,0 +1,102 @@ +setSession(new Session(new SessionHandler())); + return $request; + } + + private function user(int $id, string $token): User + { + $user = new User(); + $user->setId($id); + $user->setAccessToken($token); + return $user; + } + + public function testLoginStoresIdentityInSessionAndNotifiesListeners() + { + $module = new Module(); + $seen = []; + $module->on('user:login', function (User $user) use (&$seen) { + $seen[] = $user; + }); + + $user = $this->user(7, 'access-token-7'); + $response = $module->login($this->request(), $user); + + $this->assertSame(7, $_SESSION['catlab-user-id']); + $this->assertSame('access-token-7', $_SESSION['catlab-openid-access-token']); + $this->assertSame([ $user ], $seen); + + $this->assertInstanceOf(Response::class, $response); + $this->assertSame(302, $response->getStatus()); + } + + public function testLoginRedirectsToTheStoredReturnUrlOnce() + { + $_SESSION['post-login-redirect'] = 'https://app.example.com/after-login'; + $_SESSION['cancel-login-redirect'] = 'https://app.example.com/cancelled'; + + $module = new Module(); + $response = $module->login($this->request(), $this->user(7, 'tok')); + + $this->assertSame('https://app.example.com/after-login', $response->getHeaders()['Location']); + // Consumed: a second login must not bounce to the stale URL again. + $this->assertNull($_SESSION['post-login-redirect']); + $this->assertNull($_SESSION['cancel-login-redirect']); + + $again = $module->login($this->request(), $this->user(7, 'tok')); + $this->assertNotEquals('https://app.example.com/after-login', $again->getHeaders()['Location']); + } + + public function testLoginFallsBackToTheApplicationRoot() + { + $module = new Module(); + $response = $module->login($this->request(), $this->user(7, 'tok')); + + $this->assertSame('/', $response->getHeaders()['Location']); + } + + public function testLogoutClearsIdentityAndNotifiesListeners() + { + $_SESSION['catlab-user-id'] = 7; + $_SESSION['catlab-openid-access-token'] = 'access-token-7'; + + $module = new Module(); + $events = 0; + $module->on('user:logout', function () use (&$events) { + $events++; + }); + + $response = $module->logout($this->request()); + + $this->assertNull($_SESSION['catlab-user-id']); + $this->assertNull($_SESSION['catlab-openid-access-token']); + $this->assertSame(1, $events); + $this->assertSame(302, $response->getStatus()); + $this->assertSame('/', $response->getHeaders()['Location']); + } +} diff --git a/tests/UserModelTest.php b/tests/UserModelTest.php new file mode 100644 index 0000000..54a2003 --- /dev/null +++ b/tests/UserModelTest.php @@ -0,0 +1,135 @@ +mapper = RecordingUserMapper::register(); + $this->mapper->reset(); + } + + public function testMergeFromInputOnlyTakesTheUsername() + { + $user = new User(); + $user->setEmail('stored@example.com'); + $user->setSub('stored-sub'); + + $user->mergeFromInput([ + 'id' => 99, + 'username' => 'Provider Name', + 'email' => 'provider@example.com', + 'sub' => 'provider-sub', + ]); + + $this->assertSame('Provider Name', $user->getDisplayName()); + // Identity fields are owned by the login flow, not by the claims payload. + $this->assertSame('stored@example.com', $user->getEmail()); + $this->assertSame('stored-sub', $user->getSub()); + } + + public function testMergeFromInputKeepsDisplayNameWhenUsernameIsAbsent() + { + $user = new User(); + $user->setDisplayName('Existing'); + + $user->mergeFromInput([ 'email' => 'x@example.com' ]); + + $this->assertSame('Existing', $user->getDisplayName()); + } + + public function testShouldPingWhenNeverPinged() + { + $user = new User(); + $before = time(); + + $this->assertTrue($user->shouldPing()); + + $this->assertInstanceOf(DateTime::class, $user->getLastPing()); + $this->assertGreaterThanOrEqual($before, $user->getLastPing()->getTimestamp()); + $this->assertSame([ $user ], $this->mapper->pinged); + $this->assertSame([], $this->mapper->updated); + } + + public function testShouldPingWhenLastPingIsOlderThanTheInterval() + { + $user = new User(); + $stale = new DateTime('@' . (time() - $user->pingInterval - 60)); + $user->setLastPing($stale); + + $this->assertTrue($user->shouldPing()); + + $this->assertNotSame($stale, $user->getLastPing()); + $this->assertGreaterThan($stale->getTimestamp(), $user->getLastPing()->getTimestamp()); + $this->assertSame([ $user ], $this->mapper->pinged); + } + + public function testShouldNotPingWithinTheInterval() + { + $user = new User(); + $recent = new DateTime('@' . (time() - 60)); + $user->setLastPing($recent); + + $this->assertFalse($user->shouldPing()); + + $this->assertSame($recent, $user->getLastPing()); + $this->assertSame([], $this->mapper->pinged); + } + + public function testPingIntervalIsConfigurablePerInstance() + { + $user = new User(); + $user->pingInterval = 10; + $user->setLastPing(new DateTime('@' . (time() - 30))); + + $this->assertTrue($user->shouldPing()); + } + + public function testAnonymizeStripsPersonalDataAndPersists() + { + $user = new User(); + $user->setId(5); + $user->setEmail('person@example.com'); + $user->setDisplayName('Real Name'); + $user->setSub('sub-5'); + $user->setAccessToken('tok'); + + $user->anonymize(); + + $this->assertNull($user->getEmail()); + $this->assertNull($user->getDisplayName()); + // The link to the accounts server survives; only personal data goes. + $this->assertSame(5, $user->getId()); + $this->assertSame('sub-5', $user->getSub()); + $this->assertSame([ $user ], $this->mapper->updated); + } + + public function testGuestIsAnAnonymousUserWithoutId() + { + $guest = new Guest(); + $guest->setId(123); + $guest->setEmail('leak@example.com'); + $guest->setDisplayName('Leak'); + + $this->assertInstanceOf(User::class, $guest); + $this->assertInstanceOf(\Neuron\Interfaces\Models\Guest::class, $guest); + $this->assertNull($guest->getId()); + $this->assertSame('Guest', $guest->getDisplayName()); + $this->assertSame('nobody@nowhere.com', $guest->getEmail()); + } +}