From b47a40fa9f4661dd094c8809c9e6a6d7dfe2136d Mon Sep 17 00:00:00 2001 From: "Michael A. Smith" Date: Tue, 12 May 2026 07:31:57 -0400 Subject: [PATCH 1/3] fix(user:reset-password): generate policy-compliant random passwords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous --random implementation produced bin2hex(random_bytes(12)) — hex-only, no uppercase or symbols. Installs with secure_password enabled rejected the password at next login, leaving a "successful" reset that produced an unusable account. Introduce Service\PasswordPolicy that mirrors AuthUtils' private validators against the install's runtime $GLOBALS, and rework the generator to produce a 20-char password covering all four character classes, validated through PasswordPolicy with a retry cap. The duplication is acknowledged in the class docblock with a pointer to the upstream lines; openemr/openemr#12127 tracks the path to deletion once a public, side-effect-free validator lands in OpenEMR. Closes #12 Assisted-by: Claude Code --- src/Command/ResetPasswordCommand.php | 82 +++++++++++++++++-- src/Service/PasswordPolicy.php | 68 +++++++++++++++ .../Unit/Command/ResetPasswordCommandTest.php | 43 +++++++++- tests/Unit/Service/PasswordPolicyTest.php | 80 ++++++++++++++++++ 4 files changed, 265 insertions(+), 8 deletions(-) create mode 100644 src/Service/PasswordPolicy.php create mode 100644 tests/Unit/Service/PasswordPolicyTest.php diff --git a/src/Command/ResetPasswordCommand.php b/src/Command/ResetPasswordCommand.php index 081d107..983e1fd 100644 --- a/src/Command/ResetPasswordCommand.php +++ b/src/Command/ResetPasswordCommand.php @@ -15,6 +15,9 @@ namespace OpenCoreEMR\CLI\ManageUsers\Command; use OpenCoreEMR\CLI\ManageUsers\Exception\ManageUsersException; +use OpenCoreEMR\CLI\ManageUsers\Service\OpenEMRConnector; +use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicy; +use OpenCoreEMR\CLI\ManageUsers\Service\UserManager; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -24,7 +27,25 @@ #[AsCommand(name: 'user:reset-password', description: "Reset an OpenEMR user's password")] class ResetPasswordCommand extends AbstractUserCommand { - private const RANDOM_PASSWORD_BYTES = 12; + private const RANDOM_PASSWORD_LENGTH = 20; + private const RANDOM_PASSWORD_MAX_ATTEMPTS = 32; + // Excludes look-alikes (0/O/o, 1/l/I) to reduce transcription errors. + private const RANDOM_LOWER = 'abcdefghijkmnpqrstuvwxyz'; + private const RANDOM_UPPER = 'ABCDEFGHJKLMNPQRSTUVWXYZ'; + private const RANDOM_DIGIT = '23456789'; + // Shell-safe symbols: no quotes, backslash, backtick, $, parens, <, >, |, ;, &, space. + private const RANDOM_SYMBOL = '!@#%^*-_=+?'; + + private readonly PasswordPolicy $policy; + + public function __construct( + ?OpenEMRConnector $connector = null, + ?UserManager $users = null, + ?PasswordPolicy $policy = null, + ) { + parent::__construct($connector, $users); + $this->policy = $policy ?? new PasswordPolicy(); + } protected function configure(): void { @@ -63,7 +84,7 @@ protected function doExecute(InputInterface $input, SymfonyStyle $io): int if ($password !== null && $password !== '') { throw new ManageUsersException("--random cannot be combined with --password"); } - $password = $this->generateRandomPassword(); + $password = $this->generatePolicyCompliantPassword(); $io->writeln("Generated password: {$password}"); } elseif ($password === null || $password === '') { $question = new Question("New password for {$username}: "); @@ -83,9 +104,60 @@ protected function doExecute(InputInterface $input, SymfonyStyle $io): int return self::SUCCESS; } - private function generateRandomPassword(): string + /** + * Generate a strong random password and verify it satisfies the install's + * configured policy. The generator already covers all four character + * classes at length 20, so a single attempt almost always suffices — + * retries exist only to absorb a future stricter policy without surprising + * the operator with a "successful" reset to an unusable account. + */ + private function generatePolicyCompliantPassword(): string { - // Hex of N bytes -> 2N chars; safely printable, easy to copy/paste. - return bin2hex(random_bytes(self::RANDOM_PASSWORD_BYTES)); + $lastError = null; + for ($i = 0; $i < self::RANDOM_PASSWORD_MAX_ATTEMPTS; $i++) { + $candidate = $this->generateRandomCandidate(); + $lastError = $this->policy->validate($candidate); + if ($lastError === null) { + return $candidate; + } + } + throw new ManageUsersException( + "Could not generate a policy-compliant random password after " + . self::RANDOM_PASSWORD_MAX_ATTEMPTS . " attempts: {$lastError}" + ); + } + + private function generateRandomCandidate(): string + { + // Guarantee at least one of each class so the strictest default OpenEMR + // policy (secure_password) accepts the result on the first try. + $chars = [ + $this->pick(self::RANDOM_LOWER), + $this->pick(self::RANDOM_UPPER), + $this->pick(self::RANDOM_DIGIT), + $this->pick(self::RANDOM_SYMBOL), + ]; + + $alphabet = self::RANDOM_LOWER . self::RANDOM_UPPER . self::RANDOM_DIGIT . self::RANDOM_SYMBOL; + for ($i = count($chars); $i < self::RANDOM_PASSWORD_LENGTH; $i++) { + $chars[] = $this->pick($alphabet); + } + + // Fisher-Yates shuffle with a CSPRNG; str_shuffle is not cryptographic. + for ($i = count($chars) - 1; $i > 0; $i--) { + $j = random_int(0, $i); + [$chars[$i], $chars[$j]] = [$chars[$j], $chars[$i]]; + } + + return implode('', $chars); + } + + private function pick(string $alphabet): string + { + $max = strlen($alphabet) - 1; + if ($max < 0) { + throw new \LogicException('pick() requires a non-empty alphabet'); + } + return $alphabet[random_int(0, $max)]; } } diff --git a/src/Service/PasswordPolicy.php b/src/Service/PasswordPolicy.php new file mode 100644 index 0000000..962a7a8 --- /dev/null +++ b/src/Service/PasswordPolicy.php @@ -0,0 +1,68 @@ + + * @copyright Copyright (c) 2026 OpenCoreEMR Inc + * @license https://github.com/openCoreEMR/oce-cli-manage-users/blob/main/LICENSE GNU General Public License 3 + */ + +declare(strict_types=1); + +namespace OpenCoreEMR\CLI\ManageUsers\Service; + +class PasswordPolicy +{ + /** + * @return string|null Null on pass; an explanatory message on fail. + */ + public function validate(string $password): ?string + { + /** @var mixed $rawMin */ + $rawMin = $GLOBALS['gbl_minimum_password_length'] ?? 0; + $min = is_numeric($rawMin) ? (int)$rawMin : 0; + if ($min > 0 && strlen($password) < $min) { + return "Password too short (minimum {$min} characters)"; + } + + /** @var mixed $rawMax */ + $rawMax = $GLOBALS['gbl_maximum_password_length'] ?? 0; + $max = is_numeric($rawMax) ? (int)$rawMax : 0; + if ($max > 0 && strlen($password) > $max) { + return "Password too long (maximum {$max} characters)"; + } + + $secure = $GLOBALS['secure_password'] ?? null; + if ($secure) { + $classes = [ + '/[a-z]/', // lowercase + '/[A-Z]/', // uppercase + '/\d/', // digit + '/[\W_]/', // symbol (non-word or underscore) + ]; + foreach ($classes as $regex) { + if (preg_match($regex, $password) !== 1) { + return "Password must contain a lowercase letter, uppercase letter, digit, and symbol"; + } + } + } + + return null; + } +} diff --git a/tests/Unit/Command/ResetPasswordCommandTest.php b/tests/Unit/Command/ResetPasswordCommandTest.php index cb9ca5f..7c555ab 100644 --- a/tests/Unit/Command/ResetPasswordCommandTest.php +++ b/tests/Unit/Command/ResetPasswordCommandTest.php @@ -16,6 +16,7 @@ use OpenCoreEMR\CLI\ManageUsers\Command\ResetPasswordCommand; use OpenCoreEMR\CLI\ManageUsers\Service\OpenEMRConnector; +use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicy; use OpenCoreEMR\CLI\ManageUsers\Service\UserManager; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -27,14 +28,16 @@ class ResetPasswordCommandTest extends TestCase { private OpenEMRConnector&MockObject $connector; private UserManager&MockObject $users; + private PasswordPolicy&MockObject $policy; private CommandTester $tester; protected function setUp(): void { $this->connector = $this->createMock(OpenEMRConnector::class); $this->users = $this->createMock(UserManager::class); + $this->policy = $this->createMock(PasswordPolicy::class); - $command = new ResetPasswordCommand($this->connector, $this->users); + $command = new ResetPasswordCommand($this->connector, $this->users, $this->policy); $app = new Application(); $app->add($command); @@ -72,14 +75,22 @@ public function resetWithExplicitPassword(): void } #[Test] - public function randomGeneratesAndPrintsPassword(): void + public function randomGeneratesPolicyCompliantPassword(): void { + $this->policy->expects(self::atLeastOnce()) + ->method('validate') + ->willReturn(null); + $captured = null; $this->users->expects(self::once()) ->method('resetPassword') ->with('bob', self::callback(function (string $password) use (&$captured): bool { $captured = $password; - return strlen($password) >= 16; + return strlen($password) === 20 + && preg_match('/[a-z]/', $password) === 1 + && preg_match('/[A-Z]/', $password) === 1 + && preg_match('/\d/', $password) === 1 + && preg_match('/[\W_]/', $password) === 1; })); $exit = $this->tester->execute(['--user' => 'bob', '--random' => true]); @@ -89,6 +100,32 @@ public function randomGeneratesAndPrintsPassword(): void self::assertStringContainsString("Generated password: {$captured}", $this->tester->getDisplay()); } + #[Test] + public function randomRetriesUntilPolicyAccepts(): void + { + $this->policy->expects(self::exactly(3)) + ->method('validate') + ->willReturnOnConsecutiveCalls('too weak', 'too weak', null); + + $this->users->expects(self::once())->method('resetPassword'); + + $exit = $this->tester->execute(['--user' => 'bob', '--random' => true]); + + self::assertSame(0, $exit); + } + + #[Test] + public function randomFailsWhenPolicyKeepsRejecting(): void + { + $this->policy->method('validate')->willReturn('arbitrary policy says no'); + $this->users->expects(self::never())->method('resetPassword'); + + $exit = $this->tester->execute(['--user' => 'bob', '--random' => true]); + + self::assertSame(1, $exit); + self::assertStringContainsString('policy-compliant', $this->tester->getDisplay()); + } + #[Test] public function randomCannotBeCombinedWithPassword(): void { diff --git a/tests/Unit/Service/PasswordPolicyTest.php b/tests/Unit/Service/PasswordPolicyTest.php new file mode 100644 index 0000000..29be382 --- /dev/null +++ b/tests/Unit/Service/PasswordPolicyTest.php @@ -0,0 +1,80 @@ + + * @copyright Copyright (c) 2026 OpenCoreEMR Inc + * @license https://github.com/openCoreEMR/oce-cli-manage-users/blob/main/LICENSE GNU General Public License 3 + */ + +declare(strict_types=1); + +namespace OpenCoreEMR\CLI\ManageUsers\Tests\Unit\Service; + +use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicy; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; + +class PasswordPolicyTest extends TestCase +{ + private PasswordPolicy $policy; + + protected function setUp(): void + { + $this->policy = new PasswordPolicy(); + } + + protected function tearDown(): void + { + unset( + $GLOBALS['secure_password'], + $GLOBALS['gbl_minimum_password_length'], + $GLOBALS['gbl_maximum_password_length'], + ); + } + + #[Test] + public function passesWhenPolicyDisabled(): void + { + self::assertNull($this->policy->validate('hex')); + } + + #[Test] + public function rejectsShortPassword(): void + { + $GLOBALS['gbl_minimum_password_length'] = 12; + $error = $this->policy->validate('short'); + self::assertNotNull($error); + self::assertStringContainsString('too short', $error); + } + + #[Test] + public function rejectsLongPassword(): void + { + $GLOBALS['gbl_maximum_password_length'] = 8; + $error = $this->policy->validate('this-is-too-long'); + self::assertNotNull($error); + self::assertStringContainsString('too long', $error); + } + + #[Test] + public function securePasswordRequiresAllFourClasses(): void + { + $GLOBALS['secure_password'] = 1; + + // hex-only string (the bug from issue #12) should be rejected + self::assertNotNull($this->policy->validate('a1b2c3d4e5f6a1b2c3d4')); + + // missing symbol + self::assertNotNull($this->policy->validate('Abcdefgh1234')); + + // all four classes present + self::assertNull($this->policy->validate('Abcdefgh1234!')); + } +} From ec130e047cf5bb26facdecd6a951298c2d6e35a2 Mon Sep 17 00:00:00 2001 From: "Michael A. Smith" Date: Tue, 12 May 2026 09:20:53 -0400 Subject: [PATCH 2/3] refactor(reset-password): replace runtime guard with non-empty-string type Self-review: the empty-alphabet LogicException in pick() was unreachable defensive code added only to satisfy PHPStan's random_int argument analysis. A non-empty-string param type expresses the same invariant statically and lets the body stay one line. Assisted-by: Claude Code --- src/Command/ResetPasswordCommand.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Command/ResetPasswordCommand.php b/src/Command/ResetPasswordCommand.php index 983e1fd..68fbc7d 100644 --- a/src/Command/ResetPasswordCommand.php +++ b/src/Command/ResetPasswordCommand.php @@ -152,12 +152,11 @@ private function generateRandomCandidate(): string return implode('', $chars); } + /** + * @param non-empty-string $alphabet + */ private function pick(string $alphabet): string { - $max = strlen($alphabet) - 1; - if ($max < 0) { - throw new \LogicException('pick() requires a non-empty alphabet'); - } - return $alphabet[random_int(0, $max)]; + return $alphabet[random_int(0, strlen($alphabet) - 1)]; } } From e71bd997f34a517734dbf9b48465411b2276f075 Mon Sep 17 00:00:00 2001 From: "Michael A. Smith" Date: Tue, 12 May 2026 09:32:29 -0400 Subject: [PATCH 3/3] refactor(password-policy): seal concrete via interface Self-review: PasswordPolicy had no extension hook, so making it final expresses intent and prevents accidental subclassing. PHPUnit's createMock cannot double final classes, so callers (the command, tests) now depend on PasswordPolicyInterface and the concrete is sealed. Assisted-by: Claude Code --- src/Command/ResetPasswordCommand.php | 5 ++-- src/Service/PasswordPolicy.php | 2 +- src/Service/PasswordPolicyInterface.php | 27 +++++++++++++++++++ .../Unit/Command/ResetPasswordCommandTest.php | 6 ++--- 4 files changed, 34 insertions(+), 6 deletions(-) create mode 100644 src/Service/PasswordPolicyInterface.php diff --git a/src/Command/ResetPasswordCommand.php b/src/Command/ResetPasswordCommand.php index 68fbc7d..a9e4b9a 100644 --- a/src/Command/ResetPasswordCommand.php +++ b/src/Command/ResetPasswordCommand.php @@ -17,6 +17,7 @@ use OpenCoreEMR\CLI\ManageUsers\Exception\ManageUsersException; use OpenCoreEMR\CLI\ManageUsers\Service\OpenEMRConnector; use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicy; +use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicyInterface; use OpenCoreEMR\CLI\ManageUsers\Service\UserManager; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Input\InputInterface; @@ -36,12 +37,12 @@ class ResetPasswordCommand extends AbstractUserCommand // Shell-safe symbols: no quotes, backslash, backtick, $, parens, <, >, |, ;, &, space. private const RANDOM_SYMBOL = '!@#%^*-_=+?'; - private readonly PasswordPolicy $policy; + private readonly PasswordPolicyInterface $policy; public function __construct( ?OpenEMRConnector $connector = null, ?UserManager $users = null, - ?PasswordPolicy $policy = null, + ?PasswordPolicyInterface $policy = null, ) { parent::__construct($connector, $users); $this->policy = $policy ?? new PasswordPolicy(); diff --git a/src/Service/PasswordPolicy.php b/src/Service/PasswordPolicy.php index 962a7a8..a48e821 100644 --- a/src/Service/PasswordPolicy.php +++ b/src/Service/PasswordPolicy.php @@ -27,7 +27,7 @@ namespace OpenCoreEMR\CLI\ManageUsers\Service; -class PasswordPolicy +final class PasswordPolicy implements PasswordPolicyInterface { /** * @return string|null Null on pass; an explanatory message on fail. diff --git a/src/Service/PasswordPolicyInterface.php b/src/Service/PasswordPolicyInterface.php new file mode 100644 index 0000000..e8d31c8 --- /dev/null +++ b/src/Service/PasswordPolicyInterface.php @@ -0,0 +1,27 @@ + + * @copyright Copyright (c) 2026 OpenCoreEMR Inc + * @license https://github.com/openCoreEMR/oce-cli-manage-users/blob/main/LICENSE GNU General Public License 3 + */ + +declare(strict_types=1); + +namespace OpenCoreEMR\CLI\ManageUsers\Service; + +interface PasswordPolicyInterface +{ + /** + * @return string|null Null on pass; an explanatory message on fail. + */ + public function validate(string $password): ?string; +} diff --git a/tests/Unit/Command/ResetPasswordCommandTest.php b/tests/Unit/Command/ResetPasswordCommandTest.php index 7c555ab..561bd93 100644 --- a/tests/Unit/Command/ResetPasswordCommandTest.php +++ b/tests/Unit/Command/ResetPasswordCommandTest.php @@ -16,7 +16,7 @@ use OpenCoreEMR\CLI\ManageUsers\Command\ResetPasswordCommand; use OpenCoreEMR\CLI\ManageUsers\Service\OpenEMRConnector; -use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicy; +use OpenCoreEMR\CLI\ManageUsers\Service\PasswordPolicyInterface; use OpenCoreEMR\CLI\ManageUsers\Service\UserManager; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -28,14 +28,14 @@ class ResetPasswordCommandTest extends TestCase { private OpenEMRConnector&MockObject $connector; private UserManager&MockObject $users; - private PasswordPolicy&MockObject $policy; + private PasswordPolicyInterface&MockObject $policy; private CommandTester $tester; protected function setUp(): void { $this->connector = $this->createMock(OpenEMRConnector::class); $this->users = $this->createMock(UserManager::class); - $this->policy = $this->createMock(PasswordPolicy::class); + $this->policy = $this->createMock(PasswordPolicyInterface::class); $command = new ResetPasswordCommand($this->connector, $this->users, $this->policy); $app = new Application();