chore: remove em-dashes from comments
This commit is contained in:
@@ -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.
|
//! failed attempts.
|
||||||
//!
|
//!
|
||||||
//! Uses a `moka` TTL cache so that:
|
//! Uses a `moka` TTL cache so that:
|
||||||
//! * Failed-attempt counters automatically expire after the lockout window.
|
//! * 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
|
//! per-instance. If OxiCloud is deployed behind a load balancer with
|
||||||
//! multiple replicas, a sticky-session or shared Redis store would be
|
//! multiple replicas, a sticky-session or shared Redis store would be
|
||||||
//! needed for cross-instance coordination (out of scope for v1).
|
//! needed for cross-instance coordination (out of scope for v1).
|
||||||
@@ -40,9 +40,9 @@ pub struct LoginLockoutService {
|
|||||||
impl LoginLockoutService {
|
impl LoginLockoutService {
|
||||||
/// Create a new lockout service.
|
/// Create a new lockout service.
|
||||||
///
|
///
|
||||||
/// * `max_failures` — e.g. `5` (lock after 5 bad passwords)
|
/// * `max_failures` , e.g. `5` (lock after 5 bad passwords)
|
||||||
/// * `lockout_secs` — e.g. `900` (15-minute lockout)
|
/// * `lockout_secs` , e.g. `900` (15-minute lockout)
|
||||||
/// * `max_accounts` — upper bound on tracked accounts (evicts LRU)
|
/// * `max_accounts` , upper bound on tracked accounts (evicts LRU)
|
||||||
pub fn new(max_failures: u32, lockout_secs: u64, max_accounts: u64) -> Self {
|
pub fn new(max_failures: u32, lockout_secs: u64, max_accounts: u64) -> Self {
|
||||||
let cache = Cache::builder()
|
let cache = Cache::builder()
|
||||||
.time_to_live(Duration::from_secs(lockout_secs))
|
.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
|
/// 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 one address cannot lock a legitimate user out of the same account
|
||||||
/// from a different address (issue #323). When the caller cannot resolve
|
/// from a different address (issue #323). When the caller cannot resolve
|
||||||
/// a real IP — e.g. `OXICLOUD_TRUST_PROXY_HEADERS=false` and the peer
|
/// 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
|
/// address isn't available, `client_ip` should be a non-empty constant
|
||||||
/// like `"unknown"`; in that pathological case we fall back to
|
/// like `"unknown"`; in that pathological case we fall back to
|
||||||
/// account-scoped lockout, which is no worse than the previous
|
/// account-scoped lockout, which is no worse than the previous
|
||||||
/// behaviour.
|
/// behaviour.
|
||||||
@@ -106,7 +106,7 @@ impl LoginLockoutService {
|
|||||||
new_count
|
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
|
/// (account, IP) pair so the user isn't penalised for stray earlier
|
||||||
/// failures from the same address.
|
/// failures from the same address.
|
||||||
pub fn record_success(&self, username: &str, client_ip: &str) {
|
pub fn record_success(&self, username: &str, client_ip: &str) {
|
||||||
@@ -137,7 +137,7 @@ mod tests {
|
|||||||
assert!(svc.check("alice", IP1).is_ok());
|
assert!(svc.check("alice", IP1).is_ok());
|
||||||
svc.record_failure("alice", IP1);
|
svc.record_failure("alice", IP1);
|
||||||
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());
|
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_failure("carol", IP1);
|
svc.record_failure("carol", IP1);
|
||||||
svc.record_success("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());
|
assert!(svc.check("carol", IP1).is_ok());
|
||||||
svc.record_failure("carol", IP1); // starts over at 1
|
svc.record_failure("carol", IP1); // starts over at 1
|
||||||
assert!(svc.check("carol", IP1).is_ok());
|
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.
|
// A legitimate user coming from IP2 must still be allowed to try.
|
||||||
assert!(
|
assert!(
|
||||||
svc.check("admin", IP2).is_ok(),
|
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
|
/// it should NOT silently absolve a separate, ongoing brute-force from
|
||||||
/// a different IP against the same account.
|
/// a different IP against the same account.
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -22,7 +22,7 @@ use crate::interfaces::middleware::auth::CurrentUserId;
|
|||||||
use crate::interfaces::middleware::trusted_proxy::client_ip_from_parts;
|
use crate::interfaces::middleware::trusted_proxy::client_ip_from_parts;
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
|
|
||||||
/// Public auth routes — no authentication required.
|
/// Public auth routes, no authentication required.
|
||||||
pub fn auth_public_routes() -> Router<Arc<AppState>> {
|
pub fn auth_public_routes() -> Router<Arc<AppState>> {
|
||||||
Router::new()
|
Router::new()
|
||||||
.route("/status", get(get_system_status))
|
.route("/status", get(get_system_status))
|
||||||
@@ -36,7 +36,7 @@ pub fn auth_public_routes() -> Router<Arc<AppState>> {
|
|||||||
.route("/magic-link/send", post(send_magic_link))
|
.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).
|
/// must be applied by the caller in main.rs).
|
||||||
pub fn auth_protected_routes() -> Router<Arc<AppState>> {
|
pub fn auth_protected_routes() -> Router<Arc<AppState>> {
|
||||||
use axum::routing::patch;
|
use axum::routing::patch;
|
||||||
@@ -48,7 +48,7 @@ pub fn auth_protected_routes() -> Router<Arc<AppState>> {
|
|||||||
.route("/logout", post(logout))
|
.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.
|
/// rate limiting middleware independently.
|
||||||
pub fn login_route() -> Router<Arc<AppState>> {
|
pub fn login_route() -> Router<Arc<AppState>> {
|
||||||
Router::new().route("/login", post(login))
|
Router::new().route("/login", post(login))
|
||||||
@@ -62,7 +62,7 @@ pub fn refresh_route() -> Router<Arc<AppState>> {
|
|||||||
Router::new().route("/refresh", post(refresh_token))
|
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>> {
|
pub fn setup_route() -> Router<Arc<AppState>> {
|
||||||
Router::new().route("/setup", post(setup_admin))
|
Router::new().route("/setup", post(setup_admin))
|
||||||
}
|
}
|
||||||
@@ -330,7 +330,7 @@ pub async fn login(
|
|||||||
.await
|
.await
|
||||||
{
|
{
|
||||||
Ok(auth_response) => {
|
Ok(auth_response) => {
|
||||||
// ── Successful login — reset lockout counter ──
|
// ── Successful login, reset lockout counter ──
|
||||||
auth_service
|
auth_service
|
||||||
.login_lockout
|
.login_lockout
|
||||||
.record_success(&dto.username, &client_ip);
|
.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);
|
cookie_auth::append_csrf_cookie(response.headers_mut(), auth_response.expires_in);
|
||||||
|
|
||||||
// Diagnostic: warn when Secure cookies are set but the request
|
// 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() {
|
if cookie_auth::is_cookie_secure() {
|
||||||
let is_tls = headers
|
let is_tls = headers
|
||||||
.get("x-forwarded-proto")
|
.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
|
// We use Uuid::nil() as a placeholder because the admin user
|
||||||
// doesn't exist yet. It will be updated to the real id below.
|
// doesn't exist yet. It will be updated to the real id below.
|
||||||
let claimed = admin_svc
|
let claimed = admin_svc
|
||||||
@@ -726,7 +726,7 @@ pub async fn setup_admin(
|
|||||||
// 5. Update the initialization record with the real admin user_id
|
// 5. Update the initialization record with the real admin user_id
|
||||||
let real_user_id = Uuid::parse_str(&user.id).unwrap_or_default();
|
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 {
|
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.
|
// and the "pending" marker is still "true" so the system stays locked.
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
"Created admin but failed to update initialized_by with real user id: {}",
|
"Created admin but failed to update initialized_by with real user id: {}",
|
||||||
@@ -937,7 +937,7 @@ pub async fn oidc_callback(
|
|||||||
|
|
||||||
match result {
|
match result {
|
||||||
OidcCallbackResult::WebLogin { exchange_code } => {
|
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 config = auth_app.oidc_config().unwrap();
|
||||||
let frontend_url = config.frontend_url.trim_end_matches('/');
|
let frontend_url = config.frontend_url.trim_end_matches('/');
|
||||||
let redirect_url = format!("{}/?oidc_code={}", frontend_url, exchange_code);
|
let redirect_url = format!("{}/?oidc_code={}", frontend_url, exchange_code);
|
||||||
@@ -949,7 +949,7 @@ pub async fn oidc_callback(
|
|||||||
user_id,
|
user_id,
|
||||||
username,
|
username,
|
||||||
} => {
|
} => {
|
||||||
// Nextcloud Login Flow v2 — create app password and complete flow
|
// Nextcloud Login Flow v2, create app password and complete flow
|
||||||
let nextcloud = state
|
let nextcloud = state
|
||||||
.nextcloud
|
.nextcloud
|
||||||
.as_ref()
|
.as_ref()
|
||||||
|
|||||||
@@ -36,9 +36,9 @@ pub struct RateLimiter {
|
|||||||
impl RateLimiter {
|
impl RateLimiter {
|
||||||
/// Create a new rate limiter.
|
/// Create a new rate limiter.
|
||||||
///
|
///
|
||||||
/// * `max_requests` — ceiling per IP within the window
|
/// * `max_requests`, ceiling per IP within the window
|
||||||
/// * `window_secs` — sliding window duration
|
/// * `window_secs` , sliding window duration
|
||||||
/// * `max_entries` — upper bound on tracked IPs (evicts LRU when exceeded)
|
/// * `max_entries` , upper bound on tracked IPs (evicts LRU when exceeded)
|
||||||
pub fn new(max_requests: u32, window_secs: u64, max_entries: u64) -> Self {
|
pub fn new(max_requests: u32, window_secs: u64, max_entries: u64) -> Self {
|
||||||
let cache = Cache::builder()
|
let cache = Cache::builder()
|
||||||
.time_to_live(Duration::from_secs(window_secs))
|
.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
|
// the *existing* value when the key was already present, we must always
|
||||||
// re-insert so the counter actually advances. The TTL of the **first**
|
// re-insert so the counter actually advances. The TTL of the **first**
|
||||||
// insert still governs eviction because moka uses insert-time TTL.
|
// 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.
|
// is fine because it means the window "slides" forward on activity.
|
||||||
self.cache.insert(ip.to_string(), count);
|
self.cache.insert(ip.to_string(), count);
|
||||||
|
|
||||||
|
|||||||
@@ -63,7 +63,7 @@ pub async fn basic_auth_middleware(
|
|||||||
parse_basic_auth(auth_header).ok_or(NextcloudAuthError::Unauthorized)?;
|
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.
|
// The lockout is per (account, IP), see #323 for rationale.
|
||||||
let client_ip =
|
let client_ip =
|
||||||
crate::interfaces::middleware::rate_limit::extract_client_ip(&request);
|
crate::interfaces::middleware::rate_limit::extract_client_ip(&request);
|
||||||
if let Some(auth_svc) = state.auth_service.as_ref()
|
if let Some(auth_svc) = state.auth_service.as_ref()
|
||||||
@@ -73,7 +73,7 @@ pub async fn basic_auth_middleware(
|
|||||||
username = %username,
|
username = %username,
|
||||||
client_ip = %client_ip,
|
client_ip = %client_ip,
|
||||||
lockout_remaining_secs = secs,
|
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);
|
return Err(NextcloudAuthError::Unauthorized);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user