From 1ee52e899f97392dee74de85e5e0f52c67a3fb82 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Fri, 27 Sep 2024 12:18:41 +0200 Subject: [PATCH 1/7] fix: remove overloaded properties of OpenIDConnectClient --- composer.json | 2 +- src/OpenIDConnectClient.php | 9 --------- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/composer.json b/composer.json index 09fed59..397a6f1 100644 --- a/composer.json +++ b/composer.json @@ -31,7 +31,7 @@ }, "require": { "php": ">=8.1", - "jumbojett/openid-connect-php": "^1.0.0", + "jumbojett/openid-connect-php": "^1.0.2", "guzzlehttp/guzzle": "^7.5", "web-token/jwt-library": "^3.4" }, diff --git a/src/OpenIDConnectClient.php b/src/OpenIDConnectClient.php index 856117a..d6924fd 100644 --- a/src/OpenIDConnectClient.php +++ b/src/OpenIDConnectClient.php @@ -20,15 +20,6 @@ class OpenIDConnectClient extends \Jumbojett\OpenIDConnectClient { protected ?JweDecryptInterface $jweDecrypter; protected ?OpenIDConfiguration $openIDConfiguration; - /** - * @var int|null Response code from the server - */ - protected ?int $responseCode; - - /** - * @var string|null Content type from the server - */ - private ?string $responseContentType; public function __construct( ?string $providerUrl = null, From 0ffd97790e546b1cbd26dbd1a58aa37e0141fc82 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Mon, 30 Sep 2024 20:57:33 +0200 Subject: [PATCH 2/7] fix: remove unneeded excluded path of phpstan config --- phpstan.neon | 2 -- 1 file changed, 2 deletions(-) diff --git a/phpstan.neon b/phpstan.neon index 8baaf5e..5ba2d4f 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -2,7 +2,5 @@ parameters: paths: - src level: 8 - excludePaths: - - src/BaseOpenIDConnectClient.php includes: - phar://phpstan.phar/conf/bleedingEdge.neon From 5216e359ccda2103836dda50fa10967a0edf03d7 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Mon, 30 Sep 2024 21:00:24 +0200 Subject: [PATCH 3/7] test: add test for state does not match exception and added needed sub claim --- .../LoginControllerResponseTest.php | 59 ++++++++++++++++--- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php index 790a004..b98a4a7 100644 --- a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php +++ b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php @@ -5,12 +5,15 @@ namespace MinVWS\OpenIDConnectLaravel\Tests\Feature\Http\Controllers; use Illuminate\Http\Client\Request; +use Illuminate\Http\Response; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Session; use Illuminate\Testing\TestResponse; +use Jumbojett\OpenIDConnectClientException; use MinVWS\OpenIDConnectLaravel\OpenIDConfiguration\OpenIDConfiguration; use MinVWS\OpenIDConnectLaravel\OpenIDConfiguration\OpenIDConfigurationLoader; +use MinVWS\OpenIDConnectLaravel\Services\ExceptionHandlerInterface; use MinVWS\OpenIDConnectLaravel\Tests\TestCase; use Mockery; @@ -129,11 +132,53 @@ public function codeChallengeMethodProvider(): array ]; } - public function testTokenSignedWithClientSecret(): void + public function testStateDoesNotMatch(): void + { + Http::fake([ + // Token requested by OpenIDConnectClient::authenticate() function. + // Currently needed because the package requests the token endpoint before checking the state. + // TODO: Remove if https://github.com/jumbojett/OpenID-Connect-PHP/pull/447 is merged. + 'https://provider.rdobeheer.nl/token' => Http::response([ + 'access_token' => 'access-token-from-token-endpoint', + 'id_token' => 'some-valid-token-not-needed-for-this-state-check', + 'token_type' => 'Bearer', + 'expires_in' => 3600, + ]), + ]); + + // Set OIDC config + $this->mockOpenIDConfigurationLoader(); + Config::set('oidc.issuer', 'https://provider.rdobeheer.nl'); + Config::set('oidc.client_id', 'test-client-id'); + Config::set('oidc.client_secret', 'the-secret-client-secret'); + + // Mock LoginResponseHandlerInterface to check handleExceptionWhileAuthenticate is called. + $mock = Mockery::mock(ExceptionHandlerInterface::class); + $mock + ->shouldReceive('handleExceptionWhileAuthenticate') + ->withArgs(function (OpenIDConnectClientException $e) { + return $e->getMessage() === 'Unable to determine state'; + }) + ->once() + ->andReturn(new Response('', 400)); + $this->app->instance(ExceptionHandlerInterface::class, $mock); + + // Set the current state, which is usually generated and saved in the session before login, + // and sent to the issuer during the login redirect. + Session::put('openid_connect_state', 'some-state'); + + // We simulate here that the state does not match with the state in the session. + // And that the repsonse of ExceptionHandlerInterface is returned. + $response = $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'a-different-state']); + $response->assertStatus(400); + } + + public function testIdTokenSignedWithClientSecret(): void { $idToken = generateJwt([ "iss" => "https://provider.rdobeheer.nl", "aud" => 'test-client-id', + "sub" => 'test-subject', ], 'the-secret-client-secret'); Http::fake([ @@ -146,7 +191,7 @@ public function testTokenSignedWithClientSecret(): void ]), // User info requested by OpenIDConnectClient::requestUserInfo() function. 'https://provider.rdobeheer.nl/userinfo?schema=openid' => Http::response([ - 'email' => 'teste@rdobeheer.nl', + 'email' => 'tester@rdobeheer.nl', ]), ]); @@ -157,8 +202,8 @@ public function testTokenSignedWithClientSecret(): void Config::set('oidc.client_id', 'test-client-id'); Config::set('oidc.client_secret', 'the-secret-client-secret'); - // Set current state, normally this is generated before logging in and send - // to the issuer, when the user is redirected for login. + // Set the current state, which is usually generated and saved in the session before login, + // and sent to the issuer during the login redirect. Session::put('openid_connect_state', 'some-state'); // We simulate here that the user now comes back after successful login at issuer. @@ -166,7 +211,7 @@ public function testTokenSignedWithClientSecret(): void $response->assertStatus(200); $response->assertJson([ 'userInfo' => [ - 'email' => 'teste@rdobeheer.nl', + 'email' => 'tester@rdobeheer.nl', ] ]); @@ -234,8 +279,8 @@ public function testTokenSignedWithPrivateKey(): void [$key, $keyResource] = generateOpenSSLKey(); Config::set('oidc.client_authentication.signing_private_key_path', stream_get_meta_data($keyResource)['uri']); - // Set current state, normally this is generated before logging in and send - // to the issuer, when the user is redirected for login. + // Set the current state, which is usually generated and saved in the session before login, + // and sent to the issuer during the login redirect. Session::put('openid_connect_state', 'some-state'); // We simulate here that the user now comes back after successful login at issuer. From 42eb9dae349c75bbdcf59668ceb57a24aeb57e79 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Mon, 30 Sep 2024 21:19:17 +0200 Subject: [PATCH 4/7] test: add test for state does not match exception and added needed sub claim --- .../Http/Controllers/LoginControllerResponseTest.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php index b98a4a7..f380f5a 100644 --- a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php +++ b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php @@ -5,7 +5,6 @@ namespace MinVWS\OpenIDConnectLaravel\Tests\Feature\Http\Controllers; use Illuminate\Http\Client\Request; -use Illuminate\Http\Response; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Session; @@ -159,8 +158,7 @@ public function testStateDoesNotMatch(): void ->withArgs(function (OpenIDConnectClientException $e) { return $e->getMessage() === 'Unable to determine state'; }) - ->once() - ->andReturn(new Response('', 400)); + ->once(); $this->app->instance(ExceptionHandlerInterface::class, $mock); // Set the current state, which is usually generated and saved in the session before login, @@ -168,9 +166,7 @@ public function testStateDoesNotMatch(): void Session::put('openid_connect_state', 'some-state'); // We simulate here that the state does not match with the state in the session. - // And that the repsonse of ExceptionHandlerInterface is returned. - $response = $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'a-different-state']); - $response->assertStatus(400); + $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'a-different-state']); } public function testIdTokenSignedWithClientSecret(): void From c683a4ba1979922ba628a2e6554202c11f277067 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Mon, 30 Sep 2024 21:24:37 +0200 Subject: [PATCH 5/7] test: add test for invalid signed id token --- .../LoginControllerResponseTest.php | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php index f380f5a..8ca1a62 100644 --- a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php +++ b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php @@ -253,6 +253,72 @@ public function testIdTokenSignedWithClientSecret(): void }); } + public function testIdTokenSignedWithIncorrectClientSecret(): void + { + $idToken = generateJwt([ + "iss" => "https://provider.rdobeheer.nl", + "aud" => 'test-client-id', + "sub" => 'test-subject', + ], 'not-the-secret-client-secret'); + + Http::fake([ + // Token requested by OpenIDConnectClient::authenticate() function. + 'https://provider.rdobeheer.nl/token' => Http::response([ + 'access_token' => 'access-token-from-token-endpoint', + 'id_token' => $idToken, + 'token_type' => 'Bearer', + 'expires_in' => 3600, + ]), + ]); + + // Set OIDC config + $this->mockOpenIDConfigurationLoader(); + Config::set('oidc.issuer', 'https://provider.rdobeheer.nl'); + Config::set('oidc.client_id', 'test-client-id'); + Config::set('oidc.client_secret', 'the-secret-client-secret'); + + // Mock LoginResponseHandlerInterface to check handleExceptionWhileAuthenticate is called. + $mock = Mockery::mock(ExceptionHandlerInterface::class); + $mock + ->shouldReceive('handleExceptionWhileAuthenticate') + ->withArgs(function (OpenIDConnectClientException $e) { + return $e->getMessage() === 'Unable to verify signature'; + }) + ->once(); + $this->app->instance(ExceptionHandlerInterface::class, $mock); + + // Set the current state, which is usually generated and saved in the session before login, + // and sent to the issuer during the login redirect. + Session::put('openid_connect_state', 'some-state'); + + // We simulate here that the user now comes back after successful login at issuer. + $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'some-state']); + + Http::assertSentCount(1); + Http::assertSentInOrder([ + 'https://provider.rdobeheer.nl/token', + ]); + Http::assertSent(function (Request $request) { + if ($request->url() === 'https://provider.rdobeheer.nl/token') { + $this->assertSame( + expected: 'POST', + actual: $request->method(), + ); + $this->assertSame( + expected: 'grant_type=authorization_code' + . '&code=some-code' + . '&redirect_uri=http%3A%2F%2Flocalhost%2Foidc%2Flogin' + . '&client_id=test-client-id' + . '&client_secret=the-secret-client-secret', + actual: $request->body(), + ); + return true; + } + return true; + }); + } + + public function testTokenSignedWithPrivateKey(): void { Http::fake([ From 6d1efec2a874612b56e5b8ae342caac6411bdb37 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Mon, 30 Sep 2024 21:26:56 +0200 Subject: [PATCH 6/7] test: add test for client secret signed user info --- .../LoginControllerResponseTest.php | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php index 8ca1a62..a49132b 100644 --- a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php +++ b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php @@ -318,6 +318,100 @@ public function testIdTokenSignedWithIncorrectClientSecret(): void }); } + public function testIdTokenAndUserinfoSignedWithClientSecret(): void + { + $idToken = generateJwt([ + "iss" => "https://provider.rdobeheer.nl", + "aud" => 'test-client-id', + "sub" => 'test-subject', + ], 'the-secret-client-secret'); + + $signedUserInfo = generateJwt([ + "iss" => "https://provider.rdobeheer.nl", + "aud" => 'test-client-id', + "sub" => 'test-subject', + "email" => 'tester@rdobeheer.nl', + ], 'the-secret-client-secret'); + + Http::fake([ + // Token requested by OpenIDConnectClient::authenticate() function. + 'https://provider.rdobeheer.nl/token' => Http::response([ + 'access_token' => 'access-token-from-token-endpoint', + 'id_token' => $idToken, + 'token_type' => 'Bearer', + 'expires_in' => 3600, + ]), + // User info requested by OpenIDConnectClient::requestUserInfo() function. + 'https://provider.rdobeheer.nl/userinfo?schema=openid' => Http::response( + body: $signedUserInfo, + status: 200, + headers: [ + 'Content-Type' => 'application/jwt', + ] + ), + ]); + + // Set OIDC config + $this->mockOpenIDConfigurationLoader(); + + Config::set('oidc.issuer', 'https://provider.rdobeheer.nl'); + Config::set('oidc.client_id', 'test-client-id'); + Config::set('oidc.client_secret', 'the-secret-client-secret'); + + // Set the current state, which is usually generated and saved in the session before login, + // and sent to the issuer during the login redirect. + Session::put('openid_connect_state', 'some-state'); + + // We simulate here that the user now comes back after successful login at issuer. + $response = $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'some-state']); + $response->assertStatus(200); + $response->assertJson([ + 'userInfo' => [ + 'email' => 'tester@rdobeheer.nl', + ] + ]); + + $this->assertEmpty(session('openid_connect_state')); + $this->assertEmpty(session('openid_connect_nonce')); + + Http::assertSentCount(2); + Http::assertSentInOrder([ + 'https://provider.rdobeheer.nl/token', + 'https://provider.rdobeheer.nl/userinfo?schema=openid', + ]); + Http::assertSent(function (Request $request) { + if ($request->url() === 'https://provider.rdobeheer.nl/token') { + $this->assertSame( + expected: 'POST', + actual: $request->method(), + ); + $this->assertSame( + expected: 'grant_type=authorization_code' + . '&code=some-code' + . '&redirect_uri=http%3A%2F%2Flocalhost%2Foidc%2Flogin' + . '&client_id=test-client-id' + . '&client_secret=the-secret-client-secret', + actual: $request->body(), + ); + return true; + } + + if ($request->url() === 'https://provider.rdobeheer.nl/userinfo?schema=openid') { + $this->assertSame( + expected: 'GET', + actual: $request->method(), + ); + $this->assertSame( + expected: [ + 'Bearer access-token-from-token-endpoint' + ], + actual: $request->header('Authorization'), + ); + } + + return true; + }); + } public function testTokenSignedWithPrivateKey(): void { From 6ba58e55a1742339529807d6359df6318d0a3700 Mon Sep 17 00:00:00 2001 From: Rick Lambrechts Date: Mon, 30 Sep 2024 21:45:46 +0200 Subject: [PATCH 7/7] test: add test to test exception when sub claims differ. --- .../LoginControllerResponseTest.php | 111 +++++++++++++++++- 1 file changed, 105 insertions(+), 6 deletions(-) diff --git a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php index a49132b..fe158d6 100644 --- a/tests/Feature/Http/Controllers/LoginControllerResponseTest.php +++ b/tests/Feature/Http/Controllers/LoginControllerResponseTest.php @@ -152,14 +152,14 @@ public function testStateDoesNotMatch(): void Config::set('oidc.client_secret', 'the-secret-client-secret'); // Mock LoginResponseHandlerInterface to check handleExceptionWhileAuthenticate is called. - $mock = Mockery::mock(ExceptionHandlerInterface::class); - $mock + $mockExceptionHandler = Mockery::mock(ExceptionHandlerInterface::class); + $mockExceptionHandler ->shouldReceive('handleExceptionWhileAuthenticate') ->withArgs(function (OpenIDConnectClientException $e) { return $e->getMessage() === 'Unable to determine state'; }) ->once(); - $this->app->instance(ExceptionHandlerInterface::class, $mock); + $this->app->instance(ExceptionHandlerInterface::class, $mockExceptionHandler); // Set the current state, which is usually generated and saved in the session before login, // and sent to the issuer during the login redirect. @@ -278,14 +278,14 @@ public function testIdTokenSignedWithIncorrectClientSecret(): void Config::set('oidc.client_secret', 'the-secret-client-secret'); // Mock LoginResponseHandlerInterface to check handleExceptionWhileAuthenticate is called. - $mock = Mockery::mock(ExceptionHandlerInterface::class); - $mock + $mockExceptionHandler = Mockery::mock(ExceptionHandlerInterface::class); + $mockExceptionHandler ->shouldReceive('handleExceptionWhileAuthenticate') ->withArgs(function (OpenIDConnectClientException $e) { return $e->getMessage() === 'Unable to verify signature'; }) ->once(); - $this->app->instance(ExceptionHandlerInterface::class, $mock); + $this->app->instance(ExceptionHandlerInterface::class, $mockExceptionHandler); // Set the current state, which is usually generated and saved in the session before login, // and sent to the issuer during the login redirect. @@ -413,6 +413,105 @@ public function testIdTokenAndUserinfoSignedWithClientSecret(): void }); } + public function testSubClaimIdTokenDoesNotEqualsSubClaimUserinfo(): void + { + $idToken = generateJwt([ + "iss" => "https://provider.rdobeheer.nl", + "aud" => 'test-client-id', + "sub" => 'test-subject', + ], 'the-secret-client-secret'); + + $signedUserInfo = generateJwt([ + "iss" => "https://provider.rdobeheer.nl", + "aud" => 'test-client-id', + "sub" => 'different-subject', + "email" => 'tester@rdobeheer.nl', + ], 'the-secret-client-secret'); + + Http::fake([ + // Token requested by OpenIDConnectClient::authenticate() function. + 'https://provider.rdobeheer.nl/token' => Http::response([ + 'access_token' => 'access-token-from-token-endpoint', + 'id_token' => $idToken, + 'token_type' => 'Bearer', + 'expires_in' => 3600, + ]), + // User info requested by OpenIDConnectClient::requestUserInfo() function. + 'https://provider.rdobeheer.nl/userinfo?schema=openid' => Http::response( + body: $signedUserInfo, + status: 200, + headers: [ + 'Content-Type' => 'application/jwt', + ] + ), + ]); + + // Set OIDC config + $this->mockOpenIDConfigurationLoader(); + + Config::set('oidc.issuer', 'https://provider.rdobeheer.nl'); + Config::set('oidc.client_id', 'test-client-id'); + Config::set('oidc.client_secret', 'the-secret-client-secret'); + + // Mock LoginResponseHandlerInterface to check handleExceptionWhileRequestUserInfo is called. + $mockExceptionHandler = Mockery::mock(ExceptionHandlerInterface::class); + $mockExceptionHandler + ->shouldReceive('handleExceptionWhileRequestUserInfo') + ->withArgs(function (OpenIDConnectClientException $e) { + return $e->getMessage() === 'Invalid JWT signature'; + }) + ->once(); + $this->app->instance(ExceptionHandlerInterface::class, $mockExceptionHandler); + + // Set the current state, which is usually generated and saved in the session before login, + // and sent to the issuer during the login redirect. + Session::put('openid_connect_state', 'some-state'); + + // We simulate here that the user now comes back after successful login at issuer. + $this->getRoute('oidc.login', ['code' => 'some-code', 'state' => 'some-state']); + + $this->assertEmpty(session('openid_connect_state')); + $this->assertEmpty(session('openid_connect_nonce')); + + Http::assertSentCount(2); + Http::assertSentInOrder([ + 'https://provider.rdobeheer.nl/token', + 'https://provider.rdobeheer.nl/userinfo?schema=openid', + ]); + Http::assertSent(function (Request $request) { + if ($request->url() === 'https://provider.rdobeheer.nl/token') { + $this->assertSame( + expected: 'POST', + actual: $request->method(), + ); + $this->assertSame( + expected: 'grant_type=authorization_code' + . '&code=some-code' + . '&redirect_uri=http%3A%2F%2Flocalhost%2Foidc%2Flogin' + . '&client_id=test-client-id' + . '&client_secret=the-secret-client-secret', + actual: $request->body(), + ); + return true; + } + + if ($request->url() === 'https://provider.rdobeheer.nl/userinfo?schema=openid') { + $this->assertSame( + expected: 'GET', + actual: $request->method(), + ); + $this->assertSame( + expected: [ + 'Bearer access-token-from-token-endpoint' + ], + actual: $request->header('Authorization'), + ); + } + + return true; + }); + } + public function testTokenSignedWithPrivateKey(): void { Http::fake([