Anomie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/288046
Change subject: Add SessionInfo force-use flag
......................................................................
Add SessionInfo force-use flag
A provider that uses SessionProvider::hashToSessionId() will likely have
issues if something such as a call to $user->setToken() causes
SessionManager::loadSessionInfoFromStore() to fail, since the provider
can't just arbitrarily change the session ID it returns.
The two solutions to this problem are:
* Somehow include everything that could cause loadSessionInfoFromStore
to fail in the data hashed by hashToSessionId.
* Flag the SessionInfo so that, if stored data and the SessionInfo
conflict, it should delete the stored data instead of discarding the
SessionInfo.
Since the second is less complexity overall due to the lack of need to
define "everything", this patch takes that approach.
Change-Id: I8c6fab2ec295e71242bbcb19d0ee5ade6bd655df
---
M includes/session/SessionInfo.php
M includes/session/SessionManager.php
M includes/session/SessionProvider.php
M tests/phpunit/includes/session/SessionInfoTest.php
M tests/phpunit/includes/session/SessionManagerTest.php
5 files changed, 102 insertions(+), 18 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/46/288046/1
diff --git a/includes/session/SessionInfo.php b/includes/session/SessionInfo.php
index 1b5a834..c235861 100644
--- a/includes/session/SessionInfo.php
+++ b/includes/session/SessionInfo.php
@@ -54,6 +54,7 @@
private $remembered = false;
private $forceHTTPS = false;
private $idIsSafe = false;
+ private $forceUse = false;
/** @var array|null */
private $providerMetadata = null;
@@ -76,6 +77,10 @@
* - idIsSafe: (bool) Set true if the 'id' did not come from the user.
* Generally you'll use this from SessionProvider::newEmptySession(),
* and not from any other method.
+ * - forceUse: (bool) Set true if the 'id' is from
+ * SessionProvider::hashToSessionId() to delete conflicting session
+ * store data instead of discarding this SessionInfo. Ignored unless
+ * both 'provider' and 'id' are given.
* - copyFrom: (SessionInfo) SessionInfo to copy other data items from.
*/
public function __construct( $priority, array $data ) {
@@ -97,6 +102,7 @@
'forceHTTPS' => $from->forceHTTPS,
'metadata' => $from->providerMetadata,
'idIsSafe' => $from->idIsSafe,
+ 'forceUse' => $from->forceUse,
// @codeCoverageIgnoreStart
];
// @codeCoverageIgnoreEnd
@@ -110,6 +116,7 @@
'forceHTTPS' => false,
'metadata' => null,
'idIsSafe' => false,
+ 'forceUse' => false,
// @codeCoverageIgnoreStart
];
// @codeCoverageIgnoreEnd
@@ -137,9 +144,11 @@
if ( $data['id'] !== null ) {
$this->id = $data['id'];
$this->idIsSafe = $data['idIsSafe'];
+ $this->forceUse = $data['forceUse'] && $this->provider;
} else {
$this->id =
$this->provider->getManager()->generateSessionId();
$this->idIsSafe = true;
+ $this->forceUse = false;
}
$this->priority = (int)$priority;
$this->userInfo = $data['userInfo'];
@@ -186,6 +195,20 @@
}
/**
+ * Force use of this SessionInfo if validation fails
+ *
+ * The normal behavior is to discard the SessionInfo if validation
against
+ * the data stored in the session store fails. If this returns true,
+ * SessionManager will instead delete the session store data so this
+ * SessionInfo may still be used.
+ *
+ * @return bool
+ */
+ final public function forceUse() {
+ return $this->forceUse;
+ }
+
+ /**
* Return the priority
* @return int
*/
diff --git a/includes/session/SessionManager.php
b/includes/session/SessionManager.php
index 6c17de5..7ba3bbe 100644
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -704,6 +704,20 @@
$key = wfMemcKey( 'MWSession', $info->getId() );
$blob = $this->store->get( $key );
+ // If we got data from the store and the SessionInfo says to
force use,
+ // "fail" means to delete the data from the store and retry.
Otherwise,
+ // "fail" is just return false.
+ if ( $info->forceUse() && $blob !== false ) {
+ $failHandler = function () use ( $key, &$info, $request
) {
+ $this->store->delete( $key );
+ return $this->loadSessionInfoFromStore( $info,
$request );
+ };
+ } else {
+ $failHandler = function () {
+ return false;
+ };
+ }
+
$newParams = [];
if ( $blob !== false ) {
@@ -713,7 +727,7 @@
'session' => $info,
] );
$this->store->delete( $key );
- return false;
+ return $failHandler();
}
// Sanity check: blob has data and metadata arrays
@@ -724,7 +738,7 @@
'session' => $info,
] );
$this->store->delete( $key );
- return false;
+ return $failHandler();
}
$data = $blob['data'];
@@ -741,7 +755,7 @@
'session' => $info,
] );
$this->store->delete( $key );
- return false;
+ return $failHandler();
}
// First, load the provider from metadata, or validate
it against the metadata.
@@ -756,7 +770,7 @@
]
);
$this->store->delete( $key );
- return false;
+ return $failHandler();
}
} elseif ( $metadata['provider'] !== (string)$provider
) {
$this->logger->warning( 'Session "{session}":
Wrong provider ' .
@@ -764,7 +778,7 @@
[
'session' => $info,
] );
- return false;
+ return $failHandler();
}
// Load provider metadata from metadata, or validate it
against the metadata
@@ -788,7 +802,7 @@
'exception' =>
$ex,
] + $ex->getContext()
);
- return false;
+ return $failHandler();
}
}
}
@@ -810,7 +824,7 @@
'session' => $info,
'exception' => $ex,
] );
- return false;
+ return $failHandler();
}
$newParams['userInfo'] = $userInfo;
} else {
@@ -825,7 +839,7 @@
'uid_a' =>
$metadata['userId'],
'uid_b' =>
$userInfo->getId(),
] );
- return false;
+ return $failHandler();
}
// If the user was renamed, probably
best to fail here.
@@ -839,7 +853,7 @@
'uname_a' =>
$metadata['userName'],
'uname_b' =>
$userInfo->getName(),
] );
- return false;
+ return $failHandler();
}
} elseif ( $metadata['userName'] !== null ) {
// Shouldn't happen, but just in case
@@ -851,7 +865,7 @@
'uname_a' =>
$metadata['userName'],
'uname_b' =>
$userInfo->getName(),
] );
- return false;
+ return $failHandler();
}
} elseif ( !$userInfo->isAnon() ) {
// Metadata specifies an anonymous
user, but the passed-in
@@ -861,7 +875,7 @@
[
'session' => $info,
] );
- return false;
+ return $failHandler();
}
}
@@ -872,7 +886,7 @@
$this->logger->warning( 'Session "{session}":
User token mismatch', [
'session' => $info,
] );
- return false;
+ return $failHandler();
}
if ( !$userInfo->isVerified() ) {
$newParams['userInfo'] = $userInfo->verified();
@@ -899,7 +913,7 @@
[
'session' => $info,
] );
- return false;
+ return $failHandler();
}
// If no user was provided and no metadata, it must be
anon.
@@ -912,7 +926,7 @@
[
'session' => $info,
] );
- return false;
+ return $failHandler();
}
} elseif ( !$info->getUserInfo()->isVerified() ) {
$this->logger->warning(
@@ -920,7 +934,7 @@
[
'session' => $info,
] );
- return false;
+ return $failHandler();
}
$data = false;
@@ -942,7 +956,7 @@
// Allow the provider to check the loaded SessionInfo
$providerMetadata = $info->getProviderMetadata();
if ( !$info->getProvider()->refreshSessionInfo( $info,
$request, $providerMetadata ) ) {
- return false;
+ return $failHandler();
}
if ( $providerMetadata !== $info->getProviderMetadata() ) {
$info = new SessionInfo( $info->getPriority(), [
@@ -962,7 +976,7 @@
$this->logger->warning( 'Session "{session}": ' .
$reason, [
'session' => $info,
] );
- return false;
+ return $failHandler();
}
return true;
diff --git a/includes/session/SessionProvider.php
b/includes/session/SessionProvider.php
index 3cd065d..1d693a1 100644
--- a/includes/session/SessionProvider.php
+++ b/includes/session/SessionProvider.php
@@ -458,7 +458,9 @@
*
* Generally this will only be used when self::persistsSessionId() is
false and
* the provider has to base the session ID on the verified user's
identity
- * or other static data.
+ * or other static data. The SessionInfo should then typically have the
+ * 'forceUse' flag set to avoid persistent session failure if
validation of
+ * the stored data fails.
*
* @param string $data
* @param string|null $key Defaults to $this->config->get( 'SecretKey' )
diff --git a/tests/phpunit/includes/session/SessionInfoTest.php
b/tests/phpunit/includes/session/SessionInfoTest.php
index ff22bfa..8f7b2a6 100644
--- a/tests/phpunit/includes/session/SessionInfoTest.php
+++ b/tests/phpunit/includes/session/SessionInfoTest.php
@@ -103,6 +103,7 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertSame( $anonInfo, $info->getUserInfo() );
$this->assertTrue( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertFalse( $info->wasPersisted() );
$this->assertFalse( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
@@ -118,6 +119,7 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertSame( $unverifiedUserInfo, $info->getUserInfo() );
$this->assertTrue( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertFalse( $info->wasPersisted() );
$this->assertFalse( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
@@ -132,6 +134,7 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertSame( $userInfo, $info->getUserInfo() );
$this->assertTrue( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertFalse( $info->wasPersisted() );
$this->assertTrue( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
@@ -150,6 +153,7 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertSame( $anonInfo, $info->getUserInfo() );
$this->assertFalse( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertTrue( $info->wasPersisted() );
$this->assertFalse( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
@@ -165,6 +169,7 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertSame( $userInfo, $info->getUserInfo() );
$this->assertFalse( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertFalse( $info->wasPersisted() );
$this->assertTrue( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
@@ -180,6 +185,7 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertSame( $userInfo, $info->getUserInfo() );
$this->assertFalse( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertTrue( $info->wasPersisted() );
$this->assertFalse( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
@@ -231,6 +237,25 @@
$this->assertSame( SessionInfo::MIN_PRIORITY + 5,
$info->getPriority() );
$this->assertTrue( $info->isIdSafe() );
+ $info = new SessionInfo( SessionInfo::MIN_PRIORITY + 5, [
+ 'id' => $id,
+ 'forceUse' => true,
+ ] );
+ $this->assertFalse( $info->forceUse(), 'no provider' );
+
+ $info = new SessionInfo( SessionInfo::MIN_PRIORITY + 5, [
+ 'provider' => $provider,
+ 'forceUse' => true,
+ ] );
+ $this->assertFalse( $info->forceUse(), 'no id' );
+
+ $info = new SessionInfo( SessionInfo::MIN_PRIORITY + 5, [
+ 'provider' => $provider,
+ 'id' => $id,
+ 'forceUse' => true,
+ ] );
+ $this->assertTrue( $info->forceUse(), 'correct use' );
+
$info = new SessionInfo( SessionInfo::MIN_PRIORITY, [
'id' => $id,
'forceHTTPS' => 1,
@@ -242,6 +267,7 @@
'provider' => $provider,
'userInfo' => $userInfo,
'idIsSafe' => true,
+ 'forceUse' => true,
'persisted' => true,
'remembered' => true,
'forceHTTPS' => true,
@@ -255,6 +281,7 @@
$this->assertSame( $provider, $info->getProvider() );
$this->assertSame( $userInfo, $info->getUserInfo() );
$this->assertTrue( $info->isIdSafe() );
+ $this->assertTrue( $info->forceUse() );
$this->assertTrue( $info->wasPersisted() );
$this->assertTrue( $info->wasRemembered() );
$this->assertTrue( $info->forceHTTPS() );
@@ -265,6 +292,7 @@
'provider' => $provider2,
'userInfo' => $unverifiedUserInfo,
'idIsSafe' => false,
+ 'forceUse' => false,
'persisted' => false,
'remembered' => false,
'forceHTTPS' => false,
@@ -276,6 +304,7 @@
$this->assertSame( $provider2, $info->getProvider() );
$this->assertSame( $unverifiedUserInfo, $info->getUserInfo() );
$this->assertFalse( $info->isIdSafe() );
+ $this->assertFalse( $info->forceUse() );
$this->assertFalse( $info->wasPersisted() );
$this->assertFalse( $info->wasRemembered() );
$this->assertFalse( $info->forceHTTPS() );
diff --git a/tests/phpunit/includes/session/SessionManagerTest.php
b/tests/phpunit/includes/session/SessionManagerTest.php
index d04d7ec..9c9115d 100644
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -1756,5 +1756,21 @@
[ LogLevel::WARNING, 'Session "{session}": Hook
aborted' ],
], $logger->getBuffer() );
$logger->clearBuffer();
+ $this->mergeMwGlobalArrayValue( 'wgHooks', [ 'SessionCheckInfo'
=> [] ] );
+
+ // forceUse deletes bad backend data
+ $this->store->setSessionMeta( $id, [ 'userToken' => 'Bad' ] +
$metadata );
+ $info = new SessionInfo( SessionInfo::MIN_PRIORITY, [
+ 'provider' => $provider,
+ 'id' => $id,
+ 'userInfo' => $userInfo,
+ 'forceUse' => true,
+ ] );
+ $this->assertTrue( $loadSessionInfoFromStore( $info ) );
+ $this->assertFalse( $this->store->getSession( $id ) );
+ $this->assertSame( [
+ [ LogLevel::WARNING, 'Session "{session}": User token
mismatch' ],
+ ], $logger->getBuffer() );
+ $logger->clearBuffer();
}
}
--
To view, visit https://gerrit.wikimedia.org/r/288046
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8c6fab2ec295e71242bbcb19d0ee5ade6bd655df
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
