Merge pull request #326 from SAY-5/fix/per-ip-account-lockout-323

src/infrastructure/services/login_lockout_service.rs

@@ -1,9 +1,9 @@
-//! Account lockout service — blocks login for an account after N consecutive
+//! Account lockout service, blocks login for an account after N consecutive
 //! failed attempts.
 //!
 //! Uses a `moka` TTL cache so that:
 //! * Failed-attempt counters automatically expire after the lockout window.
-//! * No database writes are needed — this is **in-memory** and therefore
+//! * No database writes are needed, this is **in-memory** and therefore
 //!   per-instance.  If OxiCloud is deployed behind a load balancer with
 //!   multiple replicas, a sticky-session or shared Redis store would be
 //!   needed for cross-instance coordination (out of scope for v1).
@@ -40,9 +40,9 @@ pub struct LoginLockoutService {
 impl LoginLockoutService {
     /// Create a new lockout service.
     ///
-    /// * `max_failures`  — e.g. `5` (lock after 5 bad passwords)
-    /// * `lockout_secs`  — e.g. `900` (15-minute lockout)
-    /// * `max_accounts`  — upper bound on tracked accounts (evicts LRU)
+    /// * `max_failures` , e.g. `5` (lock after 5 bad passwords)
+    /// * `lockout_secs` , e.g. `900` (15-minute lockout)
+    /// * `max_accounts` , upper bound on tracked accounts (evicts LRU)
     pub fn new(max_failures: u32, lockout_secs: u64, max_accounts: u64) -> Self {
         let cache = Cache::builder()
             .time_to_live(Duration::from_secs(lockout_secs))
@@ -55,12 +55,28 @@ impl LoginLockoutService {
         }
     }
 
-    /// Check whether the account is currently locked.
+    /// Build the cache key from the (lowercased) username and the client IP.
+    ///
+    /// The IP is part of the key so that an attacker flooding bad passwords
+    /// from one address cannot lock a legitimate user out of the same account
+    /// from a different address (issue #323). When the caller cannot resolve
+    /// a real IP, e.g. `OXICLOUD_TRUST_PROXY_HEADERS=false` and the peer
+    /// address isn't available, `client_ip` should be a non-empty constant
+    /// like `"unknown"`; in that pathological case we fall back to
+    /// account-scoped lockout, which is no worse than the previous
+    /// behaviour.
+    fn key(username: &str, client_ip: &str) -> String {
+        // `|` is not valid in either a username or an IP literal so it makes
+        // the username/ip boundary unambiguous.
+        format!("{}|{}", username.to_lowercase(), client_ip)
+    }
+
+    /// Check whether the (account, IP) pair is currently locked.
     ///
     /// Returns `Ok(())` if the user may attempt login, or
     /// `Err(remaining_secs)` with the *approximate* remaining lockout time.
-    pub fn check(&self, username: &str) -> Result<(), u64> {
-        if let Some(rec) = self.cache.get(&username.to_lowercase())
+    pub fn check(&self, username: &str, client_ip: &str) -> Result<(), u64> {
+        if let Some(rec) = self.cache.get(&Self::key(username, client_ip))
             && rec.count >= self.max_failures
         {
             // The entry exists and is over the threshold.  Because moka
@@ -71,8 +87,8 @@ impl LoginLockoutService {
     }
 
     /// Record a failed login attempt.  Returns the new failure count.
-    pub fn record_failure(&self, username: &str) -> u32 {
-        let key = username.to_lowercase();
+    pub fn record_failure(&self, username: &str, client_ip: &str) -> u32 {
+        let key = Self::key(username, client_ip);
         let new_count = self.cache.get(&key).map(|r| r.count + 1).unwrap_or(1);
         self.cache
             .insert(key.clone(), FailureRecord { count: new_count });
@@ -80,18 +96,21 @@ impl LoginLockoutService {
         if new_count >= self.max_failures {
             tracing::warn!(
                 username = %username,
+                client_ip = %client_ip,
                 attempts = new_count,
                 lockout_secs = self.lockout_secs,
-                "Account temporarily locked after {} consecutive failed login attempts",
+                "Account temporarily locked after {} consecutive failed login attempts from this IP",
                 new_count,
             );
         }
         new_count
     }
 
-    /// Record a successful login — resets the failure counter.
-    pub fn record_success(&self, username: &str) {
-        self.cache.invalidate(&username.to_lowercase());
+    /// Record a successful login, resets the failure counter for this
+    /// (account, IP) pair so the user isn't penalised for stray earlier
+    /// failures from the same address.
+    pub fn record_success(&self, username: &str, client_ip: &str) {
+        self.cache.invalidate(&Self::key(username, client_ip));
     }
 
     /// Maximum failures before lockout (used to inform callers / error messages).
@@ -109,42 +128,88 @@ impl LoginLockoutService {
 mod tests {
     use super::*;
 
+    const IP1: &str = "1.1.1.1";
+    const IP2: &str = "2.2.2.2";
+
     #[test]
     fn allows_login_under_threshold() {
         let svc = LoginLockoutService::new(3, 60, 100);
-        assert!(svc.check("alice").is_ok());
-        svc.record_failure("alice");
-        svc.record_failure("alice");
-        // 2 failures — still under threshold
-        assert!(svc.check("alice").is_ok());
+        assert!(svc.check("alice", IP1).is_ok());
+        svc.record_failure("alice", IP1);
+        svc.record_failure("alice", IP1);
+        // 2 failures, still under threshold
+        assert!(svc.check("alice", IP1).is_ok());
     }
 
     #[test]
     fn locks_after_threshold() {
         let svc = LoginLockoutService::new(3, 60, 100);
-        svc.record_failure("bob");
-        svc.record_failure("bob");
-        svc.record_failure("bob");
-        assert!(svc.check("bob").is_err());
+        svc.record_failure("bob", IP1);
+        svc.record_failure("bob", IP1);
+        svc.record_failure("bob", IP1);
+        assert!(svc.check("bob", IP1).is_err());
     }
 
     #[test]
     fn resets_on_success() {
         let svc = LoginLockoutService::new(3, 60, 100);
-        svc.record_failure("carol");
-        svc.record_failure("carol");
-        svc.record_success("carol");
-        // Counter reset — should be allowed again
-        assert!(svc.check("carol").is_ok());
-        svc.record_failure("carol"); // starts over at 1
-        assert!(svc.check("carol").is_ok());
+        svc.record_failure("carol", IP1);
+        svc.record_failure("carol", IP1);
+        svc.record_success("carol", IP1);
+        // Counter reset, should be allowed again
+        assert!(svc.check("carol", IP1).is_ok());
+        svc.record_failure("carol", IP1); // starts over at 1
+        assert!(svc.check("carol", IP1).is_ok());
     }
 
     #[test]
     fn case_insensitive() {
         let svc = LoginLockoutService::new(2, 60, 100);
-        svc.record_failure("Dave");
-        svc.record_failure("dave");
-        assert!(svc.check("DAVE").is_err());
+        svc.record_failure("Dave", IP1);
+        svc.record_failure("dave", IP1);
+        assert!(svc.check("DAVE", IP1).is_err());
+    }
+
+    /// Regression test for #323: flooding bad passwords from one IP must
+    /// NOT lock the account out for legitimate users coming from a
+    /// different IP.
+    #[test]
+    fn does_not_lock_out_other_ips_for_same_account() {
+        let svc = LoginLockoutService::new(3, 60, 100);
+
+        // Attacker hammers the account from IP1 until it locks for that IP.
+        for _ in 0..3 {
+            svc.record_failure("admin", IP1);
+        }
+        assert!(
+            svc.check("admin", IP1).is_err(),
+            "attacker IP must be locked"
+        );
+
+        // A legitimate user coming from IP2 must still be allowed to try.
+        assert!(
+            svc.check("admin", IP2).is_ok(),
+            "second IP must not inherit the lockout, that's the #323 DOS"
+        );
+    }
+
+    /// A successful login on one IP must clear *that* IP's counter only,
+    /// it should NOT silently absolve a separate, ongoing brute-force from
+    /// a different IP against the same account.
+    #[test]
+    fn success_resets_only_the_acting_ip() {
+        let svc = LoginLockoutService::new(3, 60, 100);
+
+        for _ in 0..3 {
+            svc.record_failure("admin", IP1);
+        }
+        // Genuine login from IP2 succeeds; should reset IP2 counter (which
+        // is already 0 here) but leave IP1's lockout intact.
+        svc.record_success("admin", IP2);
+
+        assert!(
+            svc.check("admin", IP1).is_err(),
+            "IP1 must remain locked after IP2's success"
+        );
     }
 }

src/interfaces/api/handlers/auth_handler.rs

@@ -1,10 +1,11 @@
 use axum::{
     Router,
-    extract::{Json, Query, State},
+    extract::{ConnectInfo, Json, Query, State},
     http::{HeaderMap, StatusCode},
     response::{IntoResponse, Redirect, Response},
     routing::{get, post, put},
 };
+use std::net::SocketAddr;
 use std::sync::Arc;
 use utoipa::ToSchema;
 use uuid::Uuid;
@@ -18,9 +19,10 @@ use crate::common::di::AppState;
 use crate::interfaces::api::cookie_auth;
 use crate::interfaces::errors::AppError;
 use crate::interfaces::middleware::auth::CurrentUserId;
+use crate::interfaces::middleware::trusted_proxy::client_ip_from_parts;
 use serde::Deserialize;
 
-/// Public auth routes — no authentication required.
+/// Public auth routes, no authentication required.
 pub fn auth_public_routes() -> Router<Arc<AppState>> {
     Router::new()
         .route("/status", get(get_system_status))
@@ -34,7 +36,7 @@ pub fn auth_public_routes() -> Router<Arc<AppState>> {
         .route("/magic-link/send", post(send_magic_link))
 }
 
-/// Protected auth routes — require authentication (auth + CSRF middleware
+/// Protected auth routes, require authentication (auth + CSRF middleware
 /// must be applied by the caller in main.rs).
 pub fn auth_protected_routes() -> Router<Arc<AppState>> {
     use axum::routing::patch;
@@ -46,7 +48,7 @@ pub fn auth_protected_routes() -> Router<Arc<AppState>> {
         .route("/logout", post(logout))
 }
 
-/// Rate-limited auth routes — split out so main.rs can apply per-endpoint
+/// Rate-limited auth routes, split out so main.rs can apply per-endpoint
 /// rate limiting middleware independently.
 pub fn login_route() -> Router<Arc<AppState>> {
     Router::new().route("/login", post(login))
@@ -60,7 +62,7 @@ pub fn refresh_route() -> Router<Arc<AppState>> {
     Router::new().route("/refresh", post(refresh_token))
 }
 
-/// Public setup route — only active before the first admin is created.
+/// Public setup route, only active before the first admin is created.
 pub fn setup_route() -> Router<Arc<AppState>> {
     Router::new().route("/setup", post(setup_admin))
 }
@@ -260,6 +262,7 @@ pub async fn register(
 )]
 pub async fn login(
     State(state): State<Arc<AppState>>,
+    ConnectInfo(peer): ConnectInfo<SocketAddr>,
     headers: HeaderMap,
     Json(dto): Json<LoginDto>,
 ) -> Result<Response, AppError> {
@@ -281,13 +284,24 @@ pub async fn login(
     };
 
     // ── Account lockout check ──────────────────────────────────────────
-    // Reject immediately if the account has too many consecutive failures.
-    // This runs BEFORE Argon2 to save CPU under brute-force attacks.
-    if let Err(lockout_secs) = auth_service.login_lockout.check(&dto.username) {
+    // Reject immediately if (this account, this IP) has too many consecutive
+    // failures. The IP is part of the key so an attacker flooding bad
+    // passwords from one address cannot lock a legitimate user out of the
+    // same account from a different address (issue #323). The check runs
+    // BEFORE Argon2 to save CPU under brute-force attacks.
+    let client_ip = client_ip_from_parts(&headers, Some(peer), false);
+    if let Err(lockout_secs) = auth_service
+        .login_lockout
+        .check(&dto.username, &client_ip)
+    {
         tracing::warn!(
+            target: "audit",
+            event = "auth.login",
+            reason = "account_ip_locked",
             username = %dto.username,
+            ip = %client_ip,
             lockout_secs = lockout_secs,
-            "Login rejected — account temporarily locked"
+            "Login rejected: account temporarily locked for this IP"
         );
         return Err(AppError::new(
             StatusCode::TOO_MANY_REQUESTS,
@@ -316,8 +330,10 @@ pub async fn login(
         .await
     {
         Ok(auth_response) => {
-            // ── Successful login — reset lockout counter ──
-            auth_service.login_lockout.record_success(&dto.username);
+            // ── Successful login, reset lockout counter ──
+            auth_service
+                .login_lockout
+                .record_success(&dto.username, &client_ip);
 
             tracing::info!("Login successful for user: {}", dto.username);
             // Log the response structure for debugging
@@ -346,7 +362,7 @@ pub async fn login(
             cookie_auth::append_csrf_cookie(response.headers_mut(), auth_response.expires_in);
 
             // Diagnostic: warn when Secure cookies are set but the request
-            // arrived over plain HTTP — the browser will reject them (#241).
+            // arrived over plain HTTP, the browser will reject them (#241).
             if cookie_auth::is_cookie_secure() {
                 let is_tls = headers
                     .get("x-forwarded-proto")
@@ -367,7 +383,9 @@ pub async fn login(
         }
         Err(err) => {
             // ── Record failed attempt for lockout tracking ──
-            auth_service.login_lockout.record_failure(&dto.username);
+            auth_service
+                .login_lockout
+                .record_failure(&dto.username, &client_ip);
             tracing::error!("Login failed for user {}: {}", dto.username, err);
             Err(err.into())
         }
@@ -672,7 +690,7 @@ pub async fn setup_admin(
         ));
     }
 
-    // 4. ATOMIC: claim initialization — only one concurrent request can win.
+    // 4. ATOMIC: claim initialization, only one concurrent request can win.
     //    We use Uuid::nil() as a placeholder because the admin user
     //    doesn't exist yet. It will be updated to the real id below.
     let claimed = admin_svc
@@ -708,7 +726,7 @@ pub async fn setup_admin(
     // 5. Update the initialization record with the real admin user_id
     let real_user_id = Uuid::parse_str(&user.id).unwrap_or_default();
     if let Err(e) = admin_svc.mark_system_initialized(real_user_id).await {
-        // Not fatal — the claim already prevents concurrent re-initialization,
+        // Not fatal, the claim already prevents concurrent re-initialization,
         // and the "pending" marker is still "true" so the system stays locked.
         tracing::error!(
             "Created admin but failed to update initialized_by with real user id: {}",
@@ -919,7 +937,7 @@ pub async fn oidc_callback(
 
     match result {
         OidcCallbackResult::WebLogin { exchange_code } => {
-            // Regular web login — redirect to frontend with exchange code
+            // Regular web login, redirect to frontend with exchange code
             let config = auth_app.oidc_config().unwrap();
             let frontend_url = config.frontend_url.trim_end_matches('/');
             let redirect_url = format!("{}/?oidc_code={}", frontend_url, exchange_code);
@@ -931,7 +949,7 @@ pub async fn oidc_callback(
             user_id,
             username,
         } => {
-            // Nextcloud Login Flow v2 — create app password and complete flow
+            // Nextcloud Login Flow v2, create app password and complete flow
             let nextcloud = state
                 .nextcloud
                 .as_ref()

src/interfaces/middleware/rate_limit.rs

@@ -36,9 +36,9 @@ pub struct RateLimiter {
 impl RateLimiter {
     /// Create a new rate limiter.
     ///
-    /// * `max_requests` — ceiling per IP within the window
-    /// * `window_secs`  — sliding window duration
-    /// * `max_entries`  — upper bound on tracked IPs (evicts LRU when exceeded)
+    /// * `max_requests`, ceiling per IP within the window
+    /// * `window_secs` , sliding window duration
+    /// * `max_entries` , upper bound on tracked IPs (evicts LRU when exceeded)
     pub fn new(max_requests: u32, window_secs: u64, max_entries: u64) -> Self {
         let cache = Cache::builder()
             .time_to_live(Duration::from_secs(window_secs))
@@ -65,7 +65,7 @@ impl RateLimiter {
         // the *existing* value when the key was already present, we must always
         // re-insert so the counter actually advances. The TTL of the **first**
         // insert still governs eviction because moka uses insert-time TTL.
-        // However, on re-insert moka resets the TTL — for rate limiting this
+        // However, on re-insert moka resets the TTL, for rate limiting this
         // is fine because it means the window "slides" forward on activity.
         self.cache.insert(ip.to_string(), count);
 

src/interfaces/middleware/trusted_proxy.rs

@@ -143,11 +143,22 @@ pub fn client_ip<B>(req: &Request<B>, include_port: bool) -> String {
         .get::<ConnectInfo<SocketAddr>>()
         .map(|ci| ci.0);
 
+    client_ip_from_parts(req.headers(), peer, include_port)
+}
+
+/// Same as [`client_ip`], but operates on already-extracted parts (headers
+/// plus an optional TCP peer).  Handlers that don't take a full `Request<B>`,
+/// e.g. those that consume the body via `Json<…>`, can still derive a stable
+/// client identifier with this entry point.
+pub fn client_ip_from_parts(
+    headers: &axum::http::HeaderMap,
+    peer: Option<SocketAddr>,
+    include_port: bool,
+) -> String {
     if let Some(peer_addr) = peer {
         if is_trusted_proxy(peer_addr.ip()) {
             // Try X-Forwarded-For first (leftmost = original client)
-            if let Some(xff) = req
-                .headers()
+            if let Some(xff) = headers
                 .get("x-forwarded-for")
                 .and_then(|v| v.to_str().ok())
                 && let Some(ip) = xff
@@ -160,8 +171,7 @@ pub fn client_ip<B>(req: &Request<B>, include_port: bool) -> String {
             }
 
             // Then X-Real-Ip
-            if let Some(xri) = req
-                .headers()
+            if let Some(xri) = headers
                 .get("x-real-ip")
                 .and_then(|v| v.to_str().ok())
                 .map(str::trim)

src/interfaces/nextcloud/basic_auth_middleware.rs

@@ -62,14 +62,18 @@ pub async fn basic_auth_middleware(
     let (username, password) =
         parse_basic_auth(auth_header).ok_or(NextcloudAuthError::Unauthorized)?;
 
-    // Check account lockout before attempting password verification (saves CPU)
+    // Check account lockout before attempting password verification (saves CPU).
+    // The lockout is per (account, IP), see #323 for rationale.
+    let client_ip =
+        crate::interfaces::middleware::rate_limit::extract_client_ip(&request);
     if let Some(auth_svc) = state.auth_service.as_ref()
-        && let Err(secs) = auth_svc.login_lockout.check(&username)
+        && let Err(secs) = auth_svc.login_lockout.check(&username, &client_ip)
     {
         tracing::warn!(
             username = %username,
+            client_ip = %client_ip,
             lockout_remaining_secs = secs,
-            "[NC] Account locked — too many failed attempts"
+            "[NC] Account locked, too many failed attempts from this IP"
         );
         return Err(NextcloudAuthError::Unauthorized);
     }
@@ -87,7 +91,7 @@ pub async fn basic_auth_middleware(
         Ok((user_id, uname, email, role)) => {
             // Reset lockout counter on success
             if let Some(auth_svc) = state.auth_service.as_ref() {
-                auth_svc.login_lockout.record_success(&username);
+                auth_svc.login_lockout.record_success(&username, &client_ip);
             }
             // External users must never authenticate against the NC
             // surface — that whole subtree (WebDAV files, uploads,
@@ -134,7 +138,7 @@ pub async fn basic_auth_middleware(
         Err(_) => {
             // Record failed attempt for lockout tracking
             if let Some(auth_svc) = state.auth_service.as_ref() {
-                auth_svc.login_lockout.record_failure(&username);
+                auth_svc.login_lockout.record_failure(&username, &client_ip);
             }
             Err(NextcloudAuthError::Unauthorized)
         }