no longer 500 on sentinel value passwords

crates/api_clients/artcraft/artcraft_api_defs/src/users/login.rs

@@ -0,0 +1,27 @@
+use serde_derive::{Deserialize, Serialize};
+use utoipa::ToSchema;
+
+#[derive(Deserialize, ToSchema)]
+pub struct LoginRequest {
+  pub username_or_email: String,
+  pub password: String,
+}
+
+#[derive(Serialize, ToSchema)]
+pub struct LoginSuccessResponse {
+  pub success: bool,
+
+  /// A signed session that can be sent as a header, bypassing cookies.
+  /// This is useful for API clients that don't support cookies or Google
+  /// browsers killing cross-domain cookies.
+  pub signed_session: String,
+}
+
+#[derive(Copy, Clone, Debug, Serialize, ToSchema)]
+pub enum LoginErrorType {
+  /// Account was created without a password and the user needs to create one
+  AccountNeedsPassword,
+  /// Invalid login credentials were supplied
+  InvalidCredentials,
+  ServerError,
+}

crates/api_clients/artcraft/artcraft_api_defs/src/users/mod.rs

@@ -1,4 +1,5 @@
 pub mod change_password;
 pub mod edit_email;
 pub mod edit_username;
+pub mod login;
 pub mod session_info;

crates/lib/password/src/bcrypt_confirm_password.rs

@@ -1,13 +1,23 @@
-use errors::AnyhowResult;
+use crate::constants::PASSWORD_HASH_SENTINEL_VALUE;
+use crate::errors::password_confirm_error::PasswordConfirmError;
 
-pub fn bcrypt_confirm_password(password: String, bcrypt_hash: &str) -> AnyhowResult<bool> {
-  let verified = bcrypt::verify(&password, &bcrypt_hash)?;
+pub fn bcrypt_confirm_password(password: String, bcrypt_hash: &str) -> Result<bool, PasswordConfirmError> {
+  if bcrypt_hash.is_empty() {
+    return Err(PasswordConfirmError::HashNotProvided);
+  }
+
+  if bcrypt_hash == PASSWORD_HASH_SENTINEL_VALUE {
+    return Err(PasswordConfirmError::HashIsSentinelValue);
+  }
+
+  let verified = bcrypt::verify(&password, bcrypt_hash)?;
   Ok(verified)
 }
 
 #[cfg(test)]
 mod tests {
   use crate::bcrypt_confirm_password::bcrypt_confirm_password;
+  use crate::errors::password_confirm_error::PasswordConfirmError;
 
   #[test]
   fn test_confirmation() {
@@ -25,4 +35,16 @@ mod tests {
       assert!(valid);
     }
   }
-}
+
+  #[test]
+  fn test_empty_hash_returns_error() {
+    let result = bcrypt_confirm_password("password".to_string(), "");
+    assert!(matches!(result, Err(PasswordConfirmError::HashNotProvided)));
+  }
+
+  #[test]
+  fn test_sentinel_value_returns_error() {
+    let result = bcrypt_confirm_password("password".to_string(), "*");
+    assert!(matches!(result, Err(PasswordConfirmError::HashIsSentinelValue)));
+  }
+}

crates/lib/password/src/bcrypt_hash_password.rs

@@ -1,6 +1,6 @@
-use errors::AnyhowResult;
+use crate::errors::password_hash_error::PasswordHashError;
 
