Make the one-time login time-to-live (TTL) value configurable in the UI

Bumping to 8.x.

Quick patch attached allowing a configurable TTL for one-time login links. Configurable through admin/config/people/accounts. Restricted length to 10 digits, which already allows for a maximum TTL of more than 317 years... should be enough ^^

Tagging Novice.

Just thinking out loud, but the size of integers is platform dependent, perhaps consider using PHP_INT_SIZE.

Interesting. Will attempt as part of my 30 min of drupal community time tomorrow :)

Also thinking out loud. Would it be a security risk to not have any time limit at all? (e.g. 0 for unlimited?)

I can confirm that the patch works as directed.

I suggest we either add in the max php size and possibly an unlimited option now or open as another ticket. Thoughts?

Some thoughts:
- limiting size to 8 numbers will already allow for a maximum timeout of more than 3 years and will always fit in an integer on 32 bits platforms
- which value is generally used throughout Drupal to mean "unlimited"? Is it 0 or is it a negative number (-1)? Currently there is no enforcement that only a positive value should be entered.

Typically 0 is used for 'unlimited' in Drupal in situations where a literal value of zero would be useless.
And 8 is probably a better limit. I don't know of any systems that support a max of fewer than 8 digits in an integer?

I agree with #9. I knew in views, for example, 0 is often used to denote no limit.

A size of 8 seems fine. Anyone wanting longer is likely to just use 0 and be done with it.

Here's a patch that is re-rolled for D8 and has shortened the length of the number of seconds to 8 digits.

I don't love that the time has to be entered in seconds...not a great way for newbies / non-technical folks to have to enter this. Could we use php's function that converts strings to time? Or maybe a select form element with 1-7 days instead?

What about either:

1) Making pre-configured settings, e.g.

1 day
2 days
1 week
2 weeks
4 weeks

like what is done on the admin/config/development/performance page for Minimum cache lifetime and Expiration of cached pages.

or

2) Have the settings so they choose the number of days and hours, e.g.

2 days and 3 hours

and then it is converted to seconds.

I like the drop down idea. Patch attached.

I applied the patch and confirmed that, after choosing a particular duration, the correct value was in the variable table in the database. In particular:

1 day = 86400
2 days = 172800
1 week = 604800
2 weeks = 1209600

Beyond that, I'm not sure what else can be easily tested.

The patch in #14 works but contains some unnecessary spaces and rules. The patch could also be simplified because the timeout already uses a variable.

I made a new patch that fixes the remarks above.

The patch code looks good, but I haven't tested it and am pressed for time... hope someone else can try out soon.

applied the patch and works well for me.

Perhaps it might be a good idea to add more alternatives as items in the drop down?

Thx both for reviewing!

I added a few more options:

• 2 days
• 3 days
• 2 weeks
• 4 weeks.

Is that long enough you think ?

Sorry, patch in #19 contained some other changes as well. The patch attached to this comment is the right one.

#20: 263932-20-user_settings_to_set_reset_timeout.patch queued for re-testing.

post feature freeze for 8.x
forwarding to 9.x

I'd put this into 8.0.x without a UI - having this hardcoded seems like an omission given the issue summary.

We could add a UI in 8.1.x.

It seems this was essentially already done during some other refactoring. See user.settings.password_reset_timeout.

Actually, keeping open to add UI after 8.0.0

Adding this to D8

Adding the configurable timeout to D8

Resubmitting patch after pull
Ignore this patch, a better patch is on the way.

New patch that allows default setting for timeout to be preserved.

One more try to get this patch to work

Re-rolled this patch, it seems to work fine on a brand new checkout.

I know this is getting crazy, there was a tab in the patch.

Rerolled the patch.

Rerolling.

Rerolled with the latest code.

Removing sprint weekend tag!!
As suggested by @YesCT

Sorry, these issues were actually worked on during the 2015 Global Sprint
Weekend https://groups.drupal.org/node/447258

