From 1448a800567dcc71e3c68c4d60c5c266a1814081 Mon Sep 17 00:00:00 2001
From: Steve Breker <sbreker@artefactual.com>
Date: Fri, 24 Jan 2025 14:31:38 -0800
Subject: [PATCH] Add OIDC 'unauthorized' failure message

If an error occurs in AtoM after successful authentication with the OIDC
endpoint, end the AtoM session and redirect to the standard AtoM
'Unauthorized' message template.

Previously, errors that occurred in AtoM after successful authentication
with the OIDC endpoint would fail silently redirecting the user to the
AtoM home page in an unauthenticated state.
---
 plugins/arOidcPlugin/lib/oidcUser.class.php         | 13 ++++++-------
 .../modules/oidc/actions/loginAction.class.php      |  7 ++++++-
 .../modules/oidc/actions/logoutAction.class.php     |  2 +-
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/plugins/arOidcPlugin/lib/oidcUser.class.php b/plugins/arOidcPlugin/lib/oidcUser.class.php
index 63b5ed7ddb..ee091d3c7f 100644
--- a/plugins/arOidcPlugin/lib/oidcUser.class.php
+++ b/plugins/arOidcPlugin/lib/oidcUser.class.php
@@ -111,7 +111,6 @@ public function authenticate($username = null, $password = null): bool
             $userMatchingSource = $this->getProviderConfigValue('user_matching_source', '');
             if (!arOidc::validateUserMatchingSource($userMatchingSource)) {
                 $this->logger->err('OIDC user matching source is configured but is not set properly. Unable to match OIDC users to AtoM users.');
-                $this->logout();
 
                 return $authenticated;
             }
@@ -133,7 +132,6 @@ public function authenticate($username = null, $password = null): bool
             $autoCreateUser = $this->getProviderConfigValue('auto_create_atom_user', true);
             if (!is_bool($autoCreateUser)) {
                 $this->logger->err('OIDC auto_create_atom_user is configured but is not set properly - value should be of type bool. Unable to match OIDC users to AtoM users.');
-                $this->logout();
 
                 return $authenticated;
             }
@@ -149,7 +147,6 @@ public function authenticate($username = null, $password = null): bool
             // If user is null and $autoCreateUser is true, then something failed.
             if (null === $user && $autoCreateUser) {
                 $this->logger->err('OIDC authentication succeeded but unable to find or create user in AtoM.');
-                $this->logout();
 
                 return $authenticated;
             }
@@ -157,7 +154,6 @@ public function authenticate($username = null, $password = null): bool
             // If user is null and $autoCreateUser is false, then user has not been previously created or matching failed.
             if (null === $user && !$autoCreateUser) {
                 $this->logger->err('OIDC authentication succeeded but user not found and auto_create_atom_user is set to false.');
-                $this->logout();
 
                 return $authenticated;
             }
@@ -168,7 +164,6 @@ public function authenticate($username = null, $password = null): bool
             $setGroupsFromClaims = $this->getProviderConfigValue('set_groups_from_attributes', false);
             if (!is_bool($setGroupsFromClaims)) {
                 $this->logger->err('OIDC set_groups_from_attributes is configured but is not set properly - value should be of type bool. Unable to complete authentication.');
-                $this->logout();
 
                 return $authenticated;
             }
@@ -281,14 +276,18 @@ public function isAuthenticated(): bool
 
     /**
      * Logout from AtoM and the OIDC server.
+     *
+     * @param mixed $sendOidcLogout
      */
-    public function logout(): void
+    public function logout($sendOidcLogout = false): void
     {
+        // Clean up AtoM session.
         $idToken = $this->getAttribute('oidc-token', null);
         $this->unsetAttributes();
         $this->signOut();
 
-        if (true == $this->getProviderConfigValue('send_oidc_logout', false) && !empty($idToken)) {
+        // Clean up OIDC session.
+        if (!empty($idToken) && $sendOidcLogout) {
             $logoutRedirectUrl = sfConfig::get('app_oidc_logout_redirect_url', '');
             if (empty($logoutRedirectUrl)) {
                 $logoutRedirectUrl = null;
diff --git a/plugins/arOidcPlugin/modules/oidc/actions/loginAction.class.php b/plugins/arOidcPlugin/modules/oidc/actions/loginAction.class.php
index bea72aa862..627187c063 100644
--- a/plugins/arOidcPlugin/modules/oidc/actions/loginAction.class.php
+++ b/plugins/arOidcPlugin/modules/oidc/actions/loginAction.class.php
@@ -39,7 +39,12 @@ public function execute($request)
                 $this->context->user->validateProviderId($providerId, true);
             }
 
-            $this->context->user->authenticate();
+            $result = $this->context->user->authenticate();
+            if (false == $result) {
+                $this->context->user->logout(false);
+
+                $this->redirect('admin/secure');
+            }
         }
 
         // Redirect to module/action the user was trying to reach before being redirected
diff --git a/plugins/arOidcPlugin/modules/oidc/actions/logoutAction.class.php b/plugins/arOidcPlugin/modules/oidc/actions/logoutAction.class.php
index 9931c32c09..28cbc0caa7 100644
--- a/plugins/arOidcPlugin/modules/oidc/actions/logoutAction.class.php
+++ b/plugins/arOidcPlugin/modules/oidc/actions/logoutAction.class.php
@@ -21,7 +21,7 @@ class OidcLogoutAction extends sfAction
 {
     public function execute($request)
     {
-        $this->getUser()->logout();
+        $this->getUser()->logout($this->getUser()->getProviderConfigValue('send_oidc_logout', false));
         $this->redirect('@homepage');
     }
 }