Implement password policy with hook

[JolienTrog]

Ref #4401

[Al2Klimov]

Fix your code style as told by the failing Github actions.

[Al2Klimov]

Please push without -f for now. This eases it for me to keep track of your changes.

[Al2Klimov]

During setup, the wizard prompts me for a username and password "to configure your first administrative account". This should enforce the chosen password policy.

[Al2Klimov]

Not done yet:

• #5419 (comment)
• #5419 (review)

Also, regarding #5419 (review): Situations where you push -f anyway, such as #5419 (comment), are good opportunities to squash at least commits which address only reviews and/or GitHub action failures.

[Al2Klimov]

Given PHP has no multiple inheritance, I wanted to preventively avoid unnecessary base classes and instructed you to make an interface.

See how tables have turned: #5442 (comment) 🙈

[lippserd]

This comment from Alex doesn't read like an instruction, so here's the explanation:

We use arbitrary strings such as PasswordPolicy to register and load hooks, which makes it difficult to actually find hook usages. I would suggest making PasswordPolicyHook an abstract class that also provides the methods all() and register() so that the string PasswordPolicy only appears in the hook itself. Please then also adjust the documentation.

[lippserd]

I think I would also do the following to make it clean:

• Rename PasswordPolicyHook to PasswordPolicy (and move it to the Authentication namespace).
• Use the PasswordPolicy interface everywhere in the code where typing is required. This is better than using the hook as a type, since the hook is used to load the functionality.
• Have the new abstract PasswordPolicyHook implement PasswordPolicy.

[Al2Klimov]

@lippserd But the framework requires hooks to extend PasswordPolicyHook anyway. Doesn't an abstract base class AND an interface rather provide more confusion than benefits?

[lippserd]

Please use Throwable;.

[lippserd]

I would adjust the log message so that it includes the topic, e.g., 'The configured password policy could not be loaded: %s', $e

[lippserd]

I want this information to be displayed permanently, so Notification should be avoided. If styling is required, there is a form element called Note that can be used instead. You can search for 'note' in the Icinga web codebase to find examples of how it is used. addDescription() can also be used. The message could then be changed to something like this: There was a problem loading the configured password policy. Please contact your administrator.

[lippserd]

License header missing.

[lippserd]

Should be 2025 not 2016.

[lippserd]

Space after ! missing.

[lippserd]

I would rename the class to PasswordPolicyValidator.

[lippserd]

validate() is sufficient.

[lippserd]

I think I would also do the following to make it clean:

• Rename PasswordPolicyHook to PasswordPolicy (and move it to the Authentication namespace).
• Use the PasswordPolicy interface everywhere in the code where typing is required. This is better than using the hook as a type, since the hook is used to load the functionality.
• Have the new abstract PasswordPolicyHook implement PasswordPolicy.

[lippserd]

Do we really need those variables? The signature of assert*() is $expected, $actual, so having the following is perfectly fine:

$this->assertSame(
    expected,
    actual
);

And if you really need such variables, please use $expected and $actual.

[Al2Klimov]

💡

@lippserd What do you think about this:

ChangePasswordForm already has the old and new password. So PasswordPolicy can take both.

It would cost us nothing, but admins could implement #5417 by themselves. They could also use e.g Levenshtein distance and/or A.I to prevent the "smartest" users from using passwords similar to their current ones.

[lippserd]

💡

@lippserd What do you think about this:

ChangePasswordForm already has the old and new password. So PasswordPolicy can take both.

It would cost us nothing, but admins could implement #5417 by themselves. They could also use e.g Levenshtein distance and/or A.I to prevent the "smartest" users from using passwords similar to their current ones.

That's a good idea! @JolienTrog Could you please adjust the implementation so that the validation function accepts both $newPassword and $oldPassword, where the latter may be null?

[Al2Klimov]

What does this code even do?

[JolienTrog]

Sorry I forgot to delete this part.

[lippserd]

It is indeed necessary to do something with the element for the old password, as your introduced validator expects the old password element to be named old_password, which may not be the case. This shows that it also makes sense to introduce test cases for an actual form that implements password elements, a policy, and the validator.

[lippserd]

In addition to the changes requested, please revert anything related to 81e6859. This does not look like a necessary change.

[lippserd]

Please annotate Any and Common as code.

[lippserd]

Suggested change

	* This class connects with the PasswordPolicy interface to use it as hook
	* Base class for hookable password policies

[lippserd]

Suggested change

	* Registers Hook as Password Policy
	* Register password policy

[lippserd]

PasswordPolicy could be a protected const, e.g., HOOK_NAME.

[lippserd]

Suggested change

	* Returns all registered password policies sorted by
	* Return all registered password policies sorted by name

[lippserd]

Please rename to passwordPolicy, as everywhere else in the code.

[lippserd]

Please rename to passwordPolicy, as everywhere else in the code.

[lippserd]

Suggested change

	public function testMethodValidatePasswordAlwaysReturnAnEmptyArray(): void
	public function testValidatePasswordValid(): void

[lippserd]

Suggested change

	$expectedResult = [
	'Password must be at least 12 characters long',
	'Password must contain at least one number',
	'Password must contain at least one special character',
	'Password must contain at least one lowercase letter',
	];
	$res =
	$this->assertSame(
	$expectedResult,
	$expected = [
	'Password must be at least 12 characters long',
	'Password must contain at least one number',
	'Password must contain at least one special character',
	'Password must contain at least one lowercase letter',
	];
	$this->assertSame(
	$expected,

[lippserd]

This test is not very useful.

Suggested change

	public function testValidatePasswordWithManyCharacters(): void
	{
	$longPassword = str_repeat('a', 1000);
	$this->assertCount(3, $this->instance->validate($longPassword, 'null'));
	}