Minor refactor in user flow and add comments (#416)

Author: nitisht

server/src/handlers/http.rs

@@ -31,7 +31,7 @@ use rustls::{Certificate, PrivateKey, ServerConfig};
 use rustls_pemfile::{certs, pkcs8_private_keys};
 
 use crate::option::CONFIG;
-use crate::rbac::get_user_map;
+use crate::rbac::user_map;
 
 mod health_check;
 mod ingest;
@@ -62,14 +62,14 @@ macro_rules! create_app {
     };
 }
 
-async fn validator(
+async fn authenticate(
     req: ServiceRequest,
     credentials: BasicAuth,
 ) -> Result<ServiceRequest, (actix_web::Error, ServiceRequest)> {
     let username = credentials.user_id().trim();
     let password = credentials.password().unwrap().trim();
 
-    if let Some(user) = get_user_map().read().unwrap().get(username) {
+    if let Some(user) = user_map().read().unwrap().get(username) {
         if user.verify(password) {
             return Ok(req);
         }
@@ -130,6 +130,7 @@ pub async fn run_http(prometheus: PrometheusMetrics) -> anyhow::Result<()> {
 pub fn configure_routes(cfg: &mut web::ServiceConfig) {
     let generated = generate();
 
+    //log stream API
     let logstream_api = web::scope("/{logstream}")
         .service(
             web::resource("")
@@ -163,14 +164,19 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) {
                 // GET "/logstream/{logstream}/retention" ==> Get retention for given logstream
                 .route(web::get().to(logstream::get_retention)),
         );
+
+    // User API
     let user_api = web::scope("/user").service(
         web::resource("/{username}")
             // POST /user/{username} => Create a new user
             .route(web::put().to(rbac::put_user))
             // DELETE /user/{username} => Delete a user
             .route(web::delete().to(rbac::delete_user))
             .wrap_fn(|req, srv| {
-                // deny request if username is same as username from config
+                // The credentials set in the env vars (P_USERNAME & P_PASSWORD) are treated
+                // as root credentials. Any other user is not allowed to modify or delete
+                // the root user. Deny request if username is same as username
+                // from env variable P_USERNAME.
                 let username = req.match_info().get("username").unwrap_or("");
                 let is_root = username == CONFIG.parseable.username;
                 let call = srv.call(req);
@@ -210,7 +216,7 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) {
                     ),
             )
             .service(user_api)
-            .wrap(HttpAuthentication::basic(validator)),
+            .wrap(HttpAuthentication::basic(authenticate)),
     )
     // GET "/" ==> Serve the static frontend directory
     .service(ResourceFiles::new("/", generated));

server/src/handlers/http/rbac.rs

@@ -19,8 +19,8 @@
 use crate::{
     option::CONFIG,
     rbac::{
-        get_user_map,
         user::{PassCode, User},
+        user_map,
     },
     storage::{self, ObjectStorageError, StorageMetadata},
     validator::{self, error::UsernameValidationError},
@@ -38,9 +38,9 @@ static UPDATE_LOCK: Mutex<()> = Mutex::const_new(());
 // returns password generated for this user
 pub async fn put_user(username: web::Path<String>) -> Result<impl Responder, RBACError> {
     let username = username.into_inner();
-    validator::verify_username(&username)?;
+    validator::user_name(&username)?;
     let _ = UPDATE_LOCK.lock().await;
-    let user_exists = get_user_map().read().unwrap().contains_key(&username);
+    let user_exists = user_map().read().unwrap().contains_key(&username);
     if user_exists {
         reset_password(username).await
     } else {
@@ -54,7 +54,7 @@ pub async fn put_user(username: web::Path<String>) -> Result<impl Responder, RBA
         metadata.users.push(user.clone());
         put_metadata(&metadata).await?;
         // set this user to user map
-        get_user_map().write().unwrap().insert(user);
+        user_map().write().unwrap().insert(user);
 
         Ok(password)
     }
@@ -65,16 +65,16 @@ pub async fn delete_user(username: web::Path<String>) -> Result<impl Responder,
     let username = username.into_inner();
     let _ = UPDATE_LOCK.lock().await;
     // fail this request if the user does not exists
-    if !get_user_map().read().unwrap().contains_key(&username) {
+    if !user_map().read().unwrap().contains_key(&username) {
         return Err(RBACError::UserDoesNotExist);
     };
     // delete from parseable.json first
     let mut metadata = get_metadata().await?;
     metadata.users.retain(|user| user.username != username);
     put_metadata(&metadata).await?;
     // update in mem table
-    get_user_map().write().unwrap().remove(&username);
-    Ok(format!("deleted user: {}", username))
+    user_map().write().unwrap().remove(&username);
+    Ok(format!("deleted user: {username}"))
 }
 
 // Reset password for given username
@@ -97,7 +97,7 @@ pub async fn reset_password(username: String) -> Result<String, RBACError> {
     put_metadata(&metadata).await?;
 
     // update in mem table
-    get_user_map()
+    user_map()
         .write()
         .unwrap()
         .get_mut(&username)

server/src/rbac.rs

@@ -26,7 +26,7 @@ pub mod user;
 
 pub static USERS: OnceCell<RwLock<UserMap>> = OnceCell::new();
 
-pub fn get_user_map() -> &'static RwLock<UserMap> {
+pub fn user_map() -> &'static RwLock<UserMap> {
     USERS.get().expect("user map is set")
 }
 

server/src/rbac/user.rs

@@ -49,8 +49,8 @@ impl User {
         )
     }
 
-    // Verification works because the PasswordHash is in PHC format
-    // $<id>[$v=<version>][$<param>=<value>(,<param>=<value>)*][$<salt>[$<hash>]]
+    // Take the password and compare with the hash stored internally (PHC format ==>
+    // $<id>[$v=<version>][$<param>=<value>(,<param>=<value>)*][$<salt>[$<hash>]])
     // ref https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md#specification
     pub fn verify(&self, password: &str) -> bool {
         let parsed_hash = PasswordHash::new(&self.password_hash).unwrap();
@@ -59,14 +59,16 @@ impl User {
             .is_ok()
     }
 
-    // gen new password
+    // generate a new password
     pub fn gen_new_password() -> PassCode {
         let password = Alphanumeric.sample_string(&mut rand::thread_rng(), 16);
         let hash = gen_hash(&password);
         PassCode { password, hash }
     }
 }
 
+// generate a one way hash for password to be stored in metadata file
+// ref https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md
 fn gen_hash(password: &str) -> String {
     let salt = SaltString::generate(&mut OsRng);
     let argon2 = Argon2::default();

server/src/validator.rs

@@ -114,7 +114,11 @@ pub fn stream_name(stream_name: &str) -> Result<(), StreamNameValidationError> {
     Ok(())
 }
 
-pub fn verify_username(username: &str) -> Result<(), UsernameValidationError> {
+// validate if username is valid
+// username should be between 3 and 64 characters long
+// username should contain only alphanumeric characters or underscores
+// username should be lowercase
+pub fn user_name(username: &str) -> Result<(), UsernameValidationError> {
     // Check if the username meets the required criteria
     if username.len() < 3 || username.len() > 64 {
         return Err(UsernameValidationError::InvalidLength);
@@ -254,10 +258,10 @@ pub mod error {
 
     #[derive(Debug, thiserror::Error)]
     pub enum UsernameValidationError {
-        #[error("Username length should be between 3 and 64 chars")]
+        #[error("Username should be between 3 and 64 chars long")]
         InvalidLength,
         #[error(
-            "Username contains invalid characters. Only lowercase aplhanumeric and _ is allowed"
+            "Username contains invalid characters. Please use lowercase, alphanumeric or underscore"
         )]
         SpecialChar,
     }