fix: 公開向けの配信・統計・補助 API で公開状態と閲覧権限を確認する - #1926
Merged
Merged
Conversation
validate_bibtex で、存在しないレコードと、詳細画面の権限判定 (page_permission_factory) で閲覧できないレコードを、必須項目不足と同じく 出力できないレコードとして返す。 validate_bibtex_export は record_ids がリストでない等の不正な入力に 400 を返す。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- レコード単位の経路に、レコードが公開済み・公開日到来済み・公開インデックス 配下であることを確認するデコレータ public_record_required を追加 (判定は weko_records_ui の check_publish_status と invenio_oaiserver の is_private_index を再利用)。版付き識別子は親レコードも確認する - 変更一覧の検索条件に公開状態の絞り込みを追加。削除の通知を維持するため 削除済みは対象に残す Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
manifest に載せるファイルを、ファイル詳細画面やエクスポートと同じ check_file_download_permission の判定でダウンロードできるものに限る。 前版のファイルは前版のレコードに対して判定する。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
アイテム詳細画面の非ログイン利用者向けの判定と同じく、check_publish_status (公開状態・公開日) と check_index_permissions (閲覧可能なインデックス配下) を 満たすアイテムだけを列挙する。判定は最新の状態を持つ親レコードで行う。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
アイテム詳細画面から呼ばれる閲覧数とファイル統計の取得 API に、 詳細画面と同じ閲覧権限(page_permission_factory)を確認するデコレータを付ける。 ファイル統計は bucket を持つレコードで判定する。 あわせて日付指定の POST で本文が不正なときは 400 を返すようにする。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RECORDS_UI_ENDPOINTS の recid_signposting に、同じ route の recid と同じ page_permission_factory を permission_factory_imp として設定する。 テストは weko-records-ui が持つ endpoint 定義をそのまま使うようにし、 閲覧できない利用者が拒否されることと、公開アイテム・管理者は引き続き 応答を得られることを確認する。 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- 閲覧権限の判定には既存の check_index_permissions を使う - 数字以外を含む指定は 400 を返し、存在しないインデックスは結果に含めない Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- invenio-stats: リクエストの後はフィクスチャのオブジェクトがセッションから 外れるので、比較に使う id を先に控える - weko-search-ui: テスト用のインデックスを、フィクスチャのインデックスと (parent, position) が重ならない位置に作る Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
Reviewer's Guide公開向けの ResourceSync、サイトマップ、統計、signposting、検索 UI、BibTeX 補助 API に、公開状態・公開日時・インデックス閲覧権限・ファイル取得権限の検証を追加し、非公開情報の露出を防ぎながら入力・認可・不存在時のHTTP応答を整理している。 Sequence diagram for permission-filtered public APIssequenceDiagram
actor Client
participant API as Public API
participant Record as WekoRecord
participant Permission as Permission checks
Client->>API: Request resource, sitemap, stats, or BibTeX data
API->>Record: Load record
API->>Permission: Check publication status and page permission
alt Invalid input
API-->>Client: 400 Bad Request
else Record not found
API-->>Client: 404 Not Found
else Record not viewable
API-->>Client: 403 Forbidden or omit item
else Public and viewable
API-->>Client: Return data or public URL
end
Sequence diagram for permission-filtered ResourceSync filessequenceDiagram
participant Client
participant ResourceSync
participant Record as WekoRecord
participant FilePermission as check_download_file
Client->>ResourceSync: Request public record manifest or file content
ResourceSync->>Record: Load record by record_id
ResourceSync->>ResourceSync: is_public_record(record_id)
alt Record is not public
ResourceSync-->>Client: 404 Not Found
else Record is public
loop Record files
ResourceSync->>FilePermission: can_download_file(record, file)
alt File is downloadable
ResourceSync-->>Client: Include file in manifest or archive
else File is not downloadable
ResourceSync->>ResourceSync: Omit file
end
end
end
Flow diagram for public-item filteringflowchart TD
A[Candidate record or index] --> B{Published and publication date reached?}
B -- No --> X[Exclude from public output]
B -- Yes --> C{Public index and browse permission granted?}
C -- No --> X
C -- Yes --> D[Expose in ResourceSync, sitemap, search UI, or signposting]
D --> E{File requested or listed?}
E -- Yes --> F{check_file_download_permission}
F -- Allowed --> G[Include file]
F -- Denied --> X
E -- No --> H[Return public record metadata]
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 |
🔍 Claude レビュー統合指摘はありません。 次にすること: 外部データは bot の要約と、レビューをスキップした旨の通知だけで、裁定すべき指摘スレッドはありませんでした。自分でも重大な問題は見つかりませんでした。ResourceSync のファイル本体の配信は モデル sonnet ／ 2 回実行して和集合 ／ コスト $0.4581。同じ入力でも結果が揺れるため複数回まわし、一部のパスでしか挙がらなかったものには回数を添えています 他レビューを踏まえた自動レビューです。誤りが含まれることがあります。 |
![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/weko-sitemap/weko_sitemap/ext.py" line_range="121-123" />
<code_context>
.limit(current_app.config['WEKO_SITEMAP_TOTAL_MAX_URL_COUNT']))
for recid, rm in q.yield_per(1000):
+ pid_value = (recid.pid_value).replace('.1', '')
+ if not self._is_public_item(pid_value):
+ continue
yield {
'loc': url_for('invenio_records_ui.recid',
</code_context>
<issue_to_address>
**issue (bug_risk):** The query applies `WEKO_SITEMAP_TOTAL_MAX_URL_COUNT` before `_is_public_item` filters records, so when the first page contains enough private, future-dated, or inaccessible records, later public records are never examined and are omitted from the sitemap.
**Triggers:** When the sitemap reaches its configured maximum candidate count and inaccessible records occur before public records in PID order.
**Suggested fix:** Filter public records in the query or continue scanning until the generator has yielded the configured maximum number of public URLs.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and these changes alter authorization decisions across public resource-sync downloads, statistics, BibTeX export, signposting, and sitemap generation; a faulty check could expose unpublished records, files, or usage data to unauthorized callers across the affected APIs. Reverting stops future exposure, but any data already disclosed cannot be recalled, and a mistaken permission policy could affect everyone immediately without a reliable failure signal.
Blocking findings: modules/weko-sitemap/weko_sitemap/ext.py:123
Comment on lines
+121
to
+123
| pid_value = (recid.pid_value).replace('.1', '') | ||
| if not self._is_public_item(pid_value): | ||
| continue |
There was a problem hiding this comment.
issue (bug_risk): The query applies WEKO_SITEMAP_TOTAL_MAX_URL_COUNT before _is_public_item filters records, so when the first page contains enough private, future-dated, or inaccessible records, later public records are never examined and are omitted from the sitemap.
Triggers: When the sitemap reaches its configured maximum candidate count and inaccessible records occur before public records in PID order.
Suggested fix: Filter public records in the query or continue scanning until the generator has yielded the configured maximum number of public URLs.
API インベントリ差分（件数のみ）
ベースラインとの差分API インベントリ差分レポート
判定: ✅ PASS (FAIL 0 / WARN 1)サマリ
[WARN] W2 実装本体が変化(data_op / 情報露出を再確認) — 2件
台帳との突き合わせスナップショット ↔ インベントリ 突き合わせ
判定: ✅ 一致 (0件)
ソース由来の経路検知ソース由来の経路検知
判定: ✅ 全検知が台帳に対応 (0件)
参考: 静的検知と結びつかなかった台帳行
|
| record = WekoRecord.get_record_by_pid(record_id) | ||
| if record: | ||
| for file in record.files: | ||
| if not can_download_file(record, file): |
| prev_record = None | ||
| if current_record: | ||
| list_file = [file for file in current_record.files] | ||
| list_file = [ |
| prev_checksum = [] | ||
| if prev_record: | ||
| list_file.extend([file for file in prev_record.files]) | ||
| list_file.extend( |
| prev_record.files | ||
| ] | ||
| for file in list_file: | ||
| for record, file in list_file: |
| "post_filter": { | ||
| "bool": { | ||
| "must": [ | ||
| { |
Contributor
There was a problem hiding this comment.
fix no.100, 102, 104, 106, 107, 108
| @@ -0,0 +1,87 @@ | |||
| # -*- coding: utf-8 -*- | |||
Contributor
There was a problem hiding this comment.
fix no.100, 102, 104, 106, 107, 108
|
|
||
|
|
||
| @blueprint.route("/resync/<index_id>/<record_id>/file_content.zip") | ||
| @public_record_required() |
|
|
||
|
|
||
| @blueprint.route("/resync/<index_id>/<record_id>/resourcedump_manifest.xml") | ||
| @public_record_required() |
|
|
||
|
|
||
| @blueprint.route("/resync/<index_id>/<record_id>/changedump_manifest.xml") | ||
| @public_record_required() |
|
|
||
|
|
||
| @blueprint.route("/resync/<index_id>/<record_id>/change_dump_content.zip") | ||
| @public_record_required() |
|
|
||
| stats_api_access = action_factory('stats-api-access') | ||
| stats_api_permission = Permission(stats_api_access) | ||
|
|
Contributor
There was a problem hiding this comment.
fix no.113,114,115,116
| return wrapper | ||
|
|
||
|
|
||
| def get_query_date(data): |
Contributor
There was a problem hiding this comment.
fix no.113,114,115,116
|
|
||
| return result | ||
|
|
||
| @record_view_permission_required |
| abort(400) | ||
| return self.make_response(self.get_data(record_uuid, get_period=True)) | ||
|
|
||
| @record_view_permission_required |
| else: | ||
| date = d['date'] | ||
| except (TypeError, ValueError): | ||
| except ValueError: |
| except ValueError: | ||
| current_app.logger.error(traceback.format_exc()) | ||
| abort(400) | ||
| date = get_query_date(request.get_json(force=False, silent=True)) |
|
|
||
| return result | ||
|
|
||
| @bucket_view_permission_required |
| file_key, | ||
| get_period=True)) | ||
|
|
||
| @bucket_view_permission_required |
| date = None | ||
| else: | ||
| date = d['date'] | ||
| date = get_query_date(request.get_json(force=False, silent=True)) |
| from weko_schema_ui.serializers import WekoBibTexSerializer | ||
| for record_id in record_ids: | ||
| record = WekoRecord.get_record_by_pid(record_id) | ||
| try: |
| from .utils import validate_bibtex | ||
| post_data = request.get_json() | ||
| record_ids = post_data['record_ids'] | ||
| post_data = request.get_json(silent=True) |
| pid_type='recid', | ||
| route='/records/<pid_value>', | ||
| view_imp='weko_signposting.api.requested_signposting', | ||
| permission_factory_imp='weko_records_ui.permissions' |
| @blueprint.route("/get_path_name_dict/<string:path_str>", methods=["GET"]) | ||
| def get_path_name_dict(path_str=""): | ||
| """Get path and name.""" | ||
| """Get path and name. |
| from weko_index_tree.utils import check_index_permissions | ||
| path_name_dict = {} | ||
| path_arr = path_str.split("_") | ||
| if not all(re.match(r"^[0-9]{1,18}$", path) for path in path_arr): |
| abort(400) | ||
| for path in path_arr: | ||
| index = Indexes.get_index(index_id=path) | ||
| index = Indexes.get_index(index_id=int(path)) |
| continue | ||
| idx_name = index.index_name | ||
| idx_name_en = index.index_name_english | ||
| idx_name_en = index.index_name_english or "" |
| .limit(current_app.config['WEKO_SITEMAP_TOTAL_MAX_URL_COUNT'])) | ||
|
|
||
| for recid, rm in q.yield_per(1000): | ||
| pid_value = (recid.pid_value).replace('.1', '') |
| 'loc': url_for('invenio_records_ui.recid', | ||
| pid_value=(recid.pid_value).replace( | ||
| '.1', ''), | ||
| pid_value=pid_value, |
| rm.updated, 'yyyy-MM-ddTHH:mm:ssz', 'full') | ||
| } | ||
|
|
||
| @staticmethod |
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
2 participants
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.
概要 (Summary)
public_record_requiredを付ける。changelist / changedump の検索条件を公開または削除済みのアイテムに限る(削除の通知は引き続き出す)。manifest に列挙するファイルをダウンロードできるものに限るpage_permission_factory)を要求するデコレータを付けるrecid_signpostingにpage_permission_factoryを設定する(同じ経路のrecidと揃える)関連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 を出した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なしで実行して確認する。手動で確認したこと
🤖 Generated with Claude Code