WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 9 months ago

Last modified 9 months ago

#23295 closed task (blessed) (fixed)

Improved login expiration warning

Reported by: mintindeed Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Autosave Keywords: has-patch commit needs-codex
Focuses: Cc:

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 (24)

23295.patch (12.3 KB) - added by mintindeed 14 months ago.
23295-2.patch (8.7 KB) - added by azaozz 14 months ago.
login1.png (38.1 KB) - added by azaozz 14 months ago.
login2.png (43.7 KB) - added by azaozz 14 months ago.
login3.png (35.5 KB) - added by azaozz 14 months ago.
23295.diff (4.0 KB) - added by nacin 14 months ago.
23295-3.patch (4.6 KB) - added by azaozz 14 months ago.
login-expiration-notices.png (460.9 KB) - added by helen 13 months ago.
23295-4.patch (17.2 KB) - added by azaozz 13 months ago.
23295-5.patch (18.2 KB) - added by azaozz 13 months ago.
23295-docs.patch (868 bytes) - added by ocean90 13 months ago.
23295-6.patch (1.5 KB) - added by azaozz 12 months ago.
23295-7.patch (4.2 KB) - added by azaozz 12 months ago.
23295-8.patch (2.2 KB) - added by azaozz 11 months ago.
23295-9.patch (868 bytes) - added by azaozz 11 months ago.
23295-10.patch (8.2 KB) - added by azaozz 10 months ago.
23295-11.patch (2.8 KB) - added by azaozz 9 months ago.
23295-12.patch (1.5 KB) - added by azaozz 9 months ago.
23295-13.patch (6.8 KB) - added by azaozz 9 months ago.
logged-out-modal.png (15.6 KB) - added by azaozz 9 months ago.
23295.2.diff (5.0 KB) - added by nacin 9 months ago.
23295.3.diff (5.1 KB) - added by nacin 9 months ago.
23295.4.diff (4.6 KB) - added by nacin 9 months ago.
23295.5.diff (4.6 KB) - added by nacin 9 months ago.

Download all attachments as: .zip

Change History (109)

comment:1 mintindeed15 months ago

  • Type changed from defect (bug) to enhancement

comment:2 dh-shredder15 months ago

  • Cc dh-shredder added

comment:3 azaozz15 months ago

  • 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.

comment:4 mbijon15 months ago

  • Cc mike@… added

comment:5 aaroncampbell15 months ago

  • Cc aaroncampbell added

comment:6 rmccue15 months ago

  • Keywords autosave-redo added

comment:8 SergeyBiryukov15 months ago

  • Component changed from Warnings/Notices to Autosave

comment:9 sirzooro15 months ago

  • Cc sirzooro added

comment:10 follow-up: mintindeed15 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 nacin15 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.

comment:12 mintindeed15 months ago

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.

comment:13 alex-ye15 months ago

  • Cc nashwan.doaqan@… added

mintindeed14 months ago

comment:14 mintindeed14 months ago

  • 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.

azaozz14 months ago

comment:15 azaozz14 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.

azaozz14 months ago

azaozz14 months ago

azaozz14 months ago

comment:16 azaozz14 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 14 months ago by azaozz (previous) (diff)

comment:17 mbijon14 months ago

+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.

comment:18 helen14 months ago

Agree on a single simple form.

comment:19 follow-up: azaozz14 months ago

In 23504:

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

comment:20 azaozz14 months ago

In 23505:

Logged out warnings: remove debug leftovers, see #23295

comment:21 in reply to: ↑ 19 ; follow-up: Archetyped14 months ago

Replying to azaozz:

In 23504:

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

