Doc review and general cleanup

* Remove notification attributes such as title-message, unused since the
  initial commit since it used the new (2015) formatter system.
* isKnownSystemSlow() is always called with a third parameter, and it
  doesn't seem to be nullable in callers.
* Yes, most of the things make sense.
* Add reason why CheckUser has no cuc_ip_hex index.
* Use foreach
* Too late to truncate the hash now

Change-Id: I310bc53ba881842845b9358309954f89c355f81c

includes/EchoHooks.php

@@ -47,14 +47,6 @@ class EchoHooks implements
 			'category' => 'login-fail',
 			'group' => 'negative',
 			'presentation-model' => PresentationModel::class,
-			// fixme, what does this actually do?
-			'title-message' => 'loginnotify-login-fail',
-			'title-params' => [],
-			// FIXME Should count be a parameter
-			'email-subject-params' => [ 'agent', 'count' ],
-			'email-body-batch-params' => [ 'agent', 'count' ],
-			// FIXME is it ok not to set batch email messages, since
-			// we have immediate flag?
 			'icon' => 'LoginNotify-user-avatar',
 			'immediate' => true,
 		];

includes/Hooks.php

@@ -1,7 +1,5 @@
 <?php
 /**
- * Body of LoginNotify extension
- *
  * @file
  * @ingroup Extensions
  */

includes/LoginNotify.php

