refactor(db, webserver): Represent passwords as nullable in db and se…

…rvice layer (#1676)

* refactor(db, webserver): Represent passwords as nullable in db and service layer

* [autofix.ci] apply automated fixes

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

ee/tabby-db/migrations/0018_user-password-nullable.down.sql

@@ -0,0 +1,5 @@
+UPDATE users SET password_encrypted =
+    CASE
+        WHEN password_encrypted IS NULL THEN ''
+        ELSE password_encrypted
+    END;

ee/tabby-db/migrations/0018_user-password-nullable.up.sql

@@ -0,0 +1,10 @@
+ALTER TABLE users ADD COLUMN password_encrypted_nullable VARCHAR(128);
+
+UPDATE users SET password_encrypted_nullable =
+    CASE
+        WHEN LENGTH(password_encrypted) = 0 THEN NULL
+        ELSE password_encrypted
+    END;
+
+ALTER TABLE users DROP COLUMN password_encrypted;
+ALTER TABLE users RENAME password_encrypted_nullable TO password_encrypted;

ee/tabby-db/src/lib.rs

@@ -285,7 +285,7 @@ pub mod testutils {
     pub async fn create_user(conn: &DbConn) -> i32 {
         let email: &str = "[email protected]";
         let password: &str = "123456789";
-        conn.create_user(email.to_string(), password.to_string(), true)
+        conn.create_user(email.to_string(), Some(password.to_string()), true)
             .await
             .unwrap()
     }

ee/tabby-db/src/users.rs

@@ -14,7 +14,7 @@ pub struct UserDAO {
 
     pub id: i32,
     pub email: String,
-    pub password_encrypted: String,
+    pub password_encrypted: Option<String>,
     pub is_admin: bool,
 
     /// To authenticate IDE extensions / plugins to access code completion / chat api endpoints.
@@ -41,7 +41,7 @@ impl DbConn {
     pub async fn create_user(
         &self,
         email: String,
-        password_encrypted: String,
+        password_encrypted: Option<String>,
         is_admin: bool,
     ) -> Result<i32> {
         self.create_user_impl(email, password_encrypted, is_admin, None)
@@ -51,7 +51,7 @@ impl DbConn {
     pub async fn create_user_with_invitation(
         &self,
         email: String,
-        password_encrypted: String,
+        password_encrypted: Option<String>,
         is_admin: bool,
         invitation_id: i32,
     ) -> Result<i32> {
@@ -62,7 +62,7 @@ impl DbConn {
     async fn create_user_impl(
         &self,
         email: String,
-        password_encrypted: String,
+        password_encrypted: Option<String>,
         is_admin: bool,
         invitation_id: Option<i32>,
     ) -> Result<i32> {
@@ -395,7 +395,7 @@ mod tests {
         );
 
         let id1 = conn
-            .create_user("[email protected]".into(), "123456".into(), false)
+            .create_user("[email protected]".into(), Some("123456".into()), false)
             .await
             .unwrap();
 
@@ -464,19 +464,19 @@ mod tests {
         );
 
         let id2 = conn
-            .create_user("[email protected]".into(), "123456".into(), false)
+            .create_user("[email protected]".into(), Some("123456".into()), false)
             .await
             .unwrap();
         let id3 = conn
-            .create_user("[email protected]".into(), "123456".into(), false)
+            .create_user("[email protected]".into(), Some("123456".into()), false)
             .await
             .unwrap();
         let id4 = conn
-            .create_user("[email protected]".into(), "123456".into(), false)
+            .create_user("[email protected]".into(), Some("123456".into()), false)
             .await
             .unwrap();
         let id5 = conn
-            .create_user("[email protected]".into(), "123456".into(), false)
+            .create_user("[email protected]".into(), Some("123456".into()), false)
             .await
             .unwrap();
 
@@ -549,15 +549,15 @@ mod tests {
     async fn test_caching() {
         let db = DbConn::new_in_memory().await.unwrap();
 
-        db.create_user("[email protected]".into(), "".into(), true)
+        db.create_user("[email protected]".into(), None, true)
             .await
             .unwrap();
 
         assert_eq!(db.count_active_users().await.unwrap(), 1);
         assert_eq!(db.count_active_admin_users().await.unwrap(), 1);
 
         let user2_id = db
-            .create_user("[email protected]".into(), "".into(), false)
+            .create_user("[email protected]".into(), None, false)
             .await
             .unwrap();
         assert_eq!(db.count_active_users().await.unwrap(), 2);
@@ -568,7 +568,7 @@ mod tests {
         assert_eq!(db.count_active_admin_users().await.unwrap(), 1);
 
         let user3_id = db
-            .create_user("[email protected]".into(), "".into(), true)
+            .create_user("[email protected]".into(), None, true)
             .await
             .unwrap();
         assert_eq!(db.count_active_users().await.unwrap(), 2);

ee/tabby-webserver/src/service/auth.rs

@@ -70,14 +70,14 @@ impl AuthenticationService for AuthenticationServiceImpl {
             self.db
                 .create_user_with_invitation(
                     email.clone(),
-                    pwd_hash,
+                    Some(pwd_hash),
                     !is_admin_initialized,
                     invitation.id as i32,
                 )
                 .await?
         } else {
             self.db
-                .create_user(email.clone(), pwd_hash, !is_admin_initialized)
+                .create_user(email.clone(), Some(pwd_hash), !is_admin_initialized)
                 .await?
         };
 
@@ -142,7 +142,8 @@ impl AuthenticationService for AuthenticationServiceImpl {
             .await?
             .expect("User must exist")
             .password_encrypted;
-        if password_verify(password, &old_pass_encrypted) {
+
+        if old_pass_encrypted.is_some_and(|old| password_verify(password, &old)) {
             return Err(anyhow!("New password cannot be the same as your current password").into());
         }
         self.db.delete_password_reset_by_user_id(user_id).await?;
@@ -164,10 +165,16 @@ impl AuthenticationService for AuthenticationServiceImpl {
             .await?
             .ok_or_else(|| anyhow!("Invalid user"))?;
 
-        let password_verified = match (user.password_encrypted.is_empty(), old_password) {
-            (true, _) => true,
-            (false, None) => false,
-            (false, Some(old_password)) => password_verify(old_password, &user.password_encrypted),
+        let password_verified = match (user.password_encrypted, old_password) {
+            // If the user had no password previously and specified no new password, that's fine
+            (None, None) => true,
+            // If they had a previous password, they must specify what it was and it must match
+            (Some(user_password), Some(old_password)) => {
+                password_verify(old_password, &user_password)
+            }
+            // Otherwise, they must have either specified a password when they did not have one
+            // or failed to specify a password when they did have one
+            _ => false,
         };
         if !password_verified {
             return Err(anyhow!("Password is incorrect").into());
@@ -190,7 +197,10 @@ impl AuthenticationService for AuthenticationServiceImpl {
             return Err(anyhow!("Invalid email address or password").into());
         };
 
-        if !password_verify(&password, &user.password_encrypted) {
+        if !user
+            .password_encrypted
+            .is_some_and(|pass| password_verify(&password, &pass))
+        {
             return Err(anyhow!("Invalid email address or password").into());
         }
 
@@ -485,15 +495,11 @@ async fn get_or_create_oauth_user(
         .map_err(|x| OAuthError::Other(x.into()))?
         .can_register_without_invitation(email)
     {
-        // it's ok to set password to empty string here, because
+        // it's ok to set password to null here, because
         // 1. both `register` & `token_auth` mutation will do input validation, so empty password won't be accepted
         // 2. `password_verify` will always return false for empty password hash read from user table
         // so user created here is only able to login by github oauth, normal login won't work
-        let res = (
-            db.create_user(email.to_owned(), "".to_owned(), false)
-                .await?,
-            false,
-        );
+        let res = (db.create_user(email.to_owned(), None, false).await?, false);
         if let Err(e) = mail.send_signup_email(email.to_string()).await {
             warn!("Failed to send signup email: {e}");
         }
@@ -504,12 +510,7 @@ async fn get_or_create_oauth_user(
         };
         // safe to create with empty password for same reasons above
         let id = db
-            .create_user_with_invitation(
-                email.to_owned(),
-                "".to_owned(),
-                false,
-                invitation.id as i32,
-            )
+            .create_user_with_invitation(email.to_owned(), None, false, invitation.id as i32)
             .await?;
         let user = db.get_user(id).await?.unwrap();
         Ok((user.id, user.is_admin))
@@ -877,7 +878,7 @@ mod tests {
         let license = service.license.read_license().await.unwrap();
         let id = service
             .db
-            .create_user("[email protected]".into(), "".into(), false)
+            .create_user("[email protected]".into(), None, false)
             .await
             .unwrap();
         service.db.update_user_active(id, false).await.unwrap();
@@ -942,13 +943,13 @@ mod tests {
         let service = test_authentication_service().await;
         let _ = service
             .db
-            .create_user("[email protected]".into(), "".into(), true)
+            .create_user("[email protected]".into(), None, true)
             .await
             .unwrap();
 
         let user_id = service
             .db
-            .create_user("[email protected]".into(), "".into(), false)
+            .create_user("[email protected]".into(), None, false)
             .await
             .unwrap();
 
@@ -963,7 +964,7 @@ mod tests {
         let service = test_authentication_service().await;
         let admin_id = service
             .db
-            .create_user("[email protected]".into(), "".into(), true)
+            .create_user("[email protected]".into(), None, true)
             .await
             .unwrap();
 
@@ -986,7 +987,7 @@ mod tests {
         // Test first reset, ensure wrong code fails
         service
             .db
-            .create_user("[email protected]".into(), "pass".into(), true)
+            .create_user("[email protected]".into(), Some("pass".into()), true)
             .await
             .unwrap();
         let user = service.get_user_by_email("[email protected]").await.unwrap();
@@ -1018,7 +1019,7 @@ mod tests {
             .await
             .unwrap()
             .unwrap();
-        assert_ne!(user.password_encrypted, "pass");
+        assert_ne!(user.password_encrypted, Some("pass".into()));
 
         service
             .request_password_reset_email("[email protected]".into())
@@ -1045,7 +1046,7 @@ mod tests {
         // Test third reset, ensure inactive users cannot reset their password
         let user_id_2 = service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
 
@@ -1090,17 +1091,17 @@ mod tests {
         let service = test_authentication_service().await;
         service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
         service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
         service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
 
@@ -1223,17 +1224,17 @@ mod tests {
 
         let user1 = service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
         let user2 = service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
         let user3 = service
             .db
-            .create_user("[email protected]".into(), "pass".into(), false)
+            .create_user("[email protected]".into(), Some("pass".into()), false)
             .await
             .unwrap();
 
@@ -1274,7 +1275,7 @@ mod tests {
         let service = test_authentication_service().await;
         let id = service
             .db
-            .create_user("[email protected]".into(), "".into(), true)
+            .create_user("[email protected]".into(), None, true)
             .await
             .unwrap();
 
@@ -1301,11 +1302,7 @@ mod tests {
         let (service, _mail) = test_authentication_service_with_mail().await;
         let id = service
             .db
-            .create_user(
-                "[email protected]".into(),
-                password_hash("pass").unwrap(),
-                true,
-            )
+            .create_user("[email protected]".into(), password_hash("pass").ok(), true)
             .await
             .unwrap();
 
@@ -1333,11 +1330,7 @@ mod tests {
 
         let id = service
             .db
-            .create_user(
-                "[email protected]".into(),
-                password_hash("pass").unwrap(),
-                true,
-            )
+            .create_user("[email protected]".into(), password_hash("pass").ok(), true)
             .await
             .unwrap();
 

ee/tabby-webserver/src/service/dao.rs

@@ -52,7 +52,7 @@ impl From<UserDAO> for auth::User {
             auth_token: val.auth_token,
             created_at: val.created_at,
             active: val.active,
-            is_password_set: !val.password_encrypted.is_empty(),
+            is_password_set: val.password_encrypted.is_some(),
         }
     }
 }

ee/tabby-webserver/src/service/event_logger.rs

@@ -93,7 +93,7 @@ mod tests {
         let db = DbConn::new_in_memory().await.unwrap();
         let logger = new_event_logger(db.clone());
         let user_id = db
-            .create_user("testuser".into(), "pass".into(), true)
+            .create_user("testuser".into(), Some("pass".into()), true)
             .await
             .unwrap();