wikimedia/mediawiki-extensions-OATHAuth
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/OATHAuth synced 2024-11-24 08:14:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

TOTPKey: Use foreach instead of for-i-length and remove dead code

Browse source 
Follows-up c943f75.

* Use foreach to iterate the list of tokens, instead of i-to-length.

* Remove the redundant 'retval = false' assignment, as this was
  within an `if (!$retval)` block where the only previous assignment
  to it was either retval = 1 or retval = false. Hence redundant.

* Remove the conditional that made the list only checked if the
  list was not identical to `[ 0 => "" ]`.
  It is unclear to me why this check existed.
  I can imagine one of two scenarios, neither of which appears
  to be the case:
  1. Maybe the scratch list contains 10 tokens plus an empty string,
     and if we see it only contains that, we know it's logically empty
     and don't "need" to iterate the list.
     Except... iterating the list is cheap, so why bother?

  2. Maybe the scratch list contains 10 tokens plus an empty string,
     and we don't want to allow the empty string to be considered
     a valid scratch token to we skip the loop if that's the only
     one left.

     Except... if that were the case we'd be in trouble as it
     isn't being disallowed when the list contains other items
     still. And again, afaik it never contains an empty string,
     and hopefully empty input is already rejected by now.

   Neither of these are good reasons to remove the code without
   knowing what it was for though, so I'd rather we figure this
   out before merging. I can restore the check if it's non-trivial
   to find out.

Bug: T256918
Change-Id: Ide4160bdc18bc47da9632791fb4321e44d6d115a
This commit is contained in:
Timo Tijhof 2020-03-24 00:49:37 +00:00 committed by Krinkle
parent 04522fa6f9
commit 6b106fe10c
1 changed files with 20 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

45
src/Key/TOTPKey.php
View file

@ -121,7 +121,7 @@ class TOTPKey implements IAuthKey {
} }
/** /**
* @return array * @return string[]
*/ */
public function getScratchTokens() { public function getScratchTokens() {
return $this->scratchTokens; return $this->scratchTokens;
@ -181,33 +181,28 @@ class TOTPKey implements IAuthKey {
// See if the user is using a scratch token // See if the user is using a scratch token
if ( !$retval ) { if ( !$retval ) {
$length = count( $this->scratchTokens ); foreach ( $this->scratchTokens as $i => $scratchToken ) {
// Detect condition where all scratch tokens have been used if ( $token === $scratchToken ) {
if ( $length === 1 && $this->scratchTokens[0] === "" ) { // If we used a scratch token, remove it from the scratch token list.
$retval = false; // This is saved below via OATHUserRepository::persist, TOTP::getDataFromUser.
} else { array_splice( $this->scratchTokens, $i, 1 );
for ( $i = 0; $i < $length; $i++ ) {
if ( $token === $this->scratchTokens[$i] ) {
// If there is a scratch token, remove it from the scratch token list
array_splice( $this->scratchTokens, $i, 1 );
$logger->info( 'OATHAuth user {user} used a scratch token from {clientip}', [ $logger->info( 'OATHAuth user {user} used a scratch token from {clientip}', [
'user' => $user->getAccount(), 'user' => $user->getAccount(),
'clientip' => $clientIP, 'clientip' => $clientIP,
] ); ] );
$auth = MediaWikiServices::getInstance()->getService( 'OATHAuth' ); $auth = MediaWikiServices::getInstance()->getService( 'OATHAuth' );
$module = $auth->getModuleByKey( 'totp' ); $module = $auth->getModuleByKey( 'totp' );
/** @var OATHUserRepository $userRepo */ /** @var OATHUserRepository $userRepo */
$userRepo = MediaWikiServices::getInstance()->getService( 'OATHUserRepository' ); $userRepo = MediaWikiServices::getInstance()->getService( 'OATHUserRepository' );
$user->addKey( $this ); $user->addKey( $this );
$user->setModule( $module ); $user->setModule( $module );
$userRepo->persist( $user, $clientIP ); $userRepo->persist( $user, $clientIP );
// Only return true if we removed it from the database // Only return true if we removed it from the database
$retval = self::SCRATCH_TOKEN; $retval = self::SCRATCH_TOKEN;
break; break;
}
} }
} }
} }