Currently if a user tries to login and their credentials aren't right they get a prompt to reset their password. They click the link and get to user/password and the form has forgotten their username.

It would be nice if we didn't require them to enter that information again.

This feature could also be useful in other places, so I made it a url parameter instead of a cookie. I don't see any way this could be abused.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

Status: Active » Needs review
FileSize
1.32 KB

Status: Needs review » Needs work

The last submitted patch, 1952196_easier_password_recovery.patch, failed testing.

greggles’s picture

Status: Needs work » Needs review
oresh’s picture

Status: Needs review » Reviewed & tested by the community

Patch applied, works good.
Better add Usability tag to it, and talk to guys at #drupal-usability on IRC, to get your patch applied.
Thanks.

greggles’s picture

Issue tags: +Usability
Bojhan’s picture

Sounds good, lets get this in!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we can add a test for this?

greggles’s picture

Sure, a test for displaying the message or a test that the default from the url works or both?

xjm’s picture

For the default value. Thanks! :)

sandhya.m’s picture

Assigned: Unassigned » sandhya.m
Status: Needs work » Needs review
FileSize
1.11 KB
sandhya.m’s picture

Status: Needs work » Needs review
FileSize
1.11 KB

Changed the patch file name.

Status: Needs review » Needs work

The last submitted patch, username-autofill-resetpw-1952196-11.patch, failed testing.

sandhya.m’s picture

Status: Needs review » Needs work
FileSize
1.13 KB

Changed patch attached.

sandhya.m’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, username-autofill-resetpw-1952196-13.patch, failed testing.

sandhya.m’s picture

Status: Needs work » Needs review
FileSize
2.46 KB

Status: Needs review » Needs work

The last submitted patch, username-autofill-resetpw-1952196-16.patch, failed testing.

sandhya.m’s picture

Status: Needs work » Needs review
FileSize
4.89 KB
greggles’s picture

#18 looks pretty solid to me. It adds a test for the "Sorry, unrecognized username or password" which we weren't testing before. However, it wasn't testing that the name gets added into the link and I think as long as we're going to test that message we should ensure the name is in there. So, attached is an updated patch based on #18 that uses assertRaw to look for the proper link.

@xjm - care to review/rtbc?

greggles’s picture

Oh, I also removed an extra * on the closing comment tag of the docblock of the test and tried to make the comment in the docblock of the test a little more descriptive.

greggles’s picture

One more note: in case you are curious why sandhya.m's file is about twice as large as mine - it's because it's utf16 (thanks to heine for helping me understand that).

greggles’s picture

Status: Needs review » Reviewed & tested by the community

Hey, why not just do this again...

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Since this was RTBCed by the security team lead, I assume this is not a problem, but:

+++ b/core/modules/user/user.pages.incundefined
@@ -38,6 +38,9 @@ function user_pass() {
+  else {
+    $form['name']['#default_value'] = isset($_GET['name']) ? $_GET['name'] : '';
+  }

Is that cool? Don't we need check_plain() at least?

Also, (in D8 at least, if not also D7) referencing $GET directly is a no-no. I'm not sure what the Symfonic equivalent of it is anymore... maybe Drupal::request()->getSomething?

webchick’s picture

Oh, that said, this is an awesome improvement! :)

greggles’s picture

Is that cool? Don't we need check_plain() at least?

I don't think so, but happy to be corrected. I make mistakes. There are lots of places where we use user supplied text into a textfield's default value without any sanitizing e.g. editing of node titles, text fields etc.

Berdir said the right thing to use is "Drupal::request() if it's not within a class/service that can get it injected."

I don't know how to judge if it should get injected, so I guess it's good this is up to sandhya.m.

dawehner’s picture

Issue tags: -Needs tests

That's a nice improvement!

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.phpundefined
@@ -108,4 +108,21 @@ public function getResetURL() {
+    $this->assertFieldByName('name',$edit['name'],'User name found.');

Missing spaces after the ","

ParisLiakos’s picture

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.phpundefined
@@ -108,4 +108,21 @@ public function getResetURL() {
+    $edit = array(
+      'name' => $this->randomName(),
+      'pass' => $this->randomName(),
+      );

indent is off

+++ b/core/modules/user/user.pages.incundefined
@@ -38,6 +38,9 @@ function user_pass() {
+    $form['name']['#default_value'] = isset($_GET['name']) ? $_GET['name'] : '';

should be
$form['name']['#default_value'] = Drupal::request()->query->get('name', '');

No need for the ternary:) you can pass the default value..if NULL is appropriate here, you can omit it

greggles’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.52 KB

After getting 2 incorrect answers in #drupal-contribute a shining angel came to my rescue and gave me the advice I needed. Thank you, shining angel, for the pointer to the get function in http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Request.html

greggles’s picture

Thanks dawehener and ParisLiakos for your reviews!

Incorporated into this patch.

greggles’s picture

ParisLiakos’s picture

+++ b/core/modules/user/user.pages.incundefined
@@ -38,6 +38,9 @@ function user_pass() {
+    $form['name']['#default_value'] = Drupal::Request()->get('name', '');

Doesnt request()->query->get() work?
Right from the link you posted:
http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Request.html...

Order of precedence: GET, PATH, POST, COOKIE
Avoid using this method in controllers:
* slow
* prefer to get from a "named" source

The query property holds $_GET:)

greggles’s picture

Indeed it does work. Thanks for the advice ParisLiakos.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good to go:)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1952196_easier_password_recovery_32.patch, failed testing.

webchick’s picture

+++ b/core/modules/user/user.pages.incundefined
@@ -38,6 +38,9 @@ function user_pass() {
+  else {
+    $form['name']['#default_value'] = Drupal::Request()->query->get('foo');
+  }

Erm. :) I think 'name', not 'foo' :)

greggles’s picture

Status: Needs work » Needs review
FileSize
2.46 KB

Zoinks.

I was testing to see if a non existant value would throw a notice and forgot to change it back. Whoops.

Edit: Oh, and tests passed locally this time so it should be all good.

greggles’s picture

Status: Needs review » Reviewed & tested by the community

Yay tests!

klonos’s picture

D7?

greggles’s picture

Yes, I think a backport makes sense though it's a bit early. Here's a patch for d7 from when I first worked on this issue. It will need the tests merged in, but that should be easy.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1952196_easier_password_recovery_d7-no-not-test.patch, failed testing.

greggles’s picture

Status: Needs work » Reviewed & tested by the community

I swear I named that right...

klonos’s picture

'n' and 'd' are far away from each other. It must have been the keyboard demon ;)

Thanx Greg!

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Active

Committed b8abe39 and pushed to 8.x. Thanks!

ParisLiakos’s picture

Status: Active » Needs review
greggles’s picture

And a straight D7 port with passing tests.

klonos’s picture

simplytest.me manual tested and it is green ;)

Anonymous’s picture

Tested on simplytest.me its green

Anonymous’s picture

oresh’s picture

Status: Needs review » Reviewed & tested by the community
greggles’s picture

Issue tags: -Needs backport to D7

Backport is done, so I think this tag should be removed.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes
klonos’s picture

Thanx! ;)

Automatically closed -- issue fixed for 2 weeks with no activity.