diff --git a/.github/workflows/site-e2e.yml b/.github/workflows/site-e2e.yml index 5d80035..b8550cb 100644 --- a/.github/workflows/site-e2e.yml +++ b/.github/workflows/site-e2e.yml @@ -21,6 +21,9 @@ jobs: steps: - uses: actions/checkout@v6 + - name: Install protoc + run: sudo apt-get update && sudo apt-get install -y protobuf-compiler + - name: Install Rust toolchain uses: dtolnay/rust-toolchain@stable diff --git a/Cargo.lock b/Cargo.lock index 15339ab..62b1fa5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1741,9 +1741,9 @@ dependencies = [ [[package]] name = "chrono" -version = "0.4.43" +version = "0.4.44" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fac4744fb15ae8337dc853fee7fb3f4e48c0fbaa23d0afe49c447b4fab126118" +checksum = "c673075a2e0e5f4a1dde27ce9dee1ea4558c7ffe648f576438a20ca1d2acc4b0" dependencies = [ "iana-time-zone", "js-sys", @@ -7201,6 +7201,7 @@ version = "0.11.0" dependencies = [ "acton-service", "argon2", + "chrono", "proptest", "schema-forge-core", "serde", diff --git a/crates/schema-forge-acton/src/routes/auth.rs b/crates/schema-forge-acton/src/routes/auth.rs index 10873c1..38fc247 100644 --- a/crates/schema-forge-acton/src/routes/auth.rs +++ b/crates/schema-forge-acton/src/routes/auth.rs @@ -14,10 +14,13 @@ use std::fmt; use std::sync::Arc; use std::time::Duration; +use acton_service::audit::{AuditEventKind, AuditSeverity, AuditSource}; use acton_service::auth::tokens::paseto_generator::PasetoGenerator; use acton_service::auth::tokens::{ClaimsBuilder, TokenGenerator}; use acton_service::middleware::Claims; use acton_service::prelude::Error as ActonError; +use acton_service::state::AppState; +use axum::extract::State; use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; use axum::{Extension, Json}; @@ -27,6 +30,7 @@ use serde::{Deserialize, Serialize}; use crate::access::OptionalClaims; use crate::authz::principal_claims::{PrincipalClaimMappings, PrincipalClaimsError}; +use crate::config::SchemaForgeConfig; use crate::state::DynAuthStore; /// Default expiry for tokens minted by this endpoint (1 hour). @@ -93,6 +97,7 @@ struct LoginErrorBody { /// surfaced as 500 so they are easy to distinguish from the 401 "bad /// credentials" case. pub async fn login( + State(state): State>, Extension(auth_store): Extension>, Extension(generator): Extension>, Extension(principal_claims): Extension>, @@ -103,14 +108,23 @@ pub async fn login( .await { Ok(Some(u)) => u, - Ok(None) => return unauthorized_response(), - Err(e) => return internal_error_response(format!("auth store error: {e}")), + Ok(None) => { + emit_login_failed(&state, &req.username).await; + return unauthorized_response(); + } + Err(e) => { + emit_login_failed(&state, &req.username).await; + return internal_error_response(format!("auth store error: {e}")); + } }; let user_entity = if principal_claims.has_user_field_sources() { match auth_store.get_user_entity(&user.username).await { Ok(Some(e)) => Some(e), - Ok(None) => return unauthorized_response(), + Ok(None) => { + emit_login_failed(&state, &req.username).await; + return unauthorized_response(); + } Err(e) => return internal_error_response(format!("auth store error: {e}")), } } else { @@ -120,7 +134,10 @@ pub async fn login( let claims = match build_login_claims(&user.username, &user.roles, user_entity.as_ref(), &principal_claims) { Ok(c) => c, - Err(BuildLoginClaimsError::NullRequired(_)) => return unauthorized_response(), + Err(BuildLoginClaimsError::NullRequired(_)) => { + emit_login_failed(&state, &req.username).await; + return unauthorized_response(); + } Err(e) => return internal_error_response(format!("failed to build claims: {e}")), }; @@ -129,7 +146,20 @@ pub async fn login( Err(e) => return internal_error_response(format!("failed to generate token: {e}")), }; - let expires_at = Utc::now() + chrono::Duration::seconds(LOGIN_TOKEN_LIFETIME.as_secs() as i64); + let issued_at = Utc::now(); + + // Persist before returning the token. Fails closed: a DB write failure + // produces a 500 rather than handing the caller a token without an + // audit trail entry. The login handler already proved the row exists + // via validate_credentials, so the only realistic failure mode is a + // backend outage — in which case 500 is the right answer. + if let Err(e) = auth_store.record_login(&user.username, issued_at).await { + return internal_error_response(format!("failed to record last_login: {e}")); + } + + emit_login_success(&state, &user.username).await; + + let expires_at = issued_at + chrono::Duration::seconds(LOGIN_TOKEN_LIFETIME.as_secs() as i64); let body = LoginResponse { token, @@ -148,6 +178,7 @@ pub async fn login( /// expired or missing token get a clean 401 — the client can then show the /// login screen without hitting a ginned-up internal error. pub async fn refresh( + State(state): State>, OptionalClaims(claims): OptionalClaims, Extension(auth_store): Extension>, Extension(generator): Extension>, @@ -197,6 +228,8 @@ pub async fn refresh( let expires_at = Utc::now() + chrono::Duration::seconds(LOGIN_TOKEN_LIFETIME.as_secs() as i64); + emit_token_refresh(&state, &user.username).await; + let body = LoginResponse { token, expires_at: expires_at.to_rfc3339(), @@ -205,6 +238,55 @@ pub async fn refresh( (StatusCode::OK, Json(body)).into_response() } +// --------------------------------------------------------------------------- +// Audit emission helpers +// --------------------------------------------------------------------------- + +async fn emit_login_success(state: &AppState, username: &str) { + if let Some(logger) = state.audit_logger() { + logger + .log_auth( + AuditEventKind::AuthLoginSuccess, + AuditSeverity::Informational, + AuditSource { + subject: Some(format!("user:{username}")), + ..AuditSource::default() + }, + ) + .await; + } +} + +async fn emit_login_failed(state: &AppState, attempted_username: &str) { + if let Some(logger) = state.audit_logger() { + logger + .log_auth( + AuditEventKind::AuthLoginFailed, + AuditSeverity::Warning, + AuditSource { + subject: Some(format!("user:{attempted_username}")), + ..AuditSource::default() + }, + ) + .await; + } +} + +async fn emit_token_refresh(state: &AppState, username: &str) { + if let Some(logger) = state.audit_logger() { + logger + .log_auth( + AuditEventKind::AuthTokenRefresh, + AuditSeverity::Informational, + AuditSource { + subject: Some(format!("user:{username}")), + ..AuditSource::default() + }, + ) + .await; + } +} + // --------------------------------------------------------------------------- // Pure helpers (unit-testable) // --------------------------------------------------------------------------- diff --git a/crates/schema-forge-acton/src/shared_auth.rs b/crates/schema-forge-acton/src/shared_auth.rs index 9f32696..b0cfca1 100644 --- a/crates/schema-forge-acton/src/shared_auth.rs +++ b/crates/schema-forge-acton/src/shared_auth.rs @@ -241,6 +241,14 @@ mod tests { ) -> Result<(), BackendError> { unimplemented!("not used by bootstrap path") } + + async fn record_login( + &self, + _username: &str, + _at: chrono::DateTime, + ) -> Result<(), BackendError> { + unimplemented!("not used by bootstrap path") + } } /// Helper: run an async block on a single-threaded current-thread runtime. diff --git a/crates/schema-forge-acton/src/state.rs b/crates/schema-forge-acton/src/state.rs index 6ce3fe2..f33bf13 100644 --- a/crates/schema-forge-acton/src/state.rs +++ b/crates/schema-forge-acton/src/state.rs @@ -3,6 +3,7 @@ use std::future::Future; use std::pin::Pin; use std::sync::Arc; +use chrono::{DateTime, Utc}; use schema_forge_backend::entity::{Entity, QueryResult}; use schema_forge_backend::error::BackendError; use schema_forge_backend::traits::{EntityStore, SchemaBackend}; @@ -295,6 +296,14 @@ pub trait DynAuthStore: Send + Sync { username: &'a str, new_password: &'a str, ) -> Pin> + Send + 'a>>; + + /// Stamp the user's `last_login` field to `at`. See + /// [`schema_forge_backend::user_store::AuthStore::record_login`]. + fn record_login<'a>( + &'a self, + username: &'a str, + at: DateTime, + ) -> Pin> + Send + 'a>>; } /// Blanket impl: any concrete `AuthStore` automatically implements `DynAuthStore`. @@ -379,6 +388,14 @@ impl DynAuthStore for T { ) -> Pin> + Send + 'a>> { Box::pin(AuthStore::change_password(self, username, new_password)) } + + fn record_login<'a>( + &'a self, + username: &'a str, + at: DateTime, + ) -> Pin> + Send + 'a>> { + Box::pin(AuthStore::record_login(self, username, at)) + } } // --------------------------------------------------------------------------- diff --git a/crates/schema-forge-acton/tests/auth_login.rs b/crates/schema-forge-acton/tests/auth_login.rs index 0ec1c5e..a68ffce 100644 --- a/crates/schema-forge-acton/tests/auth_login.rs +++ b/crates/schema-forge-acton/tests/auth_login.rs @@ -101,20 +101,21 @@ fn build_test_generator() -> (Arc, NamedTempFile) { } /// Build a router that mounts only `/auth/login` with the Extensions the -/// login handler now depends on. -async fn login_app() -> (Router, NamedTempFile) { +/// login handler now depends on. The auth store is returned so tests can +/// read back the User row to verify side-effects (e.g. `last_login`). +async fn login_app() -> (Router, NamedTempFile, Arc) { let auth_store = seeded_auth_store().await; let (generator, key_tmp) = build_test_generator(); let principal_claims = Arc::new(schema_forge_acton::authz::PrincipalClaimMappings::default()); let router = auth_routes() - .layer(Extension(auth_store)) + .layer(Extension(auth_store.clone())) .layer(Extension(generator)) .layer(Extension(principal_claims)) .with_state(acton_service::state::AppState::< schema_forge_acton::SchemaForgeConfig, >::default()); - (router, key_tmp) + (router, key_tmp, auth_store) } async fn post_login(app: Router, body: &str) -> (StatusCode, serde_json::Value) { @@ -133,7 +134,7 @@ async fn post_login(app: Router, body: &str) -> (StatusCode, serde_json::Value) #[tokio::test] async fn login_with_correct_credentials_returns_token() { - let (app, _key) = login_app().await; + let (app, _key, _store) = login_app().await; let (status, body) = post_login(app, r#"{"username":"admin","password":"dev"}"#).await; assert_eq!(status, StatusCode::OK, "body: {body}"); @@ -155,7 +156,7 @@ async fn login_with_correct_credentials_returns_token() { #[tokio::test] async fn login_with_wrong_password_returns_401_envelope() { - let (app, _key) = login_app().await; + let (app, _key, _store) = login_app().await; let (status, body) = post_login(app, r#"{"username":"admin","password":"wrong"}"#).await; assert_eq!(status, StatusCode::UNAUTHORIZED); @@ -166,7 +167,7 @@ async fn login_with_wrong_password_returns_401_envelope() { #[tokio::test] async fn login_with_unknown_user_returns_401_envelope() { - let (app, _key) = login_app().await; + let (app, _key, _store) = login_app().await; let (status, body) = post_login(app, r#"{"username":"ghost","password":"dev"}"#).await; assert_eq!(status, StatusCode::UNAUTHORIZED); @@ -174,3 +175,56 @@ async fn login_with_unknown_user_returns_401_envelope() { assert_eq!(body["code"], "UNAUTHORIZED"); assert_eq!(body["status"], 401); } + +/// Regression for issue #59: a successful login must stamp `last_login` on +/// the User row. Prior to the fix the field was declared by the schema but +/// no code ever wrote it, so admins could not answer "who logged in +/// recently?" from `entity list User`. +#[tokio::test] +async fn login_success_stamps_last_login_on_user_row() { + use schema_forge_core::types::DynamicValue; + + let (app, _key, store) = login_app().await; + + let before = chrono::Utc::now(); + let (status, _body) = post_login(app, r#"{"username":"admin","password":"dev"}"#).await; + assert_eq!(status, StatusCode::OK); + + let entity = store + .get_user_entity("admin") + .await + .expect("get_user_entity ok") + .expect("admin row must exist"); + let last_login = match entity.field("last_login") { + Some(DynamicValue::DateTime(dt)) => *dt, + other => panic!("expected DynamicValue::DateTime on last_login, got {other:?}"), + }; + assert!( + last_login >= before, + "last_login {last_login} must be at-or-after the request start {before}" + ); + assert!( + last_login <= chrono::Utc::now(), + "last_login {last_login} must not be in the future" + ); +} + +/// Failed credential validation must not stamp `last_login` — that field +/// records *successful* logins only. +#[tokio::test] +async fn login_failure_leaves_last_login_untouched() { + let (app, _key, store) = login_app().await; + + let (status, _body) = post_login(app, r#"{"username":"admin","password":"wrong"}"#).await; + assert_eq!(status, StatusCode::UNAUTHORIZED); + + let entity = store + .get_user_entity("admin") + .await + .expect("get_user_entity ok") + .expect("admin row must exist"); + assert!( + entity.field("last_login").is_none(), + "last_login must remain unset after a failed login" + ); +} diff --git a/crates/schema-forge-backend/Cargo.toml b/crates/schema-forge-backend/Cargo.toml index f908ca6..fec7291 100644 --- a/crates/schema-forge-backend/Cargo.toml +++ b/crates/schema-forge-backend/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [dependencies] acton-service = { version = "0.26.1", default-features = false, features = ["crypto-aws-lc-rs"] } argon2 = { version = "0.5", features = ["std"] } +chrono = { version = "0.4.44", features = ["serde"] } schema-forge-core = { path = "../schema-forge-core" } serde = { version = "1", features = ["derive"] } serde_json = "1" diff --git a/crates/schema-forge-backend/src/entity_auth_store.rs b/crates/schema-forge-backend/src/entity_auth_store.rs index 1237471..8ded596 100644 --- a/crates/schema-forge-backend/src/entity_auth_store.rs +++ b/crates/schema-forge-backend/src/entity_auth_store.rs @@ -23,6 +23,7 @@ use std::sync::Arc; use argon2::password_hash::rand_core::OsRng; use argon2::password_hash::SaltString; use argon2::{Argon2, PasswordHash, PasswordHasher, PasswordVerifier}; +use chrono::{DateTime, Utc}; use schema_forge_core::query::{FieldPath, Filter, Query}; use schema_forge_core::types::{ DynamicValue, EntityId, FieldName, FieldType, IntegerConstraints, SchemaDefinition, SchemaName, @@ -40,6 +41,7 @@ const ROLES_FIELD: &str = "roles"; const ROLE_RANK_FIELD: &str = "role_rank"; const DISPLAY_NAME_FIELD: &str = "display_name"; const ACTIVE_FIELD: &str = "active"; +const LAST_LOGIN_FIELD: &str = "last_login"; /// Function that maps a role name to a numeric rank, returning `None` /// for unregistered roles. @@ -529,6 +531,25 @@ impl AuthStore for EntityAuthStore { self.store.update(&entity).await?; Ok(()) } + + async fn record_login( + &self, + username: &str, + at: DateTime, + ) -> Result<(), BackendError> { + let mut entity = match self.find_entity_by_username(username).await? { + Some(e) => e, + // Idempotent on a missing row: a delete-mid-login race shouldn't + // 500 the caller. The login handler already proved the row + // existed when validate_credentials succeeded. + None => return Ok(()), + }; + entity + .fields + .insert(LAST_LOGIN_FIELD.to_string(), DynamicValue::DateTime(at)); + self.store.update(&entity).await?; + Ok(()) + } } // Suppress dead-code warnings for unused FieldType / FieldName / etc. diff --git a/crates/schema-forge-backend/src/user_store.rs b/crates/schema-forge-backend/src/user_store.rs index 84b2add..8e8b4fe 100644 --- a/crates/schema-forge-backend/src/user_store.rs +++ b/crates/schema-forge-backend/src/user_store.rs @@ -1,5 +1,6 @@ use std::future::Future; +use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use crate::entity::Entity; @@ -95,6 +96,20 @@ pub trait AuthStore: Send + Sync { username: &str, new_password: &str, ) -> impl Future> + Send; + + /// Stamp the user's `last_login` field to `at`. + /// + /// Called from the login handler after credentials validate, before the + /// token is returned. Implementations must be idempotent against a + /// missing user row (return `Ok(())`) so a delete-mid-login race does + /// not 500 a legitimate caller. Failure to persist surfaces as + /// [`BackendError`] and is treated as fatal by the caller — audit + /// completeness takes precedence over login throughput. + fn record_login( + &self, + username: &str, + at: DateTime, + ) -> impl Future> + Send; } #[cfg(test)]