UserAuthTest password test audit

Problem/Motivation

This is a follow-up to #3569423: Convert expectation-less test mocks to stubs - User module. While I was updating tests to use stubs instead of mocks, I became concerned that the authentication-with-password test methods aren't testing things as intended. These methods are:

• testAuthenticateWithIncorrectPassword
• testAuthenticateWithCorrectPassword
• testAuthenticateWithZeroPassword

The best example may be testAuthenticateWithZeroPassword:

  public function testAuthenticateWithZeroPassword(): void {
    $this->testUser->expects($this->once())
      ->method('id')
      ->willReturn(2);

    $this->userStorage->expects($this->once())
      ->method('loadByProperties')
      ->with(['name' => $this->username])
      ->willReturn([$this->testUser]);

    $this->passwordService->expects($this->once())
      ->method('check')
      ->with(0, 0)
      ->willReturn(TRUE);

    $this->assertSame(2, $this->userAuth->authenticate($this->username, 0));
  }

The ->with(0, 0) near the end is a red flag that something is going wrong. The check() function takes two parameters:

1. The plain-text password as entered into a login form
2. The user's hashed password as stored in their account

In other words, these two parameters should not be the same! The other two functions are actually just as bad because we provide the result of a mocked $this->testUser->getPassword() as the second parameter. But we provide no return value from that mocked function. So what we're actually providing as the second parameter is NULL.

In #3569423: Convert expectation-less test mocks to stubs - User module I chose to remove these bad ->with() statements because it made the mock-to-stub conversion easier. But my concern is that these tests haven't been testing what we think they're testing. The functions need to be audited to ensure we have proper test coverage.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet