Skip to content

Commit

Permalink
MDL-80064 authentication: password can be null
Browse files Browse the repository at this point in the history
The Open ID Connect plugin uses null for the password,
which makes the internal password update fail to proceed.
Allowing null resolved the problem.

As a note, there is a potential issue if the authentication method has
a false return for the prevent_local_password because it will trigger
the hash_internal_user_password() where  the $password can not be null.
Since this only addresses the oauth2 issue, we should ignore it.
  • Loading branch information
meirzamoodle committed Jul 29, 2024
1 parent 1a33da6 commit 2bd774d
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 5 deletions.
4 changes: 2 additions & 2 deletions lib/moodlelib.php
Original file line number Diff line number Diff line change
Expand Up @@ -4360,7 +4360,7 @@ function hash_internal_user_password(#[\SensitiveParameter] string $password, $f
* It will remove Web Services user tokens too.
*
* @param stdClass $user User object (password property may be updated).
* @param string $password Plain text password.
* @param string|null $password Plain text password.
* @param bool $fasthash If true, use a low cost factor when generating the hash
* This is much faster to generate but makes the hash
* less secure. It is used when lots of hashes need to
Expand All @@ -4369,7 +4369,7 @@ function hash_internal_user_password(#[\SensitiveParameter] string $password, $f
*/
function update_internal_user_password(
stdClass $user,
#[\SensitiveParameter] string $password,
#[\SensitiveParameter] ?string $password,
bool $fasthash = false
): bool {
global $CFG, $DB;
Expand Down
29 changes: 26 additions & 3 deletions lib/tests/moodlelib_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -2920,20 +2920,43 @@ public function test_update_internal_user_password(): void {
/**
* Testing that if the password is not cached, that it does not update
* the user table and fire event.
*
* @dataProvider update_internal_user_password_no_cache_provider
* @covers ::update_internal_user_password
*
* @param string $authmethod The authentication method to set for the user.
* @param string|null $password The new password to set for the user.
*/
public function test_update_internal_user_password_no_cache(): void {
public function test_update_internal_user_password_no_cache(
string $authmethod,
?string $password,
): void {
global $DB;
$this->resetAfterTest();

$user = $this->getDataGenerator()->create_user(array('auth' => 'cas'));
$user = $this->getDataGenerator()->create_user(['auth' => $authmethod]);
$DB->update_record('user', ['id' => $user->id, 'password' => AUTH_PASSWORD_NOT_CACHED]);
$user->password = AUTH_PASSWORD_NOT_CACHED;

$sink = $this->redirectEvents();
update_internal_user_password($user, 'wonkawonka');
update_internal_user_password($user, $password);
$this->assertEquals(0, $sink->count(), 'User updated event should not fire');
}

/**
* The data provider will test the {@see test_update_internal_user_password_no_cache}
* for accounts using the authentication method with prevent_local_passwords set to true (no cache).
*
* @return array
*/
public static function update_internal_user_password_no_cache_provider(): array {
return [
'Password is not empty' => ['cas', 'wonkawonka'],
'Password is an empty string' => ['oauth2', ''],
'Password is null' => ['oauth2', null],
];
}

/**
* Test if the user has a password hash, but now their auth method
* says not to cache it. Then it should update.
Expand Down

0 comments on commit 2bd774d

Please sign in to comment.