@@ -101,6 +101,11 @@ class LoginNotify implements LoggerAwareInterface {
 	/**
 	 * Get just network part of an IP (assuming /24 or /64)
 	 *
+	 * It would be nice if we could use IPUtils::getSubnet(), which also gets
+	 * the /24 or /64 network in support of a similar use case, but its
+	 * behaviour is broken for IPv6 addresses, returning the hex range start
+	 * rather than the prefix. (T344963)
+	 *
 	 * @param string $ip Either IPv4 or IPv6 address
 	 * @return string Just the network part (e.g. 127.0.0.)
 	 * @throws UnexpectedValueException If given something not an IP
@@ -168,11 +173,11 @@ class LoginNotify implements LoggerAwareInterface {
 	 *
 	 * @param User $user User in question
 	 * @param string $subnet User's current subnet
-	 * @param string|null $resultSoFar Value returned by isKnownSystemFast() or null if
+	 * @param string $resultSoFar Value returned by isKnownSystemFast() or null if
 	 *        not available.
 	 * @return bool true if the user has used this computer before
 	 */
-	private function isKnownSystemSlow( User $user, $subnet, $resultSoFar = null ) {
+	private function isKnownSystemSlow( User $user, $subnet, $resultSoFar ) {
 		$result = $this->checkUserAllWikis( $user, $subnet );
 
 		$this->log->debug( 'Checking user {user} from {subnet} (result so far: {soFar}): {result}',
@@ -185,24 +190,23 @@ class LoginNotify implements LoggerAwareInterface {
 			]
 		);
 
-		if ( $resultSoFar !== null ) {
-			$result = $this->mergeResults( $result, $resultSoFar );
-		}
+		$result = $this->mergeResults( $result, $resultSoFar );
 
-		// If we have no check user data for the user, and there was
-		// no cookie supplied, just pass the user in, since we don't have
-		// enough info to determine if from known ip.
-		// FIXME: Does this make sense
+		// If we have no CheckUser data for the user, and there was no cookie
+		// supplied, then treat the computer as known.
 		if ( $result === self::USER_NO_INFO ) {
 			// We have to be careful here. Whether $cookieResult is
 			// self::USER_NO_INFO, is under control of the attacker.
 			// If checking CheckUser is disabled, then we should not
 			// hit this branch.
 
-			$this->log->info( "Assuming {user} is from known IP since no info available", [
-				'method' => __METHOD__,
-				'user' => $user->getName()
-			] );
+			$this->log->info(
+				"Assuming the user {user} is from a known IP since no info is available",
+				[
+					'method' => __METHOD__,
+					'user' => $user->getName()
+				]
+			);
 			return true;
 		}
 
@@ -227,7 +231,7 @@ class LoginNotify implements LoggerAwareInterface {
 	}
 
 	/**
-	 * Is the subnet of the current IP in the check user data for the user.
+	 * Is the subnet of the current IP in the CheckUser data for the user.
 	 *
 	 * If CentralAuth is installed, this will check not only the current wiki,
 	 * but also the ten wikis where user has most edits on.
@@ -242,7 +246,7 @@ class LoginNotify implements LoggerAwareInterface {
 		if ( !$this->config->get( 'LoginNotifyCheckKnownIPs' )
 			|| !$this->isCheckUserInstalled()
 		) {
-			// Checkuser checks disabled.
+			// CheckUser checks disabled.
 			// Note: It's important this be USER_NOT_KNOWN and not USER_NO_INFO.
 			return self::USER_NOT_KNOWN;
 		}
@@ -265,11 +269,9 @@ class LoginNotify implements LoggerAwareInterface {
 		if ( ExtensionRegistry::getInstance()->isLoaded( 'CentralAuth' ) ) {
 			$globalUser = CentralAuthUser::getInstance( $user );
 			if ( $globalUser->exists() ) {
-				// This is expensive. However, On WMF wikis, probably
-				// already done as part of password complexity check, and
-				// will be cached.
+				// This is expensive, up to ~5 seconds (T167731)
 				$info = $globalUser->queryAttached();
-				// already checked the local wiki.
+				// Already checked the local wiki.
 				unset( $info[WikiMap::getCurrentWikiId()] );
 				usort( $info,
 					static function ( $a, $b ) {
@@ -293,11 +295,9 @@ class LoginNotify implements LoggerAwareInterface {
 					$dbrLocal = $lb->getMaintenanceConnectionRef( DB_REPLICA, [], $wiki );
 
 					if ( !$this->hasCheckUserTables( $dbrLocal ) ) {
-						// Skip this wiki, no checkuser table.
+						// Skip this wiki, no CheckUser table.
 						continue;
 					}
-					// FIXME The case where there are no CU entries for
-					// this user.
 					$res = $this->checkUserOneWiki(
 						$localInfo['id'],
 						$subnet,
@@ -320,21 +320,20 @@ class LoginNotify implements LoggerAwareInterface {
 	}
 
 	/**
-	 * Actually do the query of the check user table.
+	 * Actually do the query of the CheckUser table.
 	 *
 	 * @note This catches and ignores database errors.
-	 * @param int $userId User id number (Not necessarily for the local wiki)
+	 * @param int $userId User ID number (Not necessarily for the local wiki)
 	 * @param string $ipFragment Prefix to match against cuc_ip (from $this->getIPNetwork())
 	 * @param IDatabase $dbr A database connection (possibly foreign)
 	 * @return string One of USER_* constants
 	 */
 	private function checkUserOneWiki( $userId, $ipFragment, IDatabase $dbr ) {
-		// TODO: Check wether this is still the case post-actor migration.
-		// For some unknown reason, the index is on
-		// (cuc_actor, cuc_ip, cuc_timestamp), instead of
-		// cuc_ip_hex which would be ideal.
-		// user-agent might also be good to look at,
-		// but no index on that.
+		// The index is on (cuc_actor, cuc_ip, cuc_timestamp), instead of
+		// cuc_ip_hex which would be ideal, but CheckUser was not designed for
+		// this specific use case and we couldn't be bothered to update it.
+		// Although it would be 100x faster to use a single global summary
+		// table instead of connecting to the database of each wiki separately.
 		$IPHasBeenUsedBefore = $dbr->newSelectQueryBuilder()
 			->select( '1' )
 			->from( 'cu_changes' )
@@ -352,23 +351,16 @@ class LoginNotify implements LoggerAwareInterface {
 	}
 
 	/**
-	 * Check if we have any check user info for this user
+	 * Check if we have any CheckUser info for this user
 	 *
 	 * If we have no info for user, we maybe don't treat it as
 	 * an unknown IP, since user has no known IPs.
 	 *
-	 * @todo FIXME Does this behaviour make sense, esp. with cookie check?
 	 * @param int $userId User id number (possibly on foreign wiki)
 	 * @param IDatabase $dbr DB connection (possibly to foreign wiki)
 	 * @return bool
 	 */
 	private function userHasCheckUserData( $userId, IDatabase $dbr ) {
-		// Verify that we actually have IP info for
-		// this user.
-		// @todo: Should this instead be if we have a
-		// a certain number of checkuser entries for this
-		// user. Or maybe it should be if we have at least
-		// 2 different IPs for this user. Or something else.
 		$haveIPInfo = $dbr->newSelectQueryBuilder()
 			->select( '1' )
 			->from( 'cu_changes' )
@@ -381,14 +373,14 @@ class LoginNotify implements LoggerAwareInterface {
 	}
 
 	/**
-	 * Does this wiki have a checkuser table?
+	 * Does this wiki have a CheckUser table?
 	 *
 	 * @param IMaintainableDatabase $dbr Database to check
 	 * @return bool
 	 */
 	private function hasCheckUserTables( IMaintainableDatabase $dbr ) {
 		if ( !$dbr->tableExists( 'cu_changes', __METHOD__ ) ) {
-			$this->log->warning( "LoginNotify: No checkuser table on {wikiId}", [
+			$this->log->warning( "No CheckUser table on {wikiId}", [
 				'method' => __METHOD__,
 				'wikiId' => $dbr->getDomainID()
 			] );
@@ -454,8 +446,8 @@ class LoginNotify implements LoggerAwareInterface {
 	 */
 	private function cacheLoginIP( User $user ) {
 		// For simplicity, this only stores the last IP subnet used.
-		// Its assumed that most of the time, we'll be able to rely on
-		// the cookie or checkuser data.
+		// It's assumed that most of the time, we'll be able to rely on
+		// the cookie or CheckUser data.
 		$expiry = $this->config->get( 'LoginNotifyCacheLoginIPExpiry' );
 		if ( $expiry !== false ) {
 			$ipPrefix = $this->getIPNetwork( $user->getRequest()->getIP() );
@@ -491,7 +483,6 @@ class LoginNotify implements LoggerAwareInterface {
 	private function userIsInCookie( User $user, WebRequest $request ) {
 		$cookie = $this->getPrevLoginCookie( $request );
 
-		// FIXME, does this really make sense?
 		if ( $cookie === '' ) {
 			$result = self::USER_NO_INFO;
 		} else {
@@ -533,16 +524,15 @@ class LoginNotify implements LoggerAwareInterface {
 		$newCookie = $this->generateUserCookieRecord( $user->getName() );
 		$maxCookieRecords = $this->config->get( 'LoginNotifyMaxCookieRecords' );
 
-		$totalCookieRecord = count( $cookieRecords );
-		for ( $i = 0; $i < $totalCookieRecord; $i++ ) {
-			if ( !$this->validateCookieRecord( $cookieRecords[$i] ) ) {
+		foreach ( $cookieRecords as $i => $cookieRecord ) {
+			if ( !$this->validateCookieRecord( $cookieRecord ) ) {
 				// Skip invalid or old cookie records.
 				continue;
 			}
-			$curUser = $this->isUserRecordGivenCookie( $user, $cookieRecords[$i] );
+			$curUser = $this->isUserRecordGivenCookie( $user, $cookieRecord );
 			$userSeenBefore = $userSeenBefore || $curUser;
 			if ( $i < $maxCookieRecords && !$curUser ) {
-				$newCookie .= '.' . $cookieRecords[$i];
+				$newCookie .= '.' . $cookieRecord;
 			}
 		}
 		return [ $userSeenBefore, $newCookie ];
@@ -625,7 +615,7 @@ class LoginNotify implements LoggerAwareInterface {
 			$salt = $this->getSalt();
 		}
 
-		// FIXME Maybe shorten, e.g. User only half the hash?
+		// TODO: would be nice to truncate the hash, but we would need b/c
 		$res = hash_hmac( 'sha1', $username . '|' . $year . $salt, $this->secret );
 		if ( !is_string( $res ) ) {
 			throw new UnexpectedValueException( "Hash failed" );
@@ -726,9 +716,9 @@ class LoginNotify implements LoggerAwareInterface {
 	}
 
 	/**
-	 * Check if we've reached limit, and increment cache key.
+	 * Check if we've reached the limit, and increment the cache key.
 	 *
-	 * @param string $key cache key
+	 * @param string $key Cache key
 	 * @param int $interval The interval of one to send notice
 	 * @param int $expiry When to expire cache key.
 	 * @return false|int false to not send notice, or number of hits
@@ -845,7 +835,7 @@ class LoginNotify implements LoggerAwareInterface {
 	}
 
 	/**
-	 * Creates and enqueues a job to do asynchronous processing of user login success/failure
+	 * Create and enqueue a job to do asynchronous processing of user login success/failure
 	 *
 	 * @param string $type Job type, one of DeferredChecksJob::TYPE_* constants
 	 * @param User $user User in question