fix widget permission issue (No. 290, 303, 304) - #1930
Conversation
Reviewer's GuideThe PR fixes widget permission gaps by applying administrative role-based authorization to widget type and uploaded-file endpoints, and adds parametrized tests verifying both permitted and forbidden access paths. Sequence diagram for widget endpoint role authorizationsequenceDiagram
actor User
participant WidgetAPI
participant Auth
participant WidgetService
User->>WidgetAPI: GET /load_widget_type or /widget/uploaded/{filename}/{community_id}
WidgetAPI->>Auth: roles_required([system, repo, community])
alt authorized role
Auth-->>WidgetAPI: access granted
WidgetAPI->>WidgetService: get_widget_type_list() or serve uploaded file
WidgetService-->>User: widget data or file
else missing required role
Auth-->>User: forbidden response
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 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 |
![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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/weko-gridlayout/weko_gridlayout/views.py" line_range="302-304" />
<code_context>
@blueprint_api.route('/load_widget_type', methods=['GET'])
@login_required
+@roles_required([WEKO_ADMIN_PERMISSION_ROLE_SYSTEM,
+ WEKO_ADMIN_PERMISSION_ROLE_REPO,
+ WEKO_ADMIN_PERMISSION_ROLE_COMMUNITY])
def load_widget_type():
"""Get Widget Type List."""
</code_context>
<issue_to_address>
**🚨 issue (security):** `roles_required` immediately calls the view for every method in Flask-Login's `EXEMPT_METHODS`, which includes GET, so these newly protected GET endpoints do not check authentication or roles. `load_widget_type` remains reachable by any authenticated user, and `uploaded_file` is reachable anonymously because it has no separate `login_required` decorator.
**Triggers:** When either widget endpoint is accessed with GET.
**Suggested fix:** Use an authorization decorator that does not exempt GET requests, or explicitly enforce authentication and the allowed roles inside these views.
</issue_to_address>
### Comment 2
<location path="modules/weko-gridlayout/tests/test_views.py" line_range="475" />
<code_context>
+]
+@pytest.mark.parametrize('id, status_code', user_results2)
+def test_load_widget_type(client, users, id, status_code):
+ login_user_via_session(client=client, email=users[id]['obj'].email)
res = client.get(
url_for("weko_gridlayout_api.load_widget_type"),
</code_context>
<issue_to_address>
**issue (testing):** The `test_load_widget_type` test logs in with `users[id]['obj'].email`, but the fixture's entry at index 4 has `email` set to `generaluser.email` while `obj` is incorrectly set to `sysadmin`; the case expecting a 403 therefore logs in as a system administrator and does not test the general user's permission.
**Triggers:** When the parameterized case uses `id == 4`.
**Suggested fix:** Log in with `users[id]['email']`, as the uploaded-file test does, or correct the fixture's `obj` value before using it.
```suggestion
login_user_via_session(client=client, email=users[id]['email'])
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and the new role decorators change the authorization boundary for widget-type and uploaded-file endpoints. If the allowlist is wrong, legitimate users can be blocked or unauthorized users can retrieve files immediately; reverting restores the old check but does not undo any access or exposure that occurred while the policy was deployed.
Blocking findings: modules/weko-gridlayout/weko_gridlayout/views.py:304, modules/weko-gridlayout/tests/test_views.py:475
| @roles_required([WEKO_ADMIN_PERMISSION_ROLE_SYSTEM, | ||
| WEKO_ADMIN_PERMISSION_ROLE_REPO, | ||
| WEKO_ADMIN_PERMISSION_ROLE_COMMUNITY]) |
There was a problem hiding this comment.
🚨 issue (security): roles_required immediately calls the view for every method in Flask-Login's EXEMPT_METHODS, which includes GET, so these newly protected GET endpoints do not check authentication or roles. load_widget_type remains reachable by any authenticated user, and uploaded_file is reachable anonymously because it has no separate login_required decorator.
Triggers: When either widget endpoint is accessed with GET.
Suggested fix: Use an authorization decorator that does not exempt GET requests, or explicitly enforce authentication and the allowed roles inside these views.
| ] | ||
| @pytest.mark.parametrize('id, status_code', user_results2) | ||
| def test_load_widget_type(client, users, id, status_code): | ||
| login_user_via_session(client=client, email=users[id]['obj'].email) |
There was a problem hiding this comment.
issue (testing): The test_load_widget_type test logs in with users[id]['obj'].email, but the fixture's entry at index 4 has email set to generaluser.email while obj is incorrectly set to sysadmin; the case expecting a 403 therefore logs in as a system administrator and does not test the general user's permission.
Triggers: When the parameterized case uses id == 4.
Suggested fix: Log in with users[id]['email'], as the uploaded-file test does, or correct the fixture's obj value before using it.
| login_user_via_session(client=client, email=users[id]['obj'].email) | |
| login_user_via_session(client=client, email=users[id]['email']) |
|
|
||
| @blueprint_api.route('/load_widget_type', methods=['GET']) | ||
| @login_required | ||
| @roles_required([WEKO_ADMIN_PERMISSION_ROLE_SYSTEM, |
| @blueprint.route('/widget/uploaded/<string:filename>/<string:community_id>', | ||
| methods=["GET"] | ||
| ) | ||
| @roles_required([WEKO_ADMIN_PERMISSION_ROLE_SYSTEM, |
API インベントリ差分（件数のみ）
ベースラインとの差分API インベントリ差分レポート
判定: ✅ PASS (FAIL 0 / WARN 0)サマリ
変化はありません。 台帳との突き合わせスナップショット ↔ インベントリ 突き合わせ
判定: ✅ 一致 (0件)
ソース由来の経路検知ソース由来の経路検知
判定: ✅ 全検知が台帳に対応 (0件)
参考: 静的検知と結びつかなかった台帳行
|
2 participants
概要 (Summary)
関連Issue / チケット (Related Issues)
変更タイプ (Type of Change)
🤖 0. CI 自動チェック (API Inventory Drift)
PR ごとに WEKO3 コンテナを起動し、
url_mapのダンプ・台帳との突き合わせ・変更行の到達可否測定を自動実行する。結果は PR コメントと Actions の artifact
(
api-inventory-summary) に出る。このリポジトリは public のため、台帳もベースラインも同梱していない。
実データはプライベートリポジトリ
RCOSDP/weko-secretにあり、CI は Secret 経由で取得する。以降この文書では、そこを単にプライベートリポジトリと呼ぶ。
台帳はブランチごとに内容が違うため、CI は weko 側と同名のブランチを
プライベートリポジトリから探して使う（head → base → 既定ブランチ の順）。
採用されたブランチ名は PR コメントの冒頭に出るので、件数を読む前にそこを見ること。
対応ブランチが無い場合は既定ブランチと比較され、コメント冒頭に警告が出る。
その件数は当てにならないので、PASS でも「確認済み」と読まないこと。
詳細:
tools/api-inventory/ci/README.md§3aSecret (
API_INVENTORY_REPO/API_INVENTORY_SSH_KEY) が未設定のリポジトリ、および fork からの PR では、このジョブは何もせずスキップされる。
API を追加・変更した場合（必須）
この PR が
fix/issue62569→develop_v2.0.4なら、プライベート側もfix/issue62569→develop_v2.0.4。同名にしておけば台帳 PR が未マージでもCI がそれを見るので、2つの PR のマージ順を気にしなくてよい。
api_snapshot.jsonを更新し、対応する PR を出したbash export WEKO_API_INVENTORY_DIR=/path/to/weko-secret ./install.sh python3 tools/api-inventory/scripts/snapshot.py --out "$WEKO_API_INVENTORY_DIR/api_snapshot.json"更新しないと CI が落ちる。公開リポジトリのコード変更とは別の PRになる。
weko3_api_list_full.tsvに行を追加・更新し、build_checklist.pyで 24 列版を再生成した（未収載だと reconcile が FAIL する）（
git statusに*.tsv/api_snapshot.jsonが出ていないこと）FAIL したときの対処（要約）
まず PR コメント冒頭の台帳ブランチを見る。警告が出ていれば、件数を追う前に
プライベートリポジトリ側の対応ブランチを用意すること（比較相手が違うので件数に意味がない）。
ジョブが落ちる条件は 3 つある。PR コメントのどのセクションに件数が出ているかで切り分ける。
drift.md)reconcile.md)*_PERMISSION_FACTORY/ CSRF 保護 等が危険側の値に変わったcan_delete/can_exportがFalse→Truedata_opを更新data_opが作成/更新/削除の経路に、未認証で到達したdata_opの記載誤りなら台帳を直すurl_mapに無いreconcile B のうち、実機に存在しないことが正当な行（プラグイン未登録・config で無効等）は
プライベートリポジトリの
reconcile_allow.jsonに理由付きで登録する。理由なしの登録は不可。登録済みの行は B'（既知・許容）として集計され、E'（endpoint が実機に無い）と併せてゲート対象外になる。
W1〜W6 は WARN でゲートは通るが、レビューでは見ること
（ModelView の追加 / 実装本体の変化 / HTTP メソッド・URL の変化 / 監視対象 config の変化 /
依存パッケージの版の変化）。特に W6（依存の版）は、ベースラインを CI と異なる環境で作ると
毎回出続けて形骸化するため、ベースラインは
install.shで作った環境から生成する。🔒 1. セキュリティ & API アクセス制御チェック (必須)
認証・認可 (Authentication & Authorization)
@login_required,@pass_record,need(...), Invenio Access Action/api/*ではPermission.require(http_exception=403)を使うこと。@login_requiredは API アプリにsecurity.loginが無いため 401 ではなく 500 になる--allow-writes付きでGET / HEAD 以外も叩く）
起動した経路のみ。ワークフロー系など未解決プレースホルダの行は skip される
Noneで無効化していない*_PERMISSION_FACTORY等を監視機能クローズ・非公開化の場合 (Feature Disable)
404 Not Foundまたは403 Forbiddenが返ることを確認した🧪 2. テストコード観点チェック (pytest / Invenio Test Suite)
権限・異常系テスト (Negative & Authorization Tests)
401 Unauthorizedまたは403 Forbidden/404 Not Foundが返ることを検証するテストがある403になるテストがある404/403を返すテストがある境界値・入力バリデーションテスト (Boundary & Validation)
400 Bad Request/ バリデーションエラーが返るテストがあるデータ整合性・トランザクションテスト (Integrity & Rollback)
🛡️ 3. データ保護 & 破壊的変更防止チェック (Data Safety)
⚙️ 4. マイグレーション & システム影響チェック (Invenio / WEKO3 Stack)
データベース (DB / Alembic)
invenio alembic upgrade（適用）およびdowngrade（ロールバック）スクリプトを作成・検証した検索インデックス (Elasticsearch / OpenSearch)
設定 & 非同期処理 (Config / Celery / Cache)
invenio.cfg/ 環境変数のデフォルト値を設定した📚 5. ドキュメント・仕様書更新チェック (weko-document)
tools/api-inventory/、台帳・調査記録はプライベートリポジトリ（public リポジトリには置かない）。
§0 のチェック項目で対応済みなら、ここは確認のみ。
weko3_api_auth_findings.md）もプライベートリポジトリに置く。台帳は二重管理しない📋 6. 動作検証エビデンス (Verification Evidence)
テスト実行結果
CI の成果物 (artifact:
api-inventory-summary)drift.mdreconcile.md明細（該当した経路名・実測結果）は公開できないため artifact に含めていない。
プライベートリポジトリ側で同じコマンドを
--summary-onlyなしで実行して確認する。手動で確認したこと
Summary by Sourcery
Enforce role-based authorization on widget-related API endpoints and verify access for supported user roles.
Bug Fixes:
Tests: