From 6b0d6fa3cd11305ef078905545fde31e7fa4bff1 Mon Sep 17 00:00:00 2001
From: Steve Breker <sbreker@artefactual.com>
Date: Wed, 19 Jun 2024 23:21:20 -0700
Subject: [PATCH] Address CR Feedback

- Removed debugging message.
- Refactored logout() method to remove duplicated call to
  setProviderId() method.
---
 plugins/arOidcPlugin/lib/oidcUser.class.php | 28 ++++++++++-----------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/plugins/arOidcPlugin/lib/oidcUser.class.php b/plugins/arOidcPlugin/lib/oidcUser.class.php
index 84692ce32e..b68721ac1e 100644
--- a/plugins/arOidcPlugin/lib/oidcUser.class.php
+++ b/plugins/arOidcPlugin/lib/oidcUser.class.php
@@ -51,8 +51,6 @@ public function authenticate($username = null, $password = null): bool
         $authenticateResult = false;
         $email = null;
 
-        $this->logger->err(sprintf('%s', json_encode(sfConfig::getAll())));
-
         // Get provider ID from session storage as it may have been set elsewhere.
         $providerId = $this->getSessionProviderId();
         // Validate and set provider ID in session.
@@ -271,20 +269,20 @@ public function logout(): void
                 $this->logger->err('Setting "app_oidc_logout_redirect_url" invalid. Unable to redirect on sign out.');
             }
 
-            // Dex does not yet implement end_session_endpoint with it's oidc connector
-            // so $this->oidcClient->signOut will fail.
-            // https://github.com/dexidp/dex/issues/1697
-            try {
-                // Get saved session provider id.
-                $providerId = $this->getSessionProviderId();
-                // Set provider details in the OIDC client using provider id.
-                if (true === $result = $this->setOidcProviderDetails($providerId)) {
+            // Get saved session provider id.
+            $providerId = $this->getSessionProviderId();
+            $this->setProviderId();
+
+            // Set provider details in the OIDC client using provider id.
+            if (true === $this->setOidcProviderDetails($providerId)) {
+                try {
+                    // Dex does not yet implement end_session_endpoint with it's oidc connector
+                    // so $this->oidcClient->signOut will fail.
+                    // https://github.com/dexidp/dex/issues/1697
                     $this->oidcClient->signOut($idToken, $logoutRedirectUrl);
+                } catch (Exception $e) {
+                    $this->logger->err($e->__toString().PHP_EOL);
                 }
-                $this->setProviderId();
-            } catch (Exception $e) {
-                $this->setProviderId();
-                $this->logger->err($e->__toString().PHP_EOL);
             }
         }
     }
@@ -373,7 +371,7 @@ protected function setOidcProviderDetails(string $providerId = ''): bool
             return false;
         }
 
-        // Get configured provider details from ID.
+        // Get configured providers.
         $providers = sfConfig::get('app_oidc_providers', []);
         if (empty($providers)) {
             $this->logger->err('OIDC providers not found in app.yml - check plugin configuration. Unable to authenticate using OIDC.');