Updates: coderabbit suggestions

Author: parmesant

src/handlers/http/modal/ingest/ingestor_rbac.rs

@@ -24,6 +24,7 @@ use tokio::sync::Mutex;
 use crate::{
     handlers::http::{modal::utils::rbac_utils::get_metadata, rbac::RBACError},
     rbac::{
+        map::roles,
         user::{self, User as ParseableUser},
         Users,
     },
@@ -73,38 +74,6 @@ pub async fn delete_user(username: web::Path<String>) -> Result<impl Responder,
     Ok(format!("deleted user: {username}"))
 }
 
-// // Handler PUT /user/{username}/roles => Put roles for user
-// // Put roles for given user
-// pub async fn put_role(
-//     username: web::Path<String>,
-//     role: web::Json<HashSet<String>>,
-// ) -> Result<String, RBACError> {
-//     let username = username.into_inner();
-//     let role = role.into_inner();
-
-//     if !Users.contains(&username) {
-//         return Err(RBACError::UserDoesNotExist);
-//     };
-//     // update parseable.json first
-//     let mut metadata = get_metadata().await?;
-//     if let Some(user) = metadata
-//         .users
-//         .iter_mut()
-//         .find(|user| user.username() == username)
-//     {
-//         user.roles.clone_from(&role);
-//     } else {
-//         // should be unreachable given state is always consistent
-//         return Err(RBACError::UserDoesNotExist);
-//     }
-
-//     let _ = storage::put_staging_metadata(&metadata);
-//     // update in mem table
-//     Users.add_roles(&username.clone(), role.clone());
-
-//     Ok(format!("Roles updated successfully for {username}"))
-// }
-
 // Handler PATCH /user/{username}/role/sync/add => Add roles to a user
 pub async fn add_roles_to_user(
     username: web::Path<String>,
@@ -116,6 +85,19 @@ pub async fn add_roles_to_user(
     if !Users.contains(&username) {
         return Err(RBACError::UserDoesNotExist);
     };
+
+    // check if all roles exist
+    let mut non_existent_roles = Vec::new();
+    roles_to_add.iter().for_each(|r| {
+        if roles().get(r).is_none() {
+            non_existent_roles.push(r.clone());
+        }
+    });
+
+    if !non_existent_roles.is_empty() {
+        return Err(RBACError::RolesDoNotExist(non_existent_roles));
+    }
+
     // update parseable.json first
     let mut metadata = get_metadata().await?;
     if let Some(user) = metadata
@@ -147,6 +129,30 @@ pub async fn remove_roles_from_user(
     if !Users.contains(&username) {
         return Err(RBACError::UserDoesNotExist);
     };
+
+    // check if all roles exist
+    let mut non_existent_roles = Vec::new();
+    roles_to_remove.iter().for_each(|r| {
+        if roles().get(r).is_none() {
+            non_existent_roles.push(r.clone());
+        }
+    });
+
+    if !non_existent_roles.is_empty() {
+        return Err(RBACError::RolesDoNotExist(non_existent_roles));
+    }
+
+    // check that user actually has these roles
+    let user_roles: HashSet<String> = HashSet::from_iter(Users.get_role(&username));
+    let roles_not_with_user: HashSet<String> =
+        HashSet::from_iter(roles_to_remove.difference(&user_roles).cloned());
+
+    if !roles_not_with_user.is_empty() {
+        return Err(RBACError::RolesNotAssigned(Vec::from_iter(
+            roles_not_with_user,
+        )));
+    }
+
     // update parseable.json first
     let mut metadata = get_metadata().await?;
     if let Some(user) = metadata

src/handlers/http/rbac.rs

@@ -115,17 +115,17 @@ pub async fn post_user(
     if user_roles.is_empty() {
         return Err(RBACError::RoleValidationError);
     } else {
-        let mut non_existant_roles = Vec::new();
+        let mut non_existent_roles = Vec::new();
         user_roles
             .iter()
             .map(|r| {
                 if !roles().contains_key(r) {
-                    non_existant_roles.push(r.clone());
+                    non_existent_roles.push(r.clone());
                 }
             })
             .for_each(drop);
-        if !non_existant_roles.is_empty() {
-            return Err(RBACError::RolesDoNotExist(non_existant_roles));
+        if !non_existent_roles.is_empty() {
+            return Err(RBACError::RolesDoNotExist(non_existent_roles));
         }
     }
     let _ = UPDATE_LOCK.lock().await;
@@ -237,24 +237,23 @@ pub async fn delete_user(username: web::Path<String>) -> Result<impl Responder,
     let mut metadata = get_metadata().await?;
     metadata.users.retain(|user| user.username() != username);
 
-    // also delete from user groups
+    // Remove user from all groups
     let user_groups = Users.get_user_groups(&username);
-    let mut groups_to_update = Vec::new();
-    for user_group in user_groups {
-        if let Some(ug) = write_user_groups().get_mut(&user_group) {
-            ug.remove_users(HashSet::from_iter([username.clone()]))?;
-            groups_to_update.push(ug.clone());
-            // ug.update_in_metadata().await?;
-        } else {
-            continue;
-        };
+    {
+        let mut groups = write_user_groups();
+        for group_name in &user_groups {
+            if let Some(group) = groups.get_mut(group_name) {
+                group.remove_users(HashSet::from_iter([username.clone()]))?;
+            }
+        }
     }
 
-    // update in metadata user groups
-    metadata
-        .user_groups
-        .retain(|x| !groups_to_update.contains(x));
-    metadata.user_groups.extend(groups_to_update);
+    // Update metadata with modified groups
+    for group in metadata.user_groups.iter_mut() {
+        if user_groups.contains(&group.name) {
+            group.users.retain(|u| u != &username);
+        }
+    }
     put_metadata(&metadata).await?;
 
     // update in mem table
@@ -274,17 +273,17 @@ pub async fn add_roles_to_user(
         return Err(RBACError::UserDoesNotExist);
     };
 
-    let mut non_existant_roles = Vec::new();
+    let mut non_existent_roles = Vec::new();
 
     // check if the role exists
     roles_to_add.iter().for_each(|r| {
         if roles().get(r).is_none() {
-            non_existant_roles.push(r.clone());
+            non_existent_roles.push(r.clone());
         }
     });
 
-    if !non_existant_roles.is_empty() {
-        return Err(RBACError::RolesDoNotExist(non_existant_roles));
+    if !non_existent_roles.is_empty() {
+        return Err(RBACError::RolesDoNotExist(non_existent_roles));
     }
 
     // update parseable.json first
@@ -319,17 +318,17 @@ pub async fn remove_roles_from_user(
         return Err(RBACError::UserDoesNotExist);
     };
 
-    let mut non_existant_roles = Vec::new();
+    let mut non_existent_roles = Vec::new();
 
     // check if the role exists
     roles_to_remove.iter().for_each(|r| {
         if roles().get(r).is_none() {
-            non_existant_roles.push(r.clone());
+            non_existent_roles.push(r.clone());
         }
     });
 
-    if !non_existant_roles.is_empty() {
-        return Err(RBACError::RolesDoNotExist(non_existant_roles));
+    if !non_existent_roles.is_empty() {
+        return Err(RBACError::RolesDoNotExist(non_existent_roles));
     }
 
     // check for role not present with user
@@ -368,31 +367,13 @@ pub async fn remove_roles_from_user(
 #[serde(rename = "camelCase")]
 pub struct InvalidUserGroupError {
     pub valid_name: bool,
-    pub non_existant_roles: Vec<String>,
-    pub non_existant_users: Vec<String>,
+    pub non_existent_roles: Vec<String>,
+    pub non_existent_users: Vec<String>,
     pub roles_not_in_group: Vec<String>,
     pub users_not_in_group: Vec<String>,
     pub comments: String,
 }
 
-// impl Display for InvalidUserGroupRequestStruct {
-//     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-//         if !self.invalid_name {
-//             write!(
-//                 f,
-//                 "Invalid user group request- {{invalidName: {}\nnonExistantRoles: {:?}\nnonExistantUsers: {:?}\nThe name should follow this regex- ^[A-Za-z0-9_-]+$}}",
-//                 self.invalid_name, self.non_existant_roles, self.non_existant_users
-//             )
-//         } else {
-//             write!(
-//                 f,
-//                 "Invalid user group request- {{nonExistantRoles: {:?}\nnonExistantUsers: {:?}}}",
-//                 self.non_existant_roles, self.non_existant_users
-//             )
-//         }
-//     }
-// }
-
 #[derive(Debug, thiserror::Error)]
 pub enum RBACError {
     #[error("User exists already")]
@@ -461,7 +442,7 @@ impl actix_web::ResponseError for RBACError {
             RBACError::RolesDoNotExist(obj) => actix_web::HttpResponse::build(self.status_code())
                 .insert_header(ContentType::plaintext())
                 .json(json!({
-                    "non_existant_roles": obj
+                    "non_existent_roles": obj
                 })),
             RBACError::InvalidUserGroupRequest(obj) => {
                 actix_web::HttpResponse::build(self.status_code())

src/handlers/http/role.rs

@@ -16,8 +16,6 @@
  *
  */
 
-use std::collections::HashSet;
-
 use actix_web::{
     http::header::ContentType,
     web::{self, Json},
@@ -103,23 +101,21 @@ pub async fn delete(name: web::Path<String>) -> Result<impl Responder, RoleError
         };
     }
 
+    // remove role from all user groups that have it
     let mut groups_to_update = Vec::new();
-    for user_group in group_names {
-        if let Some(ug) = write_user_groups().get_mut(&user_group) {
-            ug.remove_roles(HashSet::from_iter([name.clone()]))
-                .map_err(|e| RoleError::Anyhow(anyhow::Error::msg(e.to_string())))?;
-            groups_to_update.push(ug.clone());
-            // ug.update_in_metadata().await?;
-        } else {
-            continue;
-        };
+    for group in write_user_groups().values_mut() {
+        if group.roles.remove(&name) {
+            groups_to_update.push(group.clone());
+        }
     }
 
-    // update in metadata
-    metadata
-        .user_groups
-        .retain(|x| !groups_to_update.contains(x));
-    metadata.user_groups.extend(groups_to_update);
+    // update metadata only if there are changes
+    if !groups_to_update.is_empty() {
+        metadata
+            .user_groups
+            .retain(|x| !groups_to_update.contains(x));
+        metadata.user_groups.extend(groups_to_update);
+    }
     put_metadata(&metadata).await?;
 
     Ok(HttpResponse::Ok().finish())

src/migration/mod.rs

@@ -41,6 +41,13 @@ use crate::{
     },
 };
 
+fn get_version(metadata: &serde_json::Value) -> Option<&str> {
+    metadata
+        .as_object()
+        .and_then(|meta| meta.get("version"))
+        .and_then(|version| version.as_str())
+}
+
 /// Migrate the metdata from v1 or v2 to v3
 /// This is a one time migration
 pub async fn run_metadata_migration(
@@ -55,13 +62,6 @@ pub async fn run_metadata_migration(
     }
     let staging_metadata = get_staging_metadata(config)?;
 
-    fn get_version(metadata: &serde_json::Value) -> Option<&str> {
-        metadata
-            .as_object()
-            .and_then(|meta| meta.get("version"))
-            .and_then(|version| version.as_str())
-    }
-
     // if storage metadata is none do nothing
     if let Some(storage_metadata) = storage_metadata {
         match get_version(&storage_metadata) {
@@ -117,37 +117,42 @@ pub async fn run_metadata_migration(
 
     // if staging metadata is none do nothing
     if let Some(staging_metadata) = staging_metadata {
-        match get_version(&staging_metadata) {
-            Some("v1") => {
-                let mut metadata = metadata_migration::v1_v3(staging_metadata);
-                metadata = metadata_migration::v3_v4(metadata);
-                put_staging_metadata(config, &metadata)?;
-            }
-            Some("v2") => {
-                let mut metadata = metadata_migration::v2_v3(staging_metadata);
-                metadata = metadata_migration::v3_v4(metadata);
-                put_staging_metadata(config, &metadata)?;
-            }
-            Some("v3") => {
-                let metadata = metadata_migration::v3_v4(staging_metadata);
-                put_staging_metadata(config, &metadata)?;
-            }
-            Some("v4") => {
-                let metadata = metadata_migration::v4_v5(staging_metadata);
-                let metadata = metadata_migration::v5_v6(metadata);
-                put_staging_metadata(config, &metadata)?;
-            }
-            Some("v5") => {
-                let metadata = metadata_migration::v5_v6(staging_metadata);
-                put_staging_metadata(config, &metadata)?;
-            }
-            _ => (),
-        }
+        migrate_staging(config, staging_metadata)?;
     }
 
     Ok(())
 }
 
+fn migrate_staging(config: &Parseable, staging_metadata: Value) -> anyhow::Result<()> {
+    match get_version(&staging_metadata) {
+        Some("v1") => {
+            let mut metadata = metadata_migration::v1_v3(staging_metadata);
+            metadata = metadata_migration::v3_v4(metadata);
+            put_staging_metadata(config, &metadata)?;
+        }
+        Some("v2") => {
+            let mut metadata = metadata_migration::v2_v3(staging_metadata);
+            metadata = metadata_migration::v3_v4(metadata);
+            put_staging_metadata(config, &metadata)?;
+        }
+        Some("v3") => {
+            let metadata = metadata_migration::v3_v4(staging_metadata);
+            put_staging_metadata(config, &metadata)?;
+        }
+        Some("v4") => {
+            let metadata = metadata_migration::v4_v5(staging_metadata);
+            let metadata = metadata_migration::v5_v6(metadata);
+            put_staging_metadata(config, &metadata)?;
+        }
+        Some("v5") => {
+            let metadata = metadata_migration::v5_v6(staging_metadata);
+            put_staging_metadata(config, &metadata)?;
+        }
+        _ => (),
+    }
+    Ok(())
+}
+
 /// run the migration for all streams concurrently
 pub async fn run_migration(config: &Parseable) -> anyhow::Result<()> {
     let storage = config.storage.get_object_store();