Page MenuHomePhabricator

Staging release of RESTBagOStuff using Kask
Open, Needs TriagePublic

Description

It would be great to have a staging server that uses KaskBagOStuff for its session management.

Event Timeline

Anomie removed a subscriber: Anomie.Apr 30 2019, 1:04 PM
EvanProdromou renamed this task from Staging release of KaskBagOStuff using Kask to Staging release of RESTBagOStuff using Kask.Jun 10 2019, 2:18 PM

So, would the best way to do this be to configure test.wikipedia.org to use RESTBagOStuff and the production Kask service?

How do we get that going?

What happens if we (in repository mediawiki-config):

  1. add kask-session and kask-transition to CommonSettings.php (per T224993)
  2. change the wgSessionCacheType section in InitialiseSettings.php to:
'wgSessionCacheType' => [
	'default' => 'redis_local',  // declared in redis.php
	'wikitech' => 'memcached-pecl',
	'testwiki' => 'kask-transition', // change this to kask-session after the transition is complete
],

(I assume that test.wikipedia.org is "testwiki")

After confirming that much works, we could change testwiki to 'kask-session'. That would take testwiki through the same deployment stages we're planning for all the live wikis.

Change 519432 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[operations/mediawiki-config@master] Add kask session storage configuration. Use only on testwiki,

https://gerrit.wikimedia.org/r/519432

Prospective configuration change pushed for review. Once this is merged and deployed, sessions for testwiki will be stored to both Redis and Kask, and will be continue to be read from Redis. (Other wikis will be unaffected and will continue to use only Redis.) Next steps will be:

  1. confirm sessions are being happily stored to Kask and that logs are clear
  2. wait a sufficient time period so that all testwiki sessions are in Kask ($wgObjectCacheSessionExpiry is currently 3600)
  3. push a config change to:
    1. change testwiki from 'kask-transition' to 'kask-session', thereby removing Redis and putting testwiki sessions solely in Kask
    2. change testwiki $wgObjectCacheSessionExpiry to match the Kask setting (if Kask uses something other than 3600)
  4. confirm sessions are happily functioning and logs are clear

Note: during the transition, testwiki will still be using a TTL ($wgObjectCacheSessionExpiry) value of 3600. The Kask TTL setting must therefore be at least 3600 seconds. If the Kask setting is longer, the transition will still be okay. But we will do more writes to Kask than we would with a longer settings, because MediaWiki will think the sessions need to be rewritten.

[ ... ]
Note: during the transition, testwiki will still be using a TTL ($wgObjectCacheSessionExpiry) value of 3600. The Kask TTL setting must therefore be at least 3600 seconds. If the Kask setting is longer, the transition will still be okay. But we will do more writes to Kask than we would with a longer settings, because MediaWiki will think the sessions need to be rewritten.

I will work on getting the sessionstore cluster reconfigured for a TTL of 3600 (currently set to 86400).

[ ... ]
Note: during the transition, testwiki will still be using a TTL ($wgObjectCacheSessionExpiry) value of 3600. The Kask TTL setting must therefore be at least 3600 seconds. If the Kask setting is longer, the transition will still be okay. But we will do more writes to Kask than we would with a longer settings, because MediaWiki will think the sessions need to be rewritten.

I will work on getting the sessionstore cluster reconfigured for a TTL of 3600 (currently set to 86400).

And this is now done.

WDoranWMF moved this task from mop to remove cpt on the Core Platform Team board.Jul 26 2019, 6:28 PM
WDoranWMF removed a project: Core Platform Team.

Change 519432 merged by jenkins-bot:
[operations/mediawiki-config@master] Add kask session storage configuration. Use only on testwiki,

https://gerrit.wikimedia.org/r/519432

Change 526790 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[operations/mediawiki-config@master] Fix a typo in CommonSettings.php

https://gerrit.wikimedia.org/r/526790

Change 526790 merged by jenkins-bot:
[operations/mediawiki-config@master] Fix a typo in CommonSettings.php

https://gerrit.wikimedia.org/r/526790

