From 3d74af3e963bdcbbdbd58e65a5efdd7819db5236 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Tue, 17 Jul 2018 16:18:59 -0400
Subject: [PATCH] AuthManager: Don't invalidate BotPasswords if a password
 reset email is sent

There's a difference between addition of credentials, which doesn't
need to invaliate BotPasswords, and changing or removal of credentials,
which does.

It seems most straightforward for the caller of
AuthManager::changeAuthenticationData() to know which is intended, so
let's add a flag there.

Bug: T199809
Change-Id: Ib8405734e605b94f3f0b66596ad95784cb365e4f
---
 includes/auth/AuthManager.php                            | 9 +++++++--
 .../auth/ConfirmLinkSecondaryAuthenticationProvider.php  | 4 +++-
 includes/user/PasswordReset.php                          | 4 +++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/includes/auth/AuthManager.php b/includes/auth/AuthManager.php
index 611a8cdc3a..f54224b8b7 100644
--- a/includes/auth/AuthManager.php
+++ b/includes/auth/AuthManager.php
@@ -882,8 +882,11 @@ class AuthManager implements LoggerAwareInterface {
 	 * returned success.
 	 *
 	 * @param AuthenticationRequest $req
+	 * @param bool $isAddition Set true if this represents an addition of
+	 *  credentials rather than a change. The main difference is that additions
+	 *  should not invalidate BotPasswords. If you're not sure, leave it false.
 	 */
-	public function changeAuthenticationData( AuthenticationRequest $req ) {
+	public function changeAuthenticationData( AuthenticationRequest $req, $isAddition = false ) {
 		$this->logger->info( 'Changing authentication data for {user} class {what}', [
 			'user' => is_string( $req->username ) ? $req->username : '<no name>',
 			'what' => get_class( $req ),
@@ -893,7 +896,9 @@ class AuthManager implements LoggerAwareInterface {
 
 		// When the main account's authentication data is changed, invalidate
 		// all BotPasswords too.
-		\BotPassword::invalidateAllPasswordsForUser( $req->username );
+		if ( !$isAddition ) {
+			\BotPassword::invalidateAllPasswordsForUser( $req->username );
+		}
 	}
 
 	/**@}*/
diff --git a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
index 7f121cdef1..c2d730cbbd 100644
--- a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
+++ b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
@@ -119,7 +119,9 @@ class ConfirmLinkSecondaryAuthenticationProvider extends AbstractSecondaryAuthen
 				$status = $this->manager->allowsAuthenticationDataChange( $req );
 				$statuses[] = [ $req, $status ];
 				if ( $status->isGood() ) {
-					$this->manager->changeAuthenticationData( $req );
+					// We're not changing credentials, just adding a new link
+					// to an already-known user.
+					$this->manager->changeAuthenticationData( $req, /* $isAddition */ true );
 				} else {
 					$anyFailed = true;
 				}
diff --git a/includes/user/PasswordReset.php b/includes/user/PasswordReset.php
index b5f7f6a513..a1638ea13e 100644
--- a/includes/user/PasswordReset.php
+++ b/includes/user/PasswordReset.php
@@ -238,7 +238,9 @@ class PasswordReset implements LoggerAwareInterface {
 		}
 
 		foreach ( $reqs as $req ) {
-			$this->authManager->changeAuthenticationData( $req );
+			// This is adding a new temporary password, not intentionally changing anything
+			// (even though it might technically invalidate an old temporary password).
+			$this->authManager->changeAuthenticationData( $req, /* $isAddition */ true );
 		}
 
 		$this->logger->info(
-- 
2.20.1