fix(auth): scope the status gate to the tenant, and gate MFA on a verified address

CHANGELOG.md

@@ -12,6 +12,35 @@ version bump.
 
 ### Security
 
+- **The account-status gate is scoped to the token's tenant.** `AuthEngine::assert_user_active`
+  resolved the account with `find_by_id(sub, None)` — and `UserRepository` says in as many words
+  that `None` is for "internal admin flows where cross-tenant access is intentional". A status
+  gate on a request path is not one of those. A repository id is unique only *within* a tenant,
+  so with a host whose ids are per-tenant serials the gate could resolve a **different tenant's**
+  account and decide on that account's status: admitting a caller whose own account is banned, or
+  refusing one who is fine. The verified token carries `tenant_id`, so there was nothing to guess.
+  `assert_user_active` now takes the tenant and the `UserStatus` extractor passes it.
+
+- **MFA management now requires a verified address, not just a live token.** `POST /auth/mfa/setup`,
+  `/verify-enable`, `/disable` and `/recovery-codes` took `AuthUser` — a valid token and nothing
+  else — so an account whose address nobody had proven could enrol, remove or re-roll a second
+  factor. Binding a factor to an unproven address binds it, and the recovery path that runs back
+  through that same address, to a mailbox that may not be the holder's. They now take a new
+  `VerifiedUser` extractor: account status **and** address verification.
+
+  `POST /auth/mfa/challenge` is deliberately **not** gated — it is part of signing in, not of
+  managing the factor. `GET /auth/me` is deliberately left on `AuthUser` alone: a pending,
+  suspended or unverified client still has to read its own profile to render the "verify your
+  email" or "suspended" screen, and `SafeAuthUser` carries `status` and `emailVerified` for
+  exactly that. Gating it would leave that client a 403 and nothing to render.
+
+  The verification half is conditional on `email_verification.required`, exactly as the login
+  path is: a deployment that does not ask for verification never marks anyone verified, so an
+  unconditional gate would make these routes permanently unreachable there.
+
+  `UserStatus` is unchanged and still gates status alone — ws-ticket, password change and the
+  session routes keep their current behaviour rather than silently acquiring a second gate.
+
 - **A delivery failure no longer logs the catalogue's subject.** `DefaultAuthEmailProvider`
   reported a failed send with the rendered subject line, and the subject comes from
   `AuthEmailCatalogue` — which is the host's. Putting the code in the subject is an ordinary

crates/bymax-auth-axum/src/extractors/mod.rs

@@ -24,7 +24,7 @@ pub use mfa::MfaSatisfied;
 pub use optional::OptionalAuthUser;
 pub use role::{RequireRole, Role};
 pub use self_or_admin::{AdminRole, SelfOrAdmin};
-pub use status::UserStatus;
+pub use status::{UserStatus, VerifiedUser};
 
 #[cfg(feature = "platform")]
 pub use platform::{PlatformRole, PlatformUser, RequirePlatformRole};

crates/bymax-auth-axum/src/extractors/status.rs

@@ -26,7 +26,49 @@ where
     async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
         let auth_state = AuthState::from_ref(state);
         let claims = verified_dashboard_claims(parts, &auth_state).await?;
