#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)
Change History (109)
#3
@
12 years ago
- Milestone changed from Awaiting Review to 3.6
- Type changed from enhancement to task (blessed)
- Version trunk deleted
#10
follow-up:
↓ 11
@
12 years 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?
#11
in reply to:
↑ 10
@
12 years 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.
#12
@
12 years 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.
#14
@
12 years 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.
#15
@
12 years 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.
#16
@
12 years 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.
#17
@
12 years 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.
#21
in reply to:
↑ 19
;
follow-up:
↓ 22
@
12 years ago
Replying to azaozz:
In 23504:
FYI, get_called_class()
supported on PHP 5.3+. Fatal error on PHP 5.2.4 (as per WordPress' requirements).
#22
in reply to:
↑ 21
@
12 years 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.
#23
@
12 years 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.
#25
@
12 years 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.
#33
follow-up:
↓ 36
@
12 years 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.
#36
in reply to:
↑ 33
@
12 years 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.
#37
@
12 years 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.
#38
@
12 years 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.
#39
@
12 years 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.
#44
@
12 years 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.
#45
@
12 years 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?
#46
@
12 years 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.
#47
@
12 years ago
23295-7.patch looks good to me. I no longer see the administrator message when the cookie expires.
#49
@
12 years ago
The commit message for [24179] doesn't mention why the sandbox attribute was removed?
#50
@
12 years 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.
#54
@
11 years ago
In 23295-9.patch: Fix same domain comparison in wp_auth_check_html() when FORCE_SSL_LOGIN && ! FORCE_SSL_ADMIN
.
#58
@
11 years 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.
#60
follow-up:
↓ 61
@
11 years 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?
#61
in reply to:
↑ 60
@
11 years 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.
#62
@
11 years 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.
#63
@
11 years 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.
#64
follow-up:
↓ 65
@
11 years 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.
Maybe disable the modal on $pagenow == network.php?
#65
in reply to:
↑ 64
@
11 years 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.
#66
@
11 years 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.
#68
@
11 years 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.
#69
@
11 years 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.
#71
@
11 years ago
In 23295-12.patch: add a simple whitelist for loading wp_auth_check. The list itself may need a bit of adjusting.
#72
@
11 years 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.
#73
@
11 years 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".
#74
@
11 years 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.
#75
follow-up:
↓ 81
@
11 years 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).
#76
@
11 years 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.
#77
follow-up:
↓ 78
@
11 years 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.
#78
in reply to:
↑ 77
@
11 years 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.
#79
@
11 years 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.
#81
in reply to:
↑ 75
@
11 years 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.
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.