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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pasqualle’s picture

Version: 6.2 » 7.x-dev
izmeez’s picture

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

izmeez’s picture

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

blisteringherb’s picture

Version: 7.x-dev » 7.4
FileSize
778 bytes

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

ericduran’s picture

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.

ericduran’s picture

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.

ericduran’s picture

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.

q0rban’s picture

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

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

ericduran’s picture

Status: Needs work » Needs review

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

ericduran’s picture

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.

rfay’s picture

Status: Needs work » Needs review
FileSize
746 bytes

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.

nico.knaepen’s picture

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.

ericduran’s picture

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

xjm’s picture

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.

rjgoldsborough’s picture

Status: Needs work » Needs review
FileSize
775 bytes

Rerolled for d8 with comment break added.

xjm’s picture

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 ¶
rjgoldsborough’s picture

Status: Needs work » Needs review
FileSize
774 bytes

Whoops. White space removed.

greggles’s picture

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

More accurate title.

Devin Carlson’s picture

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.

xjm’s picture

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.

zserno’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

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
Steven Jones’s picture

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.

zserno’s picture

Status: Needs work » Needs review
FileSize
3.03 KB

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

marcingy’s picture

Status: Needs review » Needs work

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

xjm’s picture

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

zserno’s picture

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

Alan Evans’s picture

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.

Alan Evans’s picture

Minor comment changes ...

marcingy’s picture

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.

Dries’s picture

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

kgoel’s picture

FileSize
2.98 KB

patch for D7.

kgoel’s picture

Status: Patch (to be ported) » Needs review
zserno’s picture

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.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Yep, looks good to me too.

pillarsdotnet’s picture

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
webchick’s picture

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

David_Rothstein’s picture

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

David_Rothstein’s picture

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 :)

webchick’s picture

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.

sun’s picture

Title: Use variables for timeout values in user module's password reset » Use 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.

David_Rothstein’s picture

Title: Use a variable for the timeout/expiration of user password reset links » Use 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.

David_Rothstein’s picture

Issue tags: +7.15 release notes

Adding tag.

junedkazi’s picture

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

junedkazi’s picture

Status: Active » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community

Ah, yeah.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

pillarsdotnet’s picture

Component: user.module » configuration system
Status: Fixed » Needs review
FileSize
963 bytes

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

slv_’s picture

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.

greggles’s picture

Component: configuration system » documentation
Category: feature » task
Status: Needs work » Needs review
FileSize
529 bytes

Great point slv_. Fixed.

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

junedkazi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

LinL’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Configuration system
FileSize
541 bytes

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

greggles’s picture

Component: documentation » configuration system

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

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies... looks good!

David_Rothstein’s picture

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.

Anonymous’s picture

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?

Anonymous’s picture

Issue summary: View changes

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

steveoliver’s picture

Issue summary: View changes

Updating with Issue Summary Template.

steveoliver’s picture

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.

steveoliver’s picture

Issue summary: View changes

Original report by izmeez.

steveoliver’s picture

Issue summary: View changes

Updated comment link + grammar edits.

goldenflower’s picture

just installed version 7.37 and got serious issue

here the one time password reset url getting expired immediate, i guess there might be issue with below if condition in modules/user/user.pages.inc

    $timeout = variable_get('user_password_reset_timeout', 86400);
    $current = REQUEST_TIME;
    // Some redundant checks for extra security ?
    $users = user_load_multiple(array($uid), array('status' => '1'));

    if ($timestamp <= $current && $account = reset($users)) {