Skip to content

fix: 2段階認証の再設定ルートを /setting 配下から移動 (#6406)#6893

Open
ttokoro20240902 wants to merge 1 commit into
4.4from
feature/6406-two-factor-auth-route
Open

fix: 2段階認証の再設定ルートを /setting 配下から移動 (#6406)#6893
ttokoro20240902 wants to merge 1 commit into
4.4from
feature/6406-two-factor-auth-route

Conversation

@ttokoro20240902

@ttokoro20240902 ttokoro20240902 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

概要(Overview・Refs Issue)

Closes #6406

権限管理でシステム設定(/setting)を拒否URLに設定すると、その権限を持つメンバーが自分の2段階認証を再設定できない問題を修正します。

2段階認証の再設定 (admin_setting_system_two_factor_auth_edit) は /setting/system/two_factor_auth/edit に配置されていたため、AuthorityVoter の前方一致による拒否URL判定で拒否URL /setting にマッチし、本人向けの操作にもかかわらず 403 (Access Denied) で弾かれていました。

一方「パスワード変更」は /change_password(/setting配下でない)のため影響を受けず、Issue の指摘通りの差になっていました。

方針(Policy)

  • Issue のコメント(@dotani1111)で合意されている ルーティング変更 方針を採用しました。
  • 2段階認証の再設定は「パスワード変更」と同じ本人向けアクションであり、システム設定(/setting/system)配下に置くのが不適切でした。admin_change_password(/change_password)に倣い、ルートパスを /two_factor_auth/edit へ移し /setting 名前空間から外します。既存の admin_two_factor_auth(/two_factor_auth)・admin_two_factor_auth_set(/two_factor_auth/set)とも名前空間が揃います。
  • なお 権限管理のアクセス制限の設定方法改修 #6242(拒否URL前方一致の設計全般の改修)とは別の、本Issue局所の修正です。

実装に関する補足(Appendix)

  • ルート名 admin_setting_system_two_factor_auth_edit は後方互換のため据え置きました。メニューのリンク(default_frame.twig)はルート名参照(url(...))のため変更不要です。プラグイン等がルート名で参照している場合も影響ありません。
  • URL パス自体は変わります(旧 /admin/setting/system/two_factor_auth/edit は 404 になります)。この画面は管理画面右上メニューの「2段階認証 設定」からルート名で遷移するため、通常導線に影響はありません。
  • /admin 配下のままなので ROLE_ADMIN の認可は引き続き有効で、認可レベルの防御は失われません(拒否URLの粒度だけが変わります=本Issueの狙い)。

テスト(Test)

  • 回帰テスト testEditIsNotForbiddenWhenSettingUrlIsDenied を追加。拒否URL /setting を持つ権限のメンバーが再設定画面にアクセスしても 403 にならないことを検証します(修正前のパスでは 403 で失敗することも確認済み)。
  • 既存の TwoFactorAuthControllerTest(5件) が通ることを確認。
  • Docker 環境で実機再現・修正後の動作を確認(旧URL:404 / 新URL:正常)。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
    • ※ 本人向け2FA再設定画面のURLパスのみ変更します(ルート名は据え置き)。
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • 2段階認証の再設定画面のアクセス経路を見直し、権限制限の設定によって画面が誤ってブロックされる問題を修正しました。
    • /setting のアクセス制限がある環境でも、2段階認証の再設定画面を開けるようになりました。

権限管理で拒否URLに `/setting` を設定すると、その権限を持つメンバーが
2段階認証を再設定できない問題を修正する。

2段階認証の再設定 (`admin_setting_system_two_factor_auth_edit`) は
`/setting/system/two_factor_auth/edit` に配置されていたため、
AuthorityVoter の前方一致による拒否URL判定で `/setting` にマッチし、
本人向けの操作にもかかわらず 403 で弾かれていた。

パスワード変更 (`/change_password`) と同様に本人向けアクションであるため、
ルートパスを `/two_factor_auth/edit` へ移し `/setting` 名前空間から外す。
ルート名は後方互換のため据え置き、メニューのリンク (default_frame.twig) は
ルート名参照のため変更不要。`/admin` 配下のため ROLE_ADMIN 認可は維持される。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

2要素認証再設定コントローラーのルートパスが /setting/system/two_factor_auth/edit から /two_factor_auth/edit に変更された。あわせて、権限管理で /setting へのアクセスが拒否されている場合でも再設定画面が403にならないことを確認する回帰テストが追加された。

Changes

2FA再設定ルート変更と検証

Layer / File(s) Summary
Routeパス変更
src/Eccube/Controller/Admin/Setting/System/TwoFactorAuthController.php
edit()のRoute属性pathを .../setting/system/two_factor_auth/edit から .../two_factor_auth/edit に変更。
回帰テスト追加
tests/Eccube/Tests/Web/Admin/Setting/System/TwoFactorAuthControllerTest.php
AuthorityRoleResponseのimportを追加し、denyUrlが/settingのAuthorityRoleを作成・保存した上で再設定画面にアクセスし、403にならないことを検証するテストを追加。

Estimated code review effort: 1 (Trivial) | ~5 minutes

Poem

ぴょんと跳ねて URL を書き換えた
拒否された道でも うさぎは通れる
`#6406` のテストで安心確認
403なんて 見せませんよ 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 2段階認証の再設定ルートを /setting 配下から移動する変更内容を正確に要約しています。
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/6406-two-factor-auth-route

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.22%. Comparing base (a387a99) to head (a9cd7a7).

Additional details and impacted files
@@            Coverage Diff             @@
##              4.4    #6893      +/-   ##
==========================================
- Coverage   75.32%   75.22%   -0.11%     
==========================================
  Files         519      519              
  Lines       25483    25483              
==========================================
- Hits        19196    19169      -27     
- Misses       6287     6314      +27     
Flag Coverage Δ
Unit 75.22% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nanasess nanasess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM です。ローカル(SQLite)で動作とテストを確認しました。

修正内容の確認

AuthorityVoterpreg_match("/^(\/{$adminRoute}{$denyUrl})/i", $path) による前方一致でパスを判定し、認可は EccubeExtension['path' => '^/%eccube_admin_route%/', 'roles' => 'ROLE_ADMIN']access_decision_manager.strategy: unanimous で効くため、拒否URL /setting があると旧パス /admin/setting/system/two_factor_auth/edit は kernel.request の AccessListener 段階で 403 になります。パスを /admin/two_factor_auth/edit へ移すことで前方一致から外れる、という因果関係を確認しました。Issue #6406 のコメントで合意された方針とも一致しています。

副作用がないことの確認

  • 2段階認証の強制リダイレクトへの影響なし: TwoFactorAuthListenerROUTE_EXCLUDE / ROUTE_EXCLUDE_WHEN_NOT_CONFIGUREDルート名の完全一致in_array($route, self::ROUTE_EXCLUDE))で判定しているため、パスが /two_factor_auth/ 配下へ移動しても除外判定には影響しません。MFAバイパスは発生しません。edit() 冒頭の isAuth() チェックも維持されています。
  • 旧パスの残存参照なし: setting/system/two_factor_auth を全文検索した結果、ヒットはテストのコメント1件のみでした。default_frame.twig はルート名参照(url("admin_setting_system_two_factor_auth_edit"))、zap/scripts/admin_member_setting.zst/admin/two_factor_auth/set のみで影響ありません。
  • ルート名を据え置いた判断が適切: ルート名はプラグインから参照され得る公開APIであり、また _route による分岐にも使われるため、据え置きが最も安全です。テンプレートを @admin/Setting/System/two_factor_auth_edit.twig のまま残した点も、app/template/ での上書き互換が保たれるので適切だと思います。

動作確認(ローカル / SQLite)

検証 結果
TwoFactorAuthControllerTest 全6件 パス
修正を revert して再実行 新規テストが 403 で失敗(回帰テストとして機能)
拒否ルール自体の実効性(/setting/shop/delivery 403(拒否設定は有効)
拒否URL /setting 下で 2FA 認証後に再設定画面へアクセス 200(Issue の再現手順が解消)

Issue #6406 の再現手順が解消されることを実機相当で確認できました。ありがとうございます!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

権限管理でシステム設定が拒否URLに追加されていると、その権限を持つメンバーが2段階認証を再設定できない

3 participants