Hotfix/issue62807 - #1914
Hotfix/issue62807#1914
Conversation
v2.0.4 (2f6b61b) で soft_delete ビューの所有者検証を record_edit_permission_required に切り出したが、このデコレータは recid を kwargs / request.form / JSON body / query string からしか探していなかった。 画面の削除ボタンは POST /items/prepare_delete_item に {"pid_value": ...} を 投げ、weko_items_ui.views.prepare_delete_item がビュー関数を soft_delete(del_value) と *位置引数* で直接呼ぶ (weko_workflow.utils.prepare_delete_workflow も同じ)。 このとき kwargs は空、ボディのキーも recid ではなく pid_value なので、 recid が見つからず abort(400) していた。判定は権限チェックより前なので、 作成者でも管理者でも一律にアイテムを削除できない。 views.py:1362 の呼び出しは try の外にあり、JSON ではなく素の 400 が返る。 inspect.signature().bind_partial() でシグネチャに束ね、位置引数からも recid を解決するようにした。in-process 呼び出しがあるのは soft_delete だけで、 restore / copy_bucket / get_file_place / replace_file には無い。 テストは本番の呼び出し形をそのまま再現する結合テスト (test_views.py) と、 recid の解決元 6 パターン + 401/400/403 を見る単体テスト (test_permissions.py) を追加した。修正前は位置引数の 2 ケースと 403 ケースが 400 に化けて落ちる。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJUSCriemeQBj1WU7ttXV1
台帳は extract_routes.py が拾う Flask のルートを単位にしている。だが WEKO3 には ビュー関数を HTTP を通さず別モジュールから直接呼ぶ「第二の入口」があり、 これは台帳のどの列にも現れない。ルート単位で認可を足していく作業では素通りする。 issue62807 がその実例。soft_delete の行は auth_required=要 / test_gap=- で 穴が無いように見えていたが、実際には prepare_delete_item が soft_delete(del_value) と位置引数で直接呼んでおり、v2.0.4 で足した record_edit_permission_required が recid を見つけられず abort(400) していた。 - audit_inprocess_views.py: ルート登録されたビューのうち、本体コードから 名前で import されているものを AST で列挙する。認可デコレータ付きを 位置引数で呼んでいるものは risk=HIGH。 - add_inproc_callers.py: 台帳に inproc_callers 列を付与する (62→63列)。 --check は書き込まず、台帳がまだ知らない呼び出し元だけを報告する。 - api-inventory-drift.yml: --check --summary-only --gate を CI に入れた。 ソースと台帳だけで済むのでコンテナ起動の前に流す。 - README: ケース1 の手順、認可を足す前の確認、スクリプトの入出力表を更新。 現状の検出結果は HIGH 5 件。うち 2 件が issue62807 (soft_delete)、 残り 3 件は HeadlessActivity が prepare_edit_item / prepare_delete_item / check_validation_error_msg を位置引数で呼ぶもの (v2.0.4 以前から存在)。 後者のデコレータはリクエストから値を読まないため同じ失敗はしないが、 prepare_delete_item 経由で issue62807 の影響は受ける。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJUSCriemeQBj1WU7ttXV1
Reviewer's GuideこのPRは、HTTP以外から位置引数で直接呼ばれる soft_delete でも認可デコレータがレコードIDを正しく解決できるようにして、画面削除の回帰を修正します。結合・権限テストを追加するとともに、ASTベースのin-process呼び出し元棚卸しをAPI台帳とCI gateに組み込み、同種の認可漏れ・回帰を検出できるようにしています。 Sequence diagram for in-process item deletion authorizationsequenceDiagram
participant User
participant PrepareDelete as prepare_delete_item
participant SoftDelete as soft_delete
participant Permission as record_edit_permission_required
participant Checker as check_created_id_by_recid
User->>PrepareDelete: POST /items/prepare_delete_item
PrepareDelete->>SoftDelete: soft_delete(del_value)
SoftDelete->>Permission: Resolve recid from bound positional argument
Permission->>Checker: check_created_id_by_recid(recid)
alt authorized
Checker-->>Permission: allowed
Permission-->>SoftDelete: invoke view with original del_value
SoftDelete-->>PrepareDelete: logical deletion result
else unauthorized
Checker-->>Permission: denied
Permission-->>SoftDelete: 403
end
Flow diagram for record ID resolution in the permission decoratorflowchart TD
A[record_edit_permission_required] --> B{Authenticated?}
B -- No --> C[401]
B -- Yes --> D{recid in kwargs?}
D -- Yes --> H[Normalize recid]
D -- No --> E{recid in bound positional arguments?}
E -- Yes --> H
E -- No --> F{recid in form, JSON, or query string?}
F -- No --> G[400]
F -- Yes --> H
H --> I[check_created_id_by_recid]
I -- Allowed --> J[Call protected view]
I -- Denied --> K[403]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
API インベントリ差分（件数のみ）
ベースラインとの差分(生成されませんでした) 台帳との突き合わせ(生成されませんでした) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tools/api-inventory/scripts/audit_inprocess_views.py" line_range="205" />
<code_context>
+ except Exception:
+ return hits
+ for node in ast.walk(tree):
+ if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) \
+ and node.func.id in names and node.args:
+ hits.setdefault(node.func.id, []).append(node.lineno)
</code_context>
<issue_to_address>
**issue (broader_impact):** The AST audit only recognizes calls whose callee is a bare `ast.Name`, so calls through an imported alias (`from ... import soft_delete as delete; delete(...)`) or a module attribute (`views.soft_delete(...)`) are omitted. A future in-process caller using either form therefore passes the CI gate unnoticed, allowing an authorization decorator regression like issue62807 to return.
**Triggers:** When an in-process view caller uses an import alias or module-qualified call.
**Suggested fix:** Track import aliases and `ast.Attribute` callees when resolving imported view references.
</issue_to_address>
### Comment 2
<location path="tools/api-inventory/scripts/add_inproc_callers.py" line_range="113-115" />
<code_context>
+ if del_value.startswith("del_ver_"):
+ mock_ver.assert_called_once_with(expected_recid)
+ mock_imp.assert_not_called()
+ else:
+ mock_imp.assert_called_once_with(expected_recid)
+ mock_ver.assert_not_called()
</code_context>
<issue_to_address>
**nitpick:** An imported view is recorded as a `kwargs` in-process caller whenever no direct positional call is found, even if the imported name is never called at all. The generated `inproc_callers` inventory consequently contains false callers and can mislead authorization reviews about which non-HTTP entry points actually exist.
**Triggers:** When production code imports a routed view for reference, re-export, or another non-call use.
**Suggested fix:** Record a caller only after finding an actual call/reference invocation, and distinguish keyword calls from imports that are not invoked.
</issue_to_address>
### Comment 3
<location path=".github/workflows/api-inventory-drift.yml" line_range="118-119" />
<code_context>
+ env:
+ WEKO_API_INVENTORY_DIR: ${{ github.workspace }}/.api-inventory-data
+ run: |
+ python3 tools/api-inventory/scripts/add_inproc_callers.py \
+ --check --summary-only --gate
+
- name: Start WEKO containers
</code_context>
<issue_to_address>
**issue (broader_impact):** The newly mandatory gate treats a missing `inproc_callers` column as an empty inventory, so the existing private ledger causes every detected in-process caller to appear as drift and exits with status 1. Until the separately coordinated private-ledger schema update and matching branch exist, the workflow blocks the PR rather than merely reporting the inventory mismatch.
**Triggers:** When the private inventory branch still contains the pre-63-column TSV, including the documented fallback to the default branch.
**Suggested fix:** Add and validate the `inproc_callers` column before enabling the gate, or explicitly detect the old schema and fail with a targeted migration message rather than treating every caller as an ordinary drift.
```suggestion
if ! awk -F '\t' 'NR == 1 { for (i = 1; i <= NF; i++) if ($i == "inproc_callers") found = 1 } END { exit !found }' \
"$WEKO_API_INVENTORY_DIR/weko3_api_list_full.tsv"; then
echo "::error::台帳に inproc_callers 列がありません。プライベート台帳のスキーマを更新してからこのゲートを有効化してください。"
exit 1
fi
python3 tools/api-inventory/scripts/add_inproc_callers.py \
--check --summary-only --gate
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if positional argument binding resolves the wrong record ID or fails to enforce the intended permission check, a caller could delete a record they should not be able to access. Reverting would stop the new behavior, but any deletions that occurred would remain and require separate recovery.
Blocking findings: tools/api-inventory/scripts/audit_inprocess_views.py:205, .github/workflows/api-inventory-drift.yml:119
| except Exception: | ||
| return hits | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) \ |
There was a problem hiding this comment.
issue (broader_impact): The AST audit only recognizes calls whose callee is a bare ast.Name, so calls through an imported alias (from ... import soft_delete as delete; delete(...)) or a module attribute (views.soft_delete(...)) are omitted. A future in-process caller using either form therefore passes the CI gate unnoticed, allowing an authorization decorator regression like issue62807 to return.
Triggers: When an in-process view caller uses an import alias or module-qualified call.
Suggested fix: Track import aliases and ast.Attribute callees when resolving imported view references.
| else: | ||
| idx[(v["file"], v["func"])].append( | ||
| "kwargs:{}:{}".format(importer, line)) |
There was a problem hiding this comment.
nitpick: An imported view is recorded as a kwargs in-process caller whenever no direct positional call is found, even if the imported name is never called at all. The generated inproc_callers inventory consequently contains false callers and can mislead authorization reviews about which non-HTTP entry points actually exist.
Triggers: When production code imports a routed view for reference, re-export, or another non-call use.
Suggested fix: Record a caller only after finding an actual call/reference invocation, and distinguish keyword calls from imports that are not invoked.
| python3 tools/api-inventory/scripts/add_inproc_callers.py \ | ||
| --check --summary-only --gate |
There was a problem hiding this comment.
issue (broader_impact): The newly mandatory gate treats a missing inproc_callers column as an empty inventory, so the existing private ledger causes every detected in-process caller to appear as drift and exits with status 1. Until the separately coordinated private-ledger schema update and matching branch exist, the workflow blocks the PR rather than merely reporting the inventory mismatch.
Triggers: When the private inventory branch still contains the pre-63-column TSV, including the documented fallback to the default branch.
Suggested fix: Add and validate the inproc_callers column before enabling the gate, or explicitly detect the old schema and fail with a targeted migration message rather than treating every caller as an ordinary drift.
| python3 tools/api-inventory/scripts/add_inproc_callers.py \ | |
| --check --summary-only --gate | |
| if ! awk -F '\t' 'NR == 1 { for (i = 1; i <= NF; i++) if ($i == "inproc_callers") found = 1 } END { exit !found }' \ | |
| "$WEKO_API_INVENTORY_DIR/weko3_api_list_full.tsv"; then | |
| echo "::error::台帳に inproc_callers 列がありません。プライベート台帳のスキーマを更新してからこのゲートを有効化してください。" | |
| exit 1 | |
| fi | |
| python3 tools/api-inventory/scripts/add_inproc_callers.py \ | |
| --check --summary-only --gate |
1 participant
概要 (Summary)
2f6b61b2f) でsoft_deleteビューの所有者検証をrecord_edit_permission_requiredに切り出したが、このデコレータは recid を kwargs / request.form / JSON body / query string からしか探していなかった。画面の削除ボタンはPOST /items/prepare_delete_itemに{"pid_value": ...}を投げ、weko_items_ui.views.prepare_delete_itemがsoft_delete(del_value)と 位置引数でビュー関数を直接呼ぶ（weko_workflow.utils.prepare_delete_workflowも同じ）。kwargs は空、ボディのキーもrecidではないので id が取れずabort(400)。判定が権限チェックより前にあるため、作成者でも管理者でも一律に削除できない（views.py:1362の呼び出しは try の外なので JSON ではなく素の 400 が返る）。inspect.signature().bind_partial()でシグネチャに束ね、位置引数からも recid を解決する。in-process 呼び出しがあるのはsoft_deleteだけで、restore/copy_bucket/get_file_place/replace_fileには無いことを確認済み。weko3_api_list_full.tsv) はextract_routes.pyが拾う Flask のルートを単位にしているため、HTTP を通さずビュー関数を直接呼ぶ「第二の入口」がどの列にも現れない。issue62807 の行はauth_required=要/test_gap=-で穴が無いように見えていた。audit_inprocess_views.py（AST で列挙・書き込みなし）とadd_inproc_callers.py（台帳にinproc_callers列を付与、62→63列）を追加し、--check --summary-only --gateを drift ワークフローのコンテナ起動前に挿入した（ソースと台帳だけで済むため）。関連Issue / チケット (Related Issues)
変更タイプ (Type of Change)
🤖 0. CI 自動チェック (API Inventory Drift)
PR 作成後に確認する。台帳ブランチの警告（冒頭）を件数より先に見ること。
下記のとおりプライベート側に同名ブランチがまだ無いため、現状は既定ブランチと
比較され警告が出る見込み。
API を追加・変更した場合（必須）
ルーティング（
url_map）は変えていない。 追加・削除・メソッド変更のあるエンドポイントは無く、api_snapshot.json/weko3_api_list_full.tsvの行そのものは変わらない。ただし 本 PR で追加した CI ステップが台帳の
inproc_callers列を読むため、プライベートリポジトリ側の台帳に列を足す PR が必要。
hotfix/issue62807) で切ったdata/v2.0.4-git-columnsで作業しており、同名ブランチが無い。先に切らないと、この PR の CI は既定ブランチの台帳
（
inproc_callers列なし）と比較され、新しい gate が正しく評価されない。api_snapshot.jsonを更新し、対応する PR を出したルート変更が無いためスナップショットの更新自体は不要。上記の同名ブランチ作成と
inproc_callers列追加の PR を、この PR とセットで出す。weko3_api_list_full.tsvに行を追加・更新し、build_checklist.pyで 24 列版を再生成した行の追加は不要。
add_inproc_callers.pyで **inproc_callers列（63列目）**を付与したうえで再生成する。
bash export WEKO_API_INVENTORY_DIR=/path/to/weko-secret python3 tools/api-inventory/scripts/add_inproc_callers.py python3 tools/api-inventory/scripts/build_checklist.py（
git statusに*.tsv/api_snapshot.jsonは出ていない。本 PR の差分はスクリプトとワークフローと実装・テストのみ）
🔒 1. セキュリティ & API アクセス制御チェック (必須)
認証・認可 (Authentication & Authorization)
デコレータの追加ではなく、既存
record_edit_permission_requiredが恒久的に 400 で落ちていて権限判定に到達していなかったのを直すもの。
判定本体は従来どおり
check_created_id（作成者 / 共有ユーザ /コミュニティ管理者 / スーパーユーザ）。
POST /records/soft_delete/<recid>と、そこへの in-process 呼び出しの両方で権限判定が実際に効くようになった（修正前は誰でも 400 = 誰も実行できない状態）。
未認証 401 / 権限なし 403 / id が取れない 400 をテストで固定した。
Noneで無効化していない該当なし（
*_PERMISSION_FACTORYは触っていない）。レビュー時の注意: デコレータは呼び出し元のリクエストコンテキストで動く。
request.form/ JSON body を読むデコレータを in-process 呼び出しのあるビューに付けると今回と同じ壊れ方をする。付けるなら位置引数からも値を解決できることを確かめること
（
tools/api-inventory/scripts/README.mdに追記済み）。機能クローズ・非公開化の場合 (Feature Disable)
🧪 2. テストコード観点チェック (pytest / Invenio Test Suite)
権限・異常系テスト (Negative & Authorization Tests)
test_record_edit_permission_required_abortsで 401 を検証check_created_id_by_recidが False のとき 403 を検証（修正前はここまで到達せず 400 に化けていた）
追加したテスト:
tests/test_views.py::test_soft_delete_called_in_processsoft_delete("1")/soft_delete("del_ver_1")を位置引数、ボディは{"pid_value": ...}）をそのまま再現する結合テスト。修正前は 2 ケースとも 400 で落ちるtests/test_permissions.py::test_record_edit_permission_required_id_sourcesdel_ver_prefix / form / JSON / query string）。check_created_id_by_recidには prefix を剥がした id、ビュー本体には受け取ったままの値を渡すことも検証tests/test_permissions.py::test_record_edit_permission_required_aborts境界値・入力バリデーションテスト (Boundary & Validation)
test_record_edit_permission_required_aborts）。巨大ファイル・MIME・スキーマ検証は本 PR の変更範囲外。
データ整合性・トランザクションテスト (Integrity & Rollback)
🛡️ 3. データ保護 & 破壊的変更防止チェック (Data Safety)
soft_delete/delete_version)。削除処理そのものは変更していない。変わったのは「どの id に対する削除かを認可デコレータが解決できるか」だけで、
ビュー本体に渡る値（
del_ver_付きのまま）は従来と同一。del_ver_prefix を剥がした id が権限判定に渡ることをテストで固定した。
db.sessionの扱いに手を入れていない）⚙️ 4. マイグレーション & システム影響チェック (Invenio / WEKO3 Stack)
該当なし。 DB スキーマ / Alembic、ES・OpenSearch のマッピング、
invenio.cfg・環境変数、Celery タスクのシグネチャ、キャッシュのいずれも変更していない。
データベース (DB / Alembic)
検索インデックス (Elasticsearch / OpenSearch)
設定 & 非同期処理 (Config / Celery / Cache)
📚 5. ドキュメント・仕様書更新チェック (weko-document)
tools/api-inventory/scripts/README.md）に「★認可を足す前に in-process の呼び出し元を見る (issue62807)」節と、
audit_inprocess_views.py/add_inproc_callers.pyの入出力表を追記した。台帳・調査記録はプライベートリポジトリ側（§0 のとおり
inproc_callers列の追加が残っている）。v2.0.4 で意図せず壊れた「作成者・管理者はアイテムを削除できる」という
既存仕様どおりの挙動に戻すだけで、仕様書・マニュアルの記述とのズレは生じない。
📋 6. 動作検証エビデンス (Verification Evidence)
テスト実行結果
修正前のコードでは、位置引数の 2 ケースと 403 ケースが 400 に化けて落ちることが
このテストの効いている証拠になるので、可能なら revert した状態でも一度流す。
CI の成果物 (artifact:
api-inventory-summary)drift.mdreconcile.md本 PR で追加した in-process チェックはコンテナ起動前に走るので、
落ちた場合は上記 artifact ではなく Actions のログ（
Check in-process view callers）を見る。明細（該当したビュー名・呼び出し元）は公開できないため出力しない。
プライベートリポジトリ側で
--summary-onlyなしで再実行して確認する。手動で確認したこと
WEKO_ROOT=/home/mhaya/wekov2 python3 tools/api-inventory/scripts/audit_inprocess_views.py --summary-only # -> in-process から呼ばれるビュー: 5 件 (HIGH=5 MEDIUM=0 LOW=0)soft_delete2 件と、HeadlessActivityがprepare_edit_item/prepare_delete_item/check_validation_error_msgを位置引数で呼ぶ 3 件（v2.0.4 以前から存在）。
後者のデコレータはリクエストから値を読まないため同じ失敗はしないが、
prepare_delete_item経由で issue62807 の影響は受ける。soft_deleteビューを in-process で呼ぶのはweko_items_ui/views.py:1381とweko_workflow/utils.py:1981の 2 箇所。weko_workflow/views.py:1915のsoft_deleteはweko_records_ui.utils.soft_delete（実装側）で、ビュー関数ではないため影響なし。restore/copy_bucket/get_file_place/replace_fileに in-process 呼び出しが無いことを確認。(
prepare_delete_workflow) の確認。Summary by Sourcery
Fix item deletion by supporting positional record IDs in authorization checks and add CI safeguards for in-process view entry points.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: