Page MenuHomePhabricator

OAuth extension uses session object store directly
Closed, ResolvedPublic

Description

In doing some analysis for making the main stash DC-aware, I came across this code in the OAuth extension:

https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/OAuth/+/refs/heads/master/includes/backend/MWOAuthUtils.php#44

It's explicitly getting the session storage using $wgSessionCacheType and using that for writing its own data. This will probably cause some problems with the test roll out.

I think the right fix here is to change it to use its own configuration variable for its storage backend, and fall back to MediaWikiServices::getInstance()->getMainObjectStash();

As far as I can tell, none of the 176 other extensions we use in production use the SessionCacheType config variable explicitly, and it also doesn't appear in unexpected places in MediaWiki core.

Event Timeline

Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptJul 10 2019, 6:02 PM

Thanks @EvanProdromou , I'm working on a change for this.

Change 521950 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/extensions/OAuth@master] Allow separating OAuth and per-wiki session storage.

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

Change 521409 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[operations/mediawiki-config@master] Specify CentralAuth and OAuth session storage separately from per-wiki session storage.

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

OAuth change posted, OAuth configuration added to the existing configuration change.

As with T227097, it does not matter what order these deploy in. If the extension change(s) deploy first, the new variables are optional so they'll just fall back to the existing $wgSessionCacheType variable that they are already using. If the configuration change deploys first, it will simply set new variables that nothing reads. Once they deploy, OAuth and CentralAuth will still point at the same place they are already pointing, so there will be no change in behavior. What we will have accomplished is the ability to move only per-wiki session storage, for first testing then deploying Kask, without affecting CentralAuth or OAuth.

Change 521950 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] Allow separating OAuth and per-wiki session storage.

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

EvanProdromou closed this task as Resolved.Tue, Jul 30, 4:48 PM

So, I think this is fixed? I'm going to mark it resolved.

BPirkle reopened this task as Open.Tue, Jul 30, 5:06 PM

We still need the config change to be merged/deployed.

Change 521409 merged by jenkins-bot:
[operations/mediawiki-config@master] Specify CentralAuth and OAuth session storage separately from per-wiki session storage.

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

Mentioned in SAL (#wikimedia-operations) [2019-07-30T23:13:20Z] <catrope@deploy1001> Synchronized wmf-config/InitialiseSettings.php: Specify CentralAuth and OAuth session storage separately from per-wiki session storage (T227097, T227696) (duration: 00m 47s)

BPirkle closed this task as Resolved.Wed, Jul 31, 1:04 PM

The config change has now been merged and deployed.