From 7bd21598e5ebc6c014ce68e0df6ca6af1888a717 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 22:34:35 -0500 Subject: [PATCH 1/2] test(rbac): cover metadata staging write failures Extract the existing durable-then-staging sequence behind injected writers without changing its behavior. Regress the staging failure that incorrectly prevents callers from publishing a successful durable update, and cover durable failure and successful ordering. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/modal/utils/rbac_utils.rs | 87 ++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/src/handlers/http/modal/utils/rbac_utils.rs b/src/handlers/http/modal/utils/rbac_utils.rs index 38da0cf03..2009200ba 100644 --- a/src/handlers/http/modal/utils/rbac_utils.rs +++ b/src/handlers/http/modal/utils/rbac_utils.rs @@ -16,6 +16,8 @@ * */ +use std::{future::Future, io}; + use crate::{ parseable::PARSEABLE, storage::{self, ObjectStorageError, StorageMetadata}, @@ -37,7 +39,88 @@ pub async fn put_metadata( metadata: &StorageMetadata, tenant_id: &Option, ) -> Result<(), ObjectStorageError> { - storage::put_remote_metadata(metadata, tenant_id).await?; - storage::put_staging_metadata(metadata, tenant_id)?; + put_metadata_with_writers(storage::put_remote_metadata(metadata, tenant_id), || { + storage::put_staging_metadata(metadata, tenant_id) + }) + .await +} + +async fn put_metadata_with_writers( + remote_write: impl Future>, + staging_write: impl FnOnce() -> io::Result<()>, +) -> Result<(), ObjectStorageError> { + remote_write.await?; + staging_write()?; Ok(()) } + +#[cfg(test)] +mod tests { + use std::cell::Cell; + + use super::*; + + #[tokio::test] + async fn staging_failure_after_durable_success_returns_success() { + let durable_written = Cell::new(false); + let staging_attempted = Cell::new(false); + let result = put_metadata_with_writers( + async { + durable_written.set(true); + Ok(()) + }, + || { + assert!( + durable_written.get(), + "durable metadata must be written first" + ); + staging_attempted.set(true); + Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "staging is read-only", + )) + }, + ) + .await; + + assert!(staging_attempted.get()); + assert!( + result.is_ok(), + "callers must publish durable changes to live authorization: {result:?}" + ); + } + + #[tokio::test] + async fn durable_failure_is_returned_without_attempting_staging() { + let result = put_metadata_with_writers( + async { Err(ObjectStorageError::Custom("durable write failed".into())) }, + || panic!("staging must not be changed after a failed durable write"), + ) + .await; + + assert!( + matches!(result, Err(ObjectStorageError::Custom(message)) if message == "durable write failed") + ); + } + + #[tokio::test] + async fn successful_writes_run_durable_before_staging() { + let durable_written = Cell::new(false); + let staging_written = Cell::new(false); + let result = put_metadata_with_writers( + async { + durable_written.set(true); + Ok(()) + }, + || { + assert!(durable_written.get()); + staging_written.set(true); + Ok(()) + }, + ) + .await; + + assert!(result.is_ok()); + assert!(staging_written.get()); + } +} From ef0b3084b30c34513cdc105eaead5ea391501145 Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sat, 24 Oct 2026 22:39:19 -0500 Subject: [PATCH 2/2] fix(rbac): publish durable metadata despite staging failures Remote metadata is authoritative at runtime and startup. Log a failed local staging refresh with tenant context and return success once the durable write commits, so callers continue their live map and session updates. Keep durable write errors fatal and route the standalone role and OIDC writers through the shared helper. Co-Authored-By: GPT-6.1 Sol --- src/handlers/http/modal/utils/rbac_utils.rs | 24 +++++++++++++++++---- src/handlers/http/oidc.rs | 15 ++++--------- src/handlers/http/role.rs | 12 ++--------- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/handlers/http/modal/utils/rbac_utils.rs b/src/handlers/http/modal/utils/rbac_utils.rs index 2009200ba..b82222af5 100644 --- a/src/handlers/http/modal/utils/rbac_utils.rs +++ b/src/handlers/http/modal/utils/rbac_utils.rs @@ -35,22 +35,35 @@ pub async fn get_metadata( Ok(serde_json::from_slice::(&metadata)?) } +/// Persist authoritative metadata and refresh its best-effort local staging copy. +/// Success lets callers publish the committed change to live RBAC state. pub async fn put_metadata( metadata: &StorageMetadata, tenant_id: &Option, ) -> Result<(), ObjectStorageError> { - put_metadata_with_writers(storage::put_remote_metadata(metadata, tenant_id), || { - storage::put_staging_metadata(metadata, tenant_id) - }) + put_metadata_with_writers( + storage::put_remote_metadata(metadata, tenant_id), + || storage::put_staging_metadata(metadata, tenant_id), + tenant_id, + ) .await } async fn put_metadata_with_writers( remote_write: impl Future>, staging_write: impl FnOnce() -> io::Result<()>, + tenant_id: &Option, ) -> Result<(), ObjectStorageError> { remote_write.await?; - staging_write()?; + // A staging failure cannot undo the durable commit. Returning an error here + // would leave callers' live authorization state behind the source of truth. + if let Err(error) = staging_write() { + tracing::error!( + error = %error, + tenant_id = ?tenant_id, + "Failed to refresh staging metadata after durable metadata write" + ); + } Ok(()) } @@ -80,6 +93,7 @@ mod tests { "staging is read-only", )) }, + &None, ) .await; @@ -95,6 +109,7 @@ mod tests { let result = put_metadata_with_writers( async { Err(ObjectStorageError::Custom("durable write failed".into())) }, || panic!("staging must not be changed after a failed durable write"), + &None, ) .await; @@ -117,6 +132,7 @@ mod tests { staging_written.set(true); Ok(()) }, + &None, ) .await; diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index 53a32c0bb..b9a7a420a 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -47,7 +47,9 @@ use crate::{ handlers::{ COOKIE_AGE_DAYS, SESSION_COOKIE_NAME, USER_COOKIE_NAME, USER_ID_COOKIE_NAME, http::{ - API_BASE_PATH, API_VERSION, cluster::sync_user_creation, modal::OIDC_CLIENT, + API_BASE_PATH, API_VERSION, + cluster::sync_user_creation, + modal::{OIDC_CLIENT, utils::rbac_utils::put_metadata}, rbac::UPDATE_LOCK, }, }, @@ -58,7 +60,7 @@ use crate::{ map::{DEFAULT_ROLE, SessionKey, mut_sessions, mut_users, roles, write_user_groups}, user::{self, GroupUser, User, UserType}, }, - storage::{self, ObjectStorageError, StorageMetadata}, + storage::{ObjectStorageError, StorageMetadata}, utils::{ actix::extract_session_key_from_req, get_tenant_id_from_key, get_tenant_id_from_request, }, @@ -825,15 +827,6 @@ async fn get_metadata( Ok(serde_json::from_slice::(&metadata)?) } -async fn put_metadata( - metadata: &StorageMetadata, - tenant_id: &Option, -) -> Result<(), ObjectStorageError> { - storage::put_remote_metadata(metadata, tenant_id).await?; - storage::put_staging_metadata(metadata, tenant_id)?; - Ok(()) -} - #[derive(Debug, thiserror::Error)] pub enum OIDCError { #[error("Failed to connect to storage: {0}")] diff --git a/src/handlers/http/role.rs b/src/handlers/http/role.rs index 3ce94d2d3..994b01f1d 100644 --- a/src/handlers/http/role.rs +++ b/src/handlers/http/role.rs @@ -25,6 +25,7 @@ use actix_web::{ web::{self, Json}, }; +use super::modal::utils::rbac_utils::put_metadata; use crate::rbac::map::roles; use crate::rbac::role::model::{Role, RoleType, RoleUI}; use crate::{ @@ -34,7 +35,7 @@ use crate::{ map::{DEFAULT_ROLE, mut_roles, mut_sessions, mut_users, read_user_groups, users}, user::UserType, }, - storage::{self, ObjectStorageError, StorageMetadata}, + storage::{ObjectStorageError, StorageMetadata}, utils::get_tenant_id_from_request, validator::{self, error::UsernameValidationError}, }; @@ -302,15 +303,6 @@ async fn get_metadata( Ok(serde_json::from_slice::(&metadata)?) } -async fn put_metadata( - metadata: &StorageMetadata, - tenant_id: &Option, -) -> Result<(), ObjectStorageError> { - storage::put_remote_metadata(metadata, tenant_id).await?; - storage::put_staging_metadata(metadata, tenant_id)?; - Ok(()) -} - #[derive(Debug, thiserror::Error)] pub enum RoleError { #[error("Default OIDC role must name an existing role.")]