FYI, get_called_class() requires PHP 5.3+. Causes fatal error on PHP 5.2.4 (as per WordPress' requirements).

Last edited 14 months ago by Archetyped (previous) (diff)

nacin14 months ago

comment:22 in reply to: ↑ 21 azaozz14 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.

comment:23 azaozz14 months ago

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.

comment:24 azaozz14 months ago

In 23543:

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

azaozz14 months ago

comment:25 azaozz14 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.

comment:26 markoheijnen14 months ago

#23655 was marked as a duplicate.

comment:27 markoheijnen14 months ago

#23656 was marked as a duplicate.

comment:28 markjaquith14 months ago

In 23625:

Unhyphenate "log-in". see #23295

comment:29 follow-up: helen13 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

comment:30 azaozz13 months ago

In 23691:

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

comment:31 azaozz13 months ago

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 azaozz13 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: esmi13 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.

comment:34 azaozz13 months ago

In 23699:

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

comment:35 DeanMarkTaylor13 months ago

  • Cc DeanMarkTaylor added

comment:36 in reply to: ↑ 33 azaozz13 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.

comment:37 azaozz13 months ago

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.

azaozz13 months ago

comment:38 azaozz13 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.

azaozz13 months ago

comment:39 azaozz13 months 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.

comment:40 azaozz13 months ago

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.

comment:41 azaozz13 months ago

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

ocean9013 months ago

comment:42 ocean9013 months ago

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

comment:43 azaozz13 months ago

In 23922:

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

azaozz12 months ago

comment:44 azaozz12 months 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.

comment:45 dh-shredder12 months ago

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?

azaozz12 months ago

comment:46 azaozz12 months 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.

comment:47 ryan12 months ago

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

comment:48 azaozz11 months ago

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

comment:49 nacin11 months ago

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

comment:50 azaozz11 months ago

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.

azaozz11 months ago

comment:51 azaozz11 months 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

comment:52 azaozz11 months ago

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

comment:53 azaozz11 months ago

Uh, that should be #23697.

azaozz11 months ago

comment:54 azaozz11 months ago

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

comment:55 azaozz11 months ago

In 24266:

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

comment:56 azaozz11 months ago

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

comment:57 azaozz11 months ago

In 24273:

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

azaozz10 months ago

comment:58 azaozz10 months ago

In 23295-10.patch:

  • Update the heartbeat nonce when refreshing nonces on the Edit Post screen.
  • After a user logs in from the auth-check dialog, speed up heatrbeat to check/refresh nonces on the Edit Post screen.
  • Add 'heartbeat-nonces-expired' custom jQuery event when nonces have expired and the user is logged in.

comment:59 azaozz10 months ago

In 24528:

Nonce refresh:

  • Update the heartbeat nonce when refreshing nonces on the Edit Post screen.
  • After a user logs in from the auth-check dialog, speed up heatrbeat to check/refresh nonces on the Edit Post screen.
  • Speeding up heartbeat: bring back the setting how long it should last (how many ticks).
  • Add 'heartbeat-nonces-expired' jQuery event when nonces have expired and the user is logged in.

See #23295, see #23216.

comment:60 follow-up: nacin9 months ago

I've been leaving windows open for weeks, and I think this is fantastic.

My only recommendation is:

  • Don't require the user to close the modal. We are graying out the screen. If the login is successful, simply close the modal and restore the screen in the background. It'll be *very* obvious that they've successfully logged in. It's kind of annoying to tell me I've logged in successfully after I hit a "Log in" button — just get me to where I need to be. Thoughts?

comment:61 in reply to: ↑ 60 helen9 months ago

Replying to nacin:

My only recommendation is:

  • Don't require the user to close the modal.

It closed for me after a few seconds. Could be faster, I guess.

Strange thing, not sure if it's our problem - when using 1Password to fill the modal, it occasionally works, but more often says password is empty.

comment:62 azaozz9 months ago

Don't require the user to close the modal.

Yes, the confirmation message is kind of redundant, I would prefer to just close the dialog too. Both the old popup window and the @mintindeed's plugin show a confirmation. Was thinking it makes it clear to more inexperienced users but perhaps they are a minority, i.e. applying the 80/20 rule at least 80% of the users wouldn't need confirmation.

...when using 1Password to fill the modal, it occasionally works, but more often says password is empty.

Suspecting 1Password has some issues with iframes. Works as expected when the password is saved in the browser.

azaozz9 months ago

comment:63 azaozz9 months ago

In 23295-11.patch:

  • Close the iframe immediately on successful login.
  • Catch "silent" iframe origin exceptions in WebKit after another page is loaded in the iframe.

comment:64 follow-up: wpdavis9 months ago

The modal currently can come up over the text boxes when someone is enabling a network after they paste in the wp-config.php settings, which logs them out, but before they paste in the htaccess rules, disabling the ability to copy out the provided rules.

http://wpdavis.com/i//Create_a_Network_of_WordPress_Sites_%E2%80%B9_Testing_%E2%80%94_WordPress-20130710-183740.jpg

Maybe disable the modal on $pagenow == network.php?

comment:65 in reply to: ↑ 64 nacin9 months ago

Replying to wpdavis:

The modal currently can come up over the text boxes when someone is enabling a network after they paste in the wp-config.php settings, which logs them out, but before they paste in the htaccess rules, disabling the ability to copy out the provided rules.

That's a fantastic catch.

comment:66 nacin9 months ago

Hmm, this could cause problems with plugin/theme/core upgrades too. This isn't the only page it'll need to be disabled on.

I think a blacklist is okay, but should heartbeat only run on a whitelist of pages? I don't have a problem with scaling it back a bit for 3.6 if necessary.

comment:67 wpdavis9 months ago

How about a blacklist with a filter?

comment:68 azaozz9 months ago

A blacklist of network.php, update.php and update-core.php would cover this.

There is 1 hour grace period for POST and XHR requests after the cookies expire. This makes the login expiration checks much less important on screens where the users spend less time, probably all screens except the Edit Post screen.

Alternatively we can limit it only to this screen by default, and maybe the Dashboard because of QuickPress.

comment:69 rmccue9 months ago

A blacklist of network.php, update.php and update-core.php would cover this.

This has the ability to have the same problem with plugins. I'd be in favour of a whitelist-based approach, at least for now.

Last edited 9 months ago by rmccue (previous) (diff)

comment:70 azaozz9 months ago

In 24655:

Logged out warnings:

  • Close the iframe immediately on successful login.
  • Catch iframe origin exceptions in WebKit when there is a server error or another page is loaded in the iframe.

See #23295.

azaozz9 months ago

comment:71 azaozz9 months ago

In 23295-12.patch: add a simple whitelist for loading wp_auth_check. The list itself may need a bit of adjusting.

comment:72 nacin9 months ago

wp_auth_check_load_whitelist misses comment.php. It should also probably use get_current_screen().

Rather than a formal array whitelist, it should probably be a filter that returns true or false for that current screen, and which receives the screen object as an argument.

No need for the is_admin() check, I imagine — just always attach it to both admin_print_footer_scripts and wp_print_footer_scripts.

I can work on this later today.

comment:73 azaozz9 months ago

I'm still unsure if a whitelist approach is best here. Showing login expiration warnings inconsistently is not that good UX. Cases where the user is blocked by the modal from accessing the screen in a critical moment are quite rare but may happen on any screen.

In that terms a better solution would be to always show the Close button. Currently it is always shown in the fallback text version (the login form is on another domain) so the user can close the modal after logging in from another tab/window.

If the modal is closed before the user logs in, i.e. the user ignored it, it will reopen on the next check. Currently the check runs every 5 min. on most screens. If heartbeat is connecting for other reasons, a check is done too. Can remove that and leave it to only check every 5 min. This will be enough time to "not bother" the user if he/she ignored it but alert them that the login has expired.

So clicking Close when not logged in would be "Don't bother me with this for 5 min".

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

comment:74 mintindeed9 months ago

Sorry, I dropped off the planet for a while, but I've been following along when I can, and the current state of this looks fantastic.

Showing login expiration warnings inconsistently is not that good UX.

I totally agree with this. Playing a little bit of devil's advocate, in our experience being logged out unexpectedly is only really an issue when doing content editing or any kind of lengthy/tedious tasks (including ajax-y tasks like moderating comments). Editing posts, moderating comments, editing comments, interacting with settings/content in plugin settings pages, and administering widgets.

Can remove that and leave it to only check every 5 min.

5 min is OK if the user dismisses the dialog, but speaking from experience (dozens/hundreds of users over multiple sites) a 5 min delay for the initial check is too long. 15 seconds is about right, but even if it's increased, as long as the duration is filterable, it will fit anyone's use-case, so the defaults can be whatever works for the majority. For context, we have had multiple times when a user has been logged in, clicked on "Edit Post", made a content change, and gotten kicked to the login screen when hitting Update — the whole interaction taking just a few seconds.

comment:75 follow-up: wpdavis9 months ago

I agree that erring on the side of showing the modal is better. Anytime there is a submit button, there is the potential to get kicked to the login page and lose any edits you made, and that is _always_ frustrating.

The absolute worst case is that somebody will have to stop what they're doing again and log back in. If I'd done that on the network page the modal would have gone away and I could have accessed the code. I think it's worth fixing in the few areas that there might be an action someone would want to continue even after they've been logged out, but to be honest, those are few and far between.

For that reason, I think a close box is a little extraneous — are there any circumstances in which someone would _not_ want to log in to continue? Would that person really not know how to delete the element out of the DOM using dev tools?

Idea: What if we had the heartbeat on all form submits in the admin? Delay the form submit until we're sure the person is logged in. If they're not, show the modal and submit the form after a successful login (or prompt them to resubmit).

comment:76 azaozz9 months ago

5 min is OK if the user dismisses the dialog, but speaking from experience (dozens/hundreds of users over multiple sites) a 5 min delay for the initial check is too long.

Ah, I see where the problem is. The login grace period, doesn't work when the user clicks "Remember Me" on the login screen. The cookies are set with the same expiration time as the $expiration component in the actual cookie. When that time comes, the browser sends no cookies and the $expired += HOUR_IN_SECONDS; is pointless. This works when the Remember Me in not checked as the cookies are set for the session, i.e. last as long as the browser is open.

There are two ways to fix this:

  • Set $GLOBALS['login_grace_period'] at least 15 min earlier than the $expiration component of the cookie, giving the user some head start to log in.
  • Set the cookie expiration 1 hour after the $expiration component when Remember Me is checked.
Last edited 9 months ago by azaozz (previous) (diff)

azaozz9 months ago

comment:77 follow-up: azaozz9 months ago

In 23295-13.patch:

  • Replace the Close button with an always visible "X" icon in the top/right corner. If a user closes the modal without logging in, it will open again on the next check.
  • Do the checks every 3 min by default.
  • Add wp_auth_check_interval filter so the interval can be set from PHP.


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

azaozz9 months ago

comment:78 in reply to: ↑ 77 nacin9 months ago

Replying to azaozz:

  • Replace the Close button with an always visible "X" icon in the top/right corner. If a user closes the modal without logging in, it will open again on the next check.

I don't understand the goal of allowing people to use WordPress if they aren't logged in.

comment:79 azaozz9 months ago

The always visible "X" is mostly for user convenience when the modal interrupts them at a critical moment, like the case reported by @wpdavis. Thinking that adding white/black list will always be hit-and-miss and one more thing plugins to worry about.

comment:80 azaozz9 months ago

In 24695:

Logged out warnings:

  • Replace the Close button with an always visible "X" icon in the top/right corner.
  • Check if the user is still logged in every 3 min. by default.
  • Add 'wp_auth_check_interval' filter so the interval can be set from PHP.

See #23295.

comment:81 in reply to: ↑ 75 azaozz9 months ago

Replying to wpdavis:

Idea: What if we had the heartbeat on all form submits in the admin? Delay the form submit until we're sure the person is logged in. If they're not, show the modal and submit the form after a successful login (or prompt them to resubmit).

Yeah, a "pre-flight" approach would work when submitting a form although it may delay the form submission a lot. It wouldn't need heartbeat, can be implemented with $('form').on('submit', function(){ check logged in });.

One problem is that it won't catch XHRs and there's no good way to plug into all of these requests to see if the user is still logged in. On the plus side a pre-flight will catch when connection is down or the server is busy/not responding.

nacin9 months ago

nacin9 months ago

nacin9 months ago

comment:82 azaozz9 months ago

  • Keywords commit added; ui-feedback removed

23295.4.diff works great. The check runs on every heartbeat request regardless of what data is sent; the JS part listens to all heartbeat responses and opens the modal. Screens can be excluded with 'wp_auth_check_load' filter.

nacin9 months ago

comment:83 ryan9 months ago

In 24738:

Fire wp_auth_check_load() from admin_enqueue_scripts instead of admin_init so that it can access the current screen object.

Black list the update and upgrade screens.

Allow plugins to white/black list screens via the wp_auth_check_load filter.

Props nacin

see #23295

comment:84 nacin9 months ago

  • Resolution set to fixed
  • Status changed from new to closed

comment:85 DrewAPicture9 months ago

  • Keywords needs-codex added; autosave-redo removed
Note: See TracTickets for help on using tickets.