Don't notify of failed logins for system or temporary users

Bug: T329774
Change-Id: I65fa3da22f45002e013d3bd5c8b0efda5f8b5edb

includes/LoginNotify.php

@@ -15,6 +15,7 @@ use ExtensionRegistry;
 use IBufferingStatsdDataFactory;
 use JobQueueGroup;
 use JobSpecification;
+use MediaWiki\Auth\AuthManager;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\CentralAuth\User\CentralAuthUser;
 use MediaWiki\Extension\Notifications\Model\Event;
@@ -91,6 +92,8 @@ class LoginNotify implements LoggerAwareInterface {
 	private $jobQueueGroup;
 	/** @var CentralIdLookup */
 	private $centralIdLookup;
+	/** @var AuthManager */
+	private $authManager;
 	/** @var int|null */
 	private $fakeTime;
 
@@ -106,6 +109,7 @@ class LoginNotify implements LoggerAwareInterface {
 	 * @param LBFactory $lbFactory
 	 * @param JobQueueGroup $jobQueueGroup
 	 * @param CentralIdLookup $centralIdLookup
+	 * @param AuthManager $authManager
 	 */
 	public function __construct(
 		ServiceOptions $options,
@@ -114,7 +118,8 @@ class LoginNotify implements LoggerAwareInterface {
 		IBufferingStatsdDataFactory $stats,
 		LBFactory $lbFactory,
 		JobQueueGroup $jobQueueGroup,
-		CentralIdLookup $centralIdLookup
+		CentralIdLookup $centralIdLookup,
+		AuthManager $authManager
 	) {
 		$this->config = $options;
 		$this->cache = $cache;
@@ -130,6 +135,7 @@ class LoginNotify implements LoggerAwareInterface {
 		$this->lbFactory = $lbFactory;
 		$this->jobQueueGroup = $jobQueueGroup;
 		$this->centralIdLookup = $centralIdLookup;
+		$this->authManager = $authManager;
 	}
 
 	/**
@@ -1188,6 +1194,14 @@ class LoginNotify implements LoggerAwareInterface {
 			return;
 		}
 
+		// No need to notify if the user can't authenticate (e.g. system or temporary users)
+		if ( !$this->authManager->userCanAuthenticate( $user->getName() ) ) {
+			$this->log->debug( "Skipping recording failure for user {user} - can't authenticate",
+				[ 'user' => $user->getName() ]
+			);
+			return;
+		}
+
 		$known = $this->isKnownSystemFast( $user, $user->getRequest() );
 		if ( $known === self::USER_KNOWN ) {
 			$this->recordLoginFailureFromKnownSystem( $user );

includes/ServiceWiring.php

@@ -17,7 +17,8 @@ return [
 			$services->getStatsdDataFactory(),
 			$services->getDBLoadBalancerFactory(),
 			$services->getJobQueueGroup(),
-			$services->getCentralIdLookup()
+			$services->getCentralIdLookup(),
+			$services->getAuthManager()
 		);
 	}
 ];

tests/phpunit/LoginNotifyTest.php

@@ -62,7 +62,8 @@ class LoginNotifyTest extends MediaWikiIntegrationTestCase {
 						'LocalDatabases' => []
 					] ),
 					$services->getDBLoadBalancerFactory()
-				)
+				),
+				$services->getAuthManager()
 			)
 		);
 		$this->inst->setLogger( LoggerFactory::getInstance( 'LoginNotify' ) );
@@ -464,6 +465,24 @@ class LoginNotifyTest extends MediaWikiIntegrationTestCase {
 		$this->assertNotificationCount( $user, 'login-fail-known', 1 );
 	}
 
+	public function testRecordFailureNoPassword() {
+		// Don't notify temp users (T329774)
+		$this->setupRecordFailure( [
+			'LoginNotifyAttemptsKnownIP' => 1,
+			'LoginNotifyAttemptsNewIP' => 1,
+		] );
+		$user = $this->getTestUser()->getUser();
+		$this->getDb()->newUpdateQueryBuilder()
+			->update( 'user' )
+			->set( [ 'user_password' => '' ] )
+			->where( [ 'user_id' => $user->getId() ] )
+			->caller( __METHOD__ )
+			->execute();
+		$this->inst->recordFailure( $user );
+		$this->assertNotificationCount( $user, 'login-fail-known', 0 );
+		$this->assertNotificationCount( $user, 'login-fail-new', 0 );
+	}
+
 	public function testSendSuccessNoticeCheckUser() {
 		$this->setupRecordFailureWithCheckUser();
 		$helper = new TestRecentChangesHelper;