Merge pull request #191 from aroskanalen/feature/#534_add_oidc_error_…

…logging #534 - Add logging for OIDC errors
os2display · Feb 29, 2024 · 05c2e13 · 05c2e13
2 parents 7ae4415 + 3a3e2c0
commit 05c2e13
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 20 deletions.
diff --git 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
@@ -537,12 +537,6 @@
     </MissingParamType>
   </file>
   <file src="src/Security/AzureOidcAuthenticator.php">
-    <InvalidNullableReturnType>
-      <code><![CDATA[User]]></code>
-    </InvalidNullableReturnType>
-    <NullableReturnStatement>
-      <code><![CDATA[$this->entityManager->getRepository(User::class)->findOneBy(['providerId' => $identifier])]]></code>
-    </NullableReturnStatement>
     <RiskyTruthyFalsyComparison>
       <code><![CDATA[empty($signInName)]]></code>
     </RiskyTruthyFalsyComparison>

diff --git 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;
     }
 }