Mentioned in SAL (#wikimedia-operations) [2019-07-31T23:12:52Z] <urbanecm@deploy1001> Synchronized wmf-config/: SWAT: Add kask session storage configuration. Use only on testwiki, (ede989e, 862df8d, T222099) (duration: 00m 56s)

Change 527120 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[operations/mediawiki-config@master] Switch testwiki to read sessions from kask, with fallback to redis

https://gerrit.wikimedia.org/r/527120

Change 527120 merged by jenkins-bot:
[operations/mediawiki-config@master] Switch testwiki to read sessions from kask, with fallback to redis

https://gerrit.wikimedia.org/r/527120

Mentioned in SAL (#wikimedia-operations) [2019-08-01T18:06:10Z] <urbanecm@deploy1001> Synchronized wmf-config/CommonSettings.php: SWAT: 469c42d: Switch testwiki to read sessions from kask, with fallback to redis (T222099) (duration: 00m 55s)

We have now completed two deploys that affect testwiki (only):

  1. use MultiWriteBagOStuff to write sessions to both redis and Kask, with reads from redis as primary, and kask as fallback
  2. use MultiWriteBagOStuff to write sessions to both redis and Kask, with reads from kask as primary, and redis as fallback

The final testwiki update will be to remove usage of MultiWriteBagOStuff and use only kask. This will fully move sessions off redis for testwiki.

Change 528130 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[operations/mediawiki-config@master] Switch testwiki to use kask (only) for sessions

https://gerrit.wikimedia.org/r/528130

Change 528130 merged by jenkins-bot:
[operations/mediawiki-config@master] Switch testwiki to use kask (only) for sessions

https://gerrit.wikimedia.org/r/528130

Mentioned in SAL (#wikimedia-operations) [2019-08-05T18:09:39Z] <urbanecm@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: 254ecc1: Switch testwiki to use kask (only) for sessions (T222099) (duration: 00m 48s)

We have completed all stages of the migration for testwiki. Testwiki is now using only kask for session storage, and is not using redis at all. Everything looks good from my end.

Reedy added a subscriber: Reedy.Wed, Sep 18, 8:36 PM

Can't say for definite, but it seems this might be causing some session issues with our 2FA extension, MediaWiki-extensions-OATHAuth

See T231786 for the original task (2nd September) and T233146 for the current task

As far as we can see, it's only seemingly broken on testwiki (something put into session data doesn't come out the same). And may have been broken for a while, as I imagine enabling 2FA on testwiki isn't how most people would do it

The class in question going in is https://github.com/wikimedia/mediawiki-extensions-OATHAuth/blob/master/src/Key/TOTPKey.php and apparently comes out as an empty array...

While the code has undergone a lot of changes this year, it's mostly quietened down now, with the code in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OATHAuth/+/499183/ being live since July and working otherwise

As far as we can see, it's only seemingly broken on testwiki (something put into session data doesn't come out the same). And may have been broken for a while, as I imagine enabling 2FA on testwiki isn't how most people would do it
The class in question going in is https://github.com/wikimedia/mediawiki-extensions-OATHAuth/blob/master/src/Key/TOTPKey.php and apparently comes out as an empty array...

I think what I'm trying to say.. Is whatever seriali[sz]ation of objects Kask does... It seemingly doesn't like this object

As far as we can see, it's only seemingly broken on testwiki (something put into session data doesn't come out the same). And may have been broken for a while, as I imagine enabling 2FA on testwiki isn't how most people would do it
The class in question going in is https://github.com/wikimedia/mediawiki-extensions-OATHAuth/blob/master/src/Key/TOTPKey.php and apparently comes out as an empty array...

Pardon any ignorance here, but is this even stored in sessionstore? I know they share a common store elsewhere (Redis/mainstash), but AIUI, the migration to sessionstore (i.e. Kask) breaks sessions out from...well everything else.

I think what I'm trying to say.. Is whatever seriali[sz]ation of objects Kask does... It seemingly doesn't like this object

Kask doesn't do any serialization; The data is opaque, just bytes in and bytes out.

Reedy added a comment.Thu, Sep 19, 3:12 PM

