diff --git a/src/infrastructure/services/login_lockout_service.rs b/src/infrastructure/services/login_lockout_service.rs index 3b45a29d..47916909 100644 --- a/src/infrastructure/services/login_lockout_service.rs +++ b/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)) @@ -60,8 +60,8 @@ impl LoginLockoutService { /// 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 + /// 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. @@ -106,7 +106,7 @@ impl LoginLockoutService { new_count } - /// Record a successful login — resets the failure counter for this + /// 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) { @@ -137,7 +137,7 @@ mod tests { assert!(svc.check("alice", IP1).is_ok()); svc.record_failure("alice", IP1); svc.record_failure("alice", IP1); - // 2 failures — still under threshold + // 2 failures, still under threshold assert!(svc.check("alice", IP1).is_ok()); } @@ -156,7 +156,7 @@ mod tests { svc.record_failure("carol", IP1); svc.record_failure("carol", IP1); svc.record_success("carol", IP1); - // Counter reset — should be allowed again + // 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()); @@ -189,11 +189,11 @@ mod tests { // 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" + "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 — + /// 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] diff --git a/src/interfaces/api/handlers/auth_handler.rs b/src/interfaces/api/handlers/auth_handler.rs index bce76db2..1261558e 100644 --- a/src/interfaces/api/handlers/auth_handler.rs +++ b/src/interfaces/api/handlers/auth_handler.rs @@ -22,7 +22,7 @@ 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> { Router::new() .route("/status", get(get_system_status)) @@ -36,7 +36,7 @@ pub fn auth_public_routes() -> Router> { .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> { use axum::routing::patch; @@ -48,7 +48,7 @@ pub fn auth_protected_routes() -> Router> { .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> { Router::new().route("/login", post(login)) @@ -62,7 +62,7 @@ pub fn refresh_route() -> Router> { 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> { Router::new().route("/setup", post(setup_admin)) } @@ -330,7 +330,7 @@ pub async fn login( .await { Ok(auth_response) => { - // ── Successful login — reset lockout counter ── + // ── Successful login, reset lockout counter ── auth_service .login_lockout .record_success(&dto.username, &client_ip); @@ -362,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") @@ -690,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 @@ -726,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: {}", @@ -937,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); @@ -949,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() diff --git a/src/interfaces/middleware/rate_limit.rs b/src/interfaces/middleware/rate_limit.rs index ede1fac2..ae765ae4 100644 --- a/src/interfaces/middleware/rate_limit.rs +++ b/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); diff --git a/src/interfaces/nextcloud/basic_auth_middleware.rs b/src/interfaces/nextcloud/basic_auth_middleware.rs index 33013361..70b3c7e1 100644 --- a/src/interfaces/nextcloud/basic_auth_middleware.rs +++ b/src/interfaces/nextcloud/basic_auth_middleware.rs @@ -63,7 +63,7 @@ pub async fn basic_auth_middleware( parse_basic_auth(auth_header).ok_or(NextcloudAuthError::Unauthorized)?; // Check account lockout before attempting password verification (saves CPU). - // The lockout is per (account, IP) — see #323 for rationale. + // 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() @@ -73,7 +73,7 @@ pub async fn basic_auth_middleware( username = %username, client_ip = %client_ip, lockout_remaining_secs = secs, - "[NC] Account locked — too many failed attempts from this IP" + "[NC] Account locked, too many failed attempts from this IP" ); return Err(NextcloudAuthError::Unauthorized); }