Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

mail: Round 4 of EmailUser refactoring

Browse source 
- Remove getTarget(). Converting a username to a user object is not a
  responsibility of this command. This logic generally belongs to
  callers.
- Use Authority for the sender, as we need to perform permission checks
  on them. Internally, we're still converting to User.

Bug: T265541
Change-Id: Ib0cd86d97dbf1cd97dcb316480171e4fbf4a09f4
This commit is contained in:
Daimona Eaytoy 2023-03-14 19:54:40 +01:00 committed by Jforrester
parent 2762ac1d5a
commit 4634d4545e
4 changed files with 77 additions and 140 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

43
includes/mail/EmailUser.php
View file

@ -30,6 +30,7 @@ use MediaWiki\HookContainer\HookRunner;
use MediaWiki\Language\RawMessage;
use MediaWiki\MainConfigNames;
use MediaWiki\MediaWikiServices;
use MediaWiki\Permissions\Authority;
use MediaWiki\Permissions\PermissionManager;
use MediaWiki\Preferences\MultiUsernameFilter;
use MediaWiki\User\UserFactory;
@ -109,33 +110,14 @@ class EmailUser {
$this->emailer = $emailer;
}
/**
* Validate target User
*
* @param string $target Target user name
* @param User $sender User sending the email
* @return StatusValue With errors, or with a User object as value on success.
*/
public function getTarget( string $target, User $sender ): StatusValue {
$nu = $this->userFactory->newFromName( $target );
if ( !$nu instanceof User ) {
return StatusValue::newFatal( 'emailnotarget' );
}
$status = $this->validateTarget( $nu, $sender );
if ( !$status->isGood() ) {
return $status;
}
return StatusValue::newGood( $nu );
}
/**
* Validate target User
*
* @param User $target Target user
* @param User $sender User sending the email
* @param Authority $sender User sending the email
* @return StatusValue
*/
public function validateTarget( User $target, User $sender ): StatusValue {
public function validateTarget( User $target, Authority $sender ): StatusValue {
if ( !$target->getId() ) {
return StatusValue::newFatal( 'emailnotarget' );
}
@ -148,6 +130,7 @@ class EmailUser {
return StatusValue::newFatal( 'nowikiemailtext' );
}
$sender = $this->userFactory->newFromAuthority( $sender );
if ( !$this->userOptionsLookup->getOption( $target, 'email-allow-new-users' ) && $sender->isNewbie() ) {
return StatusValue::newFatal( 'nowikiemailtext' );
}
@ -192,12 +175,12 @@ class EmailUser {
/**
* Check whether a user is allowed to send email
*
* @param User $user
* @param Authority $performer
* @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( User $user, string $editToken ): StatusValue {
public function getPermissionsError( Authority $performer, string $editToken ): StatusValue {
if (
!$this->options->get( MainConfigNames::EnableEmail ) ||
!$this->options->get( MainConfigNames::EnableUserEmail )
@ -205,6 +188,8 @@ class EmailUser {
return StatusValue::newFatal( 'usermaildisabled' );
}
$user = $this->userFactory->newFromAuthority( $performer );
// Run this before checking 'sendemail' permission
// to show appropriate message to anons (T160309)
if ( !$user->isEmailConfirmed() ) {
@ -246,27 +231,27 @@ class EmailUser {
* getPermissionsError(). It is probably also a good
* idea to check the edit token and ping limiter in advance.
*
* @param string $targetName
* @param User $target
* @param string $subject
* @param string $text
* @param bool $CCMe
* @param User $sender
* @param Authority $sender
* @param MessageLocalizer $messageLocalizer
* @return StatusValue
*/
public function submit(
string $targetName,
User $target,
string $subject,
string $text,
bool $CCMe,
User $sender,
Authority $sender,
MessageLocalizer $messageLocalizer
): StatusValue {
$targetStatus = $this->getTarget( $targetName, $sender );
$sender = $this->userFactory->newFromAuthority( $sender );
$targetStatus = $this->validateTarget( $target, $sender );
if ( !$targetStatus->isGood() ) {
return $targetStatus;
}
$target = $targetStatus->getValue();
$toAddress = MailAddress::newFromUser( $target );
$fromAddress = MailAddress::newFromUser( $sender );

1
includes/specialpage/SpecialPageFactory.php
View file

@ -978,6 +978,7 @@ class SpecialPageFactory {
'UserNamePrefixSearch',
'UserOptionsLookup',
'EmailUser',
'UserFactory',
]
],
'Movepage' => [

35
includes/specials/SpecialEmailUser.php
View file

@ -29,6 +29,7 @@ use HTMLForm;
use IContextSource;
use MediaWiki\Mail\EmailUser;
use MediaWiki\MediaWikiServices;
use MediaWiki\User\UserFactory;
use MediaWiki\User\UserNamePrefixSearch;
use MediaWiki\User\UserNameUtils;
use MediaWiki\User\UserOptionsLookup;
@ -64,23 +65,29 @@ class SpecialEmailUser extends UnlistedSpecialPage {
/** @var EmailUser */
private EmailUser $emailUser;
/** @var UserFactory */
private UserFactory $userFactory;
/**
* @param UserNameUtils $userNameUtils
* @param UserNamePrefixSearch $userNamePrefixSearch
* @param UserOptionsLookup $userOptionsLookup
* @param EmailUser $emailUser
* @param UserFactory $userFactory
*/
public function __construct(
UserNameUtils $userNameUtils,
UserNamePrefixSearch $userNamePrefixSearch,
UserOptionsLookup $userOptionsLookup,
EmailUser $emailUser
EmailUser $emailUser,
UserFactory $userFactory
) {
parent::__construct( 'Emailuser' );
$this->userNameUtils = $userNameUtils;
$this->userNamePrefixSearch = $userNamePrefixSearch;
$this->userOptionsLookup = $userOptionsLookup;
$this->emailUser = $emailUser;
$this->userFactory = $userFactory;
}
public function doesWrites() {
@ -207,12 +214,17 @@ class SpecialEmailUser extends UnlistedSpecialPage {
* @return User|string User object on success or a string on error
*/
public static function getTarget( $target, User $sender ) {
$status = MediaWikiServices::getInstance()->getEmailUser()->getTarget( (string)$target, $sender );
$targetObject = MediaWikiServices::getInstance()->getUserFactory()->newFromName( $target );
if ( !$targetObject instanceof User ) {
return 'notarget';
}
$status = MediaWikiServices::getInstance()->getEmailUser()->validateTarget( $targetObject, $sender );
if ( !$status->isGood() ) {
$msg = $status->getErrors()[0]['message'];
$ret = $msg === 'emailnotarget' ? 'notarget' : preg_replace( '/text$/', '', $msg );
} else {
$ret = $status->getValue();
$ret = $targetObject;
}
return $ret;
}
@ -347,12 +359,16 @@ class SpecialEmailUser extends UnlistedSpecialPage {
* @return Status|false
*/
public function onFormSubmit( array $data ) {
$target = $this->userFactory->newFromName( $data['Target'] );
if ( !$target instanceof User ) {
return Status::newFatal( 'emailnotarget' );
}
$res = $this->emailUser->submit(
$data['Target'],
$target,
$data['Subject'],
$data['Text'],
$data['CCMe'],
$this->getUser(),
$this->getAuthority(),
$this
);
if ( $res->hasMessage( 'hookaborted' ) ) {
@ -375,15 +391,20 @@ class SpecialEmailUser extends UnlistedSpecialPage {
* @return Status|false
*/
public static function submit( array $data, IContextSource $context ) {
$target = MediaWikiServices::getInstance()->getUserFactory()->newFromName( (string)$data['Target'] );
if ( !$target instanceof User ) {
return Status::newFatal( 'emailnotarget' );
}
$emailUser = MediaWikiServices::getInstance()->getEmailUser();
try {
$emailUser->overrideOptionsFromConfig( $context->getConfig() );
$ret = $emailUser->submit(
(string)$data['Target'],
$target,
(string)$data['Subject'],
(string)$data['Text'],
(bool)$data['CCMe'],
$context->getUser(),
$context->getAuthority(),
$context
);
if ( $ret->hasMessage( 'hookaborted' ) ) {

138
tests/phpunit/unit/includes/mail/EmailUserTest.php
View file

@ -9,6 +9,7 @@ use MediaWiki\Language\RawMessage;
use MediaWiki\Mail\EmailUser;
use MediaWiki\Mail\IEmailer;
use MediaWiki\MainConfigNames;
use MediaWiki\Permissions\Authority;
use MediaWiki\Permissions\PermissionManager;
use MediaWiki\User\UserFactory;
use MediaWiki\User\UserOptionsLookup;
@ -55,19 +56,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
);
}
private function getValidTargetUserFactory( string $targetName ): UserFactory {
$validTarget = $this->createMock( User::class );
$validTarget->method( 'getId' )->willReturn( 1 );
$validTarget->method( 'isEmailConfirmed' )->willReturn( true );
$validTarget->method( 'canReceiveEmail' )->willReturn( true );
$validTargetUserFactory = $this->createMock( UserFactory::class );
$validTargetUserFactory->expects( $this->atLeastOnce() )
->method( 'newFromName' )
->with( $targetName )
->willReturn( $validTarget );
return $validTargetUserFactory;
}
/**
* Returns a valid MessageLocalizer mock. We don't care about MessageLocalizer at all, but since the return value
* of ::msg() is not typehinted, we're forced to specify a mocked Message to return so that chained method calls
@ -80,62 +68,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
return $messageLocalizer;
}
/**
* @param mixed $target
* @param User $sender
* @param StatusValue|null $expected StatusValue object to compare for errors, or null to indicate that we're
* expecting a good StatusValue with a User object as value.
* @param UserFactory|null $userFactory
* @covers ::getTarget
* @dataProvider provideGetTarget
*/
public function testGetTarget( $target, User $sender, ?StatusValue $expected, UserFactory $userFactory = null ) {
$emailUser = $this->getEmailUser( null, null, $userFactory );
$actualStatus = $emailUser->getTarget( $target, $sender );
if ( $expected === null ) {
$this->assertStatusGood( $actualStatus );
$this->assertInstanceOf( User::class, $actualStatus->getValue() );
} else {
$this->assertEquals( $expected, $actualStatus );
}
}
public function provideGetTarget(): Generator {
$targetName = 'John Doe';
$noopSender = $this->createMock( User::class );
$invalidUsername = '123<>|[]';
$invalidUsernameUserFactory = $this->createMock( UserFactory::class );
$invalidUsernameUserFactory->expects( $this->atLeastOnce() )
->method( 'newFromName' )
->with( $invalidUsername )
->willReturn( null );
yield 'Invalid username' => [
$invalidUsername,
$noopSender,
StatusValue::newFatal( 'emailnotarget' ),
$invalidUsernameUserFactory
];
$noEmailTarget = $this->createMock( User::class );
$noEmailTarget->method( 'getId' )->willReturn( 1 );
$noEmailTarget->method( 'isEmailConfirmed' )->willReturn( false );
$noEmailTargetUserFactory = $this->createMock( UserFactory::class );
$noEmailTargetUserFactory->expects( $this->atLeastOnce() )
->method( 'newFromName' )
->with( $targetName )
->willReturn( $noEmailTarget );
yield 'Invalid target' => [
$targetName,
$noopSender,
StatusValue::newFatal( 'noemailtext' ),
$noEmailTargetUserFactory
];
$validTargetUserFactory = $this->getValidTargetUserFactory( $targetName );
yield 'Valid target' => [ $targetName, $noopSender, null, $validTargetUserFactory ];
}
/**
* @covers ::validateTarget
* @dataProvider provideValidateTarget
@ -147,16 +79,15 @@ class EmailUserTest extends MediaWikiUnitTestCase {
UserOptionsLookup $userOptionsLookup = null,
CentralIdLookup $centralIdLookup = null
) {
$emailUser = $this->getEmailUser( $userOptionsLookup, $centralIdLookup );
$userFactory = $this->createMock( UserFactory::class );
$userFactory->method( 'newFromAuthority' )->willReturn( $sender );
$emailUser = $this->getEmailUser( $userOptionsLookup, $centralIdLookup, $userFactory );
$this->assertEquals( $expected, $emailUser->validateTarget( $target, $sender ) );
}
public function provideValidateTarget(): Generator {
$noopUserMock = $this->createMock( User::class );
$validTarget = $this->createMock( User::class );
$validTarget->method( 'getId' )->willReturn( 1 );
$validTarget->method( 'isEmailConfirmed' )->willReturn( true );
$validTarget->method( 'canReceiveEmail' )->willReturn( true );
$validTarget = $this->getValidTarget();
$anonTarget = $this->createMock( User::class );
$anonTarget->expects( $this->atLeastOnce() )->method( 'getId' )->willReturn( 0 );
@ -223,14 +154,16 @@ class EmailUserTest extends MediaWikiUnitTestCase {
* @dataProvider providePermissionsError
*/
public function testGetPermissionsError(
User $user,
User $performerUser,
StatusValue $expected,
PermissionManager $permissionManager = null,
array $configOverrides = [],
array $hooks = []
) {
$emailUser = $this->getEmailUser( null, null, null, $permissionManager, $configOverrides, $hooks );
$this->assertEquals( $expected, $emailUser->getPermissionsError( $user, 'some-token' ) );
$userFactory = $this->createMock( UserFactory::class );
$userFactory->method( 'newFromAuthority' )->willReturn( $performerUser );
$emailUser = $this->getEmailUser( null, null, $userFactory, $permissionManager, $configOverrides, $hooks );
$this->assertEquals( $expected, $emailUser->getPermissionsError( $performerUser, 'some-token' ) );
}
public function providePermissionsError(): Generator {
@ -336,16 +269,15 @@ class EmailUserTest extends MediaWikiUnitTestCase {
* @dataProvider provideSubmit
*/
public function testSubmit(
string $targetName,
User $sender,
User $target,
Authority $sender,
$expected,
UserFactory $userFactory = null,
array $hooks = [],
IEmailer $emailer = null
) {
$emailUser = $this->getEmailUser( null, null, $userFactory, null, [], $hooks, $emailer );
$emailUser = $this->getEmailUser( null, null, null, null, [], $hooks, $emailer );
$res = $emailUser->submit(
$targetName,
$target,
'subject',
'text',
false,
@ -360,11 +292,12 @@ class EmailUserTest extends MediaWikiUnitTestCase {
}
public function provideSubmit(): Generator {
$validSender = $this->createMock( User::class );
$validTarget = 'John Doe';
$validTargetUserFactory = $this->getValidTargetUserFactory( $validTarget );
$validSender = $this->createMock( Authority::class );
$validTarget = $this->getValidTarget();
yield 'Invalid target' => [ '', $validSender, StatusValue::newFatal( 'emailnotarget' ) ];
$invalidTarget = $this->createMock( User::class );
$invalidTarget->method( 'getId' )->willReturn( 0 );
yield 'Invalid target' => [ $invalidTarget, $validSender, StatusValue::newFatal( 'emailnotarget' ) ];
$hookStatusError = StatusValue::newFatal( 'some-hook-error' );
$emailUserHookUsingStatusHooks = [
@ -377,7 +310,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
$hookStatusError,
$validTargetUserFactory,
$emailUserHookUsingStatusHooks
];
@ -391,7 +323,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
StatusValue::newFatal( 'hookaborted' ),
$validTargetUserFactory,
$emailUserHookUsingBooleanFalseHooks
];
@ -405,7 +336,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
StatusValue::newGood(),
$validTargetUserFactory,
$emailUserHookUsingBooleanTrueHooks
];
@ -420,7 +350,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
StatusValue::newFatal( $hookError ),
$validTargetUserFactory,
$emailUserHookUsingArrayHooks
];
@ -435,7 +364,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
StatusValue::newFatal( $hookErrorMsg ),
$validTargetUserFactory,
$emailUserHookUsingMessageHooks
];
@ -446,7 +374,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
$emailerErrorStatus,
$validTargetUserFactory,
[],
$errorEmailer
];
@ -458,7 +385,6 @@ class EmailUserTest extends MediaWikiUnitTestCase {
$validTarget,
$validSender,
$emailerSuccessStatus,
$validTargetUserFactory,
[],
$successEmailer
];
@ -468,27 +394,31 @@ class EmailUserTest extends MediaWikiUnitTestCase {
* @covers ::submit
*/
public function testSubmit__rateLimited() {
$sender = $this->createMock( User::class );
$sender->method( 'pingLimiter' )->with( 'sendemail' )->willReturn( true );
$validTarget = $this->createMock( User::class );
$validTarget->method( 'getId' )->willReturn( 1 );
$validTarget->method( 'isEmailConfirmed' )->willReturn( true );
$validTarget->method( 'canReceiveEmail' )->willReturn( true );
$validUserFactory = $this->createMock( UserFactory::class );
$validUserFactory->method( 'newFromName' )->willReturn( $validTarget );
$senderUser = $this->createMock( User::class );
$senderUser->method( 'pingLimiter' )->with( 'sendemail' )->willReturn( true );
$senderUserFactory = $this->createMock( UserFactory::class );
$senderUserFactory->method( 'newFromAuthority' )->willReturn( $senderUser );
$this->expectException( RuntimeException::class );
$this->expectExceptionMessage( 'You are throttled' );
$res = $this->getEmailUser( null, null, $validUserFactory )->submit(
'Some target',
$res = $this->getEmailUser( null, null, $senderUserFactory )->submit(
$this->getValidTarget(),
'subject',
'text',
false,
$sender,
$senderUser,
$this->getMockMessageLocalizer()
);
// This assertion should not be reached if the test passes, but it can be helpful to determine why
// the test is failing.
$this->assertStatusGood( $res );
}
private function getValidTarget(): User {
$validTarget = $this->createMock( User::class );
$validTarget->method( 'getId' )->willReturn( 1 );
$validTarget->method( 'isEmailConfirmed' )->willReturn( true );
$validTarget->method( 'canReceiveEmail' )->willReturn( true );
return $validTarget;
}
}