diff --git a/src/handlers/http/modal/utils/rbac_utils.rs b/src/handlers/http/modal/utils/rbac_utils.rs index 38da0cf03..b82222af5 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}, @@ -33,11 +35,108 @@ 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> { - 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), + 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?; + // 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(()) } + +#[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", + )) + }, + &None, + ) + .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"), + &None, + ) + .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(()) + }, + &None, + ) + .await; + + assert!(result.is_ok()); + assert!(staging_written.get()); + } +} 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.")]