Updated: Comment #57

Problem/Motivation

One-time login (password reset) links expired after a fixed amount of time--24 hours (86400 seconds)--and sites were unable to customize this value without hacking core.

Proposed resolution

Support user password reset link expiration time as variable (D7) or config (D8) value.

Implemented solution

The expiration of user reset links are now configurable.

In Drupal 7:

Set user_password_reset_timeout variable (i.e. in settings.php, or strongarm):
i.e. in settings.php:

$conf['user_password_reset_timeout'] = '604800';

In Drupal 8:

In user.settings.yml:

password_reset_timeout: '604800'

Remaining tasks

None.

User interface changes

None.

API changes

None.

None.

Original report by izmeez

Note: Changing user_password_reset_timeout does affect the expiration date of one-time-login links that have already been sent. This is because the link contains the time it was generated not the time it expires. Expiration is checked based on the current user_password_reset_timeout value when the link is followed.

When users are authenticated or request a new password they receive a url link that expires in 24 hours. This time limit is set by the value $timeout = 86400 (in seconds) in modules/user/user.pages.inc

What is the best way to change this value? I am reluctant to simply edit the core file but cannot find any other way to change this.

Can a patch be created to change this so that it can be applied to the current release and to any updates when released? Di I need a CVS install to create a patch or is there a way to create the patch with the standard install?

Any help would be appreciated. Thanks,

Izzy

Files: 
CommentFileSizeAuthor
#52 document-setting-246029-52.patch541 bytesLinL
PASSED: [[SimpleTest]]: [MySQL] 49,314 pass(es).
[ View ]
#50 246029-document-settting_50.patch529 bytesgreggles
PASSED: [[SimpleTest]]: [MySQL] 49,381 pass(es).
[ View ]
#48 246029-document-settting.patch963 bytespillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 42,175 pass(es).
[ View ]
#44 remove_bogus_profile-246029-44.patch562 bytesjunedkazi
PASSED: [[SimpleTest]]: [MySQL] 40,631 pass(es).
[ View ]
#32 246029-32.patch2.98 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 39,163 pass(es).
[ View ]
#29 use-variables-for-timeout-values-in-user-module-246029-29.patch3.02 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 36,179 pass(es).
[ View ]
#27 use-variables-for-timeout-values-in-user-module-246029-27.patch3.03 KBzserno
PASSED: [[SimpleTest]]: [MySQL] 36,184 pass(es).
[ View ]
#27 use-variables-for-timeout-values-in-user-module-246029-27-fail.patch2.27 KBzserno
FAILED: [[SimpleTest]]: [MySQL] 36,120 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#24 use-variables-for-timeout-values-in-user-module-246029-24.patch3.03 KBzserno
PASSED: [[SimpleTest]]: [MySQL] 36,080 pass(es).
[ View ]
#22 use-variables-for-timeout-values-in-user-module-246029-22.patch2.72 KBzserno
PASSED: [[SimpleTest]]: [MySQL] 36,064 pass(es).
[ View ]
#18 use-variables-for-timeout-values-in-user-module-246029-18.patch774 bytesrjgoldsborough
PASSED: [[SimpleTest]]: [MySQL] 33,805 pass(es).
[ View ]
#16 use-variables-for-timeout-values-in-user-module-246029-16.patch775 bytesrjgoldsborough
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es).
[ View ]
#12 drupal.variables_for_timeout_values_246029_12.patch746 bytesrfay
PASSED: [[SimpleTest]]: [MySQL] 33,281 pass(es).
[ View ]
#4 change-link-url-timeout-246029-4.patch778 bytesblisteringherb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch change-link-url-timeout-246029-4.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Comments

Version:6.2» 7.x-dev

Since I keep hearing the Drupal 7 code freeze is going to happen on September 1, I decided to go back over a few of my previous comments to find out how to have them considered for Drupal 7 if deemed worthy.

The ability to change the $timeout value for length of time before expiry of a new user login link is one item that I think is important.

In Drupal 6 I am using a core hack changing the value in modules/user/user.pages.inc, which I am sure is not the right way to do it, but I am still not learned enough to know what the Drupal way is.