-        auth_state.engine().assert_user_active(&claims.sub).await?;
+        // The tenant comes from the verified token, never from the request: a repository id is
+        // unique only within a tenant, so an unscoped lookup can resolve someone else's account.
+        auth_state
+            .engine()
+            .assert_user_active(&claims.sub, Some(&claims.tenant_id))
+            .await?;
+        Ok(Self(claims))
+    }
+}
+
+/// Requires everything [`UserStatus`] does **and** that the account's address is verified.
+///
+/// Separate from [`UserStatus`] rather than folded into it, because the two gates protect
+/// different things and the routes that want them differ. `UserStatus` guards operations a
+/// signed-in account may perform regardless of whether its address is proven — listing its own
+/// sessions, changing its password, opening a socket. This one guards the operations that must
+/// not be reachable from an address nobody has proven, MFA enrolment above all: enrolling a
+/// second factor on an unverified account binds it to a mailbox that was never shown to belong
+/// to the person, and the recovery path for that factor runs back through the same address.
+///
+/// `GET /auth/me` deliberately takes neither: a pending or suspended client still has to be able
+/// to read its own profile to render the "verify your email" or "suspended" screen, and
+/// `SafeAuthUser` carries `status` and `email_verified` for exactly that.
+///
+/// The verified half is conditional on `email_verification.required`; see
+/// `AuthEngine::assert_user_active_and_verified`.
+#[derive(Debug, Clone)]
+pub struct VerifiedUser(pub DashboardClaims);
+
+impl<S> FromRequestParts<S> for VerifiedUser
+where
+    AuthState: FromRef<S>,
+    S: Send + Sync,
+{
+    type Rejection = AuthRejection;
+
+    async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
+        let auth_state = AuthState::from_ref(state);
+        let claims = verified_dashboard_claims(parts, &auth_state).await?;
+        auth_state
+            .engine()
+            .assert_user_active_and_verified(&claims.sub, Some(&claims.tenant_id))
+            .await?;
         Ok(Self(claims))
     }
 }
@@ -35,7 +77,7 @@ where
 mod tests {
     use super::*;
     use crate::response::AuthRejection;
-    use crate::test_support::{dashboard_token, parts_with_cookie, scaffold, seed};
+    use crate::test_support::{dashboard_token, mint_token, parts_with_cookie, scaffold, seed};
     use bymax_auth_core::config::TokenDelivery;
     use bymax_auth_core::traits::UserRepository;
     use bymax_auth_types::AuthError;
@@ -59,4 +101,87 @@ mod tests {
             Err(AuthRejection(AuthError::AccountBanned))
         ));
     }
+
+    #[tokio::test]
+    async fn the_status_lookup_is_scoped_to_the_token_tenant() {
+        // A repository id is unique only WITHIN a tenant, and the repository contract says to
+        // pass `None` only for internal admin flows. This gate used to pass `None`, so a host
+        // whose ids are per-tenant serials could have it resolve a different tenant's row and
+        // decide on that account's status. The in-memory repository honours the tenant argument,
+        // so a token whose tenant does not hold the id must be refused rather than silently
+        // answered by whatever row shares the id.
+        let Some(s) = scaffold(TokenDelivery::Cookie) else { return };
+        let id = seed(&s.users, "scoped@e.com", "USER").await;
+        let token = dashboard_token(&s, &id).await;
+
+        // The seeded account lives in `t1`, and the token says `t1`: it resolves.
+        let mut parts = parts_with_cookie(&token);
+        assert!(matches!(
+            UserStatus::from_request_parts(&mut parts, &s.state).await,
+            Ok(UserStatus(_))
+        ));
+
+        // The regression this guards against is the EXTRACTOR passing `None`, so it has to be
+        // driven through the extractor. Calling the engine directly with another tenant would
+        // only prove the engine scopes — and would keep passing if the extractor stopped
+        // sending the tenant at all, which is exactly the bug.
+        //
+        // A token is minted whose claims name a tenant the account does not belong to. Scoped,
+        // the lookup finds nothing and the gate refuses; unscoped, it finds the row by bare id
+        // and lets the request through.
+        let elsewhere = DashboardClaims {
+            iss: None,
+            aud: None,
+            sub: id.clone(),
+            jti: "jti-other-tenant".to_owned(),
+            tenant_id: "some-other-tenant".to_owned(),
+            role: "USER".to_owned(),
+            token_type: bymax_auth_types::DashboardType::Dashboard,
+            status: "ACTIVE".to_owned(),
+            mfa_enabled: false,
+            mfa_verified: false,
+            iat: 1_700_000_000,
+            exp: 4_102_444_800,
+            epoch: 0,
+        };
+        let mut parts = parts_with_cookie(&mint_token(&elsewhere));
+        let refused = UserStatus::from_request_parts(&mut parts, &s.state).await;
+        assert!(
+            matches!(refused, Err(AuthRejection(AuthError::TokenInvalid))),
+            "the gate resolved an id outside the token's tenant: {refused:?}"
+        );
+    }
+
+    #[tokio::test]
+    async fn verified_user_refuses_an_unproven_address_and_passes_a_proven_one() {
+        // The MFA-management gate. `seed` creates verified accounts, so the pass arm is the
+        // seeded one; flipping the flag off is what proves the check is load-bearing rather
+        // than a status gate wearing a different name.
+        let Some(s) = scaffold(TokenDelivery::Cookie) else { return };
+        let id = seed(&s.users, "vfy@e.com", "USER").await;
+        let token = dashboard_token(&s, &id).await;
+
+        let mut parts = parts_with_cookie(&token);
+        assert!(matches!(
+            VerifiedUser::from_request_parts(&mut parts, &s.state).await,
+            Ok(VerifiedUser(_))
+        ));
+
+        let _ = s.users.update_email_verified(&id, false).await;
+        let mut parts = parts_with_cookie(&token);
+        let denied = VerifiedUser::from_request_parts(&mut parts, &s.state).await;
+        assert!(
+            matches!(denied, Err(AuthRejection(AuthError::EmailNotVerified))),
+            "an unverified address reached an MFA-management route: {denied:?}"
+        );
+
+        // The status half still applies, so the two gates compose rather than replace.
+        let _ = s.users.update_email_verified(&id, true).await;
+        let _ = s.users.update_status(&id, "SUSPENDED").await;
+        let mut parts = parts_with_cookie(&token);
+        assert!(matches!(
+            VerifiedUser::from_request_parts(&mut parts, &s.state).await,
+            Err(AuthRejection(AuthError::AccountSuspended))
+        ));
+    }
 }