-pub fn bcrypt_hash_password(password: String) -> AnyhowResult<String> {
+pub fn bcrypt_hash_password(password: String) -> Result<String, PasswordHashError> {
   let hash = bcrypt::hash(&password, bcrypt::DEFAULT_COST)?;
   Ok(hash)
 }

crates/lib/password/src/constants.rs

@@ -0,0 +1,3 @@
+/// Special database sentinel value indicating that a password is not set.
+/// This is used to support "no-password" signups, since the password hash database column is itself non-nullable.
+pub const PASSWORD_HASH_SENTINEL_VALUE: &str = "*";

crates/lib/password/src/errors/mod.rs

@@ -0,0 +1,3 @@
+pub mod password_error;
+pub mod password_confirm_error;
+pub mod password_hash_error;

crates/lib/password/src/errors/password_confirm_error.rs

@@ -0,0 +1,30 @@
+use std::fmt;
+
+#[derive(Debug)]
+pub enum PasswordConfirmError {
+  /// The password hash is the empty string.
+  HashNotProvided,
+  /// The password hash is the special database sentinel value "*",
+  /// which is used to indicate that the password is not set.
+  HashIsSentinelValue,
+  /// An error with verifying the password.
+  BcryptVerifyError(bcrypt::BcryptError),
+}
+
+impl fmt::Display for PasswordConfirmError {
+  fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    match self {
+      Self::HashNotProvided => write!(f, "password hash not provided"),
+      Self::HashIsSentinelValue => write!(f, "password hash is sentinel value"),
+      Self::BcryptVerifyError(e) => write!(f, "bcrypt verify error: {}", e),
+    }
+  }
+}
+
+impl std::error::Error for PasswordConfirmError {}
+
+impl From<bcrypt::BcryptError> for PasswordConfirmError {
+  fn from(err: bcrypt::BcryptError) -> Self {
+    Self::BcryptVerifyError(err)
+  }
+}

crates/lib/password/src/errors/password_error.rs

@@ -0,0 +1,33 @@
+use std::fmt;
+
+use crate::errors::password_confirm_error::PasswordConfirmError;
+use crate::errors::password_hash_error::PasswordHashError;
+
+#[derive(Debug)]
+pub enum PasswordError {
+  CheckError(PasswordConfirmError),
+  HashError(PasswordHashError),
+}
+
+impl fmt::Display for PasswordError {
+  fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    match self {
+      Self::CheckError(e) => write!(f, "{}", e),
+      Self::HashError(e) => write!(f, "{}", e),
+    }
+  }
+}
+
+impl std::error::Error for PasswordError {}
+
+impl From<PasswordConfirmError> for PasswordError {
+  fn from(err: PasswordConfirmError) -> Self {
+    Self::CheckError(err)
+  }
+}
+
+impl From<PasswordHashError> for PasswordError {
+  fn from(err: PasswordHashError) -> Self {
+    Self::HashError(err)
+  }
+}

crates/lib/password/src/errors/password_hash_error.rs

@@ -0,0 +1,23 @@
+use std::fmt;
+
+#[derive(Debug)]
+pub enum PasswordHashError {
+  /// An error with hashing the password.
+  BcryptHashError(bcrypt::BcryptError),
+}
+
+impl fmt::Display for PasswordHashError {
+  fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    match self {
+      Self::BcryptHashError(e) => write!(f, "bcrypt hash error: {}", e),
+    }
+  }
+}
+
+impl std::error::Error for PasswordHashError {}
+
+impl From<bcrypt::BcryptError> for PasswordHashError {
+  fn from(err: bcrypt::BcryptError) -> Self {
+    Self::BcryptHashError(err)
+  }
+}

crates/lib/password/src/lib.rs

@@ -23,3 +23,5 @@
 //  https://stytch.com/blog/argon2-vs-bcrypt-vs-scrypt/
 pub mod bcrypt_hash_password;
 pub mod bcrypt_confirm_password;
+pub mod constants;
+pub mod errors;

crates/service/web/storyteller_web/src/docs/api_doc.rs

@@ -176,7 +176,8 @@ use crate::http_server::endpoints::users::edit_email_handler::*;
 use crate::http_server::endpoints::users::edit_username_handler::*;
 use crate::http_server::endpoints::users::get_profile_handler::*;
 use crate::http_server::endpoints::users::google_sso::google_sso_handler::*;
-use crate::http_server::endpoints::users::login_handler::*;
+use artcraft_api_defs::users::login::{LoginRequest, LoginSuccessResponse, LoginErrorType};
+use crate::http_server::endpoints::users::login_handler::LoginErrorResponse;
 use crate::http_server::endpoints::users::logout_handler::*;
 use crate::http_server::endpoints::users::session_info_handler::*;
 use crate::http_server::endpoints::users::session_token_info_handler::*;

