Page MenuHomePhabricator

Logging-out via sticky header and user menu without confirm step on Special:UserLogout
Closed, ResolvedPublic5 Estimated Story Points

Description

Two issues:

  • The standard logout goes to Special:Userlogout, it used to work ,but was broken due to a refactor of HTML
  • The sticky header logout never worked.

problems

  • The sticky header is added via JavaScript after page load
  • Since Vector 2022 doesn't wire up the logout code itself, any HTML changes can break the integration.

new proposal

Currently the way this works is code runnning in mediawiki core runs on page load and adds a click handler to anything which matches the selector. The selector can be configured on a skin basis (and is done that way inside Vector). It's proposed that we do one of two things:

  1. Switch the click handler to run on document - the performance team has stated they prefer we don't do this
  2. Switch to an event based approach mw.hook('mw.logout').add and add additionalevent bindings inside Vector 2022.

More background

When I try to log-out via sticky header, I always have to click twice: once on the log-out button, then on the blue button from Special:UserLogout. This is annoying.

(source)

On the other hand though, to me, this looks like a possible solution to the problem of too many misclicks.

Is this intentional? If yes, can we keep this as a two-step process but without redirecting the user to different wiki page?

QA Results - Prod

ACStatusDetails
1T324638#8606737
2T324638#8606737

Event Timeline

Jdlrobson subscribed.

Seems like this has regressed.

ovasileva renamed this task from Logging-out via sticky header without going to Special:UserLogout? to Regression: Logging-out via sticky header without going to Special:UserLogout?.Dec 13 2022, 5:39 PM
ovasileva triaged this task as High priority.
ovasileva moved this task from Incoming to Current Fiscal Year on the Web-Team-Backlog board.

This was not a good regression as this only impacts the sticky header logout link so removing the tag (I initially thought this was the default user menu link).

The sticky header is built in JavaScript
The code that wires up the logout code must run on code generated by the server (for security reasons), https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.page.ready/ready.js#L167

We would need to render the sticky header on the server (instead of client) if we wanted to make this work (quite a lot of work) or add our own bespoke JavaScript solution here (not good from a technical standpoint).

How important is this behaviour? Is it worth the technical debt and/or a rewrite of the sticky header?

@Jdlrobson another idea I had is that the logout link selector is configurable via $config['selectorLogoutLink'] in the onSkinPageReadyConfig hook. We could potentially add two selectors to that string, one for the normal menu and one for the sticky header.
e.g.

$config['selectorLogoutLink'] = '#p-personal .vector-user-menu-logout a[data-mw="interface"], #p-personal-sticky-header .vector-user-menu-logout a[data-mw="interface"]';

Then, if we change the jquery event handler in ready.js to use event delegation instead, the element doesn't have to be present at pageload to trigger the event:
$( 'document' ).on( 'click', config.selectorLogoutLink, function ( e ) {

But for some reason, I can't get this working locally. 🤷‍♂️

Jdlrobson renamed this task from Regression: Logging-out via sticky header without going to Special:UserLogout? to Logging-out via sticky header without going to Special:UserLogout.Jan 12 2023, 5:43 PM

. Then, if we change the jquery event handler in ready.js to use event delegation instead, the element doesn't have to be present at pageload to trigger the event:

I believe a live click handler was intentionally not used here, as it's intentional that gadgets can't wire up. That said template can't create this markup as they are unable to create data-mw attributes. Maybe @Ladsgroup has some more context on whether it would be okay to modify this code in this way e.g. switching from

$( config.selectorLogoutLink ).on( 'click', function ( e ) {

to

$( 'document' ).on( 'click', config.selectorLogoutLink, function ( e ) {
ovasileva lowered the priority of this task from High to Medium.Jan 12 2023, 6:13 PM

I don't remember security considerations of gadgets, if a gadget is compromised they can do way more damage (the most logout can provide is a csrf token which is already accessible in the js) We don't allow gadgets in pages that deal with passwords or email address). There was a security reason to change logout though, to add CSRF (T25227 and T222626) but it shouldn't be a big deal.

Change 880993 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Allow trusted logout scripts added via JavaScript to be wired up

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

LGoto set the point value for this task to 5.Jan 26 2023, 6:26 PM
Jdlrobson renamed this task from Logging-out via sticky header without going to Special:UserLogout to Logging-out via sticky header and user menu without going to Special:UserLogout.Jan 26 2023, 9:23 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Tgr.
Jdlrobson renamed this task from Logging-out via sticky header and user menu without going to Special:UserLogout to Logging-out via sticky header and user menu without confirm step on Special:UserLogout.Jan 26 2023, 9:38 PM

Change 884100 had a related patch set uploaded (by Jdlrobson; author: Jdrewniak):

[mediawiki/skins/Vector@master] Add missing ID param to VectorComponentMenuListItem constructor

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

Change 884100 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Add missing ID param to VectorComponentMenuListItem constructor

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

Change 880993 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.page.ready: Add hook to trigger logout from other code

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

Change 886182 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Sticky header logout goes via API avoiding a second click

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

Jdlrobson moved this task from Doing to Code Review on the Web-Team FY2022-23 Q3 Sprint 2 board.

Change 886182 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Sticky header logout goes via API avoiding a second click

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

bwang removed bwang as the assignee of this task.Feb 7 2023, 4:34 PM
bwang subscribed.
Edtadros subscribed.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Logout from User Menu without confirm step.

Screen Recording 2023-02-10 at 4.32.16 PM.mov.gif (966×1 px, 508 KB)

✅ AC2: Logout from Sticky Header without confirm step.

Screen Recording 2023-02-10 at 4.32.48 PM.mov.gif (966×1 px, 362 KB)