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

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterReset SkinOptions after logging out

Event Timeline

pmiazga created this task.Apr 16 2019, 7:51 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 16 2019, 7:51 PM
pmiazga renamed this task from SkinOptions are initialized too early to Skin should reset SkinOptions on UserLogout action..Apr 16 2019, 7:51 PM

Tagging AMC since it's impacted too.

pmiazga updated the task description. (Show Details)Apr 16 2019, 7:56 PM
Rchard2scout updated the task description. (Show Details)Apr 16 2019, 8:03 PM
ovasileva triaged this task as Medium priority.Apr 17 2019, 2:14 PM
Jdlrobson reassigned this task from Jdlrobson to pmiazga.Jul 31 2019, 11:54 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

Jdlrobson reassigned this task from pmiazga to Niedzielski.Aug 1 2019, 10:23 PM

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

Jdlrobson updated the task description. (Show Details)Aug 1 2019, 10:24 PM
Niedzielski closed this task as Resolved.Aug 2 2019, 1:13 AM