Opened 4 months ago

Last modified 6 days ago

#23295 new task (blessed)

Improved login expiration warning

Reported by: mintindeed Owned by:
Priority: normal Milestone: 3.6
Component: Autosave Version:
Severity: normal Keywords: autosave-redo has-patch ui-feedback
Cc: dh-shredder, mike@…, aaroncampbell, sirzooro, nashwan.doaqan@…, DeanMarkTaylor

Description

The goal here is to improve the user experience when your login has / will expire, as discussed in WordPress 3.6: Autosave and Post Locking and the autosave and post locking team meeting in IRC.

The suggested tactic is to integrate the PMC Post Savior plugin into core. That part should be pretty straightforward.

I think 3 valuable enhancements that are on the plugin's roadmap, but not yet complete would be:

  • Alert properly on failed requests and lost connectivity. If my internet connection has gone down and I'm still working, I should be alerted that I'm "Working offline" and my changes aren't being automatically saved. I don't think this should be a blocking UI, but it should be obvious.
  • Block publish/save until login has been verified. Polling duration can be decreased (or maybe even done away with entirely) if we block the Publish/Save/etc actions until we've verified the user's cookie.
  • Pre-emptive notification. When approaching the user's login cookie expiration time, say 1 hour before, display a message and allow the user to extend their login. For example, how banking sites notify you when you've been inactive too long and they're about to log you out.

Attachments (15)

23295.patch (12.3 KB) - added by mintindeed 3 months ago.
23295-2.patch (8.7 KB) - added by azaozz 3 months ago.
login1.png (38.1 KB) - added by azaozz 3 months ago.
login2.png (43.7 KB) - added by azaozz 3 months ago.
login3.png (35.5 KB) - added by azaozz 3 months ago.
23295.diff (4.0 KB) - added by nacin 3 months ago.
23295-3.patch (4.6 KB) - added by azaozz 3 months ago.
login-expiration-notices.png (460.9 KB) - added by helen 2 months ago.
23295-4.patch (17.2 KB) - added by azaozz 2 months ago.
23295-5.patch (18.2 KB) - added by azaozz 8 weeks ago.
23295-docs.patch (868 bytes) - added by ocean90 7 weeks ago.
23295-6.patch (1.5 KB) - added by azaozz 4 weeks ago.
23295-7.patch (4.2 KB) - added by azaozz 3 weeks ago.
23295-8.patch (2.2 KB) - added by azaozz 2 weeks ago.
23295-9.patch (868 bytes) - added by azaozz 12 days ago.

Download all attachments as: .zip

Change History (72)

  • Type changed from defect (bug) to enhancement
  • Cc dh-shredder added
  • Milestone changed from Awaiting Review to 3.6
  • Type changed from enhancement to task (blessed)
  • Version trunk deleted

We can use the new Heartbeat API from JS to request time left to cookies expiration, or on the PHP side, send notification to the browser one hour before.

  • Cc mike@… added
  • Cc aaroncampbell added
  • Keywords autosave-redo added
  • Component changed from Warnings/Notices to Autosave
  • Cc sirzooro added

comment:10 follow-up: ↓ 11   mintindeed4 months ago

Right now this login warning only appears on the Edit Post page (and works on custom post types, pages, etc) because that's the most critical point where you really, really don't want to get logged out unexpectedly. But I'm thinking if it's a core feature, maybe it's appropriate for it to work on every admin page?

comment:11 in reply to: ↑ 10   nacin4 months ago

Replying to mintindeed:

Right now this login warning only appears on the Edit Post page (and works on custom post types, pages, etc) because that's the most critical point where you really, really don't want to get logged out unexpectedly. But I'm thinking if it's a core feature, maybe it's appropriate for it to work on every admin page?

Given that this can build off the heartbeat API, it can realistically work anywhere. If it is not too much, I would architect this in such a way that it is standalone so it can be used on more than just post.php and post-new.php, even if only those pages will use it for the time being. If 3.6 only supports post.php and post-new.php, that's certainly hitting our goal, though.