crates/service/web/storyteller_web/src/http_server/endpoints/users/login_handler.rs

@@ -1,8 +1,3 @@
-// NB: Incrementally getting rid of build warnings...
-#![forbid(unused_imports)]
-#![forbid(unused_mut)]
-#![forbid(unused_variables)]
-
 use std::fmt;
 use std::fmt::Formatter;
 
@@ -21,25 +16,11 @@ use sqlx::MySqlPool;
 use tokens::tokens::user_sessions::UserSessionToken;
 use utoipa::ToSchema;
 
+use artcraft_api_defs::users::login::{LoginErrorType, LoginRequest, LoginSuccessResponse};
+use password::errors::password_confirm_error::PasswordConfirmError;
 use crate::http_server::session::lookup::user_session_feature_flags::UserSessionFeatureFlags;
 use crate::util::enroll_in_studio::enroll_in_studio;
 
-#[derive(Deserialize, ToSchema)]
-pub struct LoginRequest {
-  pub username_or_email: String,
-  pub password: String,
-}
-
-#[derive(Serialize, ToSchema)]
-pub struct LoginSuccessResponse {
-  pub success: bool,
-
-  /// A signed session that can be sent as a header, bypassing cookies.
-  /// This is useful for API clients that don't support cookies or Google
-  /// browsers killing cross-domain cookies.
-  pub signed_session: String,
-}
-
 #[derive(Serialize, Debug, ToSchema)]
 pub struct LoginErrorResponse {
   pub success: bool,
@@ -47,29 +28,6 @@ pub struct LoginErrorResponse {
   pub error_message: String,
 }
 
-#[derive(Copy, Clone, Debug, Serialize, ToSchema)]
-pub enum LoginErrorType {
-  InvalidCredentials,
-  ServerError,
-}
-
-impl LoginErrorResponse {
-  fn invalid_credentials() -> Self {
-    Self {
-      success: false,
-      error_type: LoginErrorType::InvalidCredentials,
-      error_message: "invalid credentials".to_string()
-    }
-  }
-  fn server_error() -> Self {
-    Self {
-      success: false,
-      error_type: LoginErrorType::ServerError,
-      error_message: "server error".to_string()
-    }
-  }
-}
-
 // NB: Not using DeriveMore since Clion doesn't understand it.
 impl fmt::Display for LoginErrorResponse {
   fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
@@ -81,6 +39,7 @@ impl ResponseError for LoginErrorResponse {
   fn status_code(&self) -> StatusCode {
     match self.error_type {
       LoginErrorType::InvalidCredentials => StatusCode::UNAUTHORIZED,
+      LoginErrorType::AccountNeedsPassword => StatusCode::UNAUTHORIZED,
       LoginErrorType::ServerError=> StatusCode::INTERNAL_SERVER_ERROR,
     }
   }
@@ -90,6 +49,32 @@ impl ResponseError for LoginErrorResponse {
   }
 }
 
+impl LoginErrorResponse {
+  fn invalid_credentials() -> Self {
+    Self {
+      success: false,
+      error_type: LoginErrorType::InvalidCredentials,
+      error_message: "invalid credentials".to_string()
+    }
+  }
+  
+  fn server_error() -> Self {
+    Self {
+      success: false,
+      error_type: LoginErrorType::ServerError,
+      error_message: "server error".to_string()
+    }
+  }
+
+  fn account_without_password() -> Self {
+    Self {
+      success: false,
+      error_type: LoginErrorType::AccountNeedsPassword,
+      error_message: "account was created without a password; please try password reset".to_string()
+    }
+  }
+}
+
 #[utoipa::path(
   post,
   tag = "Users",
@@ -146,6 +131,10 @@ pub async fn login_handler(
   };
 
   match bcrypt_confirm_password(request.password.clone(), &actual_hash) {
+    Err(PasswordConfirmError::HashIsSentinelValue) => {
+      warn!("Attempting to log in with a passwordless account");
+      return Err(LoginErrorResponse::account_without_password());
+    }
     Err(e) => {
       warn!("Login hash comparison error: {:?}", e);
       return Err(LoginErrorResponse::server_error());