Page MenuHomePhabricator

Skin should reset SkinOptions on UserLogout action.
Closed, ResolvedPublic

Description

There is an edge case on the Special:UserLogout page, when Minerva $skinOptions object in initialized improperly. The SkinOptions keeps user preferences because it's initialized before MediaWiki core logs out the user.

Code flow:
  1. User visits Special::UserLogout page, MediaWiki code starts running
  2. During RequestContextCreateSkin hook, MobileFrontend calls RequestContextCreateSkinMobile hook. which triggers MinervaHooks::onRequestContextCreateSkinMobile hook handler.
  3. User object is loaded from RequestContext, SkinOptions is set with proper options (beta flag, amc flag, etc)
  4. Special::UserLogout page calls $user->logout(),
  5. MediaWiki starts rendering page
  6. Based on SkinOptions set in step 2 (which is outdated because user object doesn't apply any more) we render page in the way logged user would see it (things like BETA features or AMC mode stays enabled).
Developer notes

Please note, that RequestContext::getMain()->getUser() will return the same User object with cleared various cached data. System do not nullify User object, only calls User::clearInstanceCache(). Most probably we should be fine, but please double check that.

Proposed solution:

The mobile skin initialization is one big hack around MediaWiki core and there is no easy and clean way to do. Most probably we cannot change the skin initialization logic, and I think we shouldn't postpone SkinOptions initialization. As an example, before MediaWiki core logs out the user, something might want to log user actions (like beta user just logged out), before MediaWiki logs out the user, the SkinOptions object is set properly.

We can listen to UserLogoutComplete hook, and when that happens - reset the SkinOptions object. When HTML rendering kicks in, the SkinOptions will contain default settings (no AMC mode, no BETA mode), and the page should be rendered correctly as an anon user.

QA Steps
  • Login to mobile Wikipedia as a AMC user
  • verify that the MainMenu has AMC entries in it and there's a personal menu icon
  • log out
  • stay on the page, verify that menu doesn't have AMC entries any more and there is no personal menu icon

Event Timeline

pmiazga renamed this task from SkinOptions are initialized too early to Skin should reset SkinOptions on UserLogout action..Apr 16 2019, 7:51 PM
ovasileva triaged this task as Medium priority.Apr 17 2019, 2:14 PM
Jdlrobson added a subscriber: Jdlrobson.

Is this ready? How does it relate to T229023 if at all?

Change 527211 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Reset SkinOptions after logging out

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

Change 527211 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Reset SkinOptions after logging out

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

Would you mind signing this off via the same reproduction steps on the beta cluster (allowing time for the change to propagate there)?