From c6ed5c5c197b8e4433f8e4eafdf4d0155f833570 Mon Sep 17 00:00:00 2001 From: asepharyana Date: Tue, 4 Aug 2026 23:35:34 +0700 Subject: [PATCH] fix(dimentorin): verify-email validates OTP before activating user - new app_otp_cache table + OtpCache entity (ResourceEnum::OtpCache) - PostgresOtpRepository upsert/find/delete keyed by email - register/resend persist otp_hash+expiry after email sent (no orphan OTP) - verify_email validates via OtpManager::validate_otp_hash, single-use delete - 8 unit tests pass, e2e verified: wrong OTP 400, correct OTP 200 --- docs/dev-audit-findings.md | 23 ++--- imphnen-backend/src/bin/create_schema.rs | 1 + imphnen-entities/src/seaorm/common/mod.rs | 1 + .../src/seaorm/common/otp_cache.rs | 31 +++++++ imphnen-iam/src/auth/application/mod.rs | 44 ++++++++++ imphnen-iam/src/auth/domain/mod.rs | 1 + imphnen-iam/src/auth/domain/otp.rs | 18 ++++ .../src/auth/infrastructure/http/routes.rs | 6 +- imphnen-iam/src/auth/infrastructure/mod.rs | 2 + .../auth/infrastructure/persistence/mod.rs | 2 +- .../persistence/postgres_otp_repository.rs | 84 +++++++++++++++++++ imphnen-utils/src/generate_otp.rs | 45 +++++++++- 12 files changed, 240 insertions(+), 18 deletions(-) create mode 100644 imphnen-entities/src/seaorm/common/otp_cache.rs create mode 100644 imphnen-iam/src/auth/domain/otp.rs create mode 100644 imphnen-iam/src/auth/infrastructure/persistence/postgres_otp_repository.rs diff --git a/docs/dev-audit-findings.md b/docs/dev-audit-findings.md index 54d2c59..3b64736 100644 --- a/docs/dev-audit-findings.md +++ b/docs/dev-audit-findings.md @@ -10,23 +10,18 @@ Semua diuji lokal (Postgres `dimentorin`, backend :4099). - **Dampak**: mentee/mentor baru tak bisa menerima OTP lewat email → tak bisa aktivasi → tak bisa login, kecuali via verify-email langsung. - **Diperlukan**: SMTP credential institution yang valid (Gmail App Password atau SMTP relay), sebaiknya dari BWS secret management, bukan hardcode. -## 2. verify-email TIDAK memverifikasi OTP (security issue) +## 2. ✅ FIXED — verify-email TIDAK memverifikasi OTP (security issue) + +**Status: FIXED di branch feat/dimentorin-postgres (2026-08-04).** `imphnen-iam/src/auth/application/mod.rs` → `verify_email()`: -```rust -async fn verify_email(&self, payload: VerifyEmailInput) -> ... { - let user = ...find_by_email...; - if user.is_active { return Err(...); } - self.user_repo.update(UserEntity { is_active: true, ..user }).await?; - Ok(()) -} -``` - -- OTP di-generate (`OtpManager::generate_otp`) + dikirim via email, TAPI **`payload.otp` tidak pernah divalidasi**. -- `OtpManager::validate_otp` ada tapi tak dipanggil di handler ini. -- **Dampak**: siapa pun yang tahu email bisa mengaktifkan akun dengan otp sembarang. Verifikasi email berbasis kepemilikan = tidak ada. -- **Fix yang disarankan**: `verify_email` harus menyimpan `OtpData` (hash+expiry) saat kirim, lalu memanggil `OtpManager::validate_otp(stored, payload.otp)` sebelum set `is_active`. +- OTP sekarang dipersist ke tabel **`app_otp_cache`** (entity baru `imphnen-entities/src/seaorm/common/otp_cache.rs`, resource `app_otp_cache` sudah direncanakan di `ResourceEnum::OtpCache`). +- `register()` & `resend_otp()` menyimpan `otp_hash` + `expires_at` setelah email terkirim (kalau email gagal, tidak ada OTP yatim / OTP lama tidak di-overwrite). +- `verify_email()` memanggil `OtpManager::validate_otp_hash(stored_hash, expires_at, payload.otp)` sebelum set `is_active`. `validate_otp_hash` ditambahkan ke `OtpManager` (pure hash+expiry tanpa perlu plaintext code). +- OTP **single-use**: di-delete setelah verifikasi sukses. Reuse / OTP tanpa cache / OTP expired semua ditolak (400). +- Uji lokal (Postgres, :4099): OTP salah → 400 "Invalid or expired OTP", user tetap inactive; OTP benar → 200 "Email verified successfully", user aktif, OTP dihapus; verify ulang → 400 "User already active"; email tanpa OTP → 400 "No OTP issued". +- Tabel dibuat via SQL manual (`create_schema.rs` ditambah `otp_cache` untuk bootstrap penuh). ## 3. (OK, sudah benar) Register mentor + booking diff --git a/imphnen-backend/src/bin/create_schema.rs b/imphnen-backend/src/bin/create_schema.rs index a2d4ed9..d85b6dc 100644 --- a/imphnen-backend/src/bin/create_schema.rs +++ b/imphnen-backend/src/bin/create_schema.rs @@ -39,6 +39,7 @@ async fn main() -> Result<(), Box> { .await?; drop_and_create_table(&db, builder, "rate_limits", common::rate_limit::Entity) .await?; + drop_and_create_table(&db, builder, "otp_cache", common::otp_cache::Entity).await?; drop_and_create_table(&db, builder, "gacha_credits", gacha::gacha_credits::Entity) .await?; diff --git a/imphnen-entities/src/seaorm/common/mod.rs b/imphnen-entities/src/seaorm/common/mod.rs index e14fd78..5092363 100644 --- a/imphnen-entities/src/seaorm/common/mod.rs +++ b/imphnen-entities/src/seaorm/common/mod.rs @@ -3,6 +3,7 @@ pub mod audit_log; pub mod enum_impls; pub mod enums; pub mod events; +pub mod otp_cache; pub mod rate_limit; pub mod roadmap_items; pub mod testimonials; diff --git a/imphnen-entities/src/seaorm/common/otp_cache.rs b/imphnen-entities/src/seaorm/common/otp_cache.rs new file mode 100644 index 0000000..dd6a35c --- /dev/null +++ b/imphnen-entities/src/seaorm/common/otp_cache.rs @@ -0,0 +1,31 @@ +use chrono::{DateTime, Utc}; +use sea_orm::entity::prelude::*; +use serde::{Deserialize, Serialize}; +use uuid::Uuid; + +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, DeriveEntityModel)] +#[sea_orm(table_name = "app_otp_cache")] +pub struct Model { + #[sea_orm(primary_key, default = "gen_random_uuid()", auto_increment = false)] + pub id: Uuid, + + #[sea_orm(unique, not_null)] + pub email: String, + + #[sea_orm(not_null)] + pub otp_hash: String, + + #[sea_orm(not_null)] + pub expires_at: DateTime, + + #[sea_orm(not_null, default = "now()")] + pub created_at: DateTime, + + #[sea_orm(not_null, default = "now()")] + pub updated_at: DateTime, +} + +#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)] +pub enum Relation {} + +impl ActiveModelBehavior for ActiveModel {} diff --git a/imphnen-iam/src/auth/application/mod.rs b/imphnen-iam/src/auth/application/mod.rs index 274969e..13a6022 100644 --- a/imphnen-iam/src/auth/application/mod.rs +++ b/imphnen-iam/src/auth/application/mod.rs @@ -1,3 +1,4 @@ +use crate::auth::domain::otp::{OtpCacheRecord, OtpRepository}; use crate::auth::domain::AuthService; use crate::auth::domain::types::{ AuthTokens, AuthUserDetail, LoginInput, LoginOutput, NewPasswordInput, @@ -22,16 +23,19 @@ use uuid::Uuid; pub struct AuthServiceImpl { user_repo: Arc, role_repo: Arc, + otp_repo: Arc, } impl AuthServiceImpl { pub fn new( user_repo: Arc, role_repo: Arc, + otp_repo: Arc, ) -> Self { Self { user_repo, role_repo, + otp_repo, } } } @@ -187,6 +191,16 @@ impl AuthService for AuthServiceImpl { ..Default::default() }) .await?; + // Persist OTP only after user creation succeeded, so a failed + // registration leaves no orphan OTP record behind. + self + .otp_repo + .save(OtpCacheRecord { + email: payload.email.clone(), + otp_hash: otp.hash.clone(), + expires_at: otp.expires_at, + }) + .await?; Ok(()) } @@ -203,6 +217,16 @@ impl AuthService for AuthServiceImpl { &format!("Your OTP code is {}", otp.code), ) .map_err(|e| AppError::BadRequestError(e.to_string()))?; + // Overwrite stored OTP only after the email was actually sent, so a + // failed resend never invalidates the previous (still valid) code. + self + .otp_repo + .save(OtpCacheRecord { + email: payload.email.clone(), + otp_hash: otp.hash.clone(), + expires_at: otp.expires_at, + }) + .await?; Ok(()) } @@ -265,6 +289,24 @@ impl AuthService for AuthServiceImpl { if user.is_active { return Err(AppError::BadRequestError("User already active".into())); } + let stored = self + .otp_repo + .find_by_email(&payload.email) + .await + .map_err(|_| { + AppError::BadRequestError( + "No OTP issued for this email, request a new code".into(), + ) + })?; + if !OtpManager::validate_otp_hash( + &stored.otp_hash, + &stored.expires_at, + payload.otp, + ) { + return Err(AppError::BadRequestError( + "Invalid or expired OTP".into(), + )); + } self .user_repo .update(UserEntity { @@ -272,6 +314,8 @@ impl AuthService for AuthServiceImpl { ..user }) .await?; + // OTP is single-use — consume it on successful verification. + self.otp_repo.delete_by_email(&payload.email).await?; Ok(()) } diff --git a/imphnen-iam/src/auth/domain/mod.rs b/imphnen-iam/src/auth/domain/mod.rs index c520630..7a4dea6 100644 --- a/imphnen-iam/src/auth/domain/mod.rs +++ b/imphnen-iam/src/auth/domain/mod.rs @@ -1,3 +1,4 @@ +pub mod otp; pub mod types; use async_trait::async_trait; diff --git a/imphnen-iam/src/auth/domain/otp.rs b/imphnen-iam/src/auth/domain/otp.rs new file mode 100644 index 0000000..98fb868 --- /dev/null +++ b/imphnen-iam/src/auth/domain/otp.rs @@ -0,0 +1,18 @@ +use async_trait::async_trait; +use chrono::{DateTime, Utc}; +use imphnen_utils::AppError; + +#[derive(Clone, Debug)] +pub struct OtpCacheRecord { + pub email: String, + pub otp_hash: String, + pub expires_at: DateTime, +} + +#[async_trait] +pub trait OtpRepository: Send + Sync { + /// Upsert OTP record keyed by email (one active OTP per email). + async fn save(&self, record: OtpCacheRecord) -> Result<(), AppError>; + async fn find_by_email(&self, email: &str) -> Result; + async fn delete_by_email(&self, email: &str) -> Result<(), AppError>; +} diff --git a/imphnen-iam/src/auth/infrastructure/http/routes.rs b/imphnen-iam/src/auth/infrastructure/http/routes.rs index 5585018..b397437 100644 --- a/imphnen-iam/src/auth/infrastructure/http/routes.rs +++ b/imphnen-iam/src/auth/infrastructure/http/routes.rs @@ -4,6 +4,7 @@ use super::handlers::{ }; use crate::auth::application::AuthServiceImpl; use crate::auth::domain::AuthService; +use crate::auth::infrastructure::PostgresOtpRepository; use crate::roles::infrastructure::persistence::PostgresRoleRepository; use crate::users::infrastructure::persistence::PostgresUserRepository; use axum::{Extension, Router, routing::post}; @@ -18,8 +19,11 @@ pub fn auth_public_routes(_db: DatabaseConnection, state: Arc) -> Rout let role_repo = Arc::new(PostgresRoleRepository::new( state.postgres_connection.conn.clone(), )); + let otp_repo = Arc::new(PostgresOtpRepository::new( + state.postgres_connection.conn.clone(), + )); let auth_service: Arc = - Arc::new(AuthServiceImpl::new(user_repo, role_repo)); + Arc::new(AuthServiceImpl::new(user_repo, role_repo, otp_repo)); Router::new() .route("/auth/login", post(post_login)) .route("/auth/login-mentor", post(post_login_mentor)) diff --git a/imphnen-iam/src/auth/infrastructure/mod.rs b/imphnen-iam/src/auth/infrastructure/mod.rs index 4c61c09..65cad17 100644 --- a/imphnen-iam/src/auth/infrastructure/mod.rs +++ b/imphnen-iam/src/auth/infrastructure/mod.rs @@ -1,2 +1,4 @@ pub mod http; pub mod persistence; + +pub use persistence::postgres_otp_repository::PostgresOtpRepository; diff --git a/imphnen-iam/src/auth/infrastructure/persistence/mod.rs b/imphnen-iam/src/auth/infrastructure/persistence/mod.rs index 8b13789..2cda7e2 100644 --- a/imphnen-iam/src/auth/infrastructure/persistence/mod.rs +++ b/imphnen-iam/src/auth/infrastructure/persistence/mod.rs @@ -1 +1 @@ - +pub mod postgres_otp_repository; diff --git a/imphnen-iam/src/auth/infrastructure/persistence/postgres_otp_repository.rs b/imphnen-iam/src/auth/infrastructure/persistence/postgres_otp_repository.rs new file mode 100644 index 0000000..b701233 --- /dev/null +++ b/imphnen-iam/src/auth/infrastructure/persistence/postgres_otp_repository.rs @@ -0,0 +1,84 @@ +use crate::auth::domain::otp::{OtpCacheRecord, OtpRepository}; +use async_trait::async_trait; +use chrono::Utc; +use imphnen_entities::seaorm::common::otp_cache::{ + ActiveModel as OtpCacheActiveModel, Column as OtpCacheColumn, Entity as OtpCacheEntity, +}; +use imphnen_utils::AppError; +use sea_orm::ActiveValue::Set; +use sea_orm::prelude::*; +use std::sync::Arc; + +pub struct PostgresOtpRepository { + db: Arc, +} + +impl PostgresOtpRepository { + pub fn new(db: DatabaseConnection) -> Self { + Self { db: Arc::new(db) } + } +} + +#[async_trait] +impl OtpRepository for PostgresOtpRepository { + async fn save(&self, record: OtpCacheRecord) -> Result<(), AppError> { + let now = Utc::now(); + let otp_hash = record.otp_hash.clone(); + let expires_at = record.expires_at; + let active_model = OtpCacheActiveModel { + id: Set(uuid::Uuid::new_v4()), + email: Set(record.email.clone()), + otp_hash: Set(otp_hash.clone()), + expires_at: Set(expires_at), + created_at: Set(now), + updated_at: Set(now), + }; + + // Upsert: replace any existing (possibly expired) OTP for the same email. + let existing = OtpCacheEntity::find() + .filter(OtpCacheColumn::Email.eq(record.email.clone())) + .one(self.db.as_ref()) + .await + .map_err(|e| AppError::InternalServerError(e.to_string()))?; + + if let Some(existing) = existing { + let mut update: OtpCacheActiveModel = existing.into(); + update.otp_hash = Set(otp_hash); + update.expires_at = Set(expires_at); + update.updated_at = Set(now); + update + .update(self.db.as_ref()) + .await + .map_err(|e| AppError::InternalServerError(e.to_string()))?; + } else { + OtpCacheEntity::insert(active_model) + .exec(self.db.as_ref()) + .await + .map_err(|e| AppError::InternalServerError(e.to_string()))?; + } + Ok(()) + } + + async fn find_by_email(&self, email: &str) -> Result { + let row = OtpCacheEntity::find() + .filter(OtpCacheColumn::Email.eq(email)) + .one(self.db.as_ref()) + .await + .map_err(|e| AppError::InternalServerError(e.to_string()))? + .ok_or_else(|| AppError::NotFoundError("No OTP issued for this email".into()))?; + Ok(OtpCacheRecord { + email: row.email, + otp_hash: row.otp_hash, + expires_at: row.expires_at, + }) + } + + async fn delete_by_email(&self, email: &str) -> Result<(), AppError> { + OtpCacheEntity::delete_many() + .filter(OtpCacheColumn::Email.eq(email)) + .exec(self.db.as_ref()) + .await + .map_err(|e| AppError::InternalServerError(e.to_string()))?; + Ok(()) + } +} diff --git a/imphnen-utils/src/generate_otp.rs b/imphnen-utils/src/generate_otp.rs index 7c5a095..3deaffb 100644 --- a/imphnen-utils/src/generate_otp.rs +++ b/imphnen-utils/src/generate_otp.rs @@ -27,14 +27,24 @@ impl OtpManager { } pub fn validate_otp(stored: &OtpData, user_otp: u32) -> bool { - if Utc::now() > stored.expires_at { + Self::validate_otp_hash(&stored.hash, &stored.expires_at, user_otp) + } + + /// Validate a user-supplied OTP against a stored hash + expiry (e.g. from a + /// cache table where the plaintext code is not persisted). + pub fn validate_otp_hash( + stored_hash: &str, + expires_at: &DateTime, + user_otp: u32, + ) -> bool { + if Utc::now() > *expires_at { return false; } let user_otp_str = user_otp.to_string(); let mut hasher = Sha256::new(); hasher.update(user_otp_str.as_bytes()); let user_hash = format!("{:x}", hasher.finalize()); - user_hash == stored.hash + user_hash == stored_hash } } @@ -63,6 +73,37 @@ mod tests { assert!(!OtpManager::validate_otp(&otp, 123456)); } + #[test] + fn test_validate_otp_hash_valid() { + let otp = OtpManager::generate_otp(); + assert!(OtpManager::validate_otp_hash( + &otp.hash, + &otp.expires_at, + otp.code + )); + } + + #[test] + fn test_validate_otp_hash_invalid() { + let otp = OtpManager::generate_otp(); + assert!(!OtpManager::validate_otp_hash( + &otp.hash, + &otp.expires_at, + 123456 + )); + } + + #[test] + fn test_validate_otp_hash_expired() { + let mut otp = OtpManager::generate_otp(); + otp.expires_at = Utc::now() - chrono::Duration::seconds(1); + assert!(!OtpManager::validate_otp_hash( + &otp.hash, + &otp.expires_at, + otp.code + )); + } + #[test] fn test_validate_otp_expired() { let mut otp = OtpManager::generate_otp();