The code goes down to

	/**
	 * Set a value in the session
	 * @param string|int $key
	 * @param mixed $value
	 */
	public function set( $key, $value ) {
		$data = &$this->backend->getData();
		if ( !array_key_exists( $key, $data ) || $data[$key] !== $value ) {
			$data[$key] = $value;
			$this->backend->dirty();
		}
	}

If we're adding something to the backends data...? Presumably?

Kask doesn't do any serialization; The data is opaque, just bytes in and bytes out.

Kask doesn't, but RESTBagOStuff that's used to talk to Kask from MediaWiki does.

And rMWbf2d6d724074: Add additional configuation parameters to RESTBagOStuff switched RESTBagOStuff from using PHP serialization to JSON. And that will mangle objects.

>>> $key = MediaWiki\Extension\OATHAuth\Key\TOTPKey::newFromRandom();
=> MediaWiki\Extension\OATHAuth\Key\TOTPKey {#489}
>>> json_decode( json_encode( $key ), true );
=> []
Reedy added a comment.Thu, Sep 19, 8:56 PM

Cheers Brad.

So, yeah, it's Kask (even if indirectly), and the way MW uses it is breaking this behaviour. It worked not using Kask. So the next question is does this just break OATHAuth? Or do other extensions put objects in and therefore will expect it to work?

It's certainly a breaking change as it stands

The docs say mixed, it doesn't say no objects. If they're not allowed, MW should let you know, and it should be documented. If they are allowed... Well, the json seriali[sz]ing needs to support it

Looking at most usages, it does seem that most are just passing bools, strings and some arrays.

	/**
	 * Set session data
	 *
	 * @note Prefer $this->getSession() instead if making multiple calls.
	 * @param string $key Name of key in the session
	 * @param mixed $data
	 */
	public function setSessionData( $key, $data ) {
		$this->getSession()->set( $key, $data );
	}

	/**
	 * Set a value in the session
	 * @param string|int $key
	 * @param mixed $value
	 */
	public function set( $key, $value ) {
		$data = &$this->backend->getData();
		if ( !array_key_exists( $key, $data ) || $data[$key] !== $value ) {
			$data[$key] = $value;
			$this->backend->dirty();
		}
	}

WebRequest::setSessionData()

Method
    setSessionData
Found usages  (44 usages found)
    Delegate to another instance method  (1 usage found)
        mediawiki  (1 usage found)
            core/includes  (1 usage found)
                DerivativeRequest.php  (1 usage found)
                    DerivativeRequest  (1 usage found)
                        setSessionData  (1 usage found)
                            70 $this->base->setSessionData( $key, $data );
    Method call  (43 usages found)
        mediawiki  (43 usages found)
            extensions/BlueSpiceWhoIsOnline/src  (2 usages found)
                Tracer.php  (2 usages found)
                    Tracer  (2 usages found)
                        trace  (2 usages found)
                            132 $context->getRequest()->setSessionData(
                            136 $context->getRequest()->setSessionData(
            extensions/BreadCrumbs  (2 usages found)
                BreadCrumbsFunctions.php  (2 usages found)
                    BreadCrumbsFunctions  (2 usages found)
                        fnBreadCrumbsShowHook  (2 usages found)
                            60 $request->setSessionData( "BreadCrumbs", $m_BreadCrumbs );
                            67 $request->setSessionData( "BreadCrumbs", $m_BreadCrumbs );
            extensions/CentralAuth/includes  (4 usages found)
                CentralAuthHooks.php  (4 usages found)
                    CentralAuthHooks  (4 usages found)
                        doCentralLoginRedirect  (3 usages found)
                            511 $request->setSessionData( 'CentralAuthDoEdgeLogin', true );
                            560 $request->setSessionData( 'CentralAuth:autologin:current-attempt', [
                            599 $request->setSessionData( 'CentralAuthDoEdgeLogin', true );
                        onBeforePageDisplay  (1 usage found)
                            1145 $out->getRequest()->setSessionData( 'CentralAuthDoEdgeLogin', null );
            extensions/CentralAuth/includes/specials  (7 usages found)
                SpecialCentralAutoLogin.php  (2 usages found)
                    SpecialCentralAutoLogin  (2 usages found)
                        execute  (2 usages found)
                            357 $request->setSessionData( 'centralautologin-token', $token );
                            499 $request->setSessionData( 'CentralAuthDoEdgeLogin', true );
                SpecialCentralLogin.php  (2 usages found)
                    SpecialCentralLogin  (2 usages found)
                        doLoginComplete  (2 usages found)
                            254 $request->setSessionData( $skey, null );
                            285 $request->setSessionData( 'CentralAuthDoEdgeLogin', true );
                SpecialMergeAccount.php  (2 usages found)
                    SpecialMergeAccount  (2 usages found)
                        addWorkingPassword  (1 usage found)
                            188 $request->setSessionData( 'wsCentralAuthMigration', $data );
                        clearWorkingPasswords  (1 usage found)
                            195 $request->setSessionData( 'wsCentralAuthMigration', $data );
                SpecialSulRenameWarning.php  (1 usage found)
                    SpecialSulRenameWarning  (1 usage found)
                        execute  (1 usage found)
                            76 $request->setSessionData( 'SulRenameWarning', null );
            extensions/CentralNotice/includes/specials  (1 usage found)
                CentralNotice.php  (1 usage found)
                    CentralNotice  (1 usage found)
                        setCNSessionVar  (1 usage found)
                            1615 $this->getRequest()->setSessionData( "centralnotice-{$variable}", $value );
            extensions/DonationInterface/gateway_common  (3 usages found)
                GatewayPage.php  (1 usage found)
                    GatewayPage  (1 usage found)
                        overrideLogo  (1 usage found)
                            513 $request->setSessionData( 'logoOverride', $storedOverride );
                ResultSwitcher.php  (1 usage found)
                    ResultSwitcher  (1 usage found)
                        handleResultRequest  (1 usage found)
                            42 $request->setSessionData( 'order_status', $sessionOrderStatus );
                WmfFramework.mediawiki.php  (1 usage found)
                    WmfFramework_Mediawiki  (1 usage found)
                        setSessionValue  (1 usage found)
                            58 RequestContext::getMain()->getRequest()->setSessionData( $key, $value );
            extensions/FlaggedRevs/backend  (1 usage found)
                FlaggedRevsHooks.php  (1 usage found)
                    FlaggedRevsHooks  (1 usage found)
                        setSessionKey  (1 usage found)
                            1272 $wgRequest->setSessionData( 'wsFlaggedRevsKey', $key );
            extensions/GoogleAnalyticsTopPages/includes  (1 usage found)
                GoogleAnalyticsTopPages.body.php  (1 usage found)
                    GoogleAnalyticsTopPages  (1 usage found)
                        getData  (1 usage found)
                            34 $request->setSessionData( 'service_token', $client->getAccessToken() );
            extensions/GoogleLogin/includes  (1 usage found)
                GoogleLoginHooks.php  (1 usage found)
                    GoogleLoginHooks  (1 usage found)
                        onUserLogoutComplete  (1 usage found)
                            16 $wgRequest->setSessionData( 'access_token', null );
            extensions/LiquidThreads/classes  (2 usages found)
                View.php  (2 usages found)
                    LqtView  (2 usages found)
                        fixFauxRequestSession  (1 usage found)
                            464 $request->setSessionData( $k, $v );
                        getInlineEditForm  (1 usage found)
                            508 $wgRequest->setSessionData( $k, $v );
            extensions/NetworkAuth  (2 usages found)
                NetworkAuth.class.php  (2 usages found)
                    NetworkAuth  (2 usages found)
                        onUserLoadAfterLoadFromSession  (2 usages found)
                            64 $user->getRequest()->setSessionData( 'wsUserID', 0 );
                            65 $user->getRequest()->setSessionData( 'wsUserName', null );
            extensions/OATHAuth/src/HTMLForm  (2 usages found)
                TOTPEnableForm.php  (2 usages found)
                    TOTPEnableForm  (2 usages found)
                        getDescriptors  (1 usage found)
                            34 $this->getRequest()->setSessionData( 'oathauth_totp_key', $key );
                        onSubmit  (1 usage found)
                            174 $this->getRequest()->setSessionData( 'oathauth_totp_key', null );
            extensions/OAuthAuthentication/store  (2 usages found)
                PhpSessionStore.php  (2 usages found)
                    PhpSessionStore  (2 usages found)
                        delete  (1 usage found)
                            23 $this->request->setSessionData( $key, null );
                        set  (1 usage found)
                            19 $this->request->setSessionData( $key, $value );
            extensions/SecureSessions  (8 usages found)
                SecureSessions.hooks.php  (8 usages found)
                    SecureSessions  (8 usages found)
                        onUserLoadFromSession  (3 usages found)
                            287 $request->setSessionData( 'wsUserID', $user->getID() );
                            288 $request->setSessionData( 'wsUserName', $user->getName() );
                            289 $request->setSessionData( 'wsToken', $user->getToken() );
                        onUserLogout  (2 usages found)
                            238 $request->setSessionData( 'wsUserAgent', null );
                            239 $request->setSessionData( 'wsIPAddress', null );
                        setup  (2 usages found)
                            68 $request->setSessionData( 'wsObsolete', true );
                            69 $request->setSessionData( 'wsExpiry', time() + 5 );
                        updateSessionCache  (1 usage found)
                            455 $this->getRequest()->setSessionData( 'id', $id );
            extensions/SemanticPageSeries/includes  (3 usages found)
                SPSSpecialSeriesEdit.php  (3 usages found)
                    SPSSpecialSeriesEdit  (3 usages found)
                        evaluateForm  (3 usages found)
                            238 $request->setSessionData( 'spsResult', $iteratorValuesCount );
                            239 $request->setSessionData( 'spsForm', $targetFormPageId );
                            240 $request->setSessionData( 'spsOrigin', $originPageId );
            extensions/Thanks/includes  (2 usages found)
                ApiCoreThank.php  (1 usage found)
                    ApiCoreThank  (1 usage found)
                        sendThanks  (1 usage found)
                            217 $user->getRequest()->setSessionData( "thanks-thanked-$type$id", true );
                ApiFlowThank.php  (1 usage found)
                    ApiFlowThank  (1 usage found)
                        sendThanks  (1 usage found)
                            170 $user->getRequest()->setSessionData( "flow-thanked-{$postId->getAlphadecimal()}", true );

Session::set()

Method
    set
Found usages  (61 usages found)
    Method call  (61 usages found)
        mediawiki  (61 usages found)
            core/includes  (2 usages found)
                FauxRequest.php  (1 usage found)
                    FauxRequest  (1 usage found)
                        __construct  (1 usage found)
                            64 $mwsession->set( $key, $value );
                WebRequest.php  (1 usage found)
                    WebRequest  (1 usage found)
                        setSessionData  (1 usage found)
                            1124 $this->getSession()->set( $key, $data );
            core/includes/api  (1 usage found)
                ApiAuthManagerHelper.php  (1 usage found)
                    ApiAuthManagerHelper  (1 usage found)
                        formatAuthenticationResponse  (1 usage found)
                            218 $this->module->getRequest()->getSession()->set(
            core/includes/auth  (5 usages found)
                AuthManager.php  (5 usages found)
                    AuthManager  (5 usages found)
                        autoCreateUser  (3 usages found)
                            1634 $session->set( 'AuthManager::AutoCreateBlacklist', 'noname' );
                            1650 $session->set( 'AuthManager::AutoCreateBlacklist', 'authmanager-autocreate-noperm' );
                            1685 $session->set( 'AuthManager::AutoCreateBlacklist', $status );
                        setSessionDataForUser  (2 usages found)
                            2409 $session->set( 'AuthManager:lastAuthId', $user->getId() );
                            2410 $session->set( 'AuthManager:lastAuthTimestamp', time() );
            core/includes/preferences  (1 usage found)
                DefaultPreferencesFactory.php  (1 usage found)
                    DefaultPreferencesFactory  (1 usage found)
                        submitForm  (1 usage found)
                            1732 $context->getRequest()->getSession()->set( 'specialPreferencesSaveSuccess', 1 );
            core/includes/session  (9 usages found)
                PHPSessionHandler.php  (3 usages found)
                    PHPSessionHandler  (3 usages found)
                        write  (3 usages found)
                            278 $session->set( $key, $value );
                            288 $session->set( $key, $value );
                            292 $session->set( $key, $value );
                Session.php  (6 usages found)
                    Session  (6 usages found)
                        getSecretKeys  (2 usages found)
                            401 $this->set( 'wsSessionSecret', $userSecret );
                            406 $this->set( 'wsSessionPbkdf2Iterations', $iterations );
                        getToken  (1 usage found)
                            359 $this->set( 'wsTokenSecrets', $secrets );
                        offsetSet  (1 usage found)
                            689 $this->set( $offset, $value );
                        resetToken  (1 usage found)
                            379 $this->set( 'wsTokenSecrets', $secrets );
                        setSecret  (1 usage found)
                            519 $this->set( $key, $encrypted );
            core/includes/specials  (2 usages found)
                SpecialPreferences.php  (1 usage found)
                    SpecialPreferences  (1 usage found)
                        submitReset  (1 usage found)
                            152 $this->getRequest()->getSession()->set( 'specialPreferencesSaveSuccess', 1 );
                SpecialUserrights.php  (1 usage found)
                    UserrightsPage  (1 usage found)
                        execute  (1 usage found)
                            205 $session->set( 'specialUserrightsSaveSuccess', 1 );
            core/includes/user  (4 usages found)
                User.php  (4 usages found)
                    User  (4 usages found)
                        doLogout  (1 usage found)
                            3907 $session->set( 'wsUserID', 0 ); // Other code expects this
                        loadFromSession  (3 usages found)
                            1269 $session->set( 'wsUserID', $this->getId() );
                            1270 $session->set( 'wsUserName', $this->getName() );
                            1271 $session->set( 'wsToken', $this->getToken() );
            core/tests/phpunit/includes/api  (1 usage found)
                ApiTestCase.php  (1 usage found)
                    ApiTestCase  (1 usage found)
                        doApiRequest  (1 usage found)
                            76 $sessionObj->set( $key, $value );
            core/tests/phpunit/includes/auth  (2 usages found)
                AuthManagerTest.php  (2 usages found)
                    AuthManagerTest  (2 usages found)
                        testAutoAccountCreation  (2 usages found)
                            2503 $session->set( 'AuthManager::AutoCreateBlacklist', 'test' );
                            2518 $session->set( 'AuthManager::AutoCreateBlacklist', StatusValue::newFatal( 'test2' ) );
            core/tests/phpunit/includes/session  (21 usages found)
                PHPSessionHandlerTest.php  (18 usages found)
                    PHPSessionHandlerTest  (18 usages found)
                        testSessionHandling  (18 usages found)
                            210 $session->set( 'Unchanged', 'setup' );
                            211 $session->set( 'Unchanged, null', null );
                            212 $session->set( 'Changed in $_SESSION', 'setup' );
                            213 $session->set( 'Changed in Session', 'setup' );
                            214 $session->set( 'Changed in both', 'setup' );
                            215 $session->set( 'Deleted in Session', 'setup' );
                            216 $session->set( 'Deleted in $_SESSION', 'setup' );
                            217 $session->set( 'Deleted in both', 'setup' );
                            218 $session->set( 'Deleted in Session, changed in $_SESSION', 'setup' );
                            219 $session->set( 'Deleted in $_SESSION, changed in Session', 'setup' );
                            225 $session->set( 'Added in Session', 'Session' );
                            226 $session->set( 'Added in both', 'Session' );
                            227 $session->set( 'Changed in Session', 'Session' );
                            228 $session->set( 'Changed in both', 'Session' );
                            229 $session->set( 'Deleted in $_SESSION, changed in Session', 'Session' );
                            258 $session->set( 42, 'forty-two' );
                            259 $session->set( 'forty-two', 42 );
                            260 $session->set( 'wrong', 43 );
                SessionTest.php  (3 usages found)
                    SessionTest  (3 usages found)
                        testSecrets  (3 usages found)
                            71 $session->set( 'test', 'foobar' );
                            83 $session->set( 'test', $encrypted . 'x' );
                            99 $session->set( 'test', $encrypted );
            core/tests/phpunit/unit/includes/session  (3 usages found)
                SessionUnitTest.php  (3 usages found)
                    SessionUnitTest  (3 usages found)
                        testDataAccess  (3 usages found)
                            111 $session->set( 'foo', 55 );
                            116 $session->set( 1, 'one' );
                            121 $session->set( 1, 'one' );
            extensions/Auth_remoteuser/src  (2 usages found)
                UserNameSessionProvider.php  (2 usages found)
                    UserNameSessionProvider  (2 usages found)
                        refreshSessionInfo  (2 usages found)
                            707 $session->set( 'AuthManager:lastAuthId', $info->getUserInfo()->getId() );
                            708 $session->set( 'AuthManager:lastAuthTimestamp', time() );
            extensions/BlueSpiceFoundation/src  (2 usages found)
                DeferredNotificationStack.php  (2 usages found)
                    DeferredNotificationStack  (2 usages found)
                        __construct  (1 usage found)
                            25 $this->request->getSession()->set( 'notificationInfo', [] );
                        push  (1 usage found)
                            37 $this->request->getSession()->set( 'notificationInfo', $notifications );
            extensions/CentralAuth/includes  (1 usage found)
                CentralAuthUtils.php  (1 usage found)
                    CentralAuthUtils  (1 usage found)
                        setCentralSession  (1 usage found)
                            283 $session->set( 'CentralAuth::centralSessionId', $id );
            extensions/ConfirmEdit/includes/auth  (1 usage found)
                CaptchaPreAuthenticationProvider.php  (1 usage found)
                    CaptchaPreAuthenticationProvider  (1 usage found)
                        testForAuthentication  (1 usage found)
                            86 $session->set( 'ConfirmEdit:loginCaptchaPerUserTriggered', true );
            extensions/ConfirmEdit/includes/store  (1 usage found)
                CaptchaSessionStore.php  (1 usage found)
                    CaptchaSessionStore  (1 usage found)
                        store  (1 usage found)
                            12 SessionManager::getGlobalSession()->set( 'captcha' . $index, $info );
            extensions/ConfirmEdit/tests/phpunit  (1 usage found)
                CaptchaPreAuthenticationProviderTest.php  (1 usage found)
                    CaptchaPreAuthenticationProviderTest  (1 usage found)
                        flagSession  (1 usage found)
                            283 ->set( 'ConfirmEdit:loginCaptchaPerUserTriggered', true );
            extensions/LDAPProvider/src/Hook  (1 usage found)
                UserLoadAfterLoadFromSession.php  (1 usage found)
                    UserLoadAfterLoadFromSession  (1 usage found)
                        doProcess  (1 usage found)
                            185 $session->set( $this->sessionDataKey, $nowTS );
            extensions/UnCaptcha/includes  (1 usage found)
                RobotPreAuthenticationProvider.php  (1 usage found)
                    RobotPreAuthenticationProvider  (1 usage found)
                        getAuthenticationRequests  (1 usage found)
                            40 $session->set( self::SESSION_KEY, $not );

So, yeah, it's Kask (even if indirectly), and the way MW uses it is breaking this behaviour. It worked not using Kask. So the next question is does this just break OATHAuth? Or do other extensions put objects in and therefore will expect it to work?

Note that per T161647: RFC: Deprecate using php serialization inside MediaWiki we should not be using PHP serialization for this purpose. It's insecure and brittle against code changes. Only "dumb" objects should get serialized (associative arrays, essentially). Any code relying on "proper" objects coming back from serialization should be fixed.

Reedy added a comment.Thu, Sep 19, 9:28 PM

So, yeah, it's Kask (even if indirectly), and the way MW uses it is breaking this behaviour. It worked not using Kask. So the next question is does this just break OATHAuth? Or do other extensions put objects in and therefore will expect it to work?

Note that per T161647: RFC: Deprecate using php serialization inside MediaWiki we should not be using PHP serialization for this purpose. It's insecure and brittle against code changes. Only "dumb" objects should get serialized (associative arrays, essentially). Any code relying on "proper" objects coming back from serialization should be fixed.

That's fair enough, but it's still a breaking change...

The code docs don't say this, MW doesn't seemingly seem to throw any deprecated warnings etc

We can trivially enough fix OATHAuth, so that's not an issue, but that doesn't prevent anyone else falling over on the same issue with MW in it's current state :)

This is clearly a breaking change, no argument there. Fortunately, it is a configuration-dependent one and we've only affected ourselves, and in a limited fashion. I'm glad we were as conservative in rolling it out as we were, and I'm suddenly thankful for the the Kask performance investigation (now complete) that delayed further rollout.

So, what are the to-do items for MW?

  • update WebRequest::setSessionData() documentation to indicate "proper" objects are not allowed
    • is there a better doxygen/phpdoc way to indicate this than "mixed" and then saying something about "no objects" in the comments?
    • given that objects do currently work for other backends, should the language be soft ("objects are not guaranteed to work")?
  • corresponding update to Session::set() documentation, and probably also a note in the class comment
  • corresponding update to functions with RESTBagOStuff, and probably also a note in the class comment
  • add something to RELEASE-NOTES?
  • does this justify an email to wikitech-l?
  • what else?

@Reedy mentioned possibly throwing deprecated warnings, but I'm not sure what to do in-code. Session storage seems performance-critical enough that I'd rather not do introspection on the session data to be serialized/encoded. Is there something both effective and inoffensive that we could/should do? If so, would it live in RESTBagOStuff (because only wikis that use that backend are currently affected) or Session (because many more wikis may eventually be affected if we truly purge serialize/unserialize)?

Reedy added a comment.Fri, Sep 20, 1:07 PM

This is clearly a breaking change, no argument there. Fortunately, it is a configuration-dependent one and we've only affected ourselves, and in a limited fashion. I'm glad we were as conservative in rolling it out as we were, and I'm suddenly thankful for the the Kask performance investigation (now complete) that delayed further rollout.
So, what are the to-do items for MW?

  • update WebRequest::setSessionData() documentation to indicate "proper" objects are not allowed
    • is there a better doxygen/phpdoc way to indicate this than "mixed" and then saying something about "no objects" in the comments?
    • given that objects do currently work for other backends, should the language be soft ("objects are not guaranteed to work")?
  • corresponding update to Session::set() documentation, and probably also a note in the class comment
  • corresponding update to functions with RESTBagOStuff, and probably also a note in the class comment
  • add something to RELEASE-NOTES?
  • does this justify an email to wikitech-l?
  • what else?

@Reedy mentioned possibly throwing deprecated warnings, but I'm not sure what to do in-code. Session storage seems performance-critical enough that I'd rather not do introspection on the session data to be serialized/encoded. Is there something both effective and inoffensive that we could/should do? If so, would it live in RESTBagOStuff (because only wikis that use that backend are currently affected) or Session (because many more wikis may eventually be affected if we truly purge serialize/unserialize)?

I think most of this sounds like a good way forward. I would suggest potentially mailing mediawiki-l too as well as wikitech-l to get a bit of a wider audience, for people who care about MediaWiki, but not so much the Wikimedia usage; especially if we're going to (eventually) be moving away from php serialisation completely (though that might be a major release of MW or two away) in the future

Should make sure any relevant docs on mediawiki.org are updated too (if there are any that need updating)

The deprecated warnings and related code to that might be somewhat OTT (as well as a potential performance hit as you suggest). If we have things sufficiently and correctly documented, published etc that this behaviour isn't supported (at least as currently it works), that goes a long way to mitigating and helping developers. When looking into this problem, I certainly couldn't tell from the MW code (which obviously the path is vague as to which underlying classes may be used during the config) that anything might've changed etc, and it wasn't until you mentioned it on IRC I had any idea that this was changing (I don't read every wikitech post etc). Was this actually announced anywhere that I probably should've seen it?

  • given that objects do currently work for other backends, should the language be soft ("objects are not guaranteed to work")?

Something like that, potentially. If the class is implementing JsonSerializable, and as such is returning something that the json code can/will seriali[sz]e... It should work, and that seems like an acceptable solution/similar for the path forward. Comments are cheap, so adding that hint certainly wouldn't be a bad thing