crates/bymax-auth-axum/src/lib.rs

@@ -48,7 +48,7 @@ pub use dto::{
 };
 pub use extractors::{
     AdminRole, AuthUser, CurrentUser, MfaSatisfied, OptionalAuthUser, RequireRole, Role,
-    SelfOrAdmin, UserStatus,
+    SelfOrAdmin, UserStatus, VerifiedUser,
 };
 pub use rate_limit::{RateLimit, RateLimitConfig};
 pub use response::{AuthRejection, error_response};

crates/bymax-auth-axum/src/routes/mfa.rs

@@ -21,7 +21,7 @@ use crate::delivery::TokenDelivery;
 use crate::dto::{
     MfaChallengeDto, MfaDisableDto, MfaRegenerateRecoveryCodesDto, MfaSetupDto, MfaVerifyDto,
 };
-use crate::extractors::AuthUser;
+use crate::extractors::{AuthUser, VerifiedUser};
 use crate::response::error_response;
 use crate::routes::{CookieDomains, RequestMeta};
 use crate::state::{AuthState, AxumAuthConfig, ClientIpSource};
@@ -69,6 +69,11 @@ pub(crate) fn routes(config: &AxumAuthConfig, ip_source: ClientIpSource) -> Rout
 async fn setup(
     State(state): State<AuthState>,
     user: AuthUser,
+    // Status + email-verified, alongside the token: enrolling or removing a second factor on an
+    // account whose address nobody has proven binds the factor — and its recovery path — to a
+    // mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
+    // part of signing in, not of managing the factor.
+    _gate: VerifiedUser,
     headers: http::HeaderMap,
     body: axum::body::Bytes,
 ) -> Response {
@@ -81,7 +86,16 @@ async fn setup(
     };
     match state
         .engine()
-        .mfa_setup(&user.0.sub, MfaContext::Dashboard, dto.password.as_deref())
+        .mfa_setup(
+            &user.0.sub,
+            MfaContext::Dashboard,
+            // The operation targets the account named by the VERIFIED token, in the tenant that
+            // token names. Without it the read and the writes behind it resolve by bare id,
+            // which the gate above no longer does — leaving the gate checking one account and
+            // the operation touching another.
+            Some(user.0.tenant_id.as_str()),
+            dto.password.as_deref(),
+        )
         .await
     {
         Ok(result) => (
@@ -101,6 +115,11 @@ async fn setup(
 async fn verify_enable(
     State(state): State<AuthState>,
     user: AuthUser,
+    // Status + email-verified, alongside the token: enrolling or removing a second factor on an
+    // account whose address nobody has proven binds the factor — and its recovery path — to a
+    // mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
+    // part of signing in, not of managing the factor.
+    _gate: VerifiedUser,
     RequestMeta(ctx): RequestMeta,
     ValidatedJson(dto): ValidatedJson<MfaVerifyDto>,
 ) -> Response {
@@ -112,6 +131,7 @@ async fn verify_enable(
             &ctx.ip,
             &ctx.user_agent,
             MfaContext::Dashboard,
+            Some(user.0.tenant_id.as_str()),
         )
         .await
     {
@@ -183,6 +203,11 @@ fn mfa_temp_cookie(cookies: &tower_cookies::Cookies) -> Option<String> {
 async fn disable(
     State(state): State<AuthState>,
     user: AuthUser,
+    // Status + email-verified, alongside the token: enrolling or removing a second factor on an
+    // account whose address nobody has proven binds the factor — and its recovery path — to a
+    // mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
+    // part of signing in, not of managing the factor.
+    _gate: VerifiedUser,
     RequestMeta(ctx): RequestMeta,
     ValidatedJson(dto): ValidatedJson<MfaDisableDto>,
 ) -> Response {
@@ -194,6 +219,7 @@ async fn disable(
             &ctx.ip,
             &ctx.user_agent,
             MfaContext::Dashboard,
+            Some(user.0.tenant_id.as_str()),
         )
         .await
     {
@@ -207,6 +233,11 @@ async fn disable(
 async fn recovery_codes(
     State(state): State<AuthState>,
     user: AuthUser,
+    // Status + email-verified, alongside the token: enrolling or removing a second factor on an
+    // account whose address nobody has proven binds the factor — and its recovery path — to a
+    // mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
+    // part of signing in, not of managing the factor.
+    _gate: VerifiedUser,
     RequestMeta(ctx): RequestMeta,
     ValidatedJson(dto): ValidatedJson<MfaRegenerateRecoveryCodesDto>,
 ) -> Response {
@@ -218,6 +249,7 @@ async fn recovery_codes(
             &ctx.ip,
             &ctx.user_agent,
             MfaContext::Dashboard,
+            Some(user.0.tenant_id.as_str()),
         )
         .await
     {

crates/bymax-auth-axum/src/routes/platform_mfa.rs

@@ -61,7 +61,13 @@ async fn setup(
         };
     match state
         .engine()
-        .mfa_setup(&user.0.sub, MfaContext::Platform, dto.password.as_deref())
+        .mfa_setup(
+            &user.0.sub,
+            MfaContext::Platform,
+            // A platform admin is cross-tenant by definition, so there is no tenant to scope by.
+            None,
+            dto.password.as_deref(),
+        )
         .await
     {
         Ok(result) => (
@@ -92,6 +98,8 @@ async fn verify_enable(
             &ctx.ip,
             &ctx.user_agent,
             MfaContext::Platform,
+            // A platform admin is cross-tenant by definition, so there is no tenant to scope by.
+            None,
         )
         .await
     {
@@ -115,6 +123,8 @@ async fn disable(
             &ctx.ip,
             &ctx.user_agent,
             MfaContext::Platform,
+            // A platform admin is cross-tenant by definition, so there is no tenant to scope by.
+            None,
         )
         .await
     {
@@ -138,6 +148,8 @@ async fn recovery_codes(
             &ctx.ip,
             &ctx.user_agent,
             MfaContext::Platform,
+            // A platform admin is cross-tenant by definition, so there is no tenant to scope by.
+            None,
         )
         .await
     {