From 6be69056d1f651d5d3120b68bae84b3819531226 Mon Sep 17 00:00:00 2001
From: Zaid Marji <cdej0cbf10ll@opayq.com>
Date: Sun, 31 May 2026 13:24:50 +0300
Subject: [PATCH] 2FA: harden enforce_2fa_policy for unauthenticated callers

The function is reachable from the unauthenticated webauthn_login grant
in addition to the existing authenticated 2FA-management callers. Two
failure modes safe for authenticated callers are not safe under an
unauthenticated grant:

- Organization::find_by_uuid().unwrap() panics the worker if the org
  was concurrently deleted. Match Some/None and skip the notification
  for that iteration; the revoke is still applied.

- A propagated SMTP error from send_2fa_removed_from_org aborted the
  loop after earlier members were already revoked + emailed, leaving
  the caller half-enforced. Log and continue; OrganizationUserRevoked
  audit entries cover recovery for any missed notifications.

member.save() retains `?` because the revoke is the security boundary
and must fail closed.
---
 src/api/core/two_factor/mod.rs | 33 ++++++++++++++++++++++++++++++---
 src/db/models/organization.rs  | 18 ++++++++++++++----
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/src/api/core/two_factor/mod.rs b/src/api/core/two_factor/mod.rs
index 2f2e47d3..1a2d7b5a 100644
--- a/src/api/core/two_factor/mod.rs
+++ b/src/api/core/two_factor/mod.rs
@@ -180,15 +180,42 @@ pub async fn enforce_2fa_policy(
     ip: &std::net::IpAddr,
     conn: &DbConn,
 ) -> EmptyResult {
-    for member in Membership::find_by_user_and_policy(&user.uuid, OrgPolicyType::TwoFactorAuthentication, conn).await {
+    for member in
+        Membership::try_find_by_user_and_policy(&user.uuid, OrgPolicyType::TwoFactorAuthentication, conn).await?
+    {
         // Policy only applies to non-Owner/non-Admin members who have accepted joining the org
         if member.atype < MembershipType::Admin {
             if CONFIG.mail_enabled() {
-                let org = Organization::find_by_uuid(&member.org_uuid, conn).await.unwrap();
-                mail::send_2fa_removed_from_org(&user.email, &org.name).await?;
+                // Best-effort notification: the revoke below is the
+                // load-bearing action and must still fire when mail is
+                // unavailable. This function is reachable from unauthenticated
+                // grants where a propagated SMTP error would (a) leave the
+                // user half-enforced (earlier loop iterations revoked +
+                // emailed, later iterations skipped) and (b) leak SMTP
+                // timing/shape upstream of the grant's generic auth-failure
+                // wrap. Log and continue; the OrganizationUserRevoked audit
+                // entries cover recovery for any missed notifications.
+                match Organization::find_by_uuid(&member.org_uuid, conn).await {
+                    Some(org) => {
+                        if let Err(e) = mail::send_2fa_removed_from_org(&user.email, &org.name).await {
+                            warn!(
+                                "enforce_2fa_policy: send_2fa_removed_from_org failed for {} / org {}: {e:#?}; revoke still applied",
+                                user.uuid, member.org_uuid
+                            );
+                        }
+                    }
+                    None => warn!(
+                        "enforce_2fa_policy: org {} disappeared mid-flight; skipping notification",
+                        member.org_uuid
+                    ),
+                }
             }
             let mut member = member;
             member.revoke();
+            // The revoke is the security boundary; unlike notification mail,
+            // it must fail closed. Propagating the error lets callers refuse
+            // the login / policy change instead of reporting success while
+            // the member remains active in a Require-2FA organization.
             member.save(conn).await?;
 
             log_event(
diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs
index d604add4..e3e2c19a 100644
--- a/src/db/models/organization.rs
+++ b/src/db/models/organization.rs
@@ -19,7 +19,7 @@ use crate::{
             organizations, users, users_collections, users_organizations,
         },
     },
-    error::MapResult,
+    error::{Error, MapResult},
 };
 use macros::UuidFromParam;
 
@@ -1033,19 +1033,29 @@ impl Membership {
     }
 
     pub async fn find_by_user_and_policy(user_uuid: &UserId, policy_type: OrgPolicyType, conn: &DbConn) -> Vec<Self> {
+        Self::try_find_by_user_and_policy(user_uuid, policy_type, conn).await.unwrap_or_default()
+    }
+
+    pub async fn try_find_by_user_and_policy(
+        user_uuid: &UserId,
+        policy_type: OrgPolicyType,
+        conn: &DbConn,
+    ) -> Result<Vec<Self>, Error> {
+        let user_uuid = user_uuid.clone();
+        let policy_type = policy_type as i32;
         conn.run(move |conn| {
             users_organizations::table
                 .inner_join(
                     org_policies::table.on(org_policies::org_uuid
                         .eq(users_organizations::org_uuid)
-                        .and(users_organizations::user_uuid.eq(user_uuid))
-                        .and(org_policies::atype.eq(policy_type as i32))
+                        .and(users_organizations::user_uuid.eq(&user_uuid))
+                        .and(org_policies::atype.eq(policy_type))
                         .and(org_policies::enabled.eq(true))),
                 )
                 .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .select(users_organizations::all_columns)
                 .load::<Self>(conn)
-                .unwrap_or_default()
+                .map_res("Error loading organization users by policy")
         })
         .await
     }