#5898 メンバー登録画面にて2段階認証が有効になっているメンバーの2段階認証をリセットするチェックボックスを追加#6546
Open
AioiLight wants to merge 5 commits intoEC-CUBE:4.3from
Open
#5898 メンバー登録画面にて2段階認証が有効になっているメンバーの2段階認証をリセットするチェックボックスを追加#6546AioiLight wants to merge 5 commits intoEC-CUBE:4.3from
AioiLight wants to merge 5 commits intoEC-CUBE:4.3from
Conversation
Contributor
|
@AioiLight |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.3 #6546 +/- ##
============================================
+ Coverage 78.65% 79.37% +0.71%
+ Complexity 6824 6670 -154
============================================
Files 476 476
Lines 27074 26705 -369
============================================
- Hits 21295 21196 -99
+ Misses 5779 5509 -270
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dotani1111
reviewed
Jan 15, 2026
Contributor
|
以下、動作確認を致しました。 |
Contributor
|
@AioiLight |
Author
|
@dotani1111 |
📝 WalkthroughWalkthroughメンバー編集フロー内に二要素認証リセット機能を追加しました。チェックボックス追加、リセット時に2FAキーをクリア、対応する翻訳キー追加、テンプレートに条件付き表示を実装しました。 Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as 管理者
participant Form as メンバーフォーム
participant Controller as MemberController
participant DB as メンバーDB
Admin->>Form: 2FAリセットをチェック
Form->>Controller: フォーム送信
Controller->>Controller: two_factor_auth_resetの値を確認
alt two_factor_auth_resetがTrueで2FAが有効な場合
Controller->>DB: two_factor_auth_keyをクリア
end
Controller->>DB: メンバー情報を保存
DB-->>Controller: 保存完了
Controller-->>Admin: 処理完了
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Eccube/Controller/Admin/Setting/System/MemberController.php`:
- Around line 169-171: 現在の条件は Member->isTwoFactorAuthEnabled() と AND
になっているため、同一送信で2FAを無効化した場合に two_factor_auth_reset が無視され旧キーが残ります。MemberController
の該当箇所で two_factor_auth_reset の値を先に確認し、true のときは isTwoFactorAuthEnabled() に依存せず必ず
Member->setTwoFactorAuthKey(null) を呼ぶように修正してください（つまり two_factor_auth_reset
のチェックを独立させるか、条件式をリセットフラグ優先に変更してください）。
In `@src/Eccube/Resource/locale/messages.en.yaml`:
- Line 1779: The tooltip text for key
tooltip.setting.system.member.two_factor_auth_reset contains awkward English
("unable to use your authentication app some reasons"); update the string to
natural, professional English such as: "Checking this box will reset the
two-factor authentication for this member. If you reset it, the code stored in
their authentication app will no longer be valid and the device will no longer
be registered. If you cannot use your authentication app for any reason, reset
it and re-register on a new device." Replace the existing value for
tooltip.setting.system.member.two_factor_auth_reset with this corrected sentence
while preserving YAML formatting and punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a9edf85-d35f-4f06-b55a-7f5f69cfedef
📒 Files selected for processing (5)
src/Eccube/Controller/Admin/Setting/System/MemberController.phpsrc/Eccube/Form/Type/Admin/MemberType.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/admin/Setting/System/member_edit.twig
Comment on lines
+169
to
+171
| if ($Member->isTwoFactorAuthEnabled() && $form->get('two_factor_auth_reset')->getData()) { | ||
| $Member->setTwoFactorAuthKey(null); | ||
| } |
There was a problem hiding this comment.
two_factor_auth_enabled とのAND条件でリセット要求が無視されます
Line 169 の条件だと、同一送信で2FAを無効化した場合に、two_factor_auth_reset をチェックしていても鍵が消えません。結果として「リセットしたつもり」で旧キーが残るため、再登録フローの期待とずれます。two_factor_auth_reset が true のときは enabled 状態に依存せず鍵をクリアするのが安全です。
💡 修正案
- if ($Member->isTwoFactorAuthEnabled() && $form->get('two_factor_auth_reset')->getData()) {
+ if ($form->get('two_factor_auth_reset')->getData()) {
$Member->setTwoFactorAuthKey(null);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Eccube/Controller/Admin/Setting/System/MemberController.php` around lines
169 - 171, 現在の条件は Member->isTwoFactorAuthEnabled() と AND
になっているため、同一送信で2FAを無効化した場合に two_factor_auth_reset が無視され旧キーが残ります。MemberController
の該当箇所で two_factor_auth_reset の値を先に確認し、true のときは isTwoFactorAuthEnabled() に依存せず必ず
Member->setTwoFactorAuthKey(null) を呼ぶように修正してください（つまり two_factor_auth_reset
のチェックを独立させるか、条件式をリセットフラグ優先に変更してください）。
| tooltip.setting.system.member.authority: You can select the authorization you have set under Authorization. | ||
| tooltip.setting.system.member.work: You can set customers to inactive temporarily. If they are no longer necessary, please delete from All Customers. | ||
| tooltip.setting.system.member.two_factor_auth_enabled: If enabled, you must log in using two-factor authentication. You will be able to login after setting up two-factor authentication. | ||
| tooltip.setting.system.member.two_factor_auth_reset: Checking this box will reset the two-factor authentication for this member. If you reset it, the code currently stored in the authentication app will no longer be usable and will no longer be registered. If you are unable to use your authentication app some reasons, reset it and re-register on a new device. |
There was a problem hiding this comment.
英語ツールチップに文法不備があります
Line 1779 の "unable to use your authentication app some reasons" は不自然な英語です。管理画面文言として修正したほうがよいです。
✍️ 修正案
-tooltip.setting.system.member.two_factor_auth_reset: Checking this box will reset the two-factor authentication for this member. If you reset it, the code currently stored in the authentication app will no longer be usable and will no longer be registered. If you are unable to use your authentication app some reasons, reset it and re-register on a new device.
+tooltip.setting.system.member.two_factor_auth_reset: Checking this box will reset two-factor authentication for this member. After reset, the code currently registered in the authentication app will no longer be usable. If you are unable to use your authentication app for some reason, reset it and re-register on a new device.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Eccube/Resource/locale/messages.en.yaml` at line 1779, The tooltip text
for key tooltip.setting.system.member.two_factor_auth_reset contains awkward
English ("unable to use your authentication app some reasons"); update the
string to natural, professional English such as: "Checking this box will reset
the two-factor authentication for this member. If you reset it, the code stored
in their authentication app will no longer be valid and the device will no
longer be registered. If you cannot use your authentication app for any reason,
reset it and re-register on a new device." Replace the existing value for
tooltip.setting.system.member.two_factor_auth_reset with this corrected sentence
while preserving YAML formatting and punctuation.
Contributor
|
@AioiLight 追加で、こちらの対応することは可能でしょうか？ |
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.
概要(Overview・Refs Issue)
fixes #5898
現在2段階認証はオフにしても新しい認証が設定できるわけではなく、再度有効にしても同じ認証コードのキーが必要になります。
現時点では、ログインした後に再設定することはできますが、ログイン前ではできないため、認証コードを登録したデバイスを紛失したなどの場合においては、データベースを触らない限り再設定ができない状態になっています。
このPRは、メンバーの編集画面にて、2要素認証のキーをリセットすることができるようになります。また、 Issue にコメントしたように、既存の認証キーは有効のまま、一時的に2段階認証を無効化したいというユースケースも考えられるため、今までの無効化の挙動はそのままにしています。
方針(Policy)
メンバーの編集画面に、2段階認証をリセットするチェックボックスを作成しました。
チェックを入れて保存した場合、対象のユーザーの2段階認証キーをnullに設定して、再登録できるようになります。
メンバーの編集が可能な権限を持つ人にリセットを依頼することで、デバイスを紛失したなどの場合でも2段階認証の再設定が可能になります。
実装に関する補足(Appendix)
2段階認証が有効の場合の時のみ、チェックボックスが出現するようになっています。
テスト（Test)
確認した挙動など：
相談（Discussion）
初のContributionになるので、こうした方がよいというところがありましたら教えていただけると幸いです！
マイナーバージョン互換性保持のための制限事項チェックリスト
レビュワー確認項目
Summary by CodeRabbit