Repository navigation
Conversation
Reviewer's Guideセキュリティ修正で導入された認証・認可挙動に合わせ、4モジュールのユニットテストを更新した。加えて、権限のないレコードを同期出力から安全に除外する未対応の呼び出し元を修正し、各権限境界と成功経路を検証している。 Sequence diagram for permission-filtered resource synchronizationsequenceDiagram
participant Client
participant Handler as ResourceListHandler_or_ChangeListHandler
participant Exporter as _export_item
Client->>Handler: get_record_content_file(record_id)
Handler->>Exporter: _export_item(...)
alt exported_item is empty
Exporter-->>Handler: {}
Handler-->>Client: None
else record is exportable
Exporter-->>Handler: exported_item
Handler-->>Client: synchronization content
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 configuration
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 |
🔍 Claude レビュー統合指摘はありません。 次にすること: 既存のレビュー指摘は、要約と「レビュー省略」の通知だけで、裁定する指摘スレッドはありませんでした。api.py の修正は妥当です。 モデル sonnet ／ 2 回実行して和集合 ／ コスト $0.2210。同じ入力でも結果が揺れるため複数回まわし、一部のパスでしか挙がらなかったものには回数を添えています 他レビューを踏まえた自動レビューです。誤りが含まれることがあります。 |
![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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/invenio-resourcesyncserver/tests/test_api.py" line_range="377" />
<code_context>
+ # 閲覧権限のないレコードは _export_item が {} を返す
+ with patch("invenio_resourcesyncserver.api._export_item", return_value=({}, {})):
+ assert test.get_record_content_file(1) is None
+
</code_context>
<issue_to_address>
**Empty-export regression goes undetected**
When the empty-export guard is removed, removing the new empty-export guard still lets this assertion pass: `ItemTypes.get_by_id(None)` is followed by an attribute access that raises, and the handler catches that exception and returns `None`. The test therefore does not catch a regression to the exception-and-error-log path it is intended to prevent.
Also assert that the downstream item-type lookup is not reached for an empty export.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: modules/invenio-resourcesyncserver/tests/test_api.py:377
|
|
||
| # 閲覧権限のないレコードは _export_item が {} を返す | ||
| with patch("invenio_resourcesyncserver.api._export_item", return_value=({}, {})): | ||
| assert test.get_record_content_file(1) is None |
There was a problem hiding this comment.
🟡 Medium · Empty-export regression goes undetected
When the empty-export guard is removed, removing the new empty-export guard still lets this assertion pass: ItemTypes.get_by_id(None) is followed by an attribute access that raises, and the handler catches that exception and returns None. The test therefore does not catch a regression to the exception-and-error-log path it is intended to prevent.
Also assert that the downstream item-type lookup is not reached for an empty export.
Prompt for AI agents
In `modules/invenio-resourcesyncserver/tests/test_api.py` at line 377:
**Empty-export regression goes undetected**
When the empty-export guard is removed, removing the new empty-export guard still lets this assertion pass: `ItemTypes.get_by_id(None)` is followed by an attribute access that raises, and the handler catches that exception and returns `None`. The test therefore does not catch a regression to the exception-and-error-log path it is intended to prevent.
Also assert that the downstream item-type lookup is not reached for an empty export.
91a34ca で weko-items-ui の _export_item は閲覧権限のないレコードに {} を返すようになったが、呼び出し元のうち対応したのは export_items だけで、ResourceListHandler / ChangeListHandler は取り残されていた。 {} のまま ItemTypes.get_by_id(None) に進んで AttributeError になり、 except で None が返るので結果は同じだが、例外経由でエラーログと トレースバックが毎回出ていた。空なら明示的に None を返す。 テストは、es_records のレコードが非公開(publish_status "1")のため 匿名では書き出されないことを確認し、書き出しの成功経路は check_publish_status を patch して公開扱いで確認する。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- weko-groups (11b0527): 管理者ロール必須化・grouplist のログイン必須化・ 招待を OPEN のグループに限定した変更に期待値を合わせる。users fixture の 一般ユーザ枠が sysadmin になっていたのを直す。1要求ごとにセッションが 片付き fixture のユーザが detach されるため、403 の確認は要求1回で終える - weko-search-ui (121eb34): check_index_permission は kwargs しか見ない ため、journal_detail をルーティングと同じく index_id= で呼ぶ - weko-workflow (121eb34): get_feedback_maillist に check_authority が 付いたため、申請者でない contributor では本体に届かない。sysadmin で確認する Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
API インベントリ差分（件数のみ）
ベースラインとの差分API インベントリ差分レポート
判定: ❌ FAIL (FAIL 1 / WARN 3)サマリ
[FAIL] G2 認証系デコレータが削除された — 3件
[WARN] W2 実装本体が変化(data_op / 情報露出を再確認) — 9件
[WARN] W6 依存パッケージの版が変化した — 2件
[WARN] W5 監視対象 config が変化した — 2件
台帳との突き合わせ(生成されませんでした) ソース由来の経路検知(生成されませんでした) |
概要
9月29日頃から Unit Tests で継続して落ちていた4モジュールを直す。いずれもセキュリティ修正で挙動が変わったのにテスト（と一部の呼び出し元）が追従していなかったもの。修正自体は戻さず、新しい挙動に合わせている。
11b05274c（管理者ロール必須化、grouplistのログイン必須化、招待を OPEN のみに）test_new_member_4に分離）91a34caa3（_export_itemが権限なしレコードに{}を返す）{}なら None を返す（呼び出し元3箇所のうちこの2箇所が未対応で、例外経由で None になりエラーログが出ていた）。テストは非公開レコードが書き出されないことと、公開扱いでの成功経路を確認121eb345e（@check_index_permission追加）journal_detail(index_id=33)で呼ぶ121eb345e（get_feedback_maillistに@check_authority追加）動作確認
scripts/ci/run-local.shと同じ compose 構成・イメージで実行（作業ツリーの所有者の都合で、同じ差分を当てた別 worktree で実行）。search-ui / workflow は全シャードを回していないので、最終確認は CI で行う。flake8 は変更前後で新規の指摘なし。
このPRで扱わないもの
@roles_required→@check_deposit_role()（229ad779a、内部でroles_requiredを呼ぶ）の意図的な置き換え。docs/RULE.mdに従い、private 側の台帳とベースライン更新で対応する。🤖 Generated with Claude Code
Summary by Sourcery
Bring authorization-sensitive code paths and unit tests into alignment with the latest permission checks and record visibility rules.
Bug Fixes:
Enhancements:
Tests: