Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
gregglesComment #3
greggles#1: 1952196_easier_password_recovery.patch queued for re-testing.
Comment #4
oresh CreditAttribution: oresh commentedPatch applied, works good.
Better add Usability tag to it, and talk to guys at #drupal-usability on IRC, to get your patch applied.
Thanks.
Comment #5
gregglesComment #6
Bojhan CreditAttribution: Bojhan commentedSounds good, lets get this in!
Comment #7
xjmI think we can add a test for this?
Comment #8
gregglesSure, a test for displaying the message or a test that the default from the url works or both?
Comment #9
xjmFor the default value. Thanks! :)
Comment #10
sandhya.m CreditAttribution: sandhya.m commentedComment #11
sandhya.m CreditAttribution: sandhya.m commentedChanged the patch file name.
Comment #13
sandhya.m CreditAttribution: sandhya.m commentedChanged patch attached.
Comment #14
sandhya.m CreditAttribution: sandhya.m commentedComment #16
sandhya.m CreditAttribution: sandhya.m commentedComment #18
sandhya.m CreditAttribution: sandhya.m commentedComment #19
greggles#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?
Comment #20
gregglesOh, 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.
Comment #21
gregglesOne 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).
Comment #22
gregglesHey, why not just do this again...
Comment #23
webchickSince this was RTBCed by the security team lead, I assume this is not a problem, but:
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?
Comment #24
webchickOh, that said, this is an awesome improvement! :)
Comment #25
gregglesI 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.
Comment #26
dawehnerThat's a nice improvement!
Missing spaces after the ","
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedindent is off
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
Comment #28
gregglesAfter 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
Comment #29
gregglesThanks dawehener and ParisLiakos for your reviews!
Incorporated into this patch.
Comment #30
gregglesComment #31
ParisLiakos CreditAttribution: ParisLiakos commentedDoesnt request()->query->get() work?
Right from the link you posted:
http://api.symfony.com/2.0/Symfony/Component/HttpFoundation/Request.html...
The query property holds $_GET:)
Comment #32
gregglesIndeed it does work. Thanks for the advice ParisLiakos.
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good to go:)
Comment #35
webchickErm. :) I think 'name', not 'foo' :)
Comment #36
gregglesZoinks.
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.
Comment #37
gregglesYay tests!
Comment #38
klonosD7?
Comment #39
gregglesYes, 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.
Comment #41
gregglesI swear I named that right...
Comment #42
klonos'n' and 'd' are far away from each other. It must have been the keyboard demon ;)
Thanx Greg!
Comment #43
alexpottCommitted b8abe39 and pushed to 8.x. Thanks!
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commented#39: 1952196_easier_password_recovery_d7-no-not-test.patch queued for re-testing.
Comment #45
gregglesAnd a straight D7 port with passing tests.
Comment #46
klonossimplytest.me manual tested and it is green ;)
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedTested on simplytest.me its green
Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commented#45: 1952196_easier_password_recovery_44.patch queued for re-testing.
Comment #49
oresh CreditAttribution: oresh commentedComment #50
gregglesBackport is done, so I think this tag should be removed.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedNice. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/1b0d6d2
Comment #52
klonosThanx! ;)