fix(authentication): Handle null or empty string password hash

[nickvergessen]

This can happen when the auth.storeCryptedPassword config is used, which previously errored with:
Hasher::verify(): Argument #2 ($hash) must be of type string, null given

As observed in https://drone.nextcloud.com/nextcloud/spreed/11732/1/4

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[come-nc]

Suggested change

	$oldTokenMatches = $randomOldToken && $randomOldToken->getPasswordHash() && $this->hasher->verify(sha1($password) . $password, $randomOldToken->getPasswordHash());
	$oldTokenMatches = $randomOldToken && ($pwHash = $randomOldToken->getPasswordHash()) && $this->hasher->verify(sha1($password) . $password, $pwHash);

[nickvergessen]

We don't do inline assignments in Nextcloud, also it's a just replacign a local member call here, so not sure it's worth the possible confusion.

[come-nc]

The problem is not to avoid the local call, it’s that you cannot know that getPasswordHash will return the same thing twice, so checking it returns a non-falsy value and then calling it againg and using it as an object is dangerous.
I’m not sure how is the pretty way to write this, and I do dislike the inline assignment, I used it to show the smalest change possible to show the idea.

psalm will yell at you for doing this kind of stuff. Especially when most of the time we only know the interface used and not the class.

Suggested change

	$oldTokenMatches = $randomOldToken && $randomOldToken->getPasswordHash() && $this->hasher->verify(sha1($password) . $password, $randomOldToken->getPasswordHash());
	if ($randomOldToken) {
	$pwHash = $randomOldToken->getPasswordHash();
	$oldTokenMatches = $pwHash && $this->hasher->verify(sha1($password) . $password, $pwHash);
	} else {
	$oldTokenMatches = false;
	}

Or if PHP>=8:

Suggested change

	$oldTokenMatches = $randomOldToken && $randomOldToken->getPasswordHash() && $this->hasher->verify(sha1($password) . $password, $randomOldToken->getPasswordHash());
	$pwHash = $randomOldToken?->getPasswordHash();
	$oldTokenMatches = $pwHash && $this->hasher->verify(sha1($password) . $password, $pwHash);

[nickvergessen]

The problem is not to avoid the local call, it’s that you cannot know that getPasswordHash will return the same thing twice

We have code like this with all our "Entities" all the time? How could the existing object change in the meantime?

[come-nc]

I still don’t like it, and I’m surprised psalm does not complain on such things.