From 47fe4166add55425163651e6456582f3d74ad42b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 29 Feb 2024 15:34:07 +0100 Subject: [PATCH 1/3] #534 - Add logging for OIDC errors --- src/Security/AzureOidcAuthenticator.php | 46 +++++++++++++++++-------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/src/Security/AzureOidcAuthenticator.php b/src/Security/AzureOidcAuthenticator.php index 77c55516..80ef1c6b 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'; @@ -34,6 +36,7 @@ class AzureOidcAuthenticator extends OpenIdLoginAuthenticator final public const APP_ADMIN_ROLE = Roles::ROLE_ADMIN; final public const APP_EDITOR_ROLE = Roles::ROLE_EDITOR; + private LoggerInterface $logger; public function __construct( private readonly EntityManagerInterface $entityManager, @@ -48,6 +51,11 @@ public function __construct( parent::__construct($providerManager); } + public function setLogger(LoggerInterface $logger): void + { + $this->logger = $logger; + } + /** * @throws InvalidProviderException */ @@ -80,7 +88,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 +119,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 +138,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 +180,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 +208,8 @@ private function getTenantKeyWithRole(string $oidcGroup): array return [$tenantKey, $role]; } - throw new \InvalidArgumentException('Unknown role for group: '.$oidcGroup); + $this->logger->warning('Unknown role for group: '.$oidcGroup, [ + 'source' => self::class, + ]); } } From f53c6bd3bd1ee9f68b4712dbe550bad5c3b483fe Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 29 Feb 2024 15:36:08 +0100 Subject: [PATCH 2/3] #534 - Update Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) 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 From 3a3e2c0ebb90a629cafea956d4014d3eee258d80 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 29 Feb 2024 15:43:02 +0100 Subject: [PATCH 3/3] #534 - Psalm fixes --- psalm-baseline.xml | 6 ------ src/Security/AzureOidcAuthenticator.php | 8 +++++--- 2 files changed, 5 insertions(+), 9 deletions(-) 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 80ef1c6b..1a15e486 100644 --- a/src/Security/AzureOidcAuthenticator.php +++ b/src/Security/AzureOidcAuthenticator.php @@ -36,6 +36,8 @@ class AzureOidcAuthenticator extends OpenIdLoginAuthenticator implements LoggerA 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( @@ -208,8 +210,8 @@ private function getTenantKeyWithRole(string $oidcGroup): array return [$tenantKey, $role]; } - $this->logger->warning('Unknown role for group: '.$oidcGroup, [ - 'source' => self::class, - ]); + $e = new \InvalidArgumentException('Unknown role for group: '.$oidcGroup); + $this->logger->error($e); + throw $e; } }