From c7eb0db5d1c58ca412f3830c8115a0a5bf540d75 Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Mon, 22 May 2023 16:58:04 +0200 Subject: [PATCH] mail: Round 6 of EmailUser refactoring - Rename methods to follow the more standard convention used for commands. - Update doc comments. - Add new hooks with final method names and parameter types, to replace the old messy hooks. - Deprecate the remaining old hooks. Bug: T265541 Change-Id: I609709a70fb58ce00b9f179ee4de2f6ac5e0a1cf --- RELEASE-NOTES-1.41 | 6 ++- autoload.php | 2 + includes/HookContainer/DeprecatedHooks.php | 2 + includes/HookContainer/HookRunner.php | 26 +++++++++++ includes/mail/EmailUser.php | 36 ++++++++++++---- .../mail/Hook/EmailUserAuthorizeSendHook.php | 26 +++++++++++ includes/mail/Hook/EmailUserSendEmailHook.php | 40 +++++++++++++++++ includes/specials/Hook/EmailUserHook.php | 2 +- .../Hook/EmailUserPermissionsErrorsHook.php | 3 +- includes/specials/SpecialEmailUser.php | 6 +-- includes/user/Hook/UserCanSendEmailHook.php | 2 +- includes/user/User.php | 2 +- .../unit/includes/mail/EmailUserTest.php | 43 ++++++++++++++++--- 13 files changed, 171 insertions(+), 25 deletions(-) create mode 100644 includes/mail/Hook/EmailUserAuthorizeSendHook.php create mode 100644 includes/mail/Hook/EmailUserSendEmailHook.php diff --git a/RELEASE-NOTES-1.41 b/RELEASE-NOTES-1.41 index d927d8f4429..189da7833e9 100644 --- a/RELEASE-NOTES-1.41 +++ b/RELEASE-NOTES-1.41 @@ -311,8 +311,10 @@ because of Phabricator reports. ::getLinkHTML() and ::getLinkWiki(). * SearchResultThumbnail::getSize() has been deprecated to be dropped in the future as it is resource intensive and degrades performance. -* The EmailUserPermissionsErrors hook has been deprecated in favour of the - UserCanSendEmail hook. +* The EmailUserPermissionsErrors and UserCanSendEmail hooks have been + deprecated in favour of the EmailUserAuthorizeSend hook. +* The EmailUser hook has been deprecated in favour of the EmailUserSendEmail + hook. * SiteConfiguration::extractVar() and ::extractGlobal() have been deprecated and also emit deprecation warnings. * Hooks::isRegistered(), ::getHandlers(), ::run() and ::runWithoutAbort(), diff --git a/autoload.php b/autoload.php index 720d8767361..2db3b3db161 100644 --- a/autoload.php +++ b/autoload.php @@ -1515,6 +1515,8 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\Mail\\EmailUser' => __DIR__ . '/includes/mail/EmailUser.php', 'MediaWiki\\Mail\\EmailUserFactory' => __DIR__ . '/includes/mail/EmailUserFactory.php', 'MediaWiki\\Mail\\Emailer' => __DIR__ . '/includes/mail/Emailer.php', + 'MediaWiki\\Mail\\Hook\\EmailUserAuthorizeSendHook' => __DIR__ . '/includes/mail/Hook/EmailUserAuthorizeSendHook.php', + 'MediaWiki\\Mail\\Hook\\EmailUserSendEmailHook' => __DIR__ . '/includes/mail/Hook/EmailUserSendEmailHook.php', 'MediaWiki\\Mail\\IEmailer' => __DIR__ . '/includes/mail/IEmailer.php', 'MediaWiki\\Mail\\UserEmailContact' => __DIR__ . '/includes/mail/UserEmailContact.php', 'MediaWiki\\MainConfigNames' => __DIR__ . '/includes/MainConfigNames.php', diff --git a/includes/HookContainer/DeprecatedHooks.php b/includes/HookContainer/DeprecatedHooks.php index 064fa7c7093..f75be4de0ba 100644 --- a/includes/HookContainer/DeprecatedHooks.php +++ b/includes/HookContainer/DeprecatedHooks.php @@ -54,6 +54,8 @@ class DeprecatedHooks { 'ArticleUndelete' => [ 'deprecatedVersion' => '1.40', 'silent' => true ], 'MessageCache::get' => [ 'deprecatedVersion' => '1.41', 'silent' => true ], 'EmailUserPermissionsErrors' => [ 'deprecatedVersion' => '1.41' ], + 'UserCanSendEmail' => [ 'deprecatedVersion' => '1.41', 'silent' => true ], + 'EmailUser' => [ 'deprecatedVersion' => '1.41', 'silent' => true ], ]; /** diff --git a/includes/HookContainer/HookRunner.php b/includes/HookContainer/HookRunner.php index 0b605039456..40160d005d4 100644 --- a/includes/HookContainer/HookRunner.php +++ b/includes/HookContainer/HookRunner.php @@ -6,9 +6,11 @@ use Article; use File; use IContextSource; use JsonContent; +use MailAddress; use ManualLogEntry; use MediaWiki\Linker\LinkRenderer; use MediaWiki\Linker\LinkTarget; +use MediaWiki\Mail\UserEmailContact; use MediaWiki\Page\PageIdentity; use MediaWiki\Page\ProperPageIdentity; use MediaWiki\Permissions\Authority; @@ -171,6 +173,8 @@ class HookRunner implements \MediaWiki\Hook\EmailUserFormHook, \MediaWiki\Hook\EmailUserHook, \MediaWiki\Hook\EmailUserPermissionsErrorsHook, + \MediaWiki\Mail\Hook\EmailUserAuthorizeSendHook, + \MediaWiki\Mail\Hook\EmailUserSendEmailHook, \MediaWiki\Hook\EnhancedChangesListModifyBlockLineDataHook, \MediaWiki\Hook\EnhancedChangesListModifyLineDataHook, \MediaWiki\Hook\EnhancedChangesList__getLogTextHook, @@ -1644,6 +1648,28 @@ class HookRunner implements ); } + public function onEmailUserAuthorizeSend( Authority $sender, StatusValue $status ) { + return $this->container->run( + 'EmailUserAuthorizeSend', + [ $sender, $status ] + ); + } + + public function onEmailUserSendEmail( + Authority $from, + MailAddress $fromAddress, + UserEmailContact $to, + MailAddress $toAddress, + string $subject, + string $text, + StatusValue $status + ) { + return $this->container->run( + 'EmailUserSendEmail', + [ $from, $fromAddress, $to, $toAddress, $subject, $text, $status ] + ); + } + public function onEnhancedChangesListModifyBlockLineData( $changesList, &$data, $rc ) { diff --git a/includes/mail/EmailUser.php b/includes/mail/EmailUser.php index 9dd3330e8ac..a9e11b38149 100644 --- a/includes/mail/EmailUser.php +++ b/includes/mail/EmailUser.php @@ -109,7 +109,8 @@ class EmailUser { } /** - * Validate target User + * @internal + * @todo This method might perhaps be moved to a UserEmailContactLookup or something. * * @param UserEmailContact $target Target user * @return StatusValue @@ -155,13 +156,13 @@ class EmailUser { } /** - * Check whether a user is allowed to send email + * Authorize the email sending, checking permissions etc. * * @param string $editToken * @return StatusValue For BC, the StatusValue's value can be set to a string representing a message key to use * with ErrorPageError. */ - public function getPermissionsError( string $editToken ): StatusValue { + public function authorizeSend( string $editToken ): StatusValue { if ( !$this->options->get( MainConfigNames::EnableEmail ) || !$this->options->get( MainConfigNames::EnableUserEmail ) @@ -194,7 +195,7 @@ class EmailUser { $hookErr = false; - // XXX Replace these hooks with versions that simply use StatusValue for errors. + // TODO Remove deprecated hooks $this->hookRunner->onUserCanSendEmail( $user, $hookErr ); $this->hookRunner->onEmailUserPermissionsErrors( $user, $editToken, $hookErr ); if ( is_array( $hookErr ) ) { @@ -204,14 +205,17 @@ class EmailUser { $ret->value = $hookErr[0]; return $ret; } + $hookStatus = StatusValue::newGood(); + $hookRes = $this->hookRunner->onEmailUserAuthorizeSend( $this->sender, $hookStatus ); + if ( !$hookRes && !$hookStatus->isGood() ) { + return $hookStatus; + } return StatusValue::newGood(); } /** - * Really send a mail. Permissions should have been checked using - * getPermissionsError(). It is probably also a good - * idea to check the edit token and ping limiter in advance. + * Really send a mail, without permission checks. * * @param UserEmailContact $target * @param string $subject @@ -220,7 +224,7 @@ class EmailUser { * @param MessageLocalizer $messageLocalizer * @return StatusValue */ - public function submit( + public function sendEmailUnsafe( UserEmailContact $target, string $subject, string $text, @@ -259,7 +263,7 @@ class EmailUser { } $error = false; - // FIXME Replace this hook with a new one that returns errors in a SINGLE format. + // TODO Remove deprecated ugly hook if ( !$this->hookRunner->onEmailUser( $toAddress, $fromAddress, $subject, $text, $error ) ) { if ( $error instanceof StatusValue ) { return $error; @@ -287,6 +291,20 @@ class EmailUser { } } + $hookStatus = StatusValue::newGood(); + $hookRes = $this->hookRunner->onEmailUserSendEmail( + $this->sender, + $fromAddress, + $target, + $toAddress, + $subject, + $text, + $hookStatus + ); + if ( !$hookRes && !$hookStatus->isGood() ) { + return $hookStatus; + } + [ $mailFrom, $replyTo ] = $this->getFromAndReplyTo( $fromAddress, $messageLocalizer ); $status = $this->emailer->send( diff --git a/includes/mail/Hook/EmailUserAuthorizeSendHook.php b/includes/mail/Hook/EmailUserAuthorizeSendHook.php new file mode 100644 index 00000000000..fa84b249abf --- /dev/null +++ b/includes/mail/Hook/EmailUserAuthorizeSendHook.php @@ -0,0 +1,26 @@ +getEmailUserFactory()->newEmailUserBC( $user, $config ); - $status = $emailUser->getPermissionsError( (string)$editToken ); + $status = $emailUser->authorizeSend( (string)$editToken ); if ( $status->isGood() ) { return null; } @@ -360,7 +360,7 @@ class SpecialEmailUser extends UnlistedSpecialPage { if ( !$target instanceof User ) { return Status::newFatal( 'emailnotarget' ); } - $res = $this->emailUserFactory->newEmailUser( $this->getAuthority() )->submit( + $res = $this->emailUserFactory->newEmailUser( $this->getAuthority() )->sendEmailUnsafe( $target, $data['Subject'], $data['Text'], @@ -395,7 +395,7 @@ class SpecialEmailUser extends UnlistedSpecialPage { $emailUser = MediaWikiServices::getInstance()->getEmailUserFactory() ->newEmailUserBC( $context->getAuthority(), $context->getConfig() ); - $ret = $emailUser->submit( + $ret = $emailUser->sendEmailUnsafe( $target, (string)$data['Subject'], (string)$data['Text'], diff --git a/includes/user/Hook/UserCanSendEmailHook.php b/includes/user/Hook/UserCanSendEmailHook.php index abe213e851a..4a558cc5f51 100644 --- a/includes/user/Hook/UserCanSendEmailHook.php +++ b/includes/user/Hook/UserCanSendEmailHook.php @@ -8,8 +8,8 @@ use User; * This is a hook handler interface, see docs/Hooks.md. * Use the hook name "UserCanSendEmail" to register handlers implementing this interface. * - * @stable to implement * @ingroup Hooks + * @deprecated since 1.41, handle the EmailUserAuthorizeSend hook instead. */ interface UserCanSendEmailHook { /** diff --git a/includes/user/User.php b/includes/user/User.php index 003442e480c..a98d36574f6 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -3202,7 +3202,7 @@ class User implements Authority, UserIdentity, UserEmailContact { $permError = MediaWikiServices::getInstance()->getEmailUserFactory() ->newEmailUser( $this->getThisAsAuthority() ) // XXX Pass an empty edit token, nobody is using it anyway. - ->getPermissionsError( '' ); + ->authorizeSend( '' ); return $permError->isGood(); } diff --git a/tests/phpunit/unit/includes/mail/EmailUserTest.php b/tests/phpunit/unit/includes/mail/EmailUserTest.php index 645ccfb4bf9..16eac4fe352 100644 --- a/tests/phpunit/unit/includes/mail/EmailUserTest.php +++ b/tests/phpunit/unit/includes/mail/EmailUserTest.php @@ -155,7 +155,7 @@ class EmailUserTest extends MediaWikiUnitTestCase { } /** - * @covers ::getPermissionsError + * @covers ::authorizeSend * @dataProvider providePermissionsError */ public function testGetPermissionsError( @@ -171,7 +171,7 @@ class EmailUserTest extends MediaWikiUnitTestCase { $this->expectDeprecationAndContinue( '/Use of EmailUserPermissionsErrors hook/' ); } $emailUser = $this->getEmailUser( $performerUser, null, null, $userFactory, $configOverrides, $hooks ); - $this->assertEquals( $expected, $emailUser->getPermissionsError( 'some-token' ) ); + $this->assertEquals( $expected, $emailUser->authorizeSend( 'some-token' ) ); } public function providePermissionsError(): Generator { @@ -269,11 +269,26 @@ class EmailUserTest extends MediaWikiUnitTestCase { true ]; + $emailUserAuthorizeSendError = 'my-hook-error'; + $emailUserAuthorizeSendHooks = [ + 'EmailUserAuthorizeSend' => + static function ( $user, StatusValue $status ) use ( $emailUserAuthorizeSendError ) { + $status->fatal( $emailUserAuthorizeSendError ); + return false; + } + ]; + yield 'EmailUserAuthorizeSend hook error' => [ + $validSender, + StatusValue::newFatal( $emailUserAuthorizeSendError ), + [], + $emailUserAuthorizeSendHooks + ]; + yield 'Successful' => [ $validSender, StatusValue::newGood() ]; } /** - * @covers ::submit + * @covers ::sendEmailUnsafe * @dataProvider provideSubmit */ public function testSubmit( @@ -288,7 +303,7 @@ class EmailUserTest extends MediaWikiUnitTestCase { ->with( $target ) ->willReturn( $target ); $emailUser = $this->getEmailUser( $sender, null, null, $userFactory, [], $hooks, $emailer ); - $res = $emailUser->submit( + $res = $emailUser->sendEmailUnsafe( $target, 'subject', 'text', @@ -375,6 +390,22 @@ class EmailUserTest extends MediaWikiUnitTestCase { $emailUserHookUsingMessageHooks ]; + $emailUserSendEmailHookError = 'some-pretty-hook-error'; + $emailUserHookUsingStatusHooks = [ + 'EmailUserSendEmail' => static function ( $a, $b, $c, $d, $e, $f, StatusValue $status ) + use ( $emailUserSendEmailHookError ) + { + $status->fatal( $emailUserSendEmailHookError ); + return false; + } + ]; + yield 'EmailUserSendEmail error as a Status' => [ + $validTarget, + $validSender, + StatusValue::newFatal( $emailUserSendEmailHookError ), + $emailUserHookUsingStatusHooks + ]; + $emailerErrorStatus = StatusValue::newFatal( 'emailer-error' ); $errorEmailer = $this->createMock( IEmailer::class ); $errorEmailer->expects( $this->atLeastOnce() )->method( 'send' )->willReturn( $emailerErrorStatus ); @@ -399,7 +430,7 @@ class EmailUserTest extends MediaWikiUnitTestCase { } /** - * @covers ::submit + * @covers ::sendEmailUnsafe */ public function testSubmit__rateLimited() { $senderUser = $this->createMock( User::class ); @@ -410,7 +441,7 @@ class EmailUserTest extends MediaWikiUnitTestCase { $this->expectException( RuntimeException::class ); $this->expectExceptionMessage( 'You are throttled' ); - $res = $this->getEmailUser( $senderUser, null, null, $senderUserFactory )->submit( + $res = $this->getEmailUser( $senderUser, null, null, $senderUserFactory )->sendEmailUnsafe( $this->getValidTarget(), 'subject', 'text',