Fixed the error: the config used in the controller and in the form (password_reset_timeout_override) was not configured in the schema.yml. Hence the failing tests.

I am going to review this issue

Thank you for fixing this problem.

I reviewed it manually and I think this needed more work. It is possible to enter anything in this field and this is not how we should do it. Instead, I changed the type of the field to "number" and added a "min"-Value to the field.

I successfully reviewed the patch from #54 and it applies and works as expected. A better UI really is needed in 8.1.x presenting a drop down menu with predefined values.

Test passed, so i set it to RTBC

I replaced the input field with a select list, with options as proposed in #15. I thought that this wasn't worth waiting for the 8.1 version, it was an easy edit.

Hmm, I don't like this completely. I already talked with McGo about this and I think we should add another option "custom" in the select list which triggers a new field which lets you add an individual intervall in seconds.

Aren't we overthinking this? I mean, it's the expiration time of the one-time-login link. I can't even remember if a client ever asked me if it could be changed...
For cache invalidation I can understand why someone would need the custom choice, but not for this...

this is a feature

The KISS principle is a good guide, let me ask the following questions:
a) what would be the use case for such additional "custom" field displaying on and off?
b) is that case popular enough to be in Drupal?
c) if yes, does it need to be in core?

Regarding the patch, I don't quite understand the need of the additional variable "password_reset_timeout_override". I'll try to achieve the same without it.

New (simpler) patch, achieved same functionality without the additional variable.

Patch re-rolled without CR line endings (doh!).

Well what if I want my link to be expired on 3 days, 4 days, 3 weeks etc. ? So I guess I am not achieving this by select list.
So shouldn't this be a text field contains timestamp. This will give more flexibility.

thoughts ?

Well, the same argument would apply to the page cache max-age, and the options there are fixed and do not allow any 'custom' value. This is for simplicity, as specific values of ie. 3524 seconds would be no different than 3600 from a business user's perspective.

IMHO, this improved scale should be enough:
1 day
2 day
3 days
5 days
7 days
14 days
30 days

Works for me. Applied the patch on both simplytest.me and my local environment.

