diff --git a/src/Factories/Grant/ImplicitGrantFactory.php b/src/Factories/Grant/ImplicitGrantFactory.php index 47d5ec6b..36b432c7 100644 --- a/src/Factories/Grant/ImplicitGrantFactory.php +++ b/src/Factories/Grant/ImplicitGrantFactory.php @@ -21,6 +21,7 @@ use SimpleSAML\Module\oidc\Server\Grants\ImplicitGrant; use SimpleSAML\Module\oidc\Server\RequestRules\RequestRulesManager; use SimpleSAML\Module\oidc\Services\IdTokenBuilder; +use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; class ImplicitGrantFactory @@ -32,6 +33,7 @@ public function __construct( private readonly AccessTokenRepository $accessTokenRepository, private readonly RequestParamsResolver $requestParamsResolver, private readonly AccessTokenEntityFactory $accessTokenEntityFactory, + private readonly LoggerService $loggerService, ) { } @@ -44,6 +46,7 @@ public function build(): ImplicitGrant $this->requestRulesManager, $this->requestParamsResolver, $this->accessTokenEntityFactory, + $this->loggerService, ); } } diff --git a/src/Factories/Grant/RefreshTokenGrantFactory.php b/src/Factories/Grant/RefreshTokenGrantFactory.php index 388b4086..5560f343 100644 --- a/src/Factories/Grant/RefreshTokenGrantFactory.php +++ b/src/Factories/Grant/RefreshTokenGrantFactory.php @@ -20,6 +20,7 @@ use SimpleSAML\Module\oidc\Repositories\RefreshTokenRepository; use SimpleSAML\Module\oidc\Server\Grants\RefreshTokenGrant; use SimpleSAML\Module\oidc\Server\TokenIssuers\RefreshTokenIssuer; +use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\AuthenticatedOAuth2ClientResolver; class RefreshTokenGrantFactory @@ -30,6 +31,7 @@ public function __construct( private readonly AccessTokenEntityFactory $accessTokenEntityFactory, private readonly RefreshTokenIssuer $refreshTokenIssuer, private readonly AuthenticatedOAuth2ClientResolver $authenticatedOAuth2ClientResolver, + private readonly LoggerService $loggerService, ) { } @@ -40,6 +42,7 @@ public function build(): RefreshTokenGrant $this->accessTokenEntityFactory, $this->refreshTokenIssuer, $this->authenticatedOAuth2ClientResolver, + $this->loggerService, ); $refreshTokenGrant->setRefreshTokenTTL($this->moduleConfig->getRefreshTokenDuration()); diff --git a/src/Server/Grants/AuthCodeGrant.php b/src/Server/Grants/AuthCodeGrant.php index 7b1ea5df..f3830d34 100644 --- a/src/Server/Grants/AuthCodeGrant.php +++ b/src/Server/Grants/AuthCodeGrant.php @@ -220,6 +220,10 @@ public function completeOidcAuthorizationRequest( ?? $this->getAuthorizationRequestClientRedirectUri($authorizationRequest); if ($authorizationRequest->isAuthorizationApproved() !== true) { + $this->loggerService->notice( + 'Authorization request denied by the user.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); // The user denied the client, redirect them back with an error throw OidcServerException::accessDenied( 'The user denied the request', @@ -239,6 +243,11 @@ public function completeOidcAuthorizationRequest( $authorizationRequest, ); + $this->loggerService->notice( + 'Authorization approved; authorization code issued.', + ['client_id' => $authCode->getClient()->getIdentifier(), 'auth_code_id' => $authCode->getIdentifier()], + ); + $payload = [ 'client_id' => $authCode->getClient()->getIdentifier(), 'redirect_uri' => $authCode->getRedirectUri(), @@ -370,7 +379,7 @@ public function respondToAccessTokenRequest( $encryptedAuthCode = $this->getRequestParameter('code', $request); if ($encryptedAuthCode === null) { - $this->loggerService->debug('Code parameter not provided.'); + $this->loggerService->notice('Token request rejected: `code` parameter not provided.'); throw OAuthServerException::invalidRequest('code'); } @@ -381,10 +390,15 @@ public function respondToAccessTokenRequest( */ $authCodePayload = json_decode($this->decrypt($encryptedAuthCode), null, 512, JSON_THROW_ON_ERROR); } catch (LogicException $e) { + $this->loggerService->warning( + 'Token request rejected: could not decrypt the authorization code.', + ['exception' => $e->getMessage()], + ); throw OAuthServerException::invalidRequest('code', 'Cannot decrypt the authorization code', $e); } if (!property_exists($authCodePayload, 'auth_code_id')) { + $this->loggerService->notice('Token request rejected: authorization code is malformed (no auth_code_id).'); throw OAuthServerException::invalidRequest('code', 'Authorization code malformed'); } @@ -395,6 +409,10 @@ public function respondToAccessTokenRequest( $storedAuthCodeEntity = $this->authCodeRepository->findById($authCodePayload->auth_code_id); if ($storedAuthCodeEntity === null) { + $this->loggerService->notice( + 'Token request rejected: authorization code not found in storage.', + ['auth_code_id' => $authCodePayload->auth_code_id], + ); throw OAuthServerException::invalidGrant('Authorization code not found'); } @@ -441,10 +459,22 @@ public function respondToAccessTokenRequest( // client_id below. If an authentication scheme for non-registered clients is introduced later (e.g. // attestation), this can be relaxed the same way it was for registered clients. if (! $clientId) { + $this->loggerService->notice( + 'Token request rejected: generic (non-registered) client did not provide required `client_id`.', + ['auth_code_id' => $authCodePayload->auth_code_id], + ); throw OidcServerException::invalidRequest('client_id'); } if ($clientId !== $storedAuthCodeEntity->getBoundClientId()) { + $this->loggerService->warning( + 'Token request rejected: `client_id` does not match the one the authorization code was bound to.', + [ + 'auth_code_id' => $authCodePayload->auth_code_id, + 'client_id' => $clientId, + 'bound_client_id' => $storedAuthCodeEntity->getBoundClientId(), + ], + ); throw OAuthServerException::invalidGrant('Authorization code not intended for this client_id.'); } @@ -455,10 +485,24 @@ public function respondToAccessTokenRequest( ); if (! $redirectUri) { + $this->loggerService->notice( + 'Token request rejected: generic (non-registered) client did not provide required `redirect_uri`.', + ['auth_code_id' => $authCodePayload->auth_code_id, 'client_id' => $clientId], + ); throw OidcServerException::invalidRequest(ParamsEnum::RedirectUri->value); } if ($redirectUri !== $storedAuthCodeEntity->getBoundRedirectUri()) { + $this->loggerService->warning( + 'Token request rejected: `redirect_uri` does not match the one the authorization code ' . + 'was bound to.', + [ + 'auth_code_id' => $authCodePayload->auth_code_id, + 'client_id' => $clientId, + 'redirect_uri' => $redirectUri, + 'bound_redirect_uri' => $storedAuthCodeEntity->getBoundRedirectUri(), + ], + ); throw OAuthServerException::invalidGrant('Authorization code not intended for this redirect_uri.'); } @@ -490,6 +534,13 @@ public function respondToAccessTokenRequest( $registeredGrantTypes !== [] && !in_array(GrantTypesEnum::AuthorizationCode->value, $registeredGrantTypes, true) ) { + $this->loggerService->warning( + 'Token request rejected: client is not authorized to use the authorization_code grant type.', + [ + 'client_id' => $client->getIdentifier(), + 'registered_grant_types' => $registeredGrantTypes, + ], + ); throw OidcServerException::unauthorizedClient( 'The client is not authorized to use the authorization_code grant type.', ); @@ -516,6 +567,11 @@ public function respondToAccessTokenRequest( } if (empty($utilizedClientAuthenticationParams)) { + $this->loggerService->warning( + 'Token request rejected: client authentication not performed (no client authentication ' . + 'method and no PKCE code_verifier presented).', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); throw OidcServerException::accessDenied('Client authentication not performed.'); } @@ -547,6 +603,11 @@ public function respondToAccessTokenRequest( // If a code challenge isn't present but a code verifier is, reject the request to block PKCE downgrade attack if (empty($authCodePayload->code_challenge) && $codeVerifier !== null) { + $this->loggerService->warning( + 'Token request rejected: `code_verifier` received but the authorization request had no ' . + '`code_challenge` (possible PKCE downgrade attempt).', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); throw OAuthServerException::invalidRequest( 'code_challenge', 'code_verifier received when no code_challenge is present', @@ -556,6 +617,11 @@ public function respondToAccessTokenRequest( // Validate code challenge if (!empty($authCodePayload->code_challenge)) { if ($codeVerifier === null) { + $this->loggerService->notice( + 'Token request rejected: `code_verifier` is missing while a `code_challenge` was used ' . + 'in the authorization request.', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); throw OAuthServerException::invalidRequest('code_verifier'); } @@ -570,8 +636,11 @@ public function respondToAccessTokenRequest( // } if (property_exists($authCodePayload, 'code_challenge_method')) { - if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) { - $codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method]; + $codeChallengeMethod = isset($authCodePayload->code_challenge_method) ? + $authCodePayload->code_challenge_method : + ''; + if (isset($this->codeChallengeVerifiers[$codeChallengeMethod])) { + $codeChallengeVerifier = $this->codeChallengeVerifiers[$codeChallengeMethod]; if ( $codeChallengeVerifier->verifyCodeChallenge( @@ -579,13 +648,30 @@ public function respondToAccessTokenRequest( $authCodePayload->code_challenge, ) === false ) { + $this->loggerService->warning( + 'Token request rejected: PKCE `code_verifier` failed verification against the ' . + 'stored `code_challenge`.', + [ + 'client_id' => $client->getIdentifier(), + 'auth_code_id' => $authCodePayload->auth_code_id, + 'code_challenge_method' => $codeChallengeMethod, + ], + ); throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.'); } } else { + $this->loggerService->error( + 'Token request failed: unsupported code challenge method stored on authorization code.', + [ + 'client_id' => $client->getIdentifier(), + 'auth_code_id' => $authCodePayload->auth_code_id, + 'code_challenge_method' => $codeChallengeMethod, + ], + ); throw OAuthServerException::serverError( sprintf( 'Unsupported code challenge method `%s`', - ($authCodePayload->code_challenge_method ?? ''), + $codeChallengeMethod, ), ); } @@ -661,6 +747,11 @@ public function respondToAccessTokenRequest( // Revoke used auth code $this->authCodeRepository->revokeAuthCode($authCodePayload->auth_code_id); + $this->loggerService->notice( + 'Authorization code redeemed; access token issued.', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); + return $responseType; } @@ -693,10 +784,19 @@ protected function validateAuthorizationCode( } if (time() > $authCodePayload->expire_time) { + $this->loggerService->notice( + 'Token request rejected: authorization code has expired.', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); throw OAuthServerException::invalidGrant('Authorization code has expired'); } if ($storedAuthCodeEntity->isRevoked()) { + $this->loggerService->warning( + 'Token request rejected: authorization code has been revoked (likely reused). Revoking all ' . + 'related access and refresh tokens.', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); // Code is reused, all related tokens must be revoked, per https://tools.ietf.org/html/rfc6749#section-4.1.2 $this->accessTokenRepository->revokeByAuthCodeId($authCodePayload->auth_code_id); $this->refreshTokenRepository->revokeByAuthCodeId($authCodePayload->auth_code_id); @@ -704,16 +804,37 @@ protected function validateAuthorizationCode( } if ($authCodePayload->client_id !== $client->getIdentifier()) { + $this->loggerService->warning( + 'Token request rejected: authorization code was not issued to the authenticated client.', + [ + 'client_id' => $client->getIdentifier(), + 'auth_code_client_id' => $authCodePayload->client_id, + 'auth_code_id' => $authCodePayload->auth_code_id, + ], + ); throw OAuthServerException::invalidRequest('code', 'Authorization code was not issued to this client'); } // The redirect URI is required in this request $redirectUri = $this->getRequestParameter('redirect_uri', $request); if (empty($authCodePayload->redirect_uri) === false && $redirectUri === null) { + $this->loggerService->notice( + 'Token request rejected: `redirect_uri` parameter is required but was not provided.', + ['client_id' => $client->getIdentifier(), 'auth_code_id' => $authCodePayload->auth_code_id], + ); throw OAuthServerException::invalidRequest('redirect_uri'); } if ($authCodePayload->redirect_uri !== $redirectUri) { + $this->loggerService->warning( + 'Token request rejected: `redirect_uri` does not match the one from the authorization request.', + [ + 'client_id' => $client->getIdentifier(), + 'auth_code_id' => $authCodePayload->auth_code_id, + 'redirect_uri' => $redirectUri, + 'authorization_redirect_uri' => $authCodePayload->redirect_uri, + ], + ); throw OAuthServerException::invalidRequest( 'redirect_uri', 'Invalid redirect URI or not the same as in authorization request', diff --git a/src/Server/Grants/ImplicitGrant.php b/src/Server/Grants/ImplicitGrant.php index 70437f01..7690c99c 100644 --- a/src/Server/Grants/ImplicitGrant.php +++ b/src/Server/Grants/ImplicitGrant.php @@ -38,6 +38,7 @@ use SimpleSAML\Module\oidc\Server\RequestTypes\AuthorizationRequest; use SimpleSAML\Module\oidc\Server\ResponseModes\FragmentResponseMode; use SimpleSAML\Module\oidc\Services\IdTokenBuilder; +use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; use SimpleSAML\OpenID\Codebooks\HttpMethodsEnum; @@ -58,6 +59,7 @@ public function __construct( protected RequestRulesManager $requestRulesManager, protected RequestParamsResolver $requestParamsResolver, AccessTokenEntityFactory $accessTokenEntityFactory, + protected LoggerService $loggerService, ) { parent::__construct($accessTokenTTL); @@ -105,6 +107,10 @@ public function completeAuthorizationRequest( return $this->completeOidcAuthorizationRequest($authorizationRequest); } + $this->loggerService->error( + 'Implicit grant failed: unexpected authorization request type.', + ['type' => $authorizationRequest::class], + ); throw new LogicException('Unexpected OAuth2AuthorizationRequest type.'); } @@ -200,12 +206,20 @@ private function completeOidcAuthorizationRequest(AuthorizationRequest $authoriz $user = $authorizationRequest->getUser(); if ($user instanceof UserEntity === false) { + $this->loggerService->error( + 'Implicit grant failed: no authenticated user set on the authorization request.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); throw new LogicException('An instance of UserEntityInterface should be set on the AuthorizationRequest'); } $redirectUrl = $this->getRedirectUrl($authorizationRequest); if ($authorizationRequest->isAuthorizationApproved() !== true) { + $this->loggerService->notice( + 'Implicit grant: authorization request denied by the user.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); throw OidcServerException::accessDenied( 'The user denied the request', $redirectUrl, @@ -237,9 +251,18 @@ private function completeOidcAuthorizationRequest(AuthorizationRequest $authoriz ); if ($accessToken instanceof EntityStringRepresentationInterface === false) { + $this->loggerService->error( + 'Implicit grant failed: issued access token does not implement ' . + EntityStringRepresentationInterface::class . '.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); throw new RuntimeException('AccessToken must implement ' . EntityStringRepresentationInterface::class); } if ($accessToken instanceof AccessTokenEntity === false) { + $this->loggerService->error( + 'Implicit grant failed: issued access token is not an instance of ' . AccessTokenEntity::class . '.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); throw new RuntimeException('AccessToken must be ' . AccessTokenEntity::class); } @@ -271,6 +294,11 @@ private function completeOidcAuthorizationRequest(AuthorizationRequest $authoriz $responseParams, ); + $this->loggerService->notice( + 'Implicit grant: authorization approved; ID token issued.', + ['client_id' => $authorizationRequest->getClient()->getIdentifier()], + ); + return $response; } diff --git a/src/Server/Grants/RefreshTokenGrant.php b/src/Server/Grants/RefreshTokenGrant.php index 813b7b87..05a4ff3f 100644 --- a/src/Server/Grants/RefreshTokenGrant.php +++ b/src/Server/Grants/RefreshTokenGrant.php @@ -18,6 +18,7 @@ use SimpleSAML\Module\oidc\Server\Exceptions\OidcServerException; use SimpleSAML\Module\oidc\Server\Grants\Traits\IssueAccessTokenTrait; use SimpleSAML\Module\oidc\Server\TokenIssuers\RefreshTokenIssuer; +use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\AuthenticatedOAuth2ClientResolver; use function is_null; @@ -36,6 +37,7 @@ public function __construct( AccessTokenEntityFactory $accessTokenEntityFactory, protected readonly RefreshTokenIssuer $refreshTokenIssuer, protected readonly AuthenticatedOAuth2ClientResolver $authenticatedOAuth2ClientResolver, + protected readonly LoggerService $loggerService, ) { parent::__construct($refreshTokenRepository); $this->accessTokenEntityFactory = $accessTokenEntityFactory; @@ -60,6 +62,10 @@ protected function validateClient(ServerRequestInterface $request): ClientEntity $resolvedClientAuthenticationMethod = $this->authenticatedOAuth2ClientResolver->forAnySupportedMethod($request); if ($resolvedClientAuthenticationMethod === null) { + $this->loggerService->warning( + 'Refresh token request rejected: client authentication failed (no supported client ' . + 'authentication method could be resolved).', + ); $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); throw OAuthServerException::invalidClient($request); } @@ -75,6 +81,10 @@ protected function validateOldRefreshToken(ServerRequestInterface $request, stri { $encryptedRefreshToken = $this->getRequestParameter('refresh_token', $request); if (is_null($encryptedRefreshToken)) { + $this->loggerService->notice( + 'Refresh token request rejected: `refresh_token` parameter not provided.', + ['client_id' => $clientId], + ); throw OidcServerException::invalidGrant('Failed to verify `refresh_token`'); } @@ -82,22 +92,45 @@ protected function validateOldRefreshToken(ServerRequestInterface $request, stri try { $refreshToken = $this->decrypt($encryptedRefreshToken); } catch (Exception $e) { + $this->loggerService->warning( + 'Refresh token request rejected: could not decrypt the refresh token.', + ['client_id' => $clientId, 'exception' => $e->getMessage()], + ); throw OidcServerException::invalidRefreshToken('Cannot decrypt the refresh token', $e); } $refreshTokenData = json_decode($refreshToken, true, 512, JSON_THROW_ON_ERROR); if (! is_array($refreshTokenData)) { + $this->loggerService->warning( + 'Refresh token request rejected: decrypted refresh token has an unexpected type.', + ['client_id' => $clientId], + ); throw OidcServerException::invalidRefreshToken('Refresh token has unexpected type'); } /** @var array $refreshTokenData */ if ($refreshTokenData['client_id'] !== $clientId) { + $this->loggerService->warning( + 'Refresh token request rejected: refresh token is not linked to the authenticated client.', + [ + 'client_id' => $clientId, + 'refresh_token_client_id' => $refreshTokenData['client_id'], + 'refresh_token_id' => $refreshTokenData['refresh_token_id'] ?? null, + ], + ); $this->getEmitter()->emit(new RequestEvent(RequestEvent::REFRESH_TOKEN_CLIENT_FAILED, $request)); throw OidcServerException::invalidRefreshToken('Refresh token is not linked to client'); } if ($refreshTokenData['expire_time'] < time()) { + $this->loggerService->notice( + 'Refresh token request rejected: refresh token has expired.', + [ + 'client_id' => $clientId, + 'refresh_token_id' => $refreshTokenData['refresh_token_id'] ?? null, + ], + ); throw OidcServerException::invalidRefreshToken('Refresh token has expired'); } @@ -106,6 +139,13 @@ protected function validateOldRefreshToken(ServerRequestInterface $request, stri (string)$refreshTokenData['refresh_token_id'], ) === true ) { + $this->loggerService->warning( + 'Refresh token request rejected: refresh token has been revoked.', + [ + 'client_id' => $clientId, + 'refresh_token_id' => $refreshTokenData['refresh_token_id'] ?? null, + ], + ); throw OidcServerException::invalidRefreshToken('Refresh token has been revoked'); } diff --git a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php index c3da1821..67519ec6 100644 --- a/src/Server/RequestRules/Rules/ClientAuthenticationRule.php +++ b/src/Server/RequestRules/Rules/ClientAuthenticationRule.php @@ -75,6 +75,9 @@ public function checkRule( ); if (is_null($resolvedClientAuthenticationMethod)) { + $loggerService->warning( + 'Client authentication failed: no supported client authentication method was presented.', + ); throw OidcServerException::accessDenied('Not a single client authentication method presented.'); } @@ -83,6 +86,10 @@ public function checkRule( $resolvedClientAuthenticationMethod->getClientAuthenticationMethod()->isNone() && $resolvedClientAuthenticationMethod->getClient()->isConfidential() ) { + $loggerService->warning( + 'Client authentication failed: confidential client used the "none" authentication method.', + ['client_id' => $resolvedClientAuthenticationMethod->getClient()->getIdentifier()], + ); throw OidcServerException::accessDenied( 'Confidential client must use an authentication method other than "none".', ); diff --git a/src/Server/RequestRules/Rules/ClientIdRule.php b/src/Server/RequestRules/Rules/ClientIdRule.php index 3ccda9f9..98d10bc1 100644 --- a/src/Server/RequestRules/Rules/ClientIdRule.php +++ b/src/Server/RequestRules/Rules/ClientIdRule.php @@ -58,6 +58,9 @@ public function checkRule( ) ?? $request->getServerParams()['PHP_AUTH_USER'] ?? null; if ($clientId === null) { + $loggerService->notice( + 'Request rejected: `client_id` not found in request parameters or HTTP Basic auth.', + ); throw OidcServerException::invalidRequest('client_id'); } diff --git a/src/Server/RequestRules/Rules/ClientRedirectUriRule.php b/src/Server/RequestRules/Rules/ClientRedirectUriRule.php index 86c47bc1..d2cae61b 100644 --- a/src/Server/RequestRules/Rules/ClientRedirectUriRule.php +++ b/src/Server/RequestRules/Rules/ClientRedirectUriRule.php @@ -63,6 +63,10 @@ public function checkRule( // On OAuth2 redirect_uri is optional if there is only one registered, however we will always require it // since this is OIDC oriented package and in OIDC this parameter is required. if ($redirectUri === null) { + $loggerService->notice( + 'Request rejected: `redirect_uri` parameter is required but was not provided.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest(ParamsEnum::RedirectUri->value); } diff --git a/src/Server/RequestRules/Rules/CodeChallengeMethodRule.php b/src/Server/RequestRules/Rules/CodeChallengeMethodRule.php index 32d4687b..be738fd1 100644 --- a/src/Server/RequestRules/Rules/CodeChallengeMethodRule.php +++ b/src/Server/RequestRules/Rules/CodeChallengeMethodRule.php @@ -58,6 +58,10 @@ public function checkRule( $codeChallengeVerifiers = $this->codeChallengeVerifiersRepository->getAll(); if ($this->codeChallengeVerifiersRepository->has($codeChallengeMethod) === false) { + $loggerService->notice( + 'Authorization request rejected: unsupported `code_challenge_method`.', + ['code_challenge_method' => $codeChallengeMethod], + ); throw OidcServerException::invalidRequest( 'code_challenge_method', 'Code challenge method must be one of ' . implode(', ', array_map( diff --git a/src/Server/RequestRules/Rules/CodeChallengeRule.php b/src/Server/RequestRules/Rules/CodeChallengeRule.php index d659be86..7730f5e0 100644 --- a/src/Server/RequestRules/Rules/CodeChallengeRule.php +++ b/src/Server/RequestRules/Rules/CodeChallengeRule.php @@ -49,6 +49,10 @@ public function checkRule( if ($codeChallenge === null) { if (! $client->isConfidential()) { + $loggerService->notice( + 'Authorization request rejected: `code_challenge` (PKCE) is required for public clients.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( ParamsEnum::CodeChallenge->value, 'Code Challenge must be provided for public clients.', @@ -65,6 +69,11 @@ public function checkRule( // Validate code_challenge according to RFC-7636 // @see: https://tools.ietf.org/html/rfc7636#section-4.2 if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeChallenge) !== 1) { + $loggerService->notice( + 'Authorization request rejected: `code_challenge` does not follow RFC-7636 (wrong length or ' . + 'character set).', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( ParamsEnum::CodeChallenge->value, 'Code Challenge must follow the specifications of RFC-7636.', diff --git a/src/Server/RequestRules/Rules/CodeVerifierRule.php b/src/Server/RequestRules/Rules/CodeVerifierRule.php index b4ab9ff2..b6c539f2 100644 --- a/src/Server/RequestRules/Rules/CodeVerifierRule.php +++ b/src/Server/RequestRules/Rules/CodeVerifierRule.php @@ -43,6 +43,10 @@ public function checkRule( if (is_null($codeVerifier)) { if (!$client->isConfidential()) { + $loggerService->notice( + 'Token request rejected: `code_verifier` (PKCE) is required for public clients.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'code_verifier', 'Code Verifier must be provided for public clients.', @@ -55,6 +59,10 @@ public function checkRule( // Validate code_verifier according to RFC-7636 // @see: https://tools.ietf.org/html/rfc7636#section-4.1 if (preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeVerifier) !== 1) { + $loggerService->notice( + 'Token request rejected: `code_verifier` does not follow RFC-7636 (wrong length or character set).', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'code_verifier', 'Code Verifier must follow the specifications of RFC-7636.', diff --git a/src/Server/RequestRules/Rules/IdTokenHintRule.php b/src/Server/RequestRules/Rules/IdTokenHintRule.php index 744b1218..389ec235 100644 --- a/src/Server/RequestRules/Rules/IdTokenHintRule.php +++ b/src/Server/RequestRules/Rules/IdTokenHintRule.php @@ -63,6 +63,7 @@ public function checkRule( } if (empty($idTokenHintParam)) { + $loggerService->notice('End session request rejected: `id_token_hint` was provided but empty.'); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, 'Received empty id_token_hint', @@ -79,6 +80,10 @@ public function checkRule( $idTokenHint = $this->core->idTokenFactory()->fromToken($idTokenHintParam); if ($idTokenHint->getIssuer() !== $this->moduleConfig->getIssuer()) { + $loggerService->notice( + 'End session request rejected: `id_token_hint` was not issued by this OP.', + ['issuer' => $idTokenHint->getIssuer(), 'expected_issuer' => $this->moduleConfig->getIssuer()], + ); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, 'Invalid ID Token Hint Issuer', @@ -91,6 +96,10 @@ public function checkRule( try { $idTokenHint->verifyWithKeySet($jwks); } catch (\Throwable $exception) { + $loggerService->notice( + 'End session request rejected: `id_token_hint` signature verification failed.', + ['exception' => $exception->getMessage()], + ); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, $exception->getMessage(), diff --git a/src/Server/RequestRules/Rules/MaxAgeRule.php b/src/Server/RequestRules/Rules/MaxAgeRule.php index d3ab6095..89caab14 100644 --- a/src/Server/RequestRules/Rules/MaxAgeRule.php +++ b/src/Server/RequestRules/Rules/MaxAgeRule.php @@ -79,6 +79,10 @@ public function checkRule( ['options' => ['min_range' => 0]], ) ) { + $loggerService->notice( + 'Authorization request rejected: `max_age` is not a valid non-negative integer.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( ParamsEnum::MaxAge->value, 'max_age must be a valid integer', diff --git a/src/Server/RequestRules/Rules/PostLogoutRedirectUriRule.php b/src/Server/RequestRules/Rules/PostLogoutRedirectUriRule.php index 5c6886a3..30ff22cc 100644 --- a/src/Server/RequestRules/Rules/PostLogoutRedirectUriRule.php +++ b/src/Server/RequestRules/Rules/PostLogoutRedirectUriRule.php @@ -82,6 +82,10 @@ public function checkRule( foreach ($auds as $aud) { $client = $this->clientRepository->findById($aud); if ($client === null) { + $loggerService->notice( + 'End session request rejected: `id_token_hint` aud claim does not match a known client.', + ['aud' => $aud], + ); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, 'aud claim not valid', @@ -97,6 +101,11 @@ public function checkRule( } if (! $isPostLogoutRedirectUriRegistered) { + $loggerService->notice( + 'End session request rejected: `post_logout_redirect_uri` is not registered for any client in ' . + 'the id_token_hint aud claim.', + ['post_logout_redirect_uri' => $postLogoutRedirectUri], + ); throw OidcServerException::invalidRequest( ParamsEnum::IdTokenHint->value, 'post_logout_redirect_uri not registered', diff --git a/src/Server/RequestRules/Rules/PromptRule.php b/src/Server/RequestRules/Rules/PromptRule.php index 3b4cbee2..276d94c0 100644 --- a/src/Server/RequestRules/Rules/PromptRule.php +++ b/src/Server/RequestRules/Rules/PromptRule.php @@ -75,12 +75,20 @@ public function checkRule( $prompt = explode(" ", (string)$requestParams[ParamsEnum::Prompt->value]); if (count($prompt) > 1 && in_array('none', $prompt, true)) { + $loggerService->notice( + 'Authorization request rejected: `prompt=none` cannot be combined with other prompt values.', + ['client_id' => $client->getIdentifier(), 'prompt' => $requestParams[ParamsEnum::Prompt->value]], + ); throw OAuthServerException::invalidRequest(ParamsEnum::Prompt->value, 'Invalid prompt parameter'); } $redirectUri = $currentResultBag->getOrFail(ClientRedirectUriRule::class)->getValue(); $state = $currentResultBag->getOrFail(StateRule::class)->getValue(); if (in_array('none', $prompt, true) && !$authSimple->isAuthenticated()) { + $loggerService->notice( + 'Authorization request rejected: `prompt=none` was requested but the user is not authenticated.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::loginRequired( null, $redirectUri, diff --git a/src/Server/RequestRules/Rules/RequestObjectRule.php b/src/Server/RequestRules/Rules/RequestObjectRule.php index e4c2e96f..1125361e 100644 --- a/src/Server/RequestRules/Rules/RequestObjectRule.php +++ b/src/Server/RequestRules/Rules/RequestObjectRule.php @@ -79,6 +79,11 @@ public function checkRule( // The Request Object source is present, but it could not be parsed (by value) or fetched/parsed (by // reference). Note that for the by-reference case, RequestUriRule would normally reject this earlier. if ($requestObjectBag === null) { + $loggerService->notice( + 'Authorization request rejected: request object could not be parsed (by value) or fetched (by ' . + 'reference).', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'request', 'Request object could not be parsed or fetched.', @@ -95,6 +100,11 @@ public function checkRule( // containing the Client ID claim. $jarRequestObject = $requestObjectBag->get(JarRequestObject::class); if (!$jarRequestObject instanceof JarRequestObject) { + $loggerService->notice( + 'Authorization request rejected: request object is not a valid JAR (RFC 9101) Request Object ' . + '(it must be signed).', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'request', 'Request object is not a valid JAR Request Object (note that it must be signed).', @@ -106,6 +116,14 @@ public function checkRule( } if ($jarRequestObject->getClientId() !== $client->getIdentifier()) { + $loggerService->warning( + 'Authorization request rejected: client_id claim in JAR request object does not match the ' . + 'client_id parameter.', + [ + 'client_id' => $client->getIdentifier(), + 'request_object_client_id' => $jarRequestObject->getClientId(), + ], + ); throw OidcServerException::invalidRequest( 'request', 'Client ID claim in request object does not match the client_id parameter.', @@ -129,6 +147,10 @@ public function checkRule( // (unless policy requires signature). $requestObject = $requestObjectBag->get(ConnectRequestObject::class); if (!$requestObject instanceof ConnectRequestObject) { + $loggerService->notice( + 'Authorization request rejected: request object is not a valid OpenID Connect Request Object.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'request', 'Request object is not a valid Request Object.', @@ -144,6 +166,11 @@ public function checkRule( $requireSigned = $this->moduleConfig->getRequireSignedRequestObject() || $client->getRequireSignedRequestObject(); if ($requireSigned) { + $loggerService->notice( + 'Authorization request rejected: an unsigned request object was provided but a signed one ' . + 'is required by server or client policy.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'request', 'Request object must be signed (alg: none is not allowed).', diff --git a/src/Server/RequestRules/Rules/RequiredNonceRule.php b/src/Server/RequestRules/Rules/RequiredNonceRule.php index 2b216aed..6dc846cb 100644 --- a/src/Server/RequestRules/Rules/RequiredNonceRule.php +++ b/src/Server/RequestRules/Rules/RequiredNonceRule.php @@ -45,6 +45,7 @@ public function checkRule( ); if ($nonce === null || $nonce === '') { + $loggerService->notice('Authorization request rejected: required `nonce` parameter is missing.'); throw OidcServerException::invalidRequest( ParamsEnum::Nonce->value, 'nonce is required', diff --git a/src/Server/RequestRules/Rules/ResponseModeRule.php b/src/Server/RequestRules/Rules/ResponseModeRule.php index f8c87a71..c6d830e2 100644 --- a/src/Server/RequestRules/Rules/ResponseModeRule.php +++ b/src/Server/RequestRules/Rules/ResponseModeRule.php @@ -60,6 +60,7 @@ public function checkRule( if ( !isset($requestParams[ParamsEnum::ClientId->value]) ) { + $loggerService->notice('Authorization request rejected: `client_id` is required to resolve response_mode.'); throw OidcServerException::invalidRequest( ParamsEnum::ClientId->value, 'Missing client_id', @@ -88,6 +89,10 @@ public function checkRule( true, ) ) { + $loggerService->notice( + 'Authorization request rejected: `response_mode` is not supported by this server.', + ['response_mode' => $responseModeValue], + ); throw OidcServerException::invalidRequest( ParamsEnum::ResponseMode->value, 'Invalid response_mode', @@ -103,6 +108,14 @@ public function checkRule( $allowedResponseModes = $client->getAllowedResponseModes(); if (!in_array($responseModeValue, $allowedResponseModes, true)) { + $loggerService->notice( + 'Authorization request rejected: `response_mode` is not allowed for this client.', + [ + 'client_id' => $client->getIdentifier(), + 'response_mode' => $responseModeValue, + 'allowed_response_modes' => $allowedResponseModes, + ], + ); throw OidcServerException::invalidRequest( 'response_mode', 'response_mode "' . $responseModeValue . '" is not allowed for this client', diff --git a/src/Server/RequestRules/Rules/ResponseTypeRule.php b/src/Server/RequestRules/Rules/ResponseTypeRule.php index f9edb712..c1c79a34 100644 --- a/src/Server/RequestRules/Rules/ResponseTypeRule.php +++ b/src/Server/RequestRules/Rules/ResponseTypeRule.php @@ -43,6 +43,9 @@ public function checkRule( !isset($requestParams[ParamsEnum::ResponseType->value]) || !isset($requestParams[ParamsEnum::ClientId->value]) ) { + $loggerService->notice( + 'Authorization request rejected: missing `response_type` or `client_id` parameter.', + ); throw OidcServerException::invalidRequest( ParamsEnum::ResponseType->value, 'Missing response_type or client_id', diff --git a/src/Server/RequestRules/Rules/ScopeOfflineAccessRule.php b/src/Server/RequestRules/Rules/ScopeOfflineAccessRule.php index ab8e31b4..12c697a2 100644 --- a/src/Server/RequestRules/Rules/ScopeOfflineAccessRule.php +++ b/src/Server/RequestRules/Rules/ScopeOfflineAccessRule.php @@ -47,6 +47,10 @@ public function checkRule( // Scope offline_access is used. Check if the client has it registered. if (! in_array('offline_access', $client->getScopes(), true)) { + $loggerService->notice( + 'Authorization request rejected: `offline_access` scope requested but not registered for the client.', + ['client_id' => $client->getIdentifier()], + ); throw OidcServerException::invalidRequest( 'scope', 'offline_access scope is not registered for the client', diff --git a/src/Services/ErrorResponder.php b/src/Services/ErrorResponder.php index a73e21e1..00267261 100644 --- a/src/Services/ErrorResponder.php +++ b/src/Services/ErrorResponder.php @@ -15,6 +15,7 @@ class ErrorResponder { public function __construct( private readonly PsrHttpBridge $psrHttpBridge, + private readonly LoggerService $loggerService, ) { } @@ -30,11 +31,15 @@ public function forException(Throwable $exception): Response } if (is_a($exception, OAuthServerException::class)) { + $this->logOAuthServerException($exception); + return $this->psrHttpBridge->getHttpFoundationFactory()->createResponse( $exception->generateHttpResponse($this->psrHttpBridge->getResponseFactory()->createResponse()), ); } + $this->logUnexpectedException($exception); + return new JsonResponse( [ 'error' => [ @@ -54,6 +59,8 @@ public function forException(Throwable $exception): Response */ public function forExceptionJson(OAuthServerException $exception): JsonResponse { + $this->logOAuthServerException($exception); + $body = [ 'error' => $exception->getErrorType(), 'error_description' => $exception->getMessage(), @@ -69,4 +76,51 @@ public function forExceptionJson(OAuthServerException $exception): JsonResponse ['Cache-Control' => 'no-cache, no-store'], ); } + + /** + * Log an OAuth error that is about to be returned to the client. This is the single place every endpoint's + * error response passes through, so it ensures the descriptive error type, description and hint (which the + * client otherwise sees but the server log does not) end up in the log, without requiring debug logging. + * The level reflects who is at fault: 5xx are server faults, 401/403 are security-relevant denials, and the + * remaining 4xx are ordinary invalid-request problems. + */ + private function logOAuthServerException(OAuthServerException $exception): void + { + $httpStatusCode = $exception->getHttpStatusCode(); + + $message = sprintf( + 'OIDC error response: %s - %s', + $exception->getErrorType(), + $exception->getMessage(), + ); + + $context = ['httpStatusCode' => $httpStatusCode]; + if (($hint = $exception->getHint()) !== null) { + $context['hint'] = $hint; + } + if (($previous = $exception->getPrevious()) !== null) { + $context['previous'] = $previous->getMessage(); + } + + if ($httpStatusCode >= 500) { + $this->loggerService->error($message, $context); + } elseif ($httpStatusCode === 401 || $httpStatusCode === 403) { + $this->loggerService->warning($message, $context); + } else { + $this->loggerService->notice($message, $context); + } + } + + private function logUnexpectedException(Throwable $exception): void + { + $context = ['exception' => $exception::class]; + if (($previous = $exception->getPrevious()) !== null) { + $context['previous'] = $previous->getMessage(); + } + + $this->loggerService->error( + 'OIDC unexpected error response: ' . $exception->getMessage(), + $context, + ); + } } diff --git a/tests/unit/src/Controllers/RegistrationControllerTest.php b/tests/unit/src/Controllers/RegistrationControllerTest.php index b1c92a8a..db6e5e05 100644 --- a/tests/unit/src/Controllers/RegistrationControllerTest.php +++ b/tests/unit/src/Controllers/RegistrationControllerTest.php @@ -70,7 +70,7 @@ protected function setUp(): void $this->loggerMock = $this->createMock(LoggerService::class); // ErrorResponder::forExceptionJson builds the JSON response itself and does not use the bridge. - $this->errorResponder = new ErrorResponder($this->createMock(PsrHttpBridge::class)); + $this->errorResponder = new ErrorResponder($this->createMock(PsrHttpBridge::class), $this->loggerMock); $this->clientMetadataValidator = new ClientMetadataValidator($this->moduleConfigMock); $this->helpers = new Helpers(); diff --git a/tests/unit/src/Server/Grants/ImplicitGrantTest.php b/tests/unit/src/Server/Grants/ImplicitGrantTest.php index 5f487746..316bb60c 100644 --- a/tests/unit/src/Server/Grants/ImplicitGrantTest.php +++ b/tests/unit/src/Server/Grants/ImplicitGrantTest.php @@ -21,6 +21,7 @@ use SimpleSAML\Module\oidc\Server\RequestRules\RequestRulesManager; use SimpleSAML\Module\oidc\Server\RequestTypes\AuthorizationRequest; use SimpleSAML\Module\oidc\Services\IdTokenBuilder; +use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\RequestParamsResolver; #[CoversClass(ImplicitGrant::class)] @@ -39,6 +40,7 @@ class ImplicitGrantTest extends TestCase protected MockObject $scopeEntityMock; protected MockObject $clientEntityMock; protected MockObject $resultBagMock; + protected MockObject $loggerServiceMock; protected function setUp(): void { @@ -56,6 +58,7 @@ protected function setUp(): void $this->scopeEntityMock = $this->createMock(ScopeEntityInterface::class); $this->clientEntityMock = $this->createMock(ClientEntity::class); $this->resultBagMock = $this->createMock(ResultBagInterface::class); + $this->loggerServiceMock = $this->createMock(LoggerService::class); } protected function sut( @@ -66,6 +69,7 @@ protected function sut( ?RequestParamsResolver $requestParamsResolver = null, ?AccessTokenEntityFactory $accessTokenEntityFactory = null, ?ScopeRepositoryInterface $scopeRepository = null, + ?LoggerService $loggerService = null, ): ImplicitGrant { $idTokenBuilder ??= $this->idTokenBuilderMock; $accessTokenTtl ??= $this->accessTokenTtl1h; @@ -74,6 +78,7 @@ protected function sut( $requestParamsResolver ??= $this->requestParamsResolverMock; $accessTokenEntityFactory ??= $this->accessTokenEntityFactoryMock; $scopeRepository ??= $this->scopeRepositoryMock; + $loggerService ??= $this->loggerServiceMock; $implicitGrant = new ImplicitGrant( @@ -83,6 +88,7 @@ protected function sut( $requestRulesManager, $requestParamsResolver, $accessTokenEntityFactory, + $loggerService, ); $implicitGrant->setScopeRepository($scopeRepository); diff --git a/tests/unit/src/Server/Grants/RefreshTokenGrantTest.php b/tests/unit/src/Server/Grants/RefreshTokenGrantTest.php index 55c6bfa2..9f8c57ea 100644 --- a/tests/unit/src/Server/Grants/RefreshTokenGrantTest.php +++ b/tests/unit/src/Server/Grants/RefreshTokenGrantTest.php @@ -14,6 +14,7 @@ use SimpleSAML\Module\oidc\Factories\Entities\AccessTokenEntityFactory; use SimpleSAML\Module\oidc\Server\Grants\RefreshTokenGrant; use SimpleSAML\Module\oidc\Server\TokenIssuers\RefreshTokenIssuer; +use SimpleSAML\Module\oidc\Services\LoggerService; use SimpleSAML\Module\oidc\Utils\AuthenticatedOAuth2ClientResolver; use SimpleSAML\Module\oidc\ValueAbstracts\ResolvedClientAuthenticationMethod; use SimpleSAML\OpenID\Codebooks\ClientAuthenticationMethodsEnum; @@ -26,6 +27,7 @@ class RefreshTokenGrantTest extends TestCase protected MockObject $refreshTokenIssuerMock; protected MockObject $clientResolverMock; protected MockObject $serverRequestMock; + protected MockObject $loggerServiceMock; protected function setUp(): void { @@ -34,6 +36,7 @@ protected function setUp(): void $this->refreshTokenIssuerMock = $this->createMock(RefreshTokenIssuer::class); $this->clientResolverMock = $this->createMock(AuthenticatedOAuth2ClientResolver::class); $this->serverRequestMock = $this->createMock(ServerRequestInterface::class); + $this->loggerServiceMock = $this->createMock(LoggerService::class); } protected function sut(): RefreshTokenGrant @@ -43,6 +46,7 @@ protected function sut(): RefreshTokenGrant $this->accessTokenEntityFactoryMock, $this->refreshTokenIssuerMock, $this->clientResolverMock, + $this->loggerServiceMock, ); } diff --git a/tests/unit/src/Services/ErrorResponderTest.php b/tests/unit/src/Services/ErrorResponderTest.php new file mode 100644 index 00000000..506374ae --- /dev/null +++ b/tests/unit/src/Services/ErrorResponderTest.php @@ -0,0 +1,73 @@ +psrHttpBridgeMock = $this->createMock(PsrHttpBridge::class); + $this->loggerServiceMock = $this->createMock(LoggerService::class); + } + + protected function sut(): ErrorResponder + { + return new ErrorResponder($this->psrHttpBridgeMock, $this->loggerServiceMock); + } + + public function testForExceptionJsonLogsClientErrorAsNotice(): void + { + $this->loggerServiceMock->expects($this->once())->method('notice'); + $this->loggerServiceMock->expects($this->never())->method('warning'); + $this->loggerServiceMock->expects($this->never())->method('error'); + + $response = $this->sut()->forExceptionJson(OidcServerException::invalidRequest('client_id')); + + $this->assertSame(400, $response->getStatusCode()); + } + + public function testForExceptionJsonLogsAccessDeniedAsWarning(): void + { + $this->loggerServiceMock->expects($this->once())->method('warning'); + $this->loggerServiceMock->expects($this->never())->method('error'); + + $response = $this->sut()->forExceptionJson(OidcServerException::accessDenied('nope')); + + $this->assertSame(401, $response->getStatusCode()); + } + + public function testForExceptionJsonLogsServerErrorAsError(): void + { + $this->loggerServiceMock->expects($this->once())->method('error'); + $this->loggerServiceMock->expects($this->never())->method('notice'); + + $response = $this->sut()->forExceptionJson(OAuthServerException::serverError('boom')); + + $this->assertSame(500, $response->getStatusCode()); + } + + public function testForExceptionLogsUnexpectedThrowableAsError(): void + { + $this->loggerServiceMock->expects($this->once())->method('error'); + + $response = $this->sut()->forException(new RuntimeException('unexpected')); + + $this->assertSame(500, $response->getStatusCode()); + } +}