The user password reset needs a test, quoting dmitrig01 'can use mail.inc to intercept the mail'.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mr.baileys’s picture

Component: tests » user system
Assigned: Unassigned » mr.baileys
Status: Active » Needs work
FileSize
4.31 KB

First stab at this. This patch relies on the new assertions and API introduced in #296001: Capture e-mails sent during tests and add e-mail assertions / API.

This tests:

  1. the password reset functionality by requesting a new password by username
  2. the password reset functionality by requesting a new password by e-mail
  3. requesting a password for a user that does not exist
  4. it verifies that the e-mails go out (and don't go out for #3)
  5. extracts the password link from those e-mails.
  6. "clicks" the one-time login link
  7. verifies that the user is granted access and can change his/her password.
  8. Also ensures that the one-time link works only once.

Some of the logic for this test was already present in the existing user registration test, but a work-around was used to create the one-time login link (direct call to user_pass_reset_url instead of capturing the e-mail).

Leaving at NW until #296001: Capture e-mails sent during tests and add e-mail assertions / API is commited, and because the tests sometimes fail for reasons unknown to me.

mr.baileys’s picture

Status: Needs work » Needs review

#296001: Capture e-mails sent during tests and add e-mail assertions / API has been commited, let's take this one for a spin...

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

The original code to test the password reset URL had the following code:

sleep(1); // TODO Find better way.

When I leave this out, tests succeed on my local computer about 25% of the time. If I put it back in, tests consistently pass, so putting it back in here to see what the testbot says. I browsed the repository but since this was part of the initial commit, I couldn't find any further reasoning as to why it's in there.

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
mr.baileys’s picture

Note to reviewers: I haven't figured out why, but the test fails 1 out of 5 runs. The other 4 test runs succeed without any problem...

Dries’s picture

That sleep(1) is also confusing me.

catch’s picture

I think there's some validation which checks that the request time for the password request is < the request time for the time it's used, and on fast systems this can cause a failure. But that's half-remembered from something else similar rather than looking at the actual code it tests.

mr.baileys’s picture

@catch: thanks for pointing me in the right direction. The current implementation for the one-time login link compares timestamps in seconds, and uses < and >. This means it might take up to one second before the link actually becomes active.

Since I think these links should be active immediately, I've created a separate issue for this: #481794: One-time link should be active immediately. Once this lands, the sleep(1) hack can be removed.

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
4.31 KB

re-rolled.

Dries’s picture

The sleep(1) is still in patch #12.

mr.baileys’s picture

FileSize
4.28 KB

Oops... sorry about that.

Re-rolled without the sleep(1)...

catch’s picture

Status: Needs review » Needs work

+ // Verify that the user received an e-mail.

missing a period

Not changed by the patch but could add one here too.

// Check password type validation

lilou’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

Add two missing periods.

kathyh’s picture

Verified that sleep() is not in the patch. Tests installed and reviewed.
User tests pass with no fails - 699 passes, 0 fails, and 0 exceptions

Status: Needs review » Needs work

The last submitted patch failed testing.

sun.core’s picture

Priority: Critical » Normal

Tests don't qualify as critical.

Status: Needs work » Needs review

Re-test of issue-293487.patch from comment #16 was requested by kathyh.

Status: Needs review » Needs work

The last submitted patch, issue-293487.patch, failed testing.

mr.baileys’s picture

Patch no longer applies after #46149: Prevent account cancellation for uid 1 was commited.

ZenDoodles’s picture

Version: 7.x-dev » 8.x-dev
Assigned: mr.baileys » ZenDoodles
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
5.71 KB

Fix in D8 first, then backport.

Ivan Zugec’s picture

Patch looks good and I was able to apply it. I'll leave this issue in "needs review" for others to test.

tomaszjureczko’s picture

I've:
- applied the patch
- run tests few times (php core/scripts/run-tests.sh --file core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php)
- review the code of the test
Everything works fine. I would apply the patch.

lucashodge’s picture

I applied the patch, ran the test and got this message:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=5&op=do_nojs&op=do StatusText: OK ResponseText: Exception: The configuration directory type <em class="placeholder">staging</em> does not exist. in config_get_config_directory() (line 526 of /Users/lukehodge/Sites/d8/www/core/includes/bootstrap.inc).

adammalone’s picture

Status: Needs review » Reviewed & tested by the community

Test works perfectly for me - marking RTBC

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Only took four years :)

Committed/pushed to 8.x, moving to 7.x for backport.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.67 KB

Backported #23 to D7.

YesCT’s picture

Assigned: ZenDoodles » Unassigned
Issue summary: View changes

un assigning since it had been a while and someone else has posted patch anyway. :)

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 29: 293487-29-password-reset-test.patch, failed testing.

  • catch committed c61c0df on 8.3.x
    Issue #293487 by mr.baileys, lilou, ZenDoodles: Fixed TestingParty08:...

  • catch committed c61c0df on 8.3.x
    Issue #293487 by mr.baileys, lilou, ZenDoodles: Fixed TestingParty08:...

  • catch committed c61c0df on 8.4.x
    Issue #293487 by mr.baileys, lilou, ZenDoodles: Fixed TestingParty08:...

  • catch committed c61c0df on 8.4.x
    Issue #293487 by mr.baileys, lilou, ZenDoodles: Fixed TestingParty08:...