Feat/add account#3
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces account deletion capability with two handler implementations: a session-based ChangesAccount Deletion Endpoint
Sequence DiagramsequenceDiagram
participant Client
participant AuthUser as AuthUser extractor
participant DeleteHandler as delete handler
participant DB as SeaORM DB
participant Session
Client->>DeleteHandler: POST /v1/accounts/me with password
DeleteHandler->>AuthUser: Extract authenticated user_id
AuthUser->>DB: Load user (DeletedAt IS NULL)
DeleteHandler->>DB: Verify password hash
DeleteHandler->>DB: Update user.deleted_at = now
DeleteHandler->>Session: Remove user_id
DeleteHandler->>Client: Return "Delete success"
🎯 3 (Moderate) | ⏱️ ~25 minutes
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
| let user_id:i64 = session | ||
| .get::<i64>("user_id") | ||
| .ok_or(AuthError::Forbidden)?; |
There was a problem hiding this comment.
セッションから user_id を取得する際、型が i64 と指定されていますが、users エンティティの主キーは uuid::Uuid 型です。auth.rs でのセッション保存時も Uuid が使用されているため、このままでは型不一致によりセッションの取得に失敗し、常に Forbidden エラーが返されることになります。
| let user_id:i64 = session | |
| .get::<i64>("user_id") | |
| .ok_or(AuthError::Forbidden)?; | |
| let user_id = session | |
| .get::<uuid::Uuid>("user_id") | |
| .ok_or(AuthError::Forbidden)?; |
| use sea_orm::{ColumnTrait, QueryFilter}; | ||
| use chrono::{Utc, FixedOffset}; | ||
| use crate::entities::users; | ||
| use crate::{AppState, models::user, utils::auth::AuthError}; |
| .one(&state.db) | ||
| .await? | ||
| .ok_or(AuthError::Forbidden)?; | ||
| let now = Utc::now().with_timezone(&FixedOffset::east_opt(0).unwrap()); |
| use utoipa_axum::router::OpenApiRouter; | ||
|
|
||
| use crate::AppState; | ||
| use crate::{AppState, routes::auth::routes}; |
Deploying storage with
|
| Latest commit: |
3cf10ad
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://27d6f9e1.storage-2lm.pages.dev |
| Branch Preview URL: | https://feat-add-account.storage-2lm.pages.dev |
ab8d356 to
c28d804
Compare
yupix
left a comment
There was a problem hiding this comment.
レビュー結果
アカウント削除機能には、動作を妨げる重大な問題が4件あります。
- 削除ルーターがアプリケーションへ登録されていない
- API仕様の
DELETE /v1/accounts/meと実装が一致しない - 論理削除後も再ログインできる
- セッションの
user_idをi64として取得している
user_id の型不一致については、既存のインラインコメントですでに具体的に指摘されていますが、現在の先端コミットでも未修正です。ログイン時には Uuid を保存しているため、削除処理側も Uuid として取得する必要があります。
4件とも、アカウント削除機能を正常に成立させるための P1(優先度: 高) の問題です。
|
|
||
| pub fn routes() -> OpenApiRouter<AppState> { | ||
| OpenApiRouter::<AppState>::new() | ||
| .routes(routes!(crate::handlers::account::delete)) |
There was a problem hiding this comment.
[P1] アカウント削除ルーターを登録する
このルーターは routes/mod.rs でモジュールとして公開されておらず、create_routes() にもネストされていません。そのため、削除エンドポイントへのリクエストは常に 404 になります。pub mod account; を追加し、/v1/accounts 配下へこのルーターを登録してください。
|
|
||
| #[utoipa::path( | ||
| post, | ||
| path = "/delete", |
There was a problem hiding this comment.
[P1] API仕様どおりのメソッドとパスを公開する
API仕様は DELETE /v1/accounts/me ですが、現在は POST /delete として定義されています。ルーターを登録しても仕様準拠のクライアントから利用できないため、メソッドを delete、パスを /me に合わせてください。
| let now = Utc::now().fixed_offset(); | ||
| // 削除 | ||
| let mut active: users::ActiveModel = user.into(); | ||
| active.deleted_at = Set(Some(now)); |
There was a problem hiding this comment.
[P1] 論理削除済みユーザーの再認証を拒否する
ここで deleted_at を設定してセッションを削除しても、ログイン処理は deleted_at を確認していません。そのため、削除直後に同じメールアドレスとパスワードで再ログインできます。ログイン検索時に未削除ユーザーだけを対象にし、認証Extractor側でも論理削除済みユーザーを拒否してください。
account削除を作成
論理削除で削除されたらDBに時間を挿入
Summary by CodeRabbit
New Features
Bug Fixes / Security
Refactor