diff --git a/CHANGELOG.md b/CHANGELOG.md index 24d61a12..10081430 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- [#191](https://github.com/os2display/display-api-service/pull/191) + - Add logging for OIDC errors - [#189](https://github.com/os2display/display-api-service/pull/189) - Updated and applied psalm and rector settings - Added psalm and rector to PR check on github actions diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 623316bf..4cc4b6dc 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -537,12 +537,6 @@ - - - - - entityManager->getRepository(User::class)->findOneBy(['providerId' => $identifier])]]> - diff --git a/src/Security/AzureOidcAuthenticator.php b/src/Security/AzureOidcAuthenticator.php index 77c55516..1a15e486 100644 --- a/src/Security/AzureOidcAuthenticator.php +++ b/src/Security/AzureOidcAuthenticator.php @@ -15,6 +15,8 @@ use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; +use Psr\Log\LoggerAwareInterface; +use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; @@ -24,7 +26,7 @@ use Symfony\Component\Security\Http\Authenticator\Passport\Passport; use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport; -class AzureOidcAuthenticator extends OpenIdLoginAuthenticator +class AzureOidcAuthenticator extends OpenIdLoginAuthenticator implements LoggerAwareInterface { final public const OIDC_PROVIDER_INTERNAL = 'internal'; final public const OIDC_PROVIDER_EXTERNAL = 'external'; @@ -35,6 +37,9 @@ class AzureOidcAuthenticator extends OpenIdLoginAuthenticator final public const APP_ADMIN_ROLE = Roles::ROLE_ADMIN; final public const APP_EDITOR_ROLE = Roles::ROLE_EDITOR; + /** @psalm-suppress PropertyNotSetInConstructor */ + private LoggerInterface $logger; + public function __construct( private readonly EntityManagerInterface $entityManager, private readonly OpenIdConfigurationProviderManager $providerManager, @@ -48,6 +53,11 @@ public function __construct( parent::__construct($providerManager); } + public function setLogger(LoggerInterface $logger): void + { + $this->logger = $logger; + } + /** * @throws InvalidProviderException */ @@ -80,7 +90,9 @@ public function authenticate(Request $request): Passport $providerId = $email; break; default: - throw new CustomUserMessageAuthenticationException('Unsupported open_id_connect_provider.'); + $e = new CustomUserMessageAuthenticationException('Unsupported open_id_connect_provider.'); + $this->logger->error($e); + throw $e; } // Check if user exists already - if not create a user @@ -109,7 +121,9 @@ public function authenticate(Request $request): Passport $this->entityManager->flush(); return new SelfValidatingPassport(new UserBadge($user->getUserIdentifier(), $this->getUser(...))); - } catch (ItkOpenIdConnectException $exception) { + } catch (CustomUserMessageAuthenticationException|InvalidProviderException|ItkOpenIdConnectException $exception) { + $this->logger->error($exception); + throw new CustomUserMessageAuthenticationException($exception->getMessage()); } } @@ -126,7 +140,15 @@ public function start(Request $request, ?AuthenticationException $authException public function getUser(string $identifier): User { - return $this->entityManager->getRepository(User::class)->findOneBy(['providerId' => $identifier]); + $user = $this->entityManager->getRepository(User::class)->findOneBy(['providerId' => $identifier]); + + if (null === $user) { + $e = new CustomUserMessageAuthenticationException('User not found.'); + $this->logger->error($e); + throw $e; + } + + return $user; } private function setTenantRoles(User $user, array $oidcGroups): void @@ -160,17 +182,13 @@ private function mapTenantKeysRoles(array $oidcGroups): array $tenantKeyRoleMap = []; foreach ($oidcGroups as $oidcGroup) { - try { - [$tenantKey, $role] = $this->getTenantKeyWithRole($oidcGroup); + [$tenantKey, $role] = $this->getTenantKeyWithRole($oidcGroup); - if (!array_key_exists($tenantKey, $tenantKeyRoleMap)) { - $tenantKeyRoleMap[$tenantKey] = []; - } - - $tenantKeyRoleMap[$tenantKey][] = $role; - } catch (\InvalidArgumentException) { - // @TODO Should we log, ignore or throw exception if unknown role is encountered? + if (!array_key_exists($tenantKey, $tenantKeyRoleMap)) { + $tenantKeyRoleMap[$tenantKey] = []; } + + $tenantKeyRoleMap[$tenantKey][] = $role; } return $tenantKeyRoleMap; @@ -192,6 +210,8 @@ private function getTenantKeyWithRole(string $oidcGroup): array return [$tenantKey, $role]; } - throw new \InvalidArgumentException('Unknown role for group: '.$oidcGroup); + $e = new \InvalidArgumentException('Unknown role for group: '.$oidcGroup); + $this->logger->error($e); + throw $e; } }