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.
Related Issues
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
Comment | File | Size | Author |
---|---|---|---|
#52 | document-setting-246029-52.patch | 541 bytes | LinL |
#50 | 246029-document-settting_50.patch | 529 bytes | greggles |
#48 | 246029-document-settting.patch | 963 bytes | pillarsdotnet |
#44 | remove_bogus_profile-246029-44.patch | 562 bytes | junedkazi |
#32 | 246029-32.patch | 2.98 KB | kgoel |
Comments
Comment #1
PasqualleComment #2
izmeez CreditAttribution: izmeez commentedSince 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
Comment #3
izmeez CreditAttribution: izmeez commentedI 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
Comment #4
blisteringherb CreditAttribution: blisteringherb commentedHere is a patch to fix this issue that adds a variable and defaults to 24 hours.
Comment #5
ericduran CreditAttribution: ericduran commentedThis 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.
Comment #6
ericduran CreditAttribution: ericduran commentedWell 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.
Comment #7
ericduran CreditAttribution: ericduran commentedI 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.
Comment #8
q0rban CreditAttribution: q0rban commentedI think the title on that issue is better, though.
Comment #9
ericduran CreditAttribution: ericduran commentedMarking it as needs-review to see if it still passes.
Comment #10
ericduran CreditAttribution: ericduran commentedIt'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.
Comment #12
rfayHere's #4 rolled as a valid git patch. No changes. I just applied it with
git apply -p2
and then recreated with git diff.Comment #13
nico.knaepen CreditAttribution: nico.knaepen commentedIs 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.
Comment #14
ericduran CreditAttribution: ericduran commented@nico.knaepen, this is a feature request not a bug. So this is very unlikely, not impossible but unlikely.
Comment #15
xjmThis seems like a good change.
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.
Comment #16
rjgoldsborough CreditAttribution: rjgoldsborough commentedRerolled for d8 with comment break added.
Comment #17
xjmThanks @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:
Comment #18
rjgoldsborough CreditAttribution: rjgoldsborough commentedWhoops. White space removed.
Comment #19
gregglesMore accurate title.
Comment #20
Devin Carlson CreditAttribution: Devin Carlson commentedThe patch in #18 applied cleanly, changed the hardcoded timeout to a configurable variable and does not have the trailing whitespace mentioned by xjm.
Comment #21
xjmThanks Devin! It still needs a test, though. :) Setting back to NW for the addition of test coverage.
Comment #22
zserno CreditAttribution: zserno commentedAdded a simple test, based on testUserCancelInvalid(). Changes:
Comment #23
Steven Jones CreditAttribution: Steven Jones commentedThanks for the patch with the test, a few comments:
You should not use
t
in your assertions: http://drupal.org/node/265828Why 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.)
You should avoid using
time()
and useREQUEST_TIME
instead, I believe.It's really not clear what is going on here. Could you add some documentation along the lines of:
Comment #24
zserno CreditAttribution: zserno commentedThanks for the great review. Re-rolled the patch according to it.
Comment #25
marcingy CreditAttribution: marcingy commentedtestUserPasswordResetExpired passes even without the patch so it seems like the tests need improving.
Comment #26
xjmIt 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.)
Comment #27
zserno CreditAttribution: zserno commented@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.
Comment #28
Alan Evans CreditAttribution: Alan Evans commentedThanks 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
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 ..."
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.
Comment #29
Alan Evans CreditAttribution: Alan Evans commentedMinor comment changes ...
Comment #30
marcingy CreditAttribution: marcingy commentedLooks 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.
Comment #31
Dries CreditAttribution: Dries commentedNice little improvement. Committed to 8.x Moving to 7.x
Comment #32
kgoel CreditAttribution: kgoel commentedpatch for D7.
Comment #33
kgoel CreditAttribution: kgoel commentedComment #34
zserno CreditAttribution: zserno commentedI 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.
Comment #35
xjmYep, looks good to me too.
Comment #36
pillarsdotnet CreditAttribution: pillarsdotnet commentedI applied this, and also added:
Comment #37
webchickWe're not quite under thresholds yet (but close!) so this'll have to wait until next release.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commented#32: 246029-32.patch queued for re-testing.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedPatch 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 :)
Comment #40
webchickJust 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.
Comment #41
sunI 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.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedAlright, 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.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedAdding tag.
Comment #44
junedkazi CreditAttribution: junedkazi commented8.x patch reroll as per sun comment in #41.
Comment #45
junedkazi CreditAttribution: junedkazi commentedComment #46
xjmAh, yeah.
Comment #47
webchickCommitted and pushed to 8.x. Thanks!
Comment #48
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded documentation in default.settings.php as per #36 above.
Comment #49
slv_ CreditAttribution: slv_ commentedFor 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.
Comment #50
gregglesGreat point slv_. Fixed.
I'd almost mark this RTBC except that I uploaded it.
Comment #51
junedkazi CreditAttribution: junedkazi commentedLooks good to me.
Comment #52
LinL CreditAttribution: LinL commentedThe
user_password_reset_timeout
variable has now been converted to config, so re-rolled to use the new yml file settings.Comment #53
gregglesThanks, LinL - I guess this is no longer just documentation then.
Comment #54
alexpottPatch applies... looks good!
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedWhat'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.
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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?
Comment #56.0
Anonymous (not verified) CreditAttribution: Anonymous commentedA summary of how this affects links in the wild is valuable to users who don't read PHP fluently.
Comment #56.1
steveoliver CreditAttribution: steveoliver commentedUpdating with Issue Summary Template.
Comment #57
steveoliver CreditAttribution: steveoliver commentedSince 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.
Comment #57.0
steveoliver CreditAttribution: steveoliver commentedOriginal report by izmeez.
Comment #57.1
steveoliver CreditAttribution: steveoliver commentedUpdated comment link + grammar edits.
Comment #58
goldenflower CreditAttribution: goldenflower commentedjust 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