feat(drive): add drive feature (Phase A — backend)#32
Open
yupix wants to merge 16 commits into
Open
Conversation
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
…ties Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Walkthroughテナント単位の Drive 機能を追加。ストレージ抽象化（Local/S3）、DB エンティティ（drive_files/drive_folders/drive_folder_shares）、ファイル・フォルダ CRUD、共有（ユーザ/公開リンク）、アクセス制御、クォータ管理、Bruno テスト、OpenAPI 統合を一貫実装。 ChangesDrive 機能実装
Sequence DiagramsequenceDiagram
participant Client
participant DriveHandler as drive_files handler
participant StorageBackend
participant Database
Client->>DriveHandler: POST /v1/tenants/{tenant_id}/drive/files (multipart stream)
DriveHandler->>StorageBackend: upload(key, ByteStream, content_length, mime)
StorageBackend-->>DriveHandler: OK / error
DriveHandler->>Database: insert drive_files(metadata with storage_key)
Database-->>DriveHandler: inserted id
DriveHandler-->>Client: 201 Created { id }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
yupix
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/bruno/drive/requests/07_files_content_project.bru`:
- Around line 18-20: The test "status 200 or 403" is too permissive and can mask
regressions; change this to assert a single expected status (e.g.,
expect(res.status).to.equal(200)) in the successful scenario and create a
separate test for the forbidden scenario that asserts
expect(res.status).to.equal(403). Locate the assertion in the test block named
"status 200 or 403" and update it to a single-value expectation (use res.status
in the existing test), and add a new test case that sets up the forbidden
condition and asserts 403.
In `@apps/backend/bruno/drive/requests/08_files_content_share_token.bru`:
- Around line 14-16: Test name "status 200 or 403" does not match the allowed
status array in the assertion; update either the test name or the allowed
statuses to be consistent. Locate the test invocation test("status 200 or 403",
...) and the assertion expect([200, 403, 404]).to.include(res.status); then
either remove 404 from the array if 404 should not be accepted, or change the
test name to something like "status 200, 403 or 404" to reflect the allowed
statuses so the test intent and assertion match.
In `@apps/backend/src/entities/drive_folder_shares.rs`:
- Around line 47-53: The XOR check treats an empty share_token as "unset"
causing a mismatch with the CHECK intent; update the token presence check so an
empty string still counts as present by changing how has_token is computed
(replace the current share_token.is_some_and(|t| !t.is_empty()) usage with a
nil-aware presence check such as share_token.is_some()), leaving the XOR
comparison (if has_user == has_token { ... }) and the error message in place;
this ensures shared_with_user_id, share_token and has_token behave consistently
with the CHECK constraint.
In `@apps/backend/src/handlers/drive_files.rs`:
- Around line 422-443: アップロード成功後に DB 挿入（drive_files::ActiveModel::insert
呼び出し）でエラーが発生した場合、ストレージ上に孤立ファイルが残るため、model.insert(&state.db).await? のエラーをハンドルして
storage.delete(&storage_key).await を呼び出してロールバックする処理を追加してください；storage.delete
の呼び出しが失敗しても元の DB
挿入エラーを上書きしないようにし（ログに失敗を記録）、最終的に適切なエラーを返すようにします（state.storage.upload と
storage_key を利用）。
- Around line 539-548: 現在は load_tenant_file ->
state.storage.delete(&file.storage_key) ->
drive_files::Entity::delete_by_id(...) とストレージ削除を先に行っているため、DB
削除失敗時にダングリング参照が発生します。修正は
drive_files::Entity::delete_by_id(id).exec(&state.db).await? を先に実行して DB
レコードを削除し、その後で
state.storage.delete(&file.storage_key).await.map_err(storage_to_app_error)?
を呼ぶように切り替えてください（load_tenant_file を先に呼んで file.storage_key
を取得するのは問題なし）。ストレージ削除が失敗した場合は適切にログを残しリトライ/ガベージコレクションを考慮する処理を入れてください。
- Around line 219-223: The check uses file.project_id.is_none() and returns
early, so calling file.project_id.ok_or(AppError::Forbidden) is redundant;
instead safely unwrap the Option (e.g., using file.project_id.unwrap() or
pattern-match) to obtain project_id without ok_or. Update the code around
project_id retrieval (the file.project_id check and the subsequent let
project_id = ...) to directly extract the Some value (referencing
file.project_id and the variable project_id) and remove the unnecessary
AppError::Forbidden ok_or call.
In `@apps/backend/src/handlers/drive_folders.rs`:
- Around line 493-495: The check guarding for share.share_token.is_none() is
unreachable because load_active_share_by_token filters by ShareToken.eq(token)
so a matched Share's share_token cannot be None; remove the if-block (at the two
occurrences around the load_active_share_by_token call) to eliminate dead code,
or if the intent was to reject user-specific shares instead, replace the
condition with share.shared_with_user_id.is_some() to explicitly deny shares
tied to a user (referencing load_active_share_by_token, share.share_token, and
share.shared_with_user_id).
- Line 10: Keep the `use rand::RngExt;` import as-is (rand 0.10 is correct);
remove the redundant/unreachable checks that test `share.share_token.is_none()`
in the public-share handlers — specifically delete the branches in
`get_public_share_folder` and `list_public_share_files` that return an error or
skip when `share.share_token.is_none()` because `load_active_share_by_token()`
already queries by `drive_folder_shares::Column::ShareToken.eq(token)` and
guarantees a token-backed share; leave the rest of the logic intact.
In `@apps/backend/src/handlers/projects.rs`:
- Around line 116-134: Wrap the project and drive_folder inserts in a single DB
transaction: begin a transaction on state.db, perform project.insert(...) using
the transaction connection, then create the drive_folders::ActiveModel and
insert it with the same transaction (use the same tenant_id, auth.user_id and
model.id), and return/commit only if both inserts succeed; on any error return
Err so the transaction rolls back. Ensure you replace the two raw calls
project.insert(&state.db) and drive_folder.insert(&state.db) to use the
transaction connection and propagate errors to trigger rollback.
In `@apps/backend/src/routes/drive.rs`:
- Around line 29-34: Replace the current .route("/files",
axum::routing::post(crate::handlers::drive_files::upload_file).layer(RequestBodyLimitLayer::new(upload_limit
as usize))) registration with a routes-based registration so utoipa-axum picks
up the #[utoipa::path(..., path = "/files", ...)] on upload_file; specifically
register the handler via routes![crate::handlers::drive_files::upload_file]
(e.g. .routes("/files", routes![crate::handlers::drive_files::upload_file])) and
apply the RequestBodyLimitLayer::new(upload_limit as usize) to that router (via
.layer(...) or equivalent) so the same body limit is preserved.
In `@apps/backend/src/utils/storage/local.rs`:
- Around line 97-99: public_url が storage_key をそのまま埋め込しており Drive API
の契約（/v1/drive/files/{id}/content）と不整合を起こしているので、public_url(&self, key: &str) 内で
storage_key をファイル ID に変換するか、呼び出し側で file id を渡すようシグネチャを変更して一貫させてください；具体的には関数
public_url を修正して storage_key をファイル ID にマップするロジック（または既存の変換ヘルパーを利用）を入れてから
format!("/v1/drive/files/{id}/content") を返すようにし、もし変換が不可能なら適切にエラーを返す（または
Option/Result を使う）ようにしてください。
In `@docs/content/2.features/drive.md`:
- Around line 253-259: The markdown code blocks showing the ASCII diagram
(containing DRIVE_SYSTEM_MAX_QUOTA_MB, tenants.drive_quota_bytes,
DRIVE_DEFAULT_QUOTA_MB) should include a language tag to satisfy markdownlint
MD040; update those fenced code blocks by changing the opening triple-backticks
to include `text` or `plaintext` (e.g., ```text) for the block at the shown diff
and the other occurrences referenced (around lines 344-346 and 641-661) so the
diagram is treated as plain text and the linter warning is resolved.
- Around line 110-115: Add blank lines before and after the fenced code block
and specify the language identifier "sql" for the fence to satisfy markdownlint
(MD031, MD040); locate the CHECK constraint block containing "CHECK (project_id
IS NULL OR folder_id IS NOT NULL)" and update its surrounding markdown so there
is an empty line above the opening ```sql and an empty line below the closing
``` fence, keeping the explanatory sentence referencing project_id and folder_id
outside the fenced block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e66edace-9224-4735-9226-0633a85ef611
⛔ Files ignored due to path filters (1)
apps/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (53)
apps/backend/.env.exampleapps/backend/Cargo.tomlapps/backend/bruno/drive/environments/local.bruapps/backend/bruno/drive/fixtures/sample.txtapps/backend/bruno/drive/folder.bruapps/backend/bruno/drive/requests/01_files_list.bruapps/backend/bruno/drive/requests/02_files_upload.bruapps/backend/bruno/drive/requests/03_files_get.bruapps/backend/bruno/drive/requests/04_files_update.bruapps/backend/bruno/drive/requests/05_files_delete.bruapps/backend/bruno/drive/requests/06_files_content_general.bruapps/backend/bruno/drive/requests/07_files_content_project.bruapps/backend/bruno/drive/requests/08_files_content_share_token.bruapps/backend/bruno/drive/requests/09_usage.bruapps/backend/bruno/drive/requests/10_quota.bruapps/backend/bruno/drive/requests/11_folders_list.bruapps/backend/bruno/drive/requests/12_folders_create.bruapps/backend/bruno/drive/requests/13_folders_update.bruapps/backend/bruno/drive/requests/14_folders_delete.bruapps/backend/bruno/drive/requests/15_shares_list.bruapps/backend/bruno/drive/requests/16_shares_create_user.bruapps/backend/bruno/drive/requests/17_shares_create_public.bruapps/backend/bruno/drive/requests/18_shares_delete.bruapps/backend/bruno/drive/requests/19_public_share_info.bruapps/backend/bruno/drive/requests/20_public_share_files.bruapps/backend/src/entities/drive_files.rsapps/backend/src/entities/drive_folder_shares.rsapps/backend/src/entities/drive_folders.rsapps/backend/src/entities/mod.rsapps/backend/src/entities/scopes.rsapps/backend/src/entities/tenants.rsapps/backend/src/error.rsapps/backend/src/extractors.rsapps/backend/src/handlers/drive_files.rsapps/backend/src/handlers/drive_folders.rsapps/backend/src/handlers/mod.rsapps/backend/src/handlers/projects.rsapps/backend/src/handlers/tenants.rsapps/backend/src/lib.rsapps/backend/src/main.rsapps/backend/src/openapi/mod.rsapps/backend/src/openapi/responses.rsapps/backend/src/routes/drive.rsapps/backend/src/routes/mod.rsapps/backend/src/routes/tenants.rsapps/backend/src/server.rsapps/backend/src/utils/drive.rsapps/backend/src/utils/mod.rsapps/backend/src/utils/storage/local.rsapps/backend/src/utils/storage/mod.rsapps/backend/src/utils/storage/s3.rsapps/backend/src/utils/storage/trait.rsdocs/content/2.features/drive.md
Comment on lines
+18
to
+20
| test("status 200 or 403", function() { | ||
| expect([200, 403]).to.include(res.status); | ||
| }); |
There was a problem hiding this comment.
テスト条件が広すぎて回帰を見逃します。
200 or 403 を許容すると、正常系が壊れて 403 になってもテストが通ります。想定シナリオごとにリクエストを分け、ここは期待値を単一（例: 200）に固定してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/bruno/drive/requests/07_files_content_project.bru` around lines
18 - 20, The test "status 200 or 403" is too permissive and can mask
regressions; change this to assert a single expected status (e.g.,
expect(res.status).to.equal(200)) in the successful scenario and create a
separate test for the forbidden scenario that asserts
expect(res.status).to.equal(403). Locate the assertion in the test block named
"status 200 or 403" and update it to a single-value expectation (use res.status
in the existing test), and add a new test case that sets up the forbidden
condition and asserts 403.
Comment on lines
+97
to
+99
| async fn public_url(&self, key: &str) -> String { | ||
| format!("/v1/drive/files/{key}/content") | ||
| } |
There was a problem hiding this comment.
public_url のパス引数が API 契約と不整合です。
Line 97-99 は {key} を埋めていますが、Drive の content ルートは file {id} 前提です。storage_key をそのまま URL 化すると 404 やアクセス制御不整合の原因になります。
修正案（どちらかに統一）
- async fn public_url(&self, key: &str) -> String {
- format!("/v1/drive/files/{key}/content")
+ async fn public_url(&self, file_id: &str) -> String {
+ format!("/v1/drive/files/{file_id}/content")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/utils/storage/local.rs` around lines 97 - 99, public_url が
storage_key をそのまま埋め込しており Drive API
の契約（/v1/drive/files/{id}/content）と不整合を起こしているので、public_url(&self, key: &str) 内で
storage_key をファイル ID に変換するか、呼び出し側で file id を渡すようシグネチャを変更して一貫させてください；具体的には関数
public_url を修正して storage_key をファイル ID にマップするロジック（または既存の変換ヘルパーを利用）を入れてから
format!("/v1/drive/files/{id}/content") を返すようにし、もし変換が不可能なら適切にエラーを返す（または
Option/Result を使う）ようにしてください。
… Scalar upload_file was registered via plain .route() which bypasses utoipa's OpenApiRouter path collection. Move it into routes!(list_files, upload_file) and apply RequestBodyLimitLayer at the router level instead. Assisted-by: multi-agent-shogun-aki-tweak
- drive_folder_shares: fix XOR check to use is_some() instead of is_some_and(|t| !t.is_empty()), preventing empty-string token bypass - drive_files/upload: rollback storage on DB insert failure to prevent orphan files accumulating in storage - drive_files/delete: delete DB record first then storage (best-effort), so failure leaves a recoverable orphan rather than a dangling reference - drive_files: replace unreachable ok_or(Forbidden) with expect() - drive_folders: remove dead share_token.is_none() checks after load_active_share_by_token (always Some by query filter) - projects: wrap project + drive_folder insert in a single transaction to prevent partial state on drive_folder insert failure - Bruno tests: fix 07 to assert status 200, fix 08 test name to match allowed statuses [200, 403, 404] Assisted-by: multi-agent-shogun-aki-tweak
- upload_file: replace stream::unfold (requires 'static) with fmpsc::channel + tokio::join! — Field<'a> can now be pumped concurrently with storage upload - upload_file: rollback storage object on DB insert failure - delete_file: delete DB record first, then storage best-effort - is_tenant_owner: extract shared helper into utils::drive - drive_folder_shares: fix XOR validation to use is_some() not is_some_and - main: add startup warning scan for tenants whose quota exceeds DRIVE_SYSTEM_MAX_QUOTA_MB - drive_folders: remove dead share_token.is_none() checks, unused tenants import Assisted-by: multi-agent-shogun-aki-tweak
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/src/handlers/drive_files.rs (1)
251-264:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftクォータ判定が非原子的で、同時アップロード時に上限超過を許します。
tenant_used_bytesの参照とdrive_filesINSERT が同一トランザクション/ロックで保護されていないため、並行リクエストで全てが「空きあり」と判定され、合計でクォータを超える可能性があります。対策は、クォータ判定とメタデータ INSERT を同一トランザクションで実施し、テナント行（またはクォータ管理行）を
FOR UPDATE相当で直列化するのが安全です。As per coding guidelines, 「認証・認可、テナント境界、SQL の正しさ、エラーハンドリング、非同期処理の安全性を優先して確認してください。」.Also applies to: 428-431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/drive_files.rs` around lines 251 - 264, ensure_quota_for_upload is performing a non-atomic check by calling tenant_used_bytes separately from the drive_files INSERT, allowing concurrent uploads to bypass the quota; fix by moving the quota check and the metadata INSERT into a single database transaction and serialize against concurrent checks by locking the tenant/quota row (e.g., SELECT ... FOR UPDATE or equivalent) before computing used bytes, then validate used + new_size <= quota and perform the INSERT within that same transaction; update the code paths that call tenant_used_bytes/ensure_quota_for_upload (and the drive_files INSERT) so they use the shared transaction and handle transaction errors/rollbacks appropriately.apps/backend/src/utils/drive.rs (1)
67-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
effective_quotaで system max の上限制約が適用されていません。
tenant.drive_quota_bytesやdefault_quota_bytesがDRIVE_SYSTEM_MAX_QUOTA_MBを超えている場合でも、そのまま返してしまいます。PR 目的の「system max → tenant setting → default」の解決順と不整合で、実運用で上限超過を許してしまいます。💡 修正例
pub fn effective_quota(tenant: &tenants::Model, config: &DriveConfig) -> Option<i64> { - match tenant.drive_quota_bytes { - Some(q) => Some(q), - None => { - if config.default_quota_bytes == 0 { - None - } else { - Some(config.default_quota_bytes) - } - } - } + let requested = tenant + .drive_quota_bytes + .unwrap_or(config.default_quota_bytes); + + let requested_opt = if requested == 0 { None } else { Some(requested) }; + let system_max_opt = config.system_max_bytes_opt(); + + match (requested_opt, system_max_opt) { + (Some(q), Some(max)) => Some(q.min(max)), + (None, Some(max)) => Some(max), + (Some(q), None) => Some(q), + (None, None) => None, + } }As per coding guidelines, 「認証・認可、テナント境界、SQL の正しさ、エラーハンドリング、非同期処理の安全性を優先して確認してください。」.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/utils/drive.rs` around lines 67 - 77, The effective_quota function must enforce the system-wide maximum: when choosing tenant.drive_quota_bytes or config.default_quota_bytes, clamp the chosen value to DRIVE_SYSTEM_MAX_QUOTA_MB (converted to bytes) before returning; replace the current direct returns in effective_quota with logic that takes the min(chosen_value, DRIVE_SYSTEM_MAX_QUOTA_MB * 1024 * 1024) and still returns None when the resolved value is zero/unspecified. Ensure you reference effective_quota and the fields tenant.drive_quota_bytes and config.default_quota_bytes and apply the cap consistently for both branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/backend/src/handlers/drive_files.rs`:
- Around line 251-264: ensure_quota_for_upload is performing a non-atomic check
by calling tenant_used_bytes separately from the drive_files INSERT, allowing
concurrent uploads to bypass the quota; fix by moving the quota check and the
metadata INSERT into a single database transaction and serialize against
concurrent checks by locking the tenant/quota row (e.g., SELECT ... FOR UPDATE
or equivalent) before computing used bytes, then validate used + new_size <=
quota and perform the INSERT within that same transaction; update the code paths
that call tenant_used_bytes/ensure_quota_for_upload (and the drive_files INSERT)
so they use the shared transaction and handle transaction errors/rollbacks
appropriately.
In `@apps/backend/src/utils/drive.rs`:
- Around line 67-77: The effective_quota function must enforce the system-wide
maximum: when choosing tenant.drive_quota_bytes or config.default_quota_bytes,
clamp the chosen value to DRIVE_SYSTEM_MAX_QUOTA_MB (converted to bytes) before
returning; replace the current direct returns in effective_quota with logic that
takes the min(chosen_value, DRIVE_SYSTEM_MAX_QUOTA_MB * 1024 * 1024) and still
returns None when the resolved value is zero/unspecified. Ensure you reference
effective_quota and the fields tenant.drive_quota_bytes and
config.default_quota_bytes and apply the cap consistently for both branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c192f275-0831-4a9b-8c5f-abdd164499dd
📒 Files selected for processing (9)
apps/backend/bruno/drive/requests/07_files_content_project.bruapps/backend/bruno/drive/requests/08_files_content_share_token.bruapps/backend/src/entities/drive_folder_shares.rsapps/backend/src/handlers/drive_files.rsapps/backend/src/handlers/drive_folders.rsapps/backend/src/handlers/projects.rsapps/backend/src/main.rsapps/backend/src/routes/drive.rsapps/backend/src/utils/drive.rs
- effective_quota: apply system_max cap when set — tenant/default quota that exceeds DRIVE_SYSTEM_MAX_QUOTA_MB is now silently capped, preventing quota enforcement bypass when system max is lowered after tenant creation - upload_file: make quota check + drive_files INSERT atomic via transaction with SELECT ... FOR UPDATE on the tenant row; concurrent uploads can no longer race through the quota check and collectively exceed the limit - tenant_used_bytes: accept &impl ConnectionTrait so callers can pass either a DatabaseConnection or a DatabaseTransaction - remove ensure_quota_for_upload helper (inlined into the transaction block) Assisted-by: multi-agent-shogun-aki-tweak
Add tag = "..." to every #[utoipa::path] annotation across all handlers so Scalar groups endpoints into labeled sections: Auth, Tenants, Projects, Project Members, Labels, Personal Tokens, Drive Files, Drive Folders, Drive Shares Register tag definitions (name + Japanese description) in openapi/mod.rs to control display order and show descriptions in Scalar. Assisted-by: multi-agent-shogun-aki-tweak
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/backend/src/handlers/drive_files.rs (3)
375-401:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftアップロード上限を超えても最後までストレージへ書き込んでいます。
pumpはbyte_countを増やすだけで継続し、actual_size > max_bytesの判定はアップロード完了後です。これだと巨大ファイルで Local/S3 の容量・帯域・multipart コストを消費できます。max_bytes超過時点でストリームへ専用エラーを流してアップロード自体を中断し、そのエラーを 413 に変換する必要があります。As per coding guidelines,apps/backend/**/*.rsでは「エラーハンドリング、非同期処理の安全性」を優先して確認してください。Also applies to: 405-413
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/drive_files.rs` around lines 375 - 401, The pump currently keeps streaming bytes and only checks limits after upload finishes; modify the pump (inside the async block named pump) to check the cumulative counter/byte_count against max_bytes on each chunk and, when exceeded, immediately send an Err through tx (e.g., a distinct StorageError variant like StorageError::TooLarge or StorageError::Other with a clear "max_bytes exceeded" message) and then return to stop streaming; ensure the byte_stream consumer (state.storage.upload) observes that error and fails fast so upload_result maps via storage_to_app_error to a 413 response; update any match arms that currently send StorageError::Other(e.to_string()) to use the new variant when limit is hit so the caller can convert it to HTTP 413.
321-363:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftファイル系 CRUD が project/share ACL を見ていません。
これらのハンドラーは
ensure_tenant_accessまでで止まっているため、同一テナントのread:driveユーザーならプロジェクトメンバーでなくてもget_fileでメタデータを取得でき、write:driveがあればプロジェクト配下へのアップロードや既存ファイルの更新・削除までできてしまいます。get_file_contentと同じ粒度の判定を共通ヘルパー化して、project 配下のファイル操作にも tenant owner / project member / folder share の制約を適用してください。As per coding guidelines,apps/backend/**/*.rsでは「認証・認可、テナント境界」を優先して確認してください。Also applies to: 497-505, 524-552, 570-584
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/drive_files.rs` around lines 321 - 363, The handlers stop at ensure_tenant_access and therefore miss project/member/share ACL checks; add a shared authorization helper (e.g., authorize_file_operation or ensure_project_file_access) and call it from the upload/update/delete/get_file/get_file_content handlers (the handlers that call ensure_tenant_access, load_folder_in_tenant, tenant_used_bytes, effective_quota, etc.). The helper should take (state, tenant_id, optional folder_id, optional project_id, auth, required_scope) and enforce tenant-owner OR project-member OR folder-share rules (reuse the same logic as get_file_content’s access checks), returning Ok(()) or an AppError::Forbidden; replace inline assumptions in the upload path (where folder_id is used to derive folder.project_id) and in the other file handlers so all project-scoped file ops go through this single authorization check.
215-216:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
project_idがNULLのファイルが匿名で取得できます。ここで即
Ok(())しているため、プロジェクト未紐付けファイルは認証も共有トークンも不要で/v1/drive/files/{id}/contentから読めます。ファイル ID を知っているだけでテナント外から取得できるので、少なくとも有効な共有トークンまたは認証済みユーザーの権限確認の後にしか許可しないようにしてください。As per coding guidelines,apps/backend/**/*.rsでは「認証・認可、テナント境界」を優先して確認してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/drive_files.rs` around lines 215 - 216, 現在のfile.project_id.is_none()ブロックは即座に Ok(()) を返しており、プロジェクト未紐付けファイルが匿名で /v1/drive/files/{id}/content から取得可能になっています。修正してください: file.project_id が None の場合でも認可チェックを行い、まず有効な共有トークンを検証する（例: validate_share_token）か、認証済みユーザーが該当ファイルへアクセスする権限があることを確認（例: ensure_user_has_project_access / check_user_permissions）していなければアクセスを拒否するようにし、権限がある場合のみ処理を継続するように変更してください（参照箇所: file.project_id とエンドポイント /v1/drive/files/{id}/content、ハンドラ内の戻り箇所を置き換える）。apps/backend/src/utils/drive.rs (1)
23-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick win負のクォータ環境変数を起動時に弾いてください。
env_i64とmb_to_bytesは-1のような値をそのまま受け入れるため、effective_quotaが負のバイト数を返し、以後のアップロードが恒常的に 413 になったり/usageが不正なクォータ値を返します。0だけを無制限として扱い、それ以外の負値は設定エラーにしてください。Also applies to: 51-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/utils/drive.rs` around lines 23 - 31, Validate environment quota values in DriveConfig::from_env: after reading default_quota_mb and system_max_mb via env_i64, reject any negative value except 0 (treat 0 as "unlimited") and fail early (panic/return Err or process::exit with a clear error) so negative quotas cannot be converted by mb_to_bytes into negative byte counts; perform the validation before calling mb_to_bytes and ensure the same checks are applied to the other quota-reading block (lines ~51-63) that uses env_i64/mb_to_bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/backend/src/handlers/drive_files.rs`:
- Around line 375-401: The pump currently keeps streaming bytes and only checks
limits after upload finishes; modify the pump (inside the async block named
pump) to check the cumulative counter/byte_count against max_bytes on each chunk
and, when exceeded, immediately send an Err through tx (e.g., a distinct
StorageError variant like StorageError::TooLarge or StorageError::Other with a
clear "max_bytes exceeded" message) and then return to stop streaming; ensure
the byte_stream consumer (state.storage.upload) observes that error and fails
fast so upload_result maps via storage_to_app_error to a 413 response; update
any match arms that currently send StorageError::Other(e.to_string()) to use the
new variant when limit is hit so the caller can convert it to HTTP 413.
- Around line 321-363: The handlers stop at ensure_tenant_access and therefore
miss project/member/share ACL checks; add a shared authorization helper (e.g.,
authorize_file_operation or ensure_project_file_access) and call it from the
upload/update/delete/get_file/get_file_content handlers (the handlers that call
ensure_tenant_access, load_folder_in_tenant, tenant_used_bytes, effective_quota,
etc.). The helper should take (state, tenant_id, optional folder_id, optional
project_id, auth, required_scope) and enforce tenant-owner OR project-member OR
folder-share rules (reuse the same logic as get_file_content’s access checks),
returning Ok(()) or an AppError::Forbidden; replace inline assumptions in the
upload path (where folder_id is used to derive folder.project_id) and in the
other file handlers so all project-scoped file ops go through this single
authorization check.
- Around line 215-216: 現在のfile.project_id.is_none()ブロックは即座に Ok(())
を返しており、プロジェクト未紐付けファイルが匿名で /v1/drive/files/{id}/content から取得可能になっています。修正してください:
file.project_id が None の場合でも認可チェックを行い、まず有効な共有トークンを検証する（例:
validate_share_token）か、認証済みユーザーが該当ファイルへアクセスする権限があることを確認（例:
ensure_user_has_project_access /
check_user_permissions）していなければアクセスを拒否するようにし、権限がある場合のみ処理を継続するように変更してください（参照箇所:
file.project_id とエンドポイント /v1/drive/files/{id}/content、ハンドラ内の戻り箇所を置き換える）。
In `@apps/backend/src/utils/drive.rs`:
- Around line 23-31: Validate environment quota values in DriveConfig::from_env:
after reading default_quota_mb and system_max_mb via env_i64, reject any
negative value except 0 (treat 0 as "unlimited") and fail early (panic/return
Err or process::exit with a clear error) so negative quotas cannot be converted
by mb_to_bytes into negative byte counts; perform the validation before calling
mb_to_bytes and ensure the same checks are applied to the other quota-reading
block (lines ~51-63) that uses env_i64/mb_to_bytes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9811f611-97ee-42ff-849d-b59636111a32
📒 Files selected for processing (10)
apps/backend/src/handlers/auth.rsapps/backend/src/handlers/drive_files.rsapps/backend/src/handlers/drive_folders.rsapps/backend/src/handlers/labels.rsapps/backend/src/handlers/personal_tokens.rsapps/backend/src/handlers/project_members.rsapps/backend/src/handlers/projects.rsapps/backend/src/handlers/tenants.rsapps/backend/src/openapi/mod.rsapps/backend/src/utils/drive.rs
- local.rs: public_url に storage_key == file_id の関係を明記するコメント追加 - drive.md: MD031（コードブロック前の空行）・MD040（言語タグなしフェンス）を修正
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/content/2.features/drive.md`:
- Around line 732-734: 見出し直下で始まっているコードフェンス containing the text "feat/drive"
violates MD031; add a blank line immediately before the opening triple-backtick
fence and a blank line immediately after the closing triple-backtick so the
fenced block is separated from the heading (ensure the fenced content
"feat/drive" remains unchanged) to satisfy the docs/**/*.md lint rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7e4567a-6bc6-4fb7-8731-e16520fb7e32
📒 Files selected for processing (2)
apps/backend/src/utils/storage/local.rsdocs/content/2.features/drive.md
Comment on lines
+732
to
734
| ```text | ||
| feat/drive | ||
| ``` |
There was a problem hiding this comment.
Line 732 のコードフェンス前後に空行を追加してください。
見出し直下でフェンスが始まっており、MD031（blanks-around-fences）に抵触しています。
修正案
### ブランチ名
+
```text
feat/drive</details>
As per coding guidelines `docs/**/*.md`: 「ドキュメントは構造と読みやすさを重視し、見出し階層、リンク切れ、手順の抜けを確認してください。」
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 732-732: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/content/2.features/drive.md around lines 732 - 734, 見出し直下で始まっているコードフェンス
containing the text "feat/drive" violates MD031; add a blank line immediately
before the opening triple-backtick fence and a blank line immediately after the
closing triple-backtick so the fenced block is separated from the heading
(ensure the fenced content "feat/drive" remains unchanged) to satisfy the
docs/**/*.md lint rule.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
テナント単位のドライブ機能（Phase A: バックエンド）を実装。S3 / ローカルストレージのストリーミングアップロード、フォルダ階層、アクセス制御、クォータ管理、フォルダ共有を追加。
変更内容
read:drive/write:driveを追加（write:drive ⊃ read:drive, admin:tenant ⊃ all）drive_folders/drive_files(url カラムなし) /drive_folder_sharesを追加、tenants.drive_quota_bytes追加GET/POST /v1/tenants/{tenant_id}/drive/files、GET/PATCH/DELETE .../files/{id}、ストリーミング配信GET /v1/drive/files/{id}/contentGET/POST/PATCH/DELETE /v1/tenants/{tenant_id}/drive/folders（ファイルありの場合は 409）GET /v1/tenants/{tenant_id}/drive/usage、PATCH .../drive/quota（3層構造: システム上限 → テナント設定 → デフォルト）GET/POST/DELETE /v1/tenants/{tenant_id}/drive/folders/{id}/shares、公開リンクGET /v1/drive/share/{token}docs/content/2.features/drive.md環境変数（新規）
テスト計画
STORAGE_BACKEND=localでファイルアップロード・ダウンロード動作確認Phase B (フロントエンド) について
フロントエンド実装（DrivePage / DriveFileGrid / DriveUploadButton 等）は別 PR で行う。
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
ドキュメント
Chores