(My last patch had passed, now it is failing? Seems like a new test in Migrate isn't doing very well?)

Ok, works for me too. There's one more thing that I find... disturbing in the force...

There is a mismatch between the options units, and the real value unit. The real value stored in the configuration is measured in seconds, while the options are in whatever days, weeks or months. So, what if someone provides a value of of 3456 seconds through a YAML configuration? When he loads this admin screen: which value will be shown set? Suppose is shows [the closest option to that value] set, but then, if you save, you will have changed the configuration without knowing.

The max-age code for page cache has to deal with this as well... would like to see how it's done.

• Added additional hour options detailed in #72.
• Updated description text.

Trying again with the correct patch file.

• Added additional hour options detailed in #72.
• Updated description text.

I think we should check the timeout value before filling it in and, if it does NOT match any of the pre-set options, alert the user so she doesn't change it inadvertently. If someone decides to "inject" her own value here, we should allow for it, but also cover the case where the user actually wants to revert that to one of the pre-set options. So the pre-set options only need to be "good enough" for most users, which I think they already are.

We are limiting the TTL for added security (that's why we're not including "forever" in the options) and scaling down to "hours" seems too tight, just my opinion.

I think we should check the timeout value before filling it in and, if it does NOT match any of the pre-set options, alert the user so she doesn't change it inadvertently. If someone decides to "inject" her own value here, we should allow for it, but also cover the case where the user actually wants to revert that to one of the pre-set options.

The case we are discussing here is not unique to this issue. I took a look at how core handles the max-age settings on the 'performance' configuration form. If a the stored setting matches one of the options, it selects it as the default. If the stored setting does not match any of the available options, it defaults to showing the first option in the list, which in this example is 'no caching'. I can see merit in improving the UX of how this is handled, but I might argue that it shouldn't be a blocker for this and could live in it's own issue.

scaling down to "hours" seems too tight

I would respectfully disagree with you there. I could easily imagine short settings being quite useful.

@marcdorison thank you for finding that code. I may add that the behavior you've described is not enforced, but a side-effect of the config value not being found in the preset options and defaulting to _some_ value, probably the first element. And the first element (no caching) is not really the safest option is it?

In that code, and in this patch we're working on, I believe the user should be alerted somehow, and I agree it could be handled in separate issues, so as to unblock this one.

I could easily imagine short settings being quite useful.

It makes little to no technical difference, but I'm just curious, could you describe one use case? Thank you in advance.

Alright, so in this new patch attached, I've:

• preserved the scale ranging from 1 hour to 2 weeks
• changed how the options are built, to match the code in max-age page cache form. It is a bit more code but it is more consistent to use a date formatter with array_map().
• reset to a default of "1 day" if a current value exists and differs (or there's no existing value for this setting).
• not added any kind of feedback if the existing value does not match any option and hence, gets reverted to the default of 1 day.

Hope you find it useful, please test, thank you all.

Patch tested fine.

• Resolved PHPCS warning RE: $timeout_scale array declaration: "If the line declaring an array spans longer than 80 characters, each element should be broken into its own line."
• Appended period to the end of the #description text.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -181,6 +191,35 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#description' => t('Select how long the one-time login link will be valid. Default is 1 day.'),

Grammatically this should use "one day" instead of "1 day". However, "1 day" matches the output of dateFormatter, so perhaps we leave it as is?

Other than that, the code looks good and seems to be working in my basic testing.

Yeah "1 day" is good as it matches the value option. Also, note that "one day" may also mean "one day..." like in ie. www . peaceoneday . org

As 3 of us have tested the last patch with success, I'm marking this one as reviewed.

It's already configurable - just not through the UI.

We should at least test that the configuration is successfully saved by the form.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -181,6 +191,35 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $login_link_default_ttl = $config->get('password_reset_timeout');
+    if (empty($login_link_default_ttl) || !in_array(intval($login_link_default_ttl), $timeout_scale)) {
+      $login_link_default_ttl = 86400;
+    }
...
+      '#default_value' => $login_link_default_ttl,

This looks unnecessary. The default value could only be empty if the user has hacked config. I would just do:
'#default_value' => $config->get('password_reset_timeout'),

Hi,
@alexpott make sense, we can directly add

'#default_value' => $config->get('password_reset_timeout'),

I manually tested it and the configuration is successfully saved by the form.
Uploading patch with making #86 changes.

Thanks.

still need an automated test. Maybe add a new test method to UserAdminTest

I added a test which sets the TTL to 1 hour and saves the form. Afterwards, it checks the config for the stored TTL.

Great effort, @Cyberschorsch. I found the following minor issues:

1. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -181,6 +191,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => t('Select how long the one-time login link will be valid. Default is 1 day.'),
   

   As per #82 this should say "one day".

2. +++ b/core/modules/user/src/Tests/UserAdminTest.php
   @@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
   +    // Check the stored value after form submit, should be 1 hour.s
   

   Typo at the end of the sentence: "...1hour.s"

Fixed issues detailed in #82 & #92.

A, I see you uploaded the wrong patch. Let me change that real quick

Fixed the typos.

+++ b/core/modules/user/src/Tests/UserAdminTest.php
@@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
+    // Check the stored value after form submit, should be 1 hours.

I believe a semi-colon here makes more sense than a comma, but I am fine with it either way.

+++ b/core/modules/user/src/Tests/UserAdminTest.php
@@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
+    // Check the stored value after form submit, should be 1 hours.

However, '1 hours' should be the singular '1 hour'.

Okay, changed it.

Good work here!
Reviewing the code I only have two tidbits:

1. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -38,8 +46,9 @@ class AccountSettingsForm extends ConfigFormBase {
   -  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler, RoleStorageInterface $role_storage) {
   +  public function __construct(ConfigFactoryInterface $config_factory, DateFormatterInterface $date_formatter, ModuleHandlerInterface $module_handler, RoleStorageInterface $role_storage) {
   

   [Documentation] The docblock of this method must be updated by mentioning the new parameter.

2. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -181,6 +191,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      3600,
   +      7200,
   +      14400,
   +      28800,
   +      43200,
   +      86400,
   +      172800,
   +      604800,
   +      1209600,
   

   [DX] Could we have some comments on each line to show the human readable value?
   ie. 3600, // 1 hour
   604800, // 7 days

Good catch, DualFR!

I updated the documentation, also rebased again.

As said it would be nice to remove the description as there is really no value in it. It says the exactly same as the title.

The changes in this patch do not conform to Drupal coding standards:

1. +++ b/core/modules/user/src/AccountSettingsForm.php
   @@ -181,6 +193,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $timeout_scale = array(
   +      3600, // 1 hour,
   +      7200, // 2 hours
   +      14400, // 4 hours
   +      28800, // 8 hours
   +      43200, // 12 hours
   +      86400, // 1 day
   +      172800, // 2 days
   +      604800, // 1 week
   +      1209600, // 2 weeks
   +    );
   

   The comments added per #98 do not pass PHPCS.

   198 | ERROR | [x] Comments may not appear after statements
   199 | ERROR | [x] Comments may not appear after statements
   200 | ERROR | [x] Comments may not appear after statements
   201 | ERROR | [x] Comments may not appear after statements
   202 | ERROR | [x] Comments may not appear after statements
   203 | ERROR | [x] Comments may not appear after statements
   204 | ERROR | [x] Comments may not appear after statements
   205 | ERROR | [x] Comments may not appear after statements

2. +++ b/core/modules/user/src/Tests/UserAdminTest.php
   @@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
   +   * Test saving the time-to-live (ttl) for the one-time login link when
   

   202 | ERROR | [ ] Doc comment short description must be on a single line, further text should be a separate paragraph

3. +++ b/core/modules/user/src/Tests/UserAdminTest.php
   @@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
   +  function testOnetimeLoginLinkTTL() {
   

   204 | ERROR | [ ] Method name "UserAdminTest::testOnetimeLoginLinkTTL" is not in lowerCamel format
   204 | ERROR | [ ] Visibility must be declared on method "testOnetimeLoginLinkTTL"

4. +++ b/core/modules/user/src/Tests/UserAdminTest.php
   @@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
   +  }
   

   216 | ERROR | [x] Expected 1 blank line after function; 0 found
   217 | ERROR | [x] The closing brace for the class must have an empty line before it

I am trying to apply patch but its not getting apply .So changing status from needs work to needs reroll.

by mistakenly marked as resolved

@Pradnya The latest patch applies cleanly against HEAD of 8.2.x. Are you trying to apply against that branch?

@markdorison Yes ,let me try Once again and get back to you.

@markdorison Sorry my bad .patch applies cleanly. Making changes as per coding standards.

Why is your patch this large? It seems like you changed the intendation?

I think we should focus on the actual issue here. When we fix the comments, I think this issue is done..

@Pradnya Pingat : Patch submitted by you seems to be incorrect patch. Please verify your submitted patch.

mistakenly uploaded file .Adding new patch file with coding standard fixes as per comment #101

The "hours" wording error was still present, corrected that.

Thank you all for your work here! You're on the right way!
Here are a few more things, though.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -181,6 +190,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $timeout_scale = array(
+      3600, // 1 hour.
+      7200, // 2 hours.
+      14400, // 4 hours.
+      28800, // 8 hours.
+      43200, // 12 hours.
+      86400, // 1 day.
+      172800, // 2 days.
+      604800, // 1 week.
+      1209600, // 2 weeks.
+    );

To pass PHPCS validation the comments have to be on the line before the statement. eg:

// 1 hour.
3600,
// 4 hours.
14400,

Some other points from #101 haven't been fixed yet.

Updated the patch with suggested changes in #101 and #115.

What an interesting find. It would be handy to have this functionality.

I read through the issue and the only thing I found not addressed was dawehner's comment in #100 about removing the description.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -49,9 +60,7 @@ public function __construct(ConfigFactoryInterface $config_factory, ModuleHandle
+      $container->get('config.factory'), $container->get('date.formatter'), $container->get('module_handler'), $container->get('entity.manager')->getStorage('user_role')

Line > 80 characters. And is there a reason the data formatter wasn't added at the end of the list of returned items?

+++ b/core/modules/user/src/Tests/UserAdminTest.php
@@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
+    $this->assertEqual($ttl, 3600, 'Testing the stored ttl of password reset in the configuration');

Capitalize the acronym, TTL.

+++ b/core/modules/user/src/Tests/UserAdminTest.php
@@ -197,4 +197,21 @@ function testNotificationEmailAddress() {
+   * Test saving TTL for the one-time login link on resetting a user password.

Shouldn't TTL be stated in full before the abbreviation? As in 'Test saving the time to live (TTL) for ...'

pure reroll of #116

hi,
here is a patch that adresses #118
Thanks

Thanks, this patch comes in handy. It makes the contrib solution User Password Reset Link Timeout no longer necessary to simply adjust a core variable.

I tested the patch and it works for me.

Some remarks on the patch:

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -51,9 +62,7 @@ public function __construct(ConfigFactoryInterface $config_factory, ModuleHandle
+      $container->get('config.factory'), $container->get('date.formatter'), $container->get('module_handler'), $container->get('entity.manager')->getStorage('user_role')

Why move the services onto one line instead of keeping them on multiple lines?

+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -202,4 +202,21 @@ public function testNotificationEmailAddress() {
+    $admin_user = $this->drupalCreateUser(array('administer account settings'));

This needs to use the short array syntax.

+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -202,4 +202,21 @@ public function testNotificationEmailAddress() {
+    $this->drupalPostForm("/admin/config/people/accounts", $edit, t('Save configuration'))

Single quotes should be used by default.

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -183,6 +192,37 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#title' => t('How long will a one-time login link be valid?'),

Use $this->t()

Hi,
Thanks for reviews
here is a patch that adresses #125 and #127
Thanks

Here's a patch that fixes the minor coding standards

@izus you beat me to it, almost submitted a patch simultaniously :)

I moved the DateFormatter service to the end of the list for classes that already extend AccountSettingsForm. Seems to make more sense to not inject new services in the middle of others.

#129 Tested and works for me

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -183,6 +195,37 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    $timeout_scale = array(
...
+    $form['registration_cancellation']['user_pass_reset_timeout'] = array(

this needs to use short array syntax

Thanks for working on this

Hi,

#134 Tested and works for me

the two modifications adviced in #133 were adressed by #134
so back to RTBTC

Thanks

Rerun the test bots again

#137 works for me

Running the testbots again

Back to rtbc

Retests are passed. We will set it again to RTBC and see what bot says.

Tagging for usability review.

What's the use case for week and two weeks? The issue summary explains business users and weekends as use case for 2 days, but weeks came up later. They could still ask for another login link. I recently got beaten multiple times by Google's login tokens expiring in a few hours, but these limits are in the interest of security.

I am using the 263932-140.patch and I use it at 2 weeks. My use case is that I import user accounts and send the links which have an average of click time of one week for the individuals to follow through with the registration process. They are not the ones who initiate the email so there needs to be time for them to find time in their schedule to finish the registration.

I skimmed through the issue and it isn't clear what the next step is, but it certainly isn't novice anymore. Also needs IS update. And note that this has security factor to consider, as mention in #145.

And the patch needs a reroll.

Patch re-rolled.

Uploaded incorrect patch in #153.