diff --git a/src/Command/ResetPasswordCommand.php b/src/Command/ResetPasswordCommand.php index 081d107..a9e4b9a 100644 --- a/src/Command/ResetPasswordCommand.php +++ b/src/Command/ResetPasswordCommand.php @@ -15,6 +15,10 @@ 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\PasswordPolicyInterface; +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 +28,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 PasswordPolicyInterface $policy; + + public function __construct( + ?OpenEMRConnector $connector = null, + ?UserManager $users = null, + ?PasswordPolicyInterface $policy = null, + ) { + parent::__construct($connector, $users); + $this->policy = $policy ?? new PasswordPolicy(); + } protected function configure(): void { @@ -63,7 +85,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 +105,59 @@ 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 + { + $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); + } + + /** + * @param non-empty-string $alphabet + */ + private function pick(string $alphabet): string { - // Hex of N bytes -> 2N chars; safely printable, easy to copy/paste. - return bin2hex(random_bytes(self::RANDOM_PASSWORD_BYTES)); + return $alphabet[random_int(0, strlen($alphabet) - 1)]; } } diff --git a/src/Service/PasswordPolicy.php b/src/Service/PasswordPolicy.php new file mode 100644 index 0000000..a48e821 --- /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; + +final class PasswordPolicy implements PasswordPolicyInterface +{ + /** + * @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/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 cb9ca5f..561bd93 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\PasswordPolicyInterface; 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 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(PasswordPolicyInterface::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!')); + } +}