It was simpler just to load it everywhere in the admin, so I did that. It would be pretty easy to further extend it to show it on non-admin pages if the user is logged in.

I wrapped it in an object so that it's as self-contained as possible, and to make it easy to override & extend as needed. I'm including the class via wp-settings.php, then adding a "loader" helper function in functions.php, and adding an action to default-filters.php to actually load it. The action runs in "init" so that plugins, themes, etc can unhook it if desired. (Not "admin_init"; the WP_Auth_Check class is responsible for determining that it only wants to run in the admin, this also allows plugins to change when/how it's loaded by overriding WP_Auth_Check::_init().)

Block publish/save until login has been verified. Polling duration can be decreased (or maybe even done away with entirely) if we block the Publish/Save/etc actions until we've verified the user's cookie.

The JS will disable the save/publish/move to trash buttons (the plugin had this behaviour). That's probably fancy enough for now?

Pre-emptive notification. When approaching the user's login cookie expiration time, say 1 hour before, display a message and allow the user to extend their login. For example, how banking sites notify you when you've been inactive too long and they're about to log you out.

We can use the new Heartbeat API from JS to request time left to cookies expiration, or on the PHP side, send notification to the browser one hour before.

The plugin polled every 15 seconds, which is the same duration as the heartbeat, so I'm hooking into the heartbeat API to run the login check. It would be pretty simple to add a message this way; I'll take a stab at that after I make any necessary revisions to the attached patch.

  • Cc nashwan.doaqan@… added
  • Keywords has-patch ui-feedback added

Updated patch to address items discussed in #wordpress-dev

The major items are:

  • Removed reliance on Thickbox, allows simplification of the JS and looks a little nicer
  • Heartbeat response now returns the HTML for the modal dialogue

I've gone back and forth a little bit on how to display the modal dialogue and whether to use an overlay. Second opinions here are welcome. Here's my rationale for what I've done in the patch:

  • Disabling critical buttons (save, publish, trash, etc) instead of using an overlay, because one of the more common use-cases is: this dialogue pops up while typing in a post. Not using an overlay seems like it's less jarring in this context, doesn't interrupt typing, and still presents the nice big notice to the user.
  • I made the modal dialogue bigger, so it no longer needs to resize to fit the interim login iframe. The "growing" animation is kinda sexy, but a larger dialogue window is a lot more noticeable.

azaozz3 months ago

In 23295-2.patch:

  • Move most html, css and js to the response sent through heartbeat, only few lines of js are loaded initially.
  • Move the Close button outside of the wp-login iframe.
  • Take out button disabling: not needed with that big message. If we have to do that, logically it would have to be everywhere, not just on the Edit Post screen, and it would be better to add a "blocking" background (so all buttons and links are blocked).
  • Tweak opening and closing of the warning: when there are multiple tabs open to the same admin or if the user is superadmin on a network, logging in at one tab would auto-close the warnings everywhere.

azaozz3 months ago

azaozz3 months ago

azaozz3 months ago

We've talked with @dh-shredder earlier about how much UI is needed for this. Were wondering if we need the two steps. Currently first we show a warning:


then after clicking the button we show:


How about we skip the warning and show directly a smaller version of the login form's iframe:


That way it's one click only and the text "You will not leave this screen" is not needed (I know this will need some beautification / UI attention).

The login form can look completely different too. There is a body class interim-login that can be used to make it horizontal or show only the username/password fields.

Last edited 3 months ago by azaozz (previous) (diff)

+1 for the single, simpler login form.

Updating the message is the only other thing I can think of. The default session expired message seems a bit abstracted from what users are concerned about. Maybe something like "You should not lose any data or changes" would reassure basic users better.

Agree on a single simple form.

comment:19 follow-up: ↓ 21   azaozz3 months ago

In 23504:

Improved logged out warnings, first run, props mintindeed, see #23295

In 23505:

Logged out warnings: remove debug leftovers, see #23295

comment:21 in reply to: ↑ 19 ; follow-up: ↓ 22   Archetyped3 months ago

Replying to azaozz:

In 23504:

Improved logged out warnings, first run, props mintindeed, see #23295

FYI, get_called_class() supported on PHP 5.3+. Fatal error on PHP 5.2.4 (as per WordPress' requirements).

Version 0, edited 3 months ago by Archetyped (next)

nacin3 months ago

comment:22 in reply to: ↑ 21   azaozz3 months ago

FYI, get_called_class() requires PHP 5.3+.

I knew something didn't look right there...

Thanks @nacin for the patch. You're right, no need for a class just to wrap couple of functions. However this will have to work from the front-end for logged in users (things like P2), so it would probably need another PHP function or two.

For now thinking to make is a static class to fix the error, then revisit in the next few days.

Actually, nevermind. We only have to split the JS that shows the iframe from heartbeat.js so they can be added separately on the front end if needed. Will revisit in few days in any case.

In 23543:

Logged out warnings: restructure the PHP code (no need for a class), props nacin, see #23295

azaozz3 months ago

In 23295-3.patch:

  • Move the initial JS into heartbeat.js.
  • Pass is_user_logged_in() through localize_script.
  • Tweak the css so it doesn't cut the login form's drop shadow.

#23655 was marked as a duplicate.

#23656 was marked as a duplicate.

In 23625:

Unhyphenate "log-in". see #23295

comment:29 follow-up: ↓ 32   helen2 months ago

Got this assortment of notices on Make Core the other day. Not sure what caused the padding/white space issues (could have been pre-bumpbot or something), but the old red notice needs to be killed off for sure.

http://core.trac.wordpress.org/raw-attachment/ticket/23295/login-expiration-notices.png

In 23691:

Logged out warnings: clear previous errors when interim_login is set, see #23295

In 23692:

Logged out warnings, heartbeat: remove nopriv_autosave as it doubles the functionality of the logged out warnings, move wp_ajax_nopriv_heartbeat() under No-privilege Ajax handlers in ajax-actions.php, see #23295, see #23216

comment:32 in reply to: ↑ 29   azaozz2 months ago

Replying to helen:

[23691] removes the top warning in the login iframe. [23692] removes the warning from under the title on the Edit Post screen. The out-of-center placement inside the iframe is likely due to old/stale css, that dialog needs refresh anyway, will fix it then.

comment:33 follow-up: ↓ 36   esmi2 months ago

Sorry to sounds like Cassandara here but we really need to check that any new solution is accessible to disabled users. The a11y group have some concerns about whether the context changes, on session expiry, will be announced to screen reader users.

In 23699:

Logged out warnings: remove the logged out warning from autosave when in login_grace_period, see #23295

  • Cc DeanMarkTaylor added

comment:36 in reply to: ↑ 33   azaozz2 months ago

Replying to esmi:

Currently when the dialog is shown it is focused (the <div> has tabindex="0"). Not sure if this is enough, suggestions welcome.

Been thinking how to organize this code better. This dialog is shown very rarely, it's not warranted loading the html, css and js on all screens all the time. Currently most of it is sent through heartbeat and injected into the DOM. This is acceptable for testing but not a good idea for production.

Another big concern is that there may be "frame busting" JS on the Log In screen added by a plugin as defence against click-jacking. These JS snippets were quite popular couple of years ago, still see them around although they are not very effective. If that happens, the Log In screen will open instead of the iframe and the user will loose any unsaved changes (and our "You will not move away from this screen" looks really silly).

To work around that we can use an XHR instead of having an iframe (XHRs can be used to set cookies). Another technique is to create an iframe then submit a <form> with target="iframe-name". In this case the main page is not reloaded and cookies are set as usual. The only problem with these approaches is that if there is branding on the Log In screen, it will have to be applied to the "local" log in form, i.e. some themes and/or plugins would need to be updated.

azaozz2 months ago

In 23295-4.patch:

  • Move the JS and CSS into separate files.
  • Output the HTML in the footer of each page.
  • Add onbeforeunload warning to counter JS redirects.
  • Let the user open new tab with the login screen when cross-domain.

azaozz8 weeks ago

In 23295-5.patch:

  • Properly load on the front-end when the user is logged in. This also loads heartbeat and jQuery. Will need a review if we drop front-end jQuery loading for audio/video.
  • Focus the iframe or the first line of the text on load. The iframe has a title which would be announced by screen readers.

In 23805:

Logged out warnings: add fallback text dialog for:

  • The login page has "X-Frame-Options: DENY" header.
  • Cross-domain when displaying on the front-end on multisite with domain mapping.
  • The site forces ssl login but not ssl admin.

Add onbeforeunload prompt to counter (frame-busting) JS redirects. Move the JS and CSS into separate files. See #23295.

In 23881:

Logged out warnings: by default, load only in the admin. Plugins and themes can load on the front-end when needed, see #23295

ocean907 weeks ago

23295-docs.patch updates the inline doc for the change in [23881].

In 23922:

Logged out warnings: fix phpdoc, props ocean90, see #23295

azaozz4 weeks ago

The current login expiration popup works well but seems unwarranted to check a few times every minute for something that's usually 48 hours away and sometimes up to 336 hours (two weeks) away. In addition it abruptly interrupts the users.

A much better user experience would be to not reach the login expiration time at all. This is not possible at the moment. The next best thing would be to prompt the user to log in in advance on page load, before she/he makes any changes and attempts to save them.

23295-6.patch introduces wp_logout_time() that returns the time left until login expiration and _login_again_early() that redirects to the login screen 30 minutes before expiration.

If we're running heartbeat for other reasons anyway (like updating post locks), then it makes sense to additionally check the login and offer an easy re-login as well, since it's very little additional effort, resource-wise, and is still a better experience than a hard redirection and loss of place in the panel.

Are you arguing we shouldn't use a login popup at all, or just under circumstances where heartbeat wouldn't otherwise be running?

azaozz3 weeks ago

In 23295-7.patch:

  • Don't remove login error messages coming from wp_signon().
  • When the login form is shown in iframe, open all links in a new tab/window.
  • Add filter for the login form error message.

23295-7.patch looks good to me. I no longer see the administrator message when the cookie expires.

In 24179:

Logged out warnings:

  • Don't remove login error messages coming from wp_signon().
  • When the login form is shown in iframe, open all links in a new tab/window.
  • Add filter for the login form error message.

See #23295

The commit message for [24179] doesn't mention why the sandbox attribute was removed?

When set to prevent parent window access, the sandbox attribute also prevents links with target="_blank" (or any other target) from working. They stop functioning completely, behave as normal text.

azaozz2 weeks ago

In 24208:

Logged out warnings:

  • Don't use <base> tag to set target="_blank". It can break form submission. Instead, set target only on links with JS.
  • Fix same domain comparison in wp_auth_check_html() when FORCE_SSL_LOGIN == true.
  • Properly show/hide the "Close" button when the dialog is shown multiple times.

See #23295

In 24209:

Post locks and autosave:

  • Move nonces refreshing from autosave to lock checking.
  • Do autosave only when there is something to save.

See #23295

Uh, that should be #23697.

azaozz12 days ago

In 23295-9.patch: Fix same domain comparison in wp_auth_check_html() when FORCE_SSL_LOGIN && ! FORCE_SSL_ADMIN.

In 24266:

Logged out warnings: fix same domain comparison in wp_auth_check_html() when FORCE_SSL_LOGIN && ! FORCE_SSL_ADMIN. See #23295

In 24271:

Logged out warnings: by default run the logged-out check every 3 min. Tag along if something else is using heartbeat. See #23295

In 24273:

Separate the nonces update from checking the post lock. Fix scheduling the logged out check. See #23697, see #23295.

Note: See TracTickets for help on using tickets.