Fix 2FA admin emails silently suppressed when default store has email disabled - #346
Open
thai2301 wants to merge 2 commits into
Open
Fix 2FA admin emails silently suppressed when default store has email disabled#346thai2301 wants to merge 2 commits into
thai2301 wants to merge 2 commits into
Conversation
EmailUserNotifier renders its emails for store 0/adminhtml, but that only affects template locale/content. The actual send-suppression check lives in Magento_Email's TransportInterfacePlugin, which reads system/smtp/disable from whatever store the environment currently resolves to - which in a live adminhtml request falls through to the default store of the default website, not store 0. If that store has email communication disabled, every 2FA notification silently gets swallowed even though it's meant to be governed by the global scope. Wrapped the sendMessage() call in App\Emulation::startEnvironmentEmulation for Store::DEFAULT_STORE_ID/adminhtml, mirroring the existing fix for the same class of bug in Magento\ProductAlert\Model\Email (ACP2E-4870), which uses the identical pattern for its own (per-store) scope.
Open
5 tasks
Author
|
@magento run all tests |
1 similar comment
Author
|
@magento run all tests |
Author
|
@magento run Unit Tests, Functional Tests B2B |
getFrontendName() is only declared on the concrete Store class, not on StoreInterface, so PHPUnit's mock generator rejected configuring it on a StoreInterface mock. Also suppressed the expected CouplingBetweenObjects PHPMD warning on the test class, matching existing test conventions.
Author
|
@magento run all tests |
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
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.
Description
EmailUserNotifier::sendConfigRequired()renders its 2FA-instructions email for'area' => 'adminhtml', 'store' => 0, but that option only affects the template's locale/content. Whether the email actually gets sent is decided by a completely separate, global plugin in Magento core:Magento\Email\Model\Mail\TransportInterfacePlugin::aroundSendMessage(), which checks:with no explicit scope code. That resolves against whatever store
StoreManagerInterface::getStore()considers "current" at that moment — in a live adminhtml request (no frontend store context), Magento's store resolution falls through to the default store of the default website, not store 0. If that store specifically has email communication disabled, every 2FA notification is silently swallowed, even though this email is architecturally meant to be governed by the global/default scope.This mirrors a very similar, already-fixed bug in
Magento\ProductAlert\Model\Email::send()(ticket ACP2E-4870, "Product Alert Consumer ignores store scoped system/smtp/disable"), which wraps its ownsendMessage()call inApp\Emulation::startEnvironmentEmulation()/stopEnvironmentEmulation()so the ambient store the suppression check reads actually matches the store the email was built for.This PR applies the same pattern here, emulating
Store::DEFAULT_STORE_ID/Area::AREA_ADMINHTMLaround thesendMessage()call, so the plugin's scope-less check resolves to the same global scope this notification is rendered for, instead of an unrelated frontend store. TheTransportInterfacePluginitself is intentionally left untouched — it's a shared plugin wrapping every outgoing email in Magento, and changing its scope resolution would break legitimate per-store suppression for genuinely store-scoped emails (newsletters, product alerts, etc., including the ACP2E-4870 fix it relies on).Related Issue
Fixes magento/magento2#40969
Manual testing scenarios
system/smtp/disable(Store Email Addresses config or directcore_config_datarow) to1for a specific store, e.g. the default store of the default website.Questions or comments
None.
Contribution checklist