Auth/OpenIDConnect: Cleanup code and make role mapping more robust

See: https://mantis.ilias.de/view.php?id=39471
surlabs · Oct 30, 2024 · 1589b83 · 1589b83
1 parent 09d734c
commit 1589b83
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 17 deletions.
diff --git a/Services/OpenIdConnect/classes/class.ilAuthProviderOpenIdConnect.php b/Services/OpenIdConnect/classes/class.ilAuthProviderOpenIdConnect.php
@@ -29,7 +29,6 @@ class ilAuthProviderOpenIdConnect extends ilAuthProvider
     private const OIDC_AUTH_IDTOKEN = "oidc_auth_idtoken";
     private ilOpenIdConnectSettings $settings;
     /** @var array $body */
-    private $body;
     private ilLogger $logger;
     private ilLanguage $lng;
 
@@ -40,7 +39,6 @@ public function __construct(ilAuthCredentials $credentials)
 
         $this->logger = $DIC->logger()->auth();
         $this->settings = ilOpenIdConnectSettings::getInstance();
-        $this->body = $DIC->http()->request()->getParsedBody();
         $this->lng = $DIC->language();
         $this->lng->loadLanguageModule('auth');
     }
@@ -97,7 +95,6 @@ public function doAuthentication(ilAuthStatus $status): bool
 
             $oidc->authenticate();
             // user is authenticated, otherwise redirected to authorization endpoint or exception
-            $this->logger->dump($this->body, ilLogLevel::DEBUG);
 
             $claims = $oidc->requestUserInfo();
             $this->logger->dump($claims, ilLogLevel::DEBUG);
@@ -136,10 +133,17 @@ private function handleUpdate(ilAuthStatus $status, $user_info): ilAuthStatus
         }
 
         $uid_field = $this->settings->getUidField();
-        $ext_account = $user_info->{$uid_field};
+        $ext_account = $user_info->{$uid_field} ?? '';
 
-        $this->logger->debug('Authenticated external account: ' . $ext_account);
+        if (!is_string($ext_account) || $ext_account === '') {
+            $this->logger->error('Could not determine valid external account, value is empty or not a string.');
+            $this->logger->dump($user_info, ilLogLevel::ERROR);
+            $status->setStatus(ilAuthStatus::STATUS_AUTHENTICATION_FAILED);
+            $status->setReason('err_wrong_login');
+            return $status;
+        }
 
+        $this->logger->debug('Authenticated external account: ' . $ext_account);
 
         $int_account = ilObjUser::_checkExternalAuthAccount(
             ilOpenIdConnectUserSync::AUTH_MODE,
@@ -148,11 +152,6 @@ private function handleUpdate(ilAuthStatus $status, $user_info): ilAuthStatus
 
         try {
             $sync = new ilOpenIdConnectUserSync($this->settings, $user_info);
-            if (!is_string($ext_account)) {
-                $status->setStatus(ilAuthStatus::STATUS_AUTHENTICATION_FAILED);
-                $status->setReason('err_wrong_login');
-                return $status;
-            }
             $sync->setExternalAccount($ext_account);
             $sync->setInternalAccount((string) $int_account);
             $sync->updateUser();

diff --git a/Services/OpenIdConnect/classes/class.ilOpenIdConnectUserSync.php b/Services/OpenIdConnect/classes/class.ilOpenIdConnectUserSync.php
@@ -192,12 +192,9 @@ protected function parseRoleAssignments(): array
                 continue;
             }
 
-            [$role_attribute, $role_value] = explode('::', $role_info['value']);
+            [$role_attribute, $role_value] = array_map(trim(...), explode('::', $role_info['value']));
 
-            if (
-                !$role_attribute ||
-                !$role_value
-            ) {
+            if (!$role_attribute || !$role_value) {
                 $this->logger->debug('No valid role mapping configuration for: ' . $role_id);
                 continue;
             }
@@ -213,14 +210,16 @@ protected function parseRoleAssignments(): array
             }
 
             if (is_array($this->user_info->{$role_attribute})) {
-                if (!in_array($role_value, $this->user_info->{$role_attribute}, true)) {
+                $roles_claim = array_map(trim(...), $this->user_info->{$role_attribute});
+                if (!in_array($role_value, $roles_claim, true)) {
                     $this->logger->debug('User account has no ' . $role_value);
                     continue;
                 }
-            } elseif (strcmp($this->user_info->{$role_attribute}, $role_value) !== 0) {
+            } elseif (strcmp(trim($this->user_info->{$role_attribute}), $role_value) !== 0) {
                 $this->logger->debug('User account has no ' . $role_value);
                 continue;
             }
+
             $this->logger->debug('Matching role mapping for role_id: ' . $role_id);
 
             $found_role = true;