Merge pull request #79 from VibeEngineeringAI/fix/put-metadata-staging-consistency

Publish RBAC changes once durable metadata is written, even if staging fails

Author: VibeSWE

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::<StorageMetadata>(&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<String>,
 ) -> 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<Output = Result<(), ObjectStorageError>>,
+    staging_write: impl FnOnce() -> io::Result<()>,
+    tenant_id: &Option<String>,
+) -> 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());
+    }
+}

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::<StorageMetadata>(&metadata)?)
 }
 
-async fn put_metadata(
-    metadata: &StorageMetadata,
-    tenant_id: &Option<String>,
-) -> 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}")]

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::<StorageMetadata>(&metadata)?)
 }
 
-async fn put_metadata(
-    metadata: &StorageMetadata,
-    tenant_id: &Option<String>,
-) -> 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.")]