What is cool though is that increasing the $timeout value here increases the length of the expiry time for a new user but leaves the expiry time for the "request new password" function unchanged at 24 hours. This is good, because if someone is requesting a new password they are likely to use it fairly quickly, whereas the steps to registering and approving a new user may be longer depending on the process and community.

Do I need to do anything else to promote this suggestion to the D7 team?

Thanks for considering.

Izzy

I was wondering if someone can suggest "the Drupal way" to change the core $timeout value without hacking core?

I was wondering if there is a way to do this in the settings.php file or in a tpl.php file?

Thanks,

Izzy

Version:7.x-dev» 7.4
StatusFileSize
new778 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch change-link-url-timeout-246029-4.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Here is a patch to fix this issue that adds a variable and defaults to 24 hours.

This should probably be marked as a dupe of #1195234: Use variables for timeout values in user module being that in order to get a new feature into d7 it 1st needs to be committed to d8 then ported back.

Well since this issue is older, the other issue should be mark as a dupe of this one and the version number should be switch to d8.

Version:7.4» 8.x-dev
Component:base system» user.module
Issue tags:+Needs tests

I marked #1195234: Use variables for timeout values in user module as a duplicate of this issue.

Also change the version to 8.x-dev because is a feature request, which will need to be back-ported to d7.

Title:Ability to change url link timeoutUse variables for timeout values in user module
Status:Active» Needs work

I think the title on that issue is better, though.

Status:Needs work» Needs review

Marking it as needs-review to see if it still passes.

It'll be nice if we can get some feedback by some of the core developers if this have any chance of getting in, or if there's a very specific reason why this timeout is hardcoded.

Status:Needs review» Needs work

The last submitted patch, change-link-url-timeout-246029-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new746 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,281 pass(es).
[ View ]

Here's #4 rolled as a valid git patch. No changes. I just applied it with git apply -p2 and then recreated with git diff.

Is there a possibility that this patch is going to be embedded in one of the next Drupal 7 updates? I just don't like messing around with the Drupal core.

@nico.knaepen, this is a feature request not a bug. So this is very unlikely, not impossible but unlikely.

Status:Needs review» Needs work
Issue tags:+Novice

This seems like a good change.

+++ b/modules/user/user.pages.incundefined
@@ -113,8 +113,8 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+    // Time out, in seconds, until login URL expires. If a timeout hasn't been set defaults to 24 hours = 86400 seconds.

Let's wrap this comment to 2 lines to fit the 80 character limit. (Looks like after the word "been" will do the trick.) Also, while we're updating the comment, let's make that initial "Time out" one word instead.

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch with the comment corrections mentioned above.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Status:Needs work» Needs review
StatusFileSize
new775 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,810 pass(es).
[ View ]

Rerolled for d8 with comment break added.

Status:Needs review» Needs work

Thanks @rjgoldsborough for fixing this. I noticed one small thing in the revised patch. There's trailing whitespace left over at the end of the line here:

+++ b/core/modules/user/user.pages.incundefined
@@ -113,8 +113,9 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+    // Time out, in seconds, until login URL expires. If a timeout hasn't been ¶

Status:Needs work» Needs review
StatusFileSize
new774 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,805 pass(es).
[ View ]

Whoops. White space removed.

Title:Use variables for timeout values in user moduleUse variables for timeout values in user module's password reset

More accurate title.

Status:Needs review» Reviewed & tested by the community

The patch in #18 applied cleanly, changed the hardcoded timeout to a configurable variable and does not have the trailing whitespace mentioned by xjm.

Status:Reviewed & tested by the community» Needs work

Thanks Devin! It still needs a test, though. :) Setting back to NW for the addition of test coverage.

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 36,064 pass(es).
[ View ]

Added a simple test, based on testUserCancelInvalid(). Changes:

  • changed variable name to 'user_password_reset_timeout', so it's obvious that it belongs to user.module
  • test sets this new variable to 3 * 86400 seconds (72 hours) then tries to use the reset password link after 72 hours + 1 minute later

Status:Needs review» Needs work

Thanks for the patch with the test, a few comments:

+++ b/core/modules/user/user.test
@@ -439,6 +439,50 @@ class UserLoginTestCase extends DrupalWebTestCase {
+    $this->drupalPost('user/password', $edit, t('E-mail new password'));

You should not use t in your assertions: http://drupal.org/node/265828

+++ b/core/modules/user/user.test
@@ -439,6 +439,50 @@ class UserLoginTestCase extends DrupalWebTestCase {
+    $this->assertText(t('Further instructions have been sent to your e-mail address.'), t('Password reset instructions mailed message displayed.'));

Why do we attempt to test the 'correct' behavior of the password reset functionality in a test method that's all about invalid password reset things? Maybe this bit of it should be moved out into another method (if we don't test it elsewhere already.)

+++ b/core/modules/user/user.test
@@ -439,6 +439,50 @@ class UserLoginTestCase extends DrupalWebTestCase {
+    $timestamp = time();

You should avoid using time() and use REQUEST_TIME instead, I believe.

+++ b/core/modules/user/user.test
@@ -439,6 +439,50 @@ class UserLoginTestCase extends DrupalWebTestCase {
+    $bogus_timestamp = $timestamp - variable_get('user_password_reset_timeout', 86400) - 60;
+
+    $this->drupalGet("user/reset/$account->uid/$bogus_timestamp/" . user_pass_rehash($account->pass, $bogus_timestamp, $account->login));

It's really not clear what is going on here. Could you add some documentation along the lines of:

Create a password reset link that will have expired as its too old.

Status:Needs work» Needs review
StatusFileSize
new3.03 KB
PASSED: [[SimpleTest]]: [MySQL] 36,080 pass(es).
[ View ]

Thanks for the great review. Re-rolled the patch according to it.

Status:Needs review» Needs work

testUserPasswordResetExpired passes even without the patch so it seems like the tests need improving.

It would be good to always upload a test-only patch to expose the (presumed) failure of the automated tests without the fix. Upload the test-only patch as the first attachment and the combined patch as the second. (See the testing policy for more info.)

Status:Needs work» Needs review
StatusFileSize
new2.27 KB
FAILED: [[SimpleTest]]: [MySQL] 36,120 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.03 KB
PASSED: [[SimpleTest]]: [MySQL] 36,184 pass(es).
[ View ]

@marcingy Well spotted, thanks. I changed the timeout in test to less than the default value, so it will fail without the patch.
Attached a fail test and the combined patch according to #26.

Thanks first of all for doing this - this change gets requested frequently, I think a lot of people will appreciate this (and reduce the common core hacks count by 1 ;) )

Just a couple of comment style issues I think, I'll post a quick patch for those. Not sure if this is too nitpicking ... if so, ignore

+++ b/core/modules/user/user.pages.inc
@@ -113,8 +113,9 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
+    // Time out, in seconds, until login URL expires. If a timeout hasn't been

The second sentence could use a comma before "defaults", but I think to be honest we can just do away with "if a timeout ...". The reason being: if we're talking about a default, then it's already clear that a value hasn't been set. I'd just go with "Defaults to ..."

+++ b/core/modules/user/user.test
@@ -439,6 +439,58 @@ class UserLoginTestCase extends DrupalWebTestCase {
+ * Test resetting a user password.

Comments need to be uniformly third person. There are a tonne of existing comments that don't conform to this, but it's the rule.

-5 days to next Drupal core point release.

StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 36,179 pass(es).
[ View ]

Minor comment changes ...

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests+needs backport to D7

Looks good. Marking as needs backport to d7 and this is a small change that could easily be backported but of course Webchick has final say on that. And removing needs tests because we do have tests now.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Nice little improvement. Committed to 8.x Moving to 7.x

StatusFileSize
new2.98 KB
PASSED: [[SimpleTest]]: [MySQL] 39,163 pass(es).
[ View ]

patch for D7.

Status:Patch (to be ported)» Needs review

I tried it on a fresh 7.x clone, applied cleanly. The test-only version failed properly, full patch passed as expected. Looks good to me.

Status:Needs review» Reviewed & tested by the community

Yep, looks good to me too.

I applied this, and also added:

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
index 87eaabb..4630168 100644
--- a/sites/default/default.settings.php
+++ b/sites/default/default.settings.php
@@ -235,6 +235,14 @@ $update_free_access = FALSE;
$drupal_hash_salt = '';
/**
+ * One-time login expiration (optional)
+ *
+ * Any one-time-login links will expire this many seconds after creation.
+ * Defaults to 24 hours = 86400 seconds.
+ */
+# $conf['user_password_reset_timeout'] = 86400;
+
+/**
  * Base URL (optional).
  *
  * If Drupal is generating incorrect URLs on your site, which could

We're not quite under thresholds yet (but close!) so this'll have to wait until next release.

#32: 246029-32.patch queued for re-testing.

Patch looks reasonable, but we're still not under thresholds currently, so I'm not committing it to D7 yet. Hopefully soon... sooner if you help with the major/criticial queue :)

Just confirming that since David has requested help on resolving release blockers, I don't feel comfortable committing feature patches to 7.x until that happens.

Title:Use variables for timeout values in user module's password resetUse a variable for the timeout/expiration of user password reset links

+++ b/core/modules/user/user.test
@@ -439,6 +439,58 @@ class UserLoginTestCase extends DrupalWebTestCase {
+class UserPasswordResetTestCase extends DrupalWebTestCase {
+  protected $profile = 'standard';

I don't see a reason for why this test needs to enforce the full Standard profile.

Doesn't matter for D7, since #1373142: Use the Testing profile. Speed up testbot by 50% hasn't been backported (yet). However, it would be great if we could quickly move this issue back to D8 after it was committed to D7 to remove that bogus install profile override.

Title:Use a variable for the timeout/expiration of user password reset linksUse a variable for the timeout/expiration of user password reset links (followup)
Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Active

Alright, since this is a small feature (with nice tests) and it's been sitting around for a while, and we're under thresholds, I think we can slip it into Drupal 7.15.

Committed to 7.x via http://drupalcode.org/project/drupal.git/commit/14b23a6 - thanks!

Moving back to Drupal 8 for the followup requested by @sun above.

Issue tags:+7.15 release notes

Adding tag.

StatusFileSize
new562 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,631 pass(es).
[ View ]

8.x patch reroll as per sun comment in #41.

Status:Active» Needs review

Status:Needs review» Reviewed & tested by the community

Ah, yeah.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Component:user.module» configuration system
Status:Fixed» Needs review
StatusFileSize
new963 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,175 pass(es).
[ View ]

Added documentation in default.settings.php as per #36 above.

Status:Needs review» Needs work

For consistency with what is on the patch, I'd change this:
+ * Any one-time-login links will expire this many seconds after creation.

for this:
+ * Any one-time login links will expire this many seconds after creation.

It seems more appropriate to use "one-time login".

Other than that it looks good to me.

Component:configuration system» documentation
Category:feature» task
Status:Needs work» Needs review
StatusFileSize
new529 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,381 pass(es).
[ View ]

Great point slv_. Fixed.

I'd almost mark this RTBC except that I uploaded it.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Status:Reviewed & tested by the community» Needs review
Issue tags:+Configuration system
StatusFileSize
new541 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,314 pass(es).
[ View ]

The user_password_reset_timeout variable has now been converted to config, so re-rolled to use the new yml file settings.

Component:documentation» configuration system

Thanks, LinL - I guess this is no longer just documentation then.

Status:Needs review» Reviewed & tested by the community

Patch applies... looks good!

Status:Reviewed & tested by the community» Needs review

What's the rationale for adding this to default.settings.php?

There are tons of variables, but most aren't added there (especially "non-technical" ones). I think we usually just assume that someone will create a contrib module with a simple user interface for it if it's important (for example, http://drupal.org/project/flood_control) and that people are more likely to learn about it that way than by browsing through an already-very-long settings.php file.

Status:Needs review» Closed (fixed)

The functionality has arrived in d8-dev, the default value is in core/modules/user/config/user.settings.yml and there is no mention in default.settings.php.
The feature has also been backported to d7.
So everything should be fine with this feature request.

Can this be closed?

Issue summary:View changes

A summary of how this affects links in the wild is valuable to users who don't read PHP fluently.

Issue summary:View changes

Updating with Issue Summary Template.

Since we aren't providing any docs i.e. in default.settings.php for Drupal 7, I updated the issue summary with examples for both D7 and D8. Also removing stale tags.

Issue summary:View changes

Original report by izmeez.

Issue summary:View changes

Updated comment link + grammar edits.