From: Gergő Tisza Date: Thu, 13 Dec 2018 00:51:25 +0000 (-0800) Subject: Include bot password app ID in audit data X-Git-Tag: 1.34.0-rc.0~3218^2 X-Git-Url: https://git.cyclocoop.org/%27.WWW_URL.%27admin/?a=commitdiff_plain;h=07a27f19cf7651ffe792f5c57bb10fb296af7878;p=lhc%2Fweb%2Fwiklou.git Include bot password app ID in audit data Authentication audit logs should indicate whether a login is via the normal password or a bot password (and which one). For failed logins it could be included in the error message, and it usually is, but for successful ones there is no message, so we'll send the app ID as a new AuthManagerLoginAuthenticateAudit parameter. Bug: T194338 Change-Id: I8aab48177b81a8e80dae118e6476a8f6a32089f1 Depends-On: Id482d2e2205960a0facd334e456d3a23bcad0ece --- diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index f68875b176..00088f2717 100644 --- a/RELEASE-NOTES-1.33 +++ b/RELEASE-NOTES-1.33 @@ -37,6 +37,8 @@ production. * (T96041) __EXPECTUNUSEDCATEGORY__ on a category page causes the category to be hidden on Special:UnusedCategories. * Add PasswordPolicy to check the password isn't in the large blacklist. +* The AuthManagerLoginAuthenticateAudit hook has a new parameter for + additional information about the authentication event. * … === External library changes in 1.33 === diff --git a/docs/hooks.txt b/docs/hooks.txt index a7ff45385d..28065fc6d2 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -786,7 +786,11 @@ $response: The MediaWiki\Auth\AuthenticationResponse in either a PASS or FAIL $user: The User object being authenticated against, or null if authentication failed before getting that far. $username: A guess at the user name being authenticated, or null if we can't - even determine that. + even determine that. When $user is not null, it can be in the form of + @ (e.g. for bot passwords). +$extraData: An array (string => string) with extra information, intended to be + added to log contexts. Fields it might include: + - appId: the application ID, only if the login was with a bot password 'AuthPluginAutoCreate': DEPRECATED since 1.27! Use the 'LocalUserCreated' hook instead. Called when creating a local account for an user logged in from an diff --git a/includes/auth/AuthManager.php b/includes/auth/AuthManager.php index aae5a83a22..e82f2b83fa 100644 --- a/includes/auth/AuthManager.php +++ b/includes/auth/AuthManager.php @@ -336,7 +336,7 @@ class AuthManager implements LoggerAwareInterface { $this->setSessionDataForUser( $user ); $this->callMethodOnProviders( 7, 'postAuthentication', [ $user, $ret ] ); $session->remove( 'AuthManager::authnState' ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, $user, $user->getName() ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, $user, $user->getName(), [] ] ); return $ret; } @@ -352,7 +352,7 @@ class AuthManager implements LoggerAwareInterface { $this->callMethodOnProviders( 7, 'postAuthentication', [ User::newFromName( $guessUserName ) ?: null, $ret ] ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, null, $guessUserName ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, null, $guessUserName, [] ] ); return $ret; } } @@ -468,7 +468,7 @@ class AuthManager implements LoggerAwareInterface { [ User::newFromName( $guessUserName ) ?: null, $res ] ); $session->remove( 'AuthManager::authnState' ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $res, null, $guessUserName ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $res, null, $guessUserName, [] ] ); return $res; case AuthenticationResponse::ABSTAIN; // Continue loop @@ -534,7 +534,7 @@ class AuthManager implements LoggerAwareInterface { [ User::newFromName( $guessUserName ) ?: null, $res ] ); $session->remove( 'AuthManager::authnState' ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $res, null, $guessUserName ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $res, null, $guessUserName, [] ] ); return $res; case AuthenticationResponse::REDIRECT; case AuthenticationResponse::UI; @@ -625,7 +625,7 @@ class AuthManager implements LoggerAwareInterface { ); $this->callMethodOnProviders( 7, 'postAuthentication', [ $user, $ret ] ); $session->remove( 'AuthManager::authnState' ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, $user, $user->getName() ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, $user, $user->getName(), [] ] ); return $ret; } } @@ -658,7 +658,7 @@ class AuthManager implements LoggerAwareInterface { $this->logger->debug( "Login failed in secondary authentication by $id" ); $this->callMethodOnProviders( 7, 'postAuthentication', [ $user, $res ] ); $session->remove( 'AuthManager::authnState' ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $res, $user, $user->getName() ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $res, $user, $user->getName(), [] ] ); return $res; case AuthenticationResponse::REDIRECT; case AuthenticationResponse::UI; @@ -694,7 +694,7 @@ class AuthManager implements LoggerAwareInterface { $this->callMethodOnProviders( 7, 'postAuthentication', [ $user, $ret ] ); $session->remove( 'AuthManager::authnState' ); $this->removeAuthenticationSessionData( null ); - \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, $user, $user->getName() ] ); + \Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $ret, $user, $user->getName(), [] ] ); return $ret; } catch ( \Exception $ex ) { $session->remove( 'AuthManager::authnState' ); diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php index 187c0a9d39..b83c209a17 100644 --- a/includes/user/BotPassword.php +++ b/includes/user/BotPassword.php @@ -459,14 +459,14 @@ class BotPassword implements IDBAccessObject { // Split name into name+appId $sep = self::getSeparator(); if ( strpos( $username, $sep ) === false ) { - return self::loginHook( $username, Status::newFatal( 'botpasswords-invalid-name', $sep ) ); + return self::loginHook( $username, null, Status::newFatal( 'botpasswords-invalid-name', $sep ) ); } list( $name, $appId ) = explode( $sep, $username, 2 ); // Find the named user $user = User::newFromName( $name ); if ( !$user || $user->isAnon() ) { - return self::loginHook( $user ?: $name, Status::newFatal( 'nosuchuser', $name ) ); + return self::loginHook( $user ?: $name, null, Status::newFatal( 'nosuchuser', $name ) ); } if ( $user->isLocked() ) { @@ -483,39 +483,39 @@ class BotPassword implements IDBAccessObject { $result = $throttle->increase( $user->getName(), $request->getIP(), __METHOD__ ); if ( $result ) { $msg = wfMessage( 'login-throttled' )->durationParams( $result['wait'] ); - return self::loginHook( $user, Status::newFatal( $msg ) ); + return self::loginHook( $user, null, Status::newFatal( $msg ) ); } } // Get the bot password $bp = self::newFromUser( $user, $appId ); if ( !$bp ) { - return self::loginHook( $user, Status::newFatal( 'botpasswords-not-exist', $name, $appId ) ); + return self::loginHook( $user, $bp, + Status::newFatal( 'botpasswords-not-exist', $name, $appId ) ); } // Check restrictions $status = $bp->getRestrictions()->check( $request ); if ( !$status->isOK() ) { - return self::loginHook( $user, Status::newFatal( 'botpasswords-restriction-failed' ) ); + return self::loginHook( $user, $bp, Status::newFatal( 'botpasswords-restriction-failed' ) ); } // Check the password $passwordObj = $bp->getPassword(); if ( $passwordObj instanceof InvalidPassword ) { - return self::loginHook( $user, Status::newFatal( 'botpasswords-needs-reset', $name, $appId ) ); + return self::loginHook( $user, $bp, + Status::newFatal( 'botpasswords-needs-reset', $name, $appId ) ); } if ( !$passwordObj->equals( $password ) ) { - return self::loginHook( $user, Status::newFatal( 'wrongpassword' ) ); + return self::loginHook( $user, $bp, Status::newFatal( 'wrongpassword' ) ); } // Ok! Create the session. if ( $throttle ) { $throttle->clear( $user->getName(), $request->getIP() ); } - return self::loginHook( - $user, - Status::newGood( $provider->newSessionForRequest( $user, $bp, $request ) ) - ); + return self::loginHook( $user, $bp, + Status::newGood( $provider->newSessionForRequest( $user, $bp, $request ) ) ); } /** @@ -525,12 +525,17 @@ class BotPassword implements IDBAccessObject { * AuthManager, call the AuthManagerLoginAuthenticateAudit hook. * * @param User|string $user User being logged in + * @param BotPassword|null $bp Bot sub-account, if it can be identified * @param Status $status Login status * @return Status The passed-in status */ - private static function loginHook( $user, Status $status ) { + private static function loginHook( $user, $bp, Status $status ) { + $extraData = []; if ( $user instanceof User ) { $name = $user->getName(); + if ( $bp ) { + $extraData['appId'] = $name . self::getSeparator() . $bp->getAppId(); + } } else { $name = $user; $user = null; @@ -541,7 +546,7 @@ class BotPassword implements IDBAccessObject { } else { $response = AuthenticationResponse::newFail( $status->getMessage() ); } - Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $response, $user, $name ] ); + Hooks::run( 'AuthManagerLoginAuthenticateAudit', [ $response, $user, $name, $extraData ] ); return $status; }