[Web] OIDC/LDAP: check the OAuth state, keep the mailbox on token refresh, require unique IdP users

data/web/inc/functions.auth.inc.php

@@ -538,7 +538,8 @@ function keycloak_mbox_login_rest($user, $pass, $extra = null){
 
   // get the mailcow_password attribute from keycloak user
   $url = "{$iam_settings['server_url']}/admin/realms/{$iam_settings['realm']}/users";
-  $queryParams = array('email' => $user, 'exact' => true);
+  // as string, http_build_query() would send exact=1 and Keycloak would search for substrings
+  $queryParams = array('email' => $user, 'exact' => 'true');
   $queryString = http_build_query($queryParams);
   $curl = curl_init();
   curl_setopt($curl, CURLOPT_TIMEOUT, 7);
@@ -559,7 +560,10 @@ function keycloak_mbox_login_rest($user, $pass, $extra = null){
   if ($code != 200) {
     throw new \Mailcow\Auth\IdpUnavailable('Keycloak returned HTTP ' . $code);
   }
-  // exact=true can still return several users with that email
+  // exact=true can still return several users with that email, only the exact address counts
+  if (is_array($users)) {
+    $users = array_values(array_filter($users, fn($u) => is_array($u) && strcasecmp((string)($u['email'] ?? ''), $user) === 0));
+  }
   if (!is_array($users) || count($users) != 1) {
     return false;
   }
@@ -710,12 +714,15 @@ function ldap_mbox_login($user, $pass, $extra = null){
       ->select(array_values(array_filter(array_merge([$iam_settings['username_field'], $iam_settings['attribute_field'], 'displayname', 'distinguishedname', 'dn'],
         \Mailcow\Identity\Directory\LdapDirectory::idAttributes($iam_settings['id_attribute'] ?? '')))));
 
-    $user_res = $ldap_query->firstOrFail();
+    $entries = $ldap_query->get();
-  } catch (\LdapRecord\Query\ObjectNotFoundException $e) {
-    return false;
   } catch (Exception $e) {
     throw new \Mailcow\Auth\IdpUnavailable('LDAP search failed: ' . $e->getMessage(), 0, $e);
   }
+  // the login name must identify exactly one entry, several matches are ambiguous
+  if (!is_array($entries) || count($entries) != 1) {
+    return false;
+  }
+  $user_res = reset($entries);
   $idp_login = \Mailcow\Auth\AuthModule::fromGlobals()->idp;
   if (!$idp_login->verifyLdapPassword($iam_provider, (string)$user_res['dn'], (string)$pass)) {
     return false;

data/web/inc/functions.inc.php

@@ -3013,6 +3013,18 @@ function identity_provider($_action = null, $_data = null, $_extra = null) {
         return false;
       }
 
+      // A refresh keeps the logged-in mailbox: a changed IdP email or another IdP user ends the session
+      if (strtolower($info['email']) !== strtolower($_SESSION['mailcow_cc_username'] ?? '') ||
+        !\Mailcow\Auth\AuthModule::fromGlobals()->idp->verifyLink($iam_settings['authsource'], $info['email'], (string)($info['sub'] ?? ''))) {
+        clear_session();
+        $_SESSION['return'][] =  array(
+          'type' => 'danger',
+          'log' => array(__FUNCTION__, $info['email'], 'The IdP user does not match the logged-in mailbox'),
+          'msg' => 'refresh_login_failed'
+        );
+        return false;
+      }
+
       // The mailbox may have been deleted or deactivated since the login
       $stmt = $pdo->prepare("SELECT `mailbox`.`authsource` FROM `mailbox`
         INNER JOIN `domain` ON `mailbox`.`domain` = `domain`.`domain`

data/web/inc/lib/mailcow/Identity/Service/SyncPlanner.php

@@ -44,6 +44,8 @@ class SyncPlanner
         /** @var array<string, DirectoryUser> $candidates email => user to create, template still open */
         $candidates = [];
         $duplicates = [];
+        /** @var array<string, true> $ambiguous unlinked mailboxes whose email several IdP users have, left as they are */
+        $ambiguous = [];
         foreach ($listing->users as $user) {
             if (isset($owners[$user->id])) {
                 $username = $owners[$user->id];
@@ -55,10 +57,19 @@ class SyncPlanner
                 $plan->skipped++;
             } elseif (isset($state->accounts[$user->email])) {
                 $account = $state->accounts[$user->email];
-                if ($account->externalId !== null || isset($plan->links[$user->email])) {
+                if ($account->externalId !== null) {
                     self::conflict($plan, $user->email . ' is linked to another IdP user');
                     continue;
                 }
+                if (isset($plan->links[$user->email]) || isset($ambiguous[$user->email])) {
+                    // Which IdP user owns the mailbox is unclear: link none of them
+                    if (!isset($ambiguous[$user->email])) {
+                        self::conflict($plan, 'Several IdP users have the email ' . $user->email);
+                    }
+                    unset($plan->links[$user->email], $matched[$user->email]);
+                    $ambiguous[$user->email] = true;
+                    continue;
+                }
                 $plan->links[$user->email] = $user->id;
                 $matched[$user->email] = $user;
             } elseif (isset($state->mailboxes[$user->email]) || !$settings->importUsers || !$user->enabled
@@ -85,7 +96,7 @@ class SyncPlanner
         if ($groups !== null) {
             $this->planGroups($settings, $groups, $state, $usernames, $matched, $plan);
         }
-        $this->planStates($settings, $state, $matched, $plan);
+        $this->planStates($settings, $state, $matched, $ambiguous, $plan);
         if ($settings->periodicSync) {
             $this->planUpdates($settings, $state, $matched, $plan);
         }
@@ -211,10 +222,13 @@ class SyncPlanner
     }
 
     /** @param array<string, DirectoryUser> $matched */
-    private function planStates(Settings $settings, State $state, array $matched, SyncPlan $plan): void
+    private function planStates(Settings $settings, State $state, array $matched, array $ambiguous, SyncPlan $plan): void
     {
         foreach ($state->accounts as $username => $account) {
             $username = (string)$username;
+            if (isset($ambiguous[$username])) {
+                continue;
+            }
             $user = $matched[$username] ?? null;
             if ($user === null && $account->missingSince === null) {
                 $plan->missing[] = $username;

data/web/inc/triggers.user.inc.php

@@ -19,8 +19,10 @@ if ($iam_provider){
       header('Location: ' . $redirect_uri);
       die();
     }
-  } elseif ($_GET['code'] && $_GET['state'] === $_SESSION['oauth2state']) {
+  } elseif (!empty($_GET['code']) && !empty($_SESSION['oauth2state']) && is_string($_GET['state'] ?? null)
-    // Check given state against previously stored one to mitigate CSRF attack
+    && hash_equals((string)$_SESSION['oauth2state'], $_GET['state'])) {
+    // Check given state against previously stored one to mitigate CSRF attack, a state is used once
+    unset($_SESSION['oauth2state']);
     // Received access token in $_GET['code']
     // extract info and verify user
     identity_provider('verify-sso');

tests/php/Identity/Service/SyncPlannerTest.php

@@ -26,6 +26,17 @@ class SyncPlannerTest extends TestCase
         $this->assertSame([], $plan->conflicts);
     }
 
+    public function testUnlinkedMailboxWithSeveralIdpUsersIsNeitherLinkedNorOffboarded(): void
+    {
+        $users = [self::user('u1', 'a@example.org'), self::user('u2', 'a@example.org', false), self::user('u3', 'a@example.org')];
+        $plan = $this->plan($users, [new Account('a@example.org', 'A', 1)]);
+
+        $this->assertSame([], $plan->links);
+        $this->assertSame(['Several IdP users have the email a@example.org'], $plan->conflicts);
+        $this->assertSame([], $plan->missing);
+        $this->assertSame([], $plan->deactivate);
+    }
+
     public function testLinkedUserWithNewEmailIsAConflictAndNotCreated(): void
     {
         $plan = $this->plan([self::user('u1', 'new@example.org')], [new Account('a@example.org', 'A', 1, 'u1')]);