jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/360787 )
Change subject: Move expensive processing into job queue
......................................................................
Move expensive processing into job queue
Bug: T167731
Depends-On: I618840fafd22d9b6471eb470ef0414e354aa17f5
Change-Id: I1fcd15f523828141e8fadee9a8ad824eacefc0f9
---
M extension.json
A includes/DeferredChecksJob.php
M includes/Hooks.php
M includes/LoginNotify.php
4 files changed, 249 insertions(+), 75 deletions(-)
Approvals:
Niharika29: Looks good to me, approved
jenkins-bot: Verified
diff --git a/extension.json b/extension.json
index b7f69ba..84334b1 100644
--- a/extension.json
+++ b/extension.json
@@ -25,8 +25,9 @@
]
},
"AutoloadClasses": {
- "LoginNotify\\LoginNotify": "includes/LoginNotify.php",
+ "LoginNotify\\DeferredChecksJob":
"includes/DeferredChecksJob.php",
"LoginNotify\\Hooks": "includes/Hooks.php",
+ "LoginNotify\\LoginNotify": "includes/LoginNotify.php",
"LoginNotify\\PresentationModel":
"includes/PresentationModel.php"
},
"Hooks": {
@@ -37,7 +38,7 @@
"LoginNotify\\Hooks::onEchoGetBundleRules"
],
"LoginAuthenticateAudit": [
- "LoginNotifyHooks::onLoginAuthenticateAudit"
+ "LoginNotify\\Hooks::onLoginAuthenticateAudit"
],
"AuthManagerLoginAuthenticateAudit": [
"LoginNotify\\Hooks::onAuthManagerLoginAuthenticateAudit"
@@ -55,6 +56,9 @@
"LoginNotify\\Hooks::onLocalUserCreated"
]
},
+ "JobClasses": {
+ "LoginNotifyChecks": "LoginNotify\\DeferredChecksJob"
+ },
"config": {
"@docLoginNotifyAttemptsKnownIP": "The number of failed login
attempts to permit from a known IP before a notification is triggered.",
"LoginNotifyAttemptsKnownIP": 5,
diff --git a/includes/DeferredChecksJob.php b/includes/DeferredChecksJob.php
new file mode 100644
index 0000000..05096eb
--- /dev/null
+++ b/includes/DeferredChecksJob.php
@@ -0,0 +1,67 @@
+<?php
+
+namespace LoginNotify;
+
+use Exception;
+use Job;
+use Title;
+use User;
+
+/**
+ * Class DeferredChecksJob
+ * @package LoginNotify
+ */
+class DeferredChecksJob extends Job {
+ const TYPE_LOGIN_FAILED = 'failed';
+ const TYPE_LOGIN_SUCCESS = 'success';
+
+ /**
+ * @param Title $title
+ * @param array|bool $params
+ */
+ public function __construct( Title $title, $params = false ) {
+ parent::__construct( 'LoginNotifyChecks', $title, $params );
+ }
+
+ /**
+ * Run the job
+ * @return bool Success
+ */
+ public function run() {
+ $checkType = $this->params['checkType'];
+ $userId = $this->params['userId'];
+ $user = User::newFromId( $userId );
+ if ( !$user ) {
+ throw new Exception( "Can't find user for user id=" .
print_r( $userId, true ) );
+ }
+ if ( !isset( $this->params['subnet'] ) || !is_string(
$this->params['subnet'] ) ) {
+ throw new Exception( __CLASS__
+ . " expected to receive a string parameter
'subnet', got "
+ . print_r( $this->params['subnet'], true )
+ );
+ }
+ $subnet = $this->params['resultSoFar'];
+ if ( !isset( $this->params['resultSoFar'] ) || !is_string(
$this->params['resultSoFar'] ) ) {
+ throw new Exception( __CLASS__
+ . " expected to receive a string parameter
'resultSoFar', got "
+ . print_r( $this->params['resultSoFar'], true )
+ );
+ }
+ $resultSoFar = $this->params['resultSoFar'];
+
+ $loginNotify = new LoginNotify();
+
+ switch ( $checkType ) {
+ case self::TYPE_LOGIN_FAILED:
+ $loginNotify->recordFailureDeferred( $user,
$subnet, $resultSoFar );
+ break;
+ case self::TYPE_LOGIN_SUCCESS:
+ $loginNotify->sendSuccessNoticeDeferred( $user,
$subnet, $resultSoFar );
+ break;
+ default:
+ throw new Exception( 'Unknown check type ' .
print_r( $checkType, true ) );
+ }
+
+ return true;
+ }
+}
diff --git a/includes/Hooks.php b/includes/Hooks.php
index 82a9cce..9685022 100644
--- a/includes/Hooks.php
+++ b/includes/Hooks.php
@@ -50,6 +50,14 @@
'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,
];
diff --git a/includes/LoginNotify.php b/includes/LoginNotify.php
index bb911e5..b0cd393 100644
--- a/includes/LoginNotify.php
+++ b/includes/LoginNotify.php
@@ -12,6 +12,8 @@
use CentralAuthUser;
use Config;
use EchoEvent;
+use JobQueueGroup;
+use JobSpecification;
use MediaWiki\MediaWikiServices;
use WebRequest;
use Wikimedia\Rdbms\Database;
@@ -34,7 +36,14 @@
class LoginNotify implements LoggerAwareInterface {
const COOKIE_NAME = 'loginnotify_prevlogins';
- const NO_INFO_AVAILABLE = 2;
+
+ // The following 3 constants specify outcomes of user search
+ /** User's system is known to us */
+ const USER_KNOWN = 'known';
+ /** User's system is new for us, based on our data */
+ const USER_NOT_KNOWN = 'not known';
+ /** We don't have data to confirm or deny this is a known system */
+ const USER_NO_INFO = 'no info';
/** @var BagOStuff */
private $cache;
@@ -111,39 +120,51 @@
}
/**
- * Is the current computer known to be used by the given user
+ * Is the current computer known to be used by the current user (fast
checks)
+ * To be used for checks that are fast enough to be run at the moment
the user logs in.
*
* @param User $user User in question
+ * @param WebRequest $request
+ * @return string One of USER_* constants
+ */
+ private function isKnownSystemFast( User $user, WebRequest $request ) {
+ $result = $this->userIsInCookie( $user, $request );
+ if ( $result === self::USER_KNOWN ) {
+ return $result;
+ }
+
+ $result = $this->mergeResults( $result, $this->userIsInCache(
$user, $request ) );
+
+ return $result;
+ }
+
+ /**
+ * Is the current computer known to be used by the current user (slow
checks)
+ * These checks are slow enough to be run via the job queue
+ *
+ * @param User $user User in question
+ * @param string $subnet User's current subnet
+ * @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 isFromKnownIP( User $user ) {
- $cookieResult = $this->isUserInCookie( $user );
- if ( $cookieResult === true ) {
- // User has cookie
+ private function isKnownSystemSlow( User $user, $subnet, $resultSoFar =
null ) {
+ $result = $this->checkUserAllWikis( $user, $subnet );
+ if ( $result === self::USER_KNOWN ) {
return true;
}
- $cacheResult = $this->isUserInCache( $user );
- if ( $cacheResult === true ) {
- return true;
- }
-
- $cuResult = $this->isUserInCheckUser( $user );
- if ( $cuResult === true ) {
- return true;
+ if ( $resultSoFar !== null ) {
+ $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 (
- $cuResult === self::NO_INFO_AVAILABLE &&
- $cookieResult === self::NO_INFO_AVAILABLE &&
- $cacheResult === self::NO_INFO_AVAILABLE
- ) {
+ if ( $result === self::USER_NO_INFO ) {
// We have to be careful here. Whether $cookieResult is
- // self::NO_INFO_AVAILABLE, is under control of the
attacker.
+ // self::USER_NO_INFO, is under control of the attacker.
// If checking CheckUser is disabled, then we should not
// hit this branch.
@@ -160,17 +181,18 @@
/**
* Check if we cached this user's ip address from last login.
*
- * @param User $user User in question.
- * @return Mixed true, false or self::NO_INFO_AVAILABLE.
+ * @param User $user User in question
+ * @param WebRequest $request
+ * @return string One of USER_* constants
*/
- private function isUserInCache( User $user ) {
- $ipPrefix = $this->getIPNetwork( $user->getRequest()->getIP() );
+ private function userIsInCache( User $user, WebRequest $request ) {
+ $ipPrefix = $this->getIPNetwork( $request->getIP() );
$key = $this->getKey( $user, 'prevSubnet' );
$res = $this->cache->get( $key );
if ( $res !== false ) {
- return $res === $ipPrefix;
+ return $res === $ipPrefix ? self::USER_KNOWN :
self::USER_NOT_KNOWN;
}
- return self::NO_INFO_AVAILABLE;
+ return self::USER_NO_INFO;
}
/**
@@ -179,29 +201,29 @@
* If CentralAuth is installed, this will check not only the current
wiki,
* but also the ten wikis where user has most edits on.
*
- * @param User $user User in question.
- * @return Mixed true, false or self::NO_INFO_AVAILABLE.
+ * @param User $user User in question
+ * @param string $subnet User's current subnet
+ * @return string One of USER_* constants
*/
- private function isUserInCheckUser( User $user ) {
+ private function checkUserAllWikis( User $user, $subnet ) {
if ( !$this->config->get( 'LoginNotifyCheckKnownIPs' )
|| !class_exists( 'CheckUser' )
) {
- // Check user checks disabled.
- // Note: Its important this be false and not
self::NO_INFO_AVAILABLE.
- return false;
+ // Checkuser checks disabled.
+ // Note: It's important this be USER_NOT_KNOWN and not
USER_NO_INFO.
+ return self::USER_NOT_KNOWN;
}
-
- $haveAnyInfo = false;
- $prefix = $this->getIPNetwork( $user->getRequest()->getIP() );
$dbr = wfGetDB( DB_SLAVE );
- $localResult = $this->isUserInCheckUserQuery( $user->getId(),
$prefix, $dbr );
- if ( $localResult ) {
- return true;
+ $result = $this->checkUserOneWiki( $user->getId(), $subnet,
$dbr );
+ if ( $result === self::USER_KNOWN ) {
+ return $result;
}
- if ( !$haveAnyInfo ) {
- $haveAnyInfo = $this->isUserInCheckUserAnyInfo(
$user->getId(), $dbr );
+ if ( $result === self::USER_NO_INFO
+ && $this->userHasCheckUserData( $user->getId(), $dbr )
+ ) {
+ $result = self::USER_NOT_KNOWN;
}
// Also check checkuser table on the top ten wikis where this
user has
@@ -244,28 +266,27 @@
}
// FIXME The case where there are no CU
entries for
// this user.
- $res = $this->isUserInCheckUserQuery(
+ $res = $this->checkUserOneWiki(
$localInfo['id'],
- $prefix,
+ $subnet,
$dbrLocal
);
if ( $res ) {
$lb->reuseConnection( $dbrLocal
);
- return true;
+ return self::USER_KNOWN;
}
- if ( !$haveAnyInfo ) {
- $haveAnyInfo =
$this->isUserInCheckUserAnyInfo( $user->getId(), $dbr );
+ if ( $result === self::USER_NO_INFO
+ &&
$this->userHasCheckUserData( $user->getId(), $dbr )
+ ) {
+ $result = self::USER_NOT_KNOWN;
}
$lb->reuseConnection( $dbrLocal );
$count++;
}
}
}
- if ( !$haveAnyInfo ) {
- return self::NO_INFO_AVAILABLE;
- }
- return false;
+ return $result;
}
/**
@@ -277,9 +298,9 @@
* @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 Database $dbr A database connection (possibly foreign)
- * @return bool If $ipFragment is in check user db
+ * @return string One of USER_* constants
*/
- private function isUserInCheckUserQuery( $userId, $ipFragment, Database
$dbr ) {
+ private function checkUserOneWiki( $userId, $ipFragment, Database $dbr
) {
// For some unknown reason, the index is on
// (cuc_user, cuc_ip, cuc_timestamp), instead of
// cuc_ip_hex which would be ideal.
@@ -297,7 +318,7 @@
],
__METHOD__
);
- return $IPHasBeenUsedBefore;
+ return $IPHasBeenUsedBefore ? self::USER_KNOWN :
self::USER_NO_INFO;
}
/**
@@ -311,9 +332,9 @@
* @todo FIXME Does this behaviour make sense, esp. with cookie check?
* @param int $userId User id number (possibly on foreign wiki)
* @param Database $dbr DB connection (possibly to foreign wiki)
- * @return bool|mixed
+ * @return bool
*/
- private function isUserInCheckUserAnyInfo( $userId, Database $dbr ) {
+ private function userHasCheckUserData( $userId, Database $dbr ) {
// Verify that we actually have IP info for
// this user.
// @todo: Should this instead be if we have a
@@ -402,19 +423,38 @@
}
/**
+ * Merges results of various isKnownSystem*() checks
+ *
+ * @param string $x One of USER_* constants
+ * @param string $y One of USER_* constants
+ * @return string
+ */
+ private function mergeResults( $x, $y ) {
+ if ( $x === self::USER_KNOWN || $y === self::USER_KNOWN ) {
+ return self::USER_KNOWN;
+ }
+ if ( $x === self::USER_NOT_KNOWN || $y === self::USER_NOT_KNOWN
) {
+ return self::USER_NOT_KNOWN;
+ }
+ return self::USER_NO_INFO;
+ }
+
+ /**
* Check if a certain user is in the cookie.
*
* @param User $user User in question
- * @return bool|integer Either true, false, or self::NO_INFO_AVAILABLE.
+ * @param WebRequest $request
+ * @return string One of USER_* constants
*/
- private function isUserInCookie( User $user ) {
- $cookie = $this->getPrevLoginCookie( $user->getRequest() );
+ private function userIsInCookie( User $user, WebRequest $request ) {
+ $cookie = $this->getPrevLoginCookie( $request );
+
+ // FIXME, does this really make sense?
if ( $cookie === '' ) {
- // FIXME, does this really make sense?
- return self::NO_INFO_AVAILABLE;
+ return self::USER_NO_INFO;
}
list( $userKnown, ) = $this->checkAndGenerateCookie( $user,
$cookie );
- return $userKnown;
+ return $userKnown ? self::USER_KNOWN : self::USER_NOT_KNOWN;
}
/**
@@ -529,7 +569,7 @@
* @param string $username Username,
* @param int|bool $year int [Optional] Year. Default to current year
* @param string|bool $salt [Optional] Salt (expected to be base-36
encoded)
- * @return String A record for the cookie
+ * @return string A record for the cookie
*/
private function generateUserCookieRecord( $username, $year = false,
$salt = false ) {
if ( $year === false ) {
@@ -570,7 +610,7 @@
*
* @param User $user
*/
- private function incNewIP( User $user ) {
+ private function recordLoginFailureFromUnknownSystem( User $user ) {
$key = $this->getKey( $user, 'new' );
$count = $this->checkAndIncKey(
$key,
@@ -582,14 +622,14 @@
}
}
- /*
+ /**
* Increment hit counters for a failed login from a known computer.
*
* If a sufficient number of hits have accumulated, send an echo notice.
*
* @param User $user
*/
- private function incKnownIP( User $user ) {
+ private function recordLoginFailureFromKnownSystem( User $user ) {
$key = $this->getKey( $user, 'known' );
$count = $this->checkAndIncKey(
$key,
@@ -661,27 +701,82 @@
/**
* On login failure, record failure and maybe send notice
*
- * @param User $user The user whose account was attempted to log into.
+ * @param User $user User in question
*/
public function recordFailure( User $user ) {
- $fromKnownIP = $this->isFromKnownIP( $user );
- if ( $fromKnownIP ) {
- $this->incKnownIP( $user );
+ $known = $this->isKnownSystemFast( $user, $user->getRequest() );
+ if ( $known === self::USER_KNOWN ) {
+ $this->recordLoginFailureFromKnownSystem( $user );
} else {
- $this->incNewIP( $user );
+ $this->createJob( DeferredChecksJob::TYPE_LOGIN_FAILED,
+ $user, $user->getRequest(), $known
+ );
}
}
/**
- * Send a notice on successful login from an unknown IP.
+ * Asynchronous part of recordFailure(), to be called from
DeferredChecksJob
+ *
+ * @param User $user User in question
+ * @param string $subnet User's current subnet
+ * @param string $resultSoFar Value returned by isKnownSystemFast()
+ */
+ public function recordFailureDeferred( User $user, $subnet,
$resultSoFar ) {
+ $isKnown = $this->isKnownSystemSlow( $user, $subnet,
$resultSoFar );
+ if ( !$isKnown ) {
+ $this->recordLoginFailureFromUnknownSystem( $user );
+ }
+ }
+
+ /**
+ * Send a notice on successful login from an unknown IP
*
* @param User $user User account in question.
*/
public function sendSuccessNotice( User $user ) {
- if ( $this->config->get( 'LoginNotifyEnableOnSuccess' )
- && !$this->isFromKnownIP( $user )
- ) {
+ if ( !$this->config->get( 'LoginNotifyEnableOnSuccess' ) ) {
+ return;
+ }
+ $result = $this->isKnownSystemFast( $user, $user->getRequest()
);
+ if ( $result !== self::USER_KNOWN ) {
+ $this->createJob( DeferredChecksJob::TYPE_LOGIN_SUCCESS,
+ $user, $user->getRequest(), $result
+ );
+ }
+ }
+
+ /**
+ * Asynchronous part of sendSuccessNotice(), to be called from
DeferredChecksJob
+ *
+ * @param User $user User in question
+ * @param string $subnet User's current subnet
+ * @param string $resultSoFar Value returned by isKnownSystemFast()
+ */
+ public function sendSuccessNoticeDeferred( User $user, $subnet,
$resultSoFar ) {
+ $isKnown = $this->isKnownSystemSlow( $user, $subnet,
$resultSoFar );
+ if ( !$isKnown ) {
$this->sendNotice( $user, 'login-success' );
}
}
+
+ /**
+ * Creates and enqueues 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
+ * @param WebRequest $request
+ * @param string $resultSoFar Value returned by isKnownSystemFast()
+ */
+ private function createJob( $type, User $user, WebRequest $request,
$resultSoFar ) {
+ $subnet = $this->getIPNetwork( $request->getIP() );
+ $job = new JobSpecification( 'LoginNotifyChecks',
+ [
+ 'checkType' => $type,
+ 'userId' => $user->getId(),
+ 'subnet' => $subnet,
+ 'resultSoFar' => $resultSoFar,
+ ]
+ );
+ JobQueueGroup::singleton()->push( $job );
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/360787
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1fcd15f523828141e8fadee9a8ad824eacefc0f9
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/extensions/LoginNotify
Gerrit-Branch: master
Gerrit-Owner: MaxSem <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Kaldari <[email protected]>
Gerrit-Reviewer: Legoktm <[email protected]>
Gerrit-Reviewer: MaxSem <[email protected]>
Gerrit-Reviewer: Niharika29 <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
