Browse Source

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.
pull/7297/head
Zaid Marji 2 weeks ago
parent
commit
6be69056d1
  1. 33
      src/api/core/two_factor/mod.rs
  2. 18
      src/db/models/organization.rs

33
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(

18
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
}

Loading…
Cancel
Save