Feature/cli password reset #32
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new interactive CLI command to reset user passwords (with provider registration and tests); updates config base_path, footer branding to "NeuronCMS", 404 view markup, .gitignore runtime entries, and the changelog. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI User
participant Cmd as ResetPasswordCommand
participant Reg as Registry
participant Repo as DatabaseUserRepository
participant Hasher as PasswordHasher
participant DB as Database
CLI->>Cmd: Execute command
Cmd->>Reg: Load Settings
Reg-->>Cmd: Settings
Cmd->>CLI: Prompt for identifier or read option
CLI-->>Cmd: Identifier
Cmd->>Repo: findByUsername/findByEmail(identifier)
Repo->>DB: Query user
DB-->>Repo: User record
Repo-->>Cmd: User object
Cmd->>CLI: Show user info & ask confirm
CLI-->>Cmd: Confirmation
alt confirmed
Cmd->>CLI: Prompt new password & confirmation
CLI-->>Cmd: Passwords
Cmd->>Hasher: Validate & hash password
Hasher-->>Cmd: Hashed password
Cmd->>Repo: update(user with new hash & metadata)
Repo->>DB: Persist changes
DB-->>Repo: Success
Repo-->>Cmd: Updated user
Cmd->>CLI: Success message
else cancelled
Cmd->>CLI: Cancelled message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 PHPMD (2.15.0)src/Cms/Cli/Commands/User/ResetPasswordCommand.php45-45: Avoid unused parameters such as '$parameters'. (undefined) (UnusedFormalParameter) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@resources/views/http_codes/404.php`:
- Around line 3-8: Both headings currently use the centered class which applies
margin-top:100px twice; remove the redundant top margin by keeping the centered
class only on the first heading or move the class to the parent container (the
div with class "row") so the margin-top is applied once; update the markup to
remove the centered class from the second heading (h2) or create/assign a single
container-level class (e.g., on the "row" div) that provides the margin, and
adjust CSS accordingly to ensure only one margin-top:100px is applied.
In `@versionlog.md`:
- Line 2: Update the changelog entry to use the actual CLI command name: replace
the documented cms:user:password-reset with cms:user:reset-password in
versionlog.md so the entry matches the registered command.
🧹 Nitpick comments (2)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (2)
124-131: Hardcoded minimum length may conflict withPasswordHasherconfiguration.The hardcoded 8-character check on line 127 may be inconsistent with
PasswordHasher::$_minLength. If the hasher's minimum length is configured differently, users could see confusing error messages (e.g., passing the 8-char check but failingmeetsRequirements()).Consider using the hasher's configured minimum length instead:
Suggested fix
// Get new password - $password = $this->prompt( "\nEnter new password (min 8 characters): " ); - - if( strlen( $password ) < 8 ) - { - $this->output->error( "Password must be at least 8 characters long!" ); - return 1; - } + $password = $this->prompt( "\nEnter new password: " ); // Validate password if( !$hasher->meetsRequirements( $password ) )This removes the redundant check since
meetsRequirements()already validates minimum length and provides detailed error messages viagetValidationErrors().
159-166: RedundantsetUpdatedAtcall.Line 160 sets
updatedAt, butrepository->update()on line 166 also sets it (seeDatabaseUserRepository::update()line 108). The second call overwrites the first, making line 160 redundant.Suggested fix
try { $user->setPasswordHash( $hasher->hash( $password ) ); - $user->setUpdatedAt( new \DateTimeImmutable() ); // Clear any lockout $user->setFailedLoginAttempts( 0 ); $user->setLockedUntil( null ); $repository->update( $user );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignoreconfig/neuron.yamlresources/views/http_codes/404.phpresources/views/layouts/default.phpresources/views/layouts/member.phpsrc/Cms/Cli/Commands/User/ResetPasswordCommand.phpsrc/Cms/Cli/Provider.phptests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.phpversionlog.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (3)
src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(19-243)src/Cms/Auth/PasswordHasher.php (4)
PasswordHasher(13-204)meetsRequirements(53-86)getValidationErrors(93-123)hash(24-30)src/Cms/Models/User.php (4)
getUsername(92-95)getRole(143-146)setPasswordHash(134-138)setFailedLoginAttempts(301-305)
tests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (3)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (4)
ResetPasswordCommand(14-206)getName(20-23)getDescription(28-31)execute(45-178)src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(19-243)src/Cms/Models/User.php (4)
setUsername(100-104)setRole(151-155)setPasswordHash(134-138)getPasswordHash(126-129)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php
45-45: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (sqlite)
🔇 Additional comments (8)
resources/views/layouts/member.php (1)
66-66: LGTM!Branding update from "NeuronPHP" to "NeuronCMS" is consistent with the related change in the default layout.
resources/views/layouts/default.php (1)
54-54: LGTM!Branding update aligns with the member layout change.
.gitignore (1)
39-42: LGTM!Adding
storage/andresources/storage/to gitignore is appropriate for excluding runtime artifacts like logs, cache, and temporary files from version control.config/neuron.yaml (1)
6-6: Verify the implementation ofbase_pathconfiguration wiring.The change from
base_path: resourcestobase_path: .is significant, but the codebase shows a potential disconnect:EmailVerifierFactory,PasswordResetterFactory, andPasswordResetInitializerretrieveRegistryKeys::BASE_PATHfrom the Registry with fallback togetcwd(), yet I cannot confirm how the configuration value fromneuron.yamlis wired to populateRegistryKeys::BASE_PATH.Confirm that the
system.base_pathconfiguration is properly transferred to the Registry during application boot, and that this change doesn't cause path resolution issues in email verification, password reset, and other base-path-dependent features.src/Cms/Cli/Provider.php (1)
48-51: LGTM!The new command registration follows the established pattern for user management commands and maintains naming consistency with the
cms:user:*namespace.tests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (1)
1-461: Comprehensive test coverage for the new command.The test file provides thorough coverage including:
- Command metadata verification
- Missing configuration error handling
- Success paths for both username and email lookups
- Failure cases (user not found, declined, short password, mismatch)
- CLI option-based flows
The use of mocking and
TestInputReadereffectively isolates the command logic for unit testing.src/Cms/Cli/Commands/User/ResetPasswordCommand.php (2)
45-45: Unused parameter$parametersis expected.The
$parametersparameter is part of the parentCommand::execute()signature and must be present for interface compliance, even if unused in this implementation. The static analysis warning can be safely ignored.
180-205: LGTM!The
getUserRepository()method provides clean separation for testability and includes proper error handling with descriptive messages for both missing configuration and database connection failures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Cms/Cli/Commands/User/ResetPasswordCommand.php`:
- Line 125: The password prompts in ResetPasswordCommand are using
$this->prompt(), exposing input; replace both uses (the new password assignment
to $password and the confirmation assignment to $passwordConfirm) with
$this->secret() so input is masked. Locate the two instances in the
ResetPasswordCommand where "$this->prompt( ... Enter new password ... )" and the
confirmation prompt are called and change them to "$this->secret(...)" while
preserving the prompt text and validation logic.
♻️ Duplicate comments (1)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (1)
166-172: Return value is now properly checked.The
update()return value is checked and appropriate error handling is in place. This addresses the previous review feedback.
🧹 Nitpick comments (5)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (1)
45-45: Consider suppressing the unused parameter warning.The
$parametersparameter is unused but required by the parentCommandinterface. Suppress the static analysis warning with a docblock annotation.Suggested fix
/** * Execute the command + * + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function execute( array $parameters = [] ): inttests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (4)
18-18: Add type hint for$rootproperty.The
$rootproperty is untyped. Consider adding the type hint for consistency.Suggested fix
- private $root; + private \org\bovigo\vfs\vfsStreamDirectory $root;
26-40: Virtual filesystem configuration appears unused.The vfsStream setup creates a
config/neuron.yamlfile, but all tests mockgetUserRepository()directly, bypassing the actual configuration loading. Consider removing this setup if it's not needed, or add a test that exercises the real configuration path.
72-72: Use the imported class reference.
Registryis already imported on line 12, so the fully qualified namespace is unnecessary.Suggested fix
- \Neuron\Patterns\Registry::getInstance()->reset(); + Registry::getInstance()->reset();
215-242: Consider adding test for email not found scenario.This test covers username not found, but there's no corresponding test for when
findByEmailreturns null. This would ensure the email lookup error path is also covered.Suggested test case
public function testExecuteFailsWhenUserNotFoundByEmail(): void { $this->inputReader->addResponse('nonexistent@example.com'); $repository = $this->createMock(DatabaseUserRepository::class); $repository->expects($this->once()) ->method('findByEmail') ->with('nonexistent@example.com') ->willReturn(null); $settings = $this->createMock(SettingManager::class); Registry::getInstance()->set('Settings', $settings); $command = $this->getMockBuilder(ResetPasswordCommand::class) ->onlyMethods(['getUserRepository']) ->getMock(); $command->expects($this->once()) ->method('getUserRepository') ->willReturn($repository); $command->setInput($this->input); $command->setOutput($this->output); $command->setInputReader($this->inputReader); $exitCode = $command->execute(); $this->assertEquals(1, $exitCode); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Cms/Cli/Commands/User/ResetPasswordCommand.phptests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.phpversionlog.md
🚧 Files skipped from review as they are similar to previous changes (1)
- versionlog.md
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (3)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (4)
ResetPasswordCommand(14-212)getName(20-23)getDescription(28-31)execute(45-184)src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(19-243)src/Cms/Models/User.php (4)
setUsername(100-104)setRole(151-155)setPasswordHash(134-138)getPasswordHash(126-129)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php
45-45: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (10)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (3)
1-31: LGTM!Namespace, imports, and command metadata methods are correctly implemented.
33-40: LGTM!Command options are properly configured with appropriate short flags.
186-211: LGTM!The repository getter has proper error handling for both missing settings and database connection failures, with clear error messages to guide debugging.
tests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (7)
54-67: LGTM!Good test of command metadata. Using reflection to test the protected
configure()method is appropriate.
98-160: LGTM!Excellent test coverage for the success path. The callback assertion verifying the password hash changed is a good approach, and validating the prompt history ensures the full user interaction flow is exercised.
162-213: LGTM!Good coverage of the email-based lookup flow, complementing the username-based test.
244-283: LGTM!Good test verifying cancellation returns exit code 0 and doesn't call
update().
285-360: LGTM!Comprehensive failure scenario coverage for password validation errors.
362-460: LGTM!Good tests verifying that CLI options bypass interactive prompts. The prompt count assertions (3 instead of 4) effectively validate this behavior.
462-505: LGTM!This test validates the fix for the previously unchecked
update()return value. Good coverage of the database failure scenario.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Cms/Cli/Commands/User/ResetPasswordCommand.php`:
- Line 58: Load password policy from the registry before validating: fetch
settings via Registry::getInstance()->get(RegistryKeys::SETTINGS), instantiate
PasswordHasher as currently done, then (inside a try/catch) read configured keys
like password.min_length, password.require_uppercase,
password.require_lowercase, password.require_numbers,
password.require_special_chars and call the corresponding setters on
PasswordHasher (e.g., setMinLength, setRequireUppercase, setRequireLowercase,
setRequireNumbers, setRequireSpecialChars) so that meetsRequirements() used
later honors site policy; fall back to defaults on any exception.
🧹 Nitpick comments (1)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (1)
127-131: Redundant minimum length check.This hardcoded 8-character check is redundant since
meetsRequirements()(line 134) already validates password length viaPasswordHasher's configuredminLength. Additionally, if the hasher is configured with a different minimum (e.g., 12 characters), this check would allow an 8-character password to pass here but then fail atmeetsRequirements(), showing confusing error messages.Consider removing this block and relying solely on
meetsRequirements():♻️ Suggested simplification
// Get new password - $password = $this->secret( "\nEnter new password (min 8 characters): " ); - - if( strlen( $password ) < 8 ) - { - $this->output->error( "Password must be at least 8 characters long!" ); - return 1; - } + $password = $this->secret( "\nEnter new password: " ); // Validate password if( !$hasher->meetsRequirements( $password ) )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (3)
src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(19-243)src/Cms/Auth/PasswordHasher.php (4)
PasswordHasher(13-204)meetsRequirements(53-86)getValidationErrors(93-123)hash(24-30)src/Cms/Models/User.php (4)
getUsername(92-95)getRole(143-146)setPasswordHash(134-138)setFailedLoginAttempts(301-305)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php
45-45: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (postgres)
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
🔇 Additional comments (5)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (5)
1-10: LGTM!Namespace and imports are clean and all imported classes are utilized within the command.
14-31: LGTM!Command name follows the established
cms:user:*pattern and the description is clear.
33-40: LGTM!Options are well-configured with both long and short forms, providing flexibility for user lookup.
156-183: LGTM!The update logic properly:
- Hashes the password before storage
- Updates the timestamp
- Clears lockout state (appropriate for admin password reset)
- Checks the repository's return value and handles failures
- Catches and reports exceptions
The previous concern about unchecked return value from
repository->update()has been addressed.
186-211: LGTM!Well-designed factory method with:
- Protected visibility for test mockability (as documented)
- Proper null-safety with informative error messages
- Exception handling for database connection failures
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Cms/Cli/Commands/User/ResetPasswordCommand.php`:
- Around line 171-177: Remove the hardcoded strlen check and the "at least 8
characters" prompt text in ResetPasswordCommand so validation and messaging come
from the configured password policy; call the same password policy used by
meetsRequirements() to read the configured minimum length for the prompt (or
omit the numeric hint) and let meetsRequirements() perform all validation,
replacing the current error path that calls $this->output->error("Password must
be at least 8 characters long!")/return 1 with the policy-based error reporting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Cms/Cli/Commands/User/ResetPasswordCommand.phptests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (3)
src/Cms/Repositories/DatabaseUserRepository.php (1)
DatabaseUserRepository(19-243)src/Cms/Auth/PasswordHasher.php (9)
PasswordHasher(13-204)setMinLength(128-132)setRequireUppercase(137-141)setRequireLowercase(146-150)setRequireNumbers(155-159)setRequireSpecialChars(164-168)meetsRequirements(53-86)getValidationErrors(93-123)hash(24-30)src/Cms/Models/User.php (4)
getUsername(92-95)getRole(143-146)setPasswordHash(134-138)setFailedLoginAttempts(301-305)
tests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (1)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (4)
ResetPasswordCommand(14-258)getName(20-23)getDescription(28-31)execute(45-230)
🪛 PHPMD (2.15.0)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php
45-45: Avoid unused parameters such as '$parameters'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: build-test (sqlite)
- GitHub Check: build-test (mysql)
- GitHub Check: build-test (postgres)
🔇 Additional comments (18)
src/Cms/Cli/Commands/User/ResetPasswordCommand.php (5)
1-40: LGTM - Command metadata and configuration are well-structured.The namespace, imports, class declaration, and command metadata methods (
getName,getDescription,configure) follow standard conventions. The options for--usernameand
45-104: Password policy loading correctly addresses configuration concerns.The implementation properly loads password requirements from the Registry settings (
auth.passwords.*) and applies them to the hasher. The fallback to defaults on exception is reasonable behavior.Regarding the static analysis hint about unused
$parameters: this is a false positive since the parameter is required by the parentCommand::execute()signature.
106-152: User lookup logic is sound.The implementation correctly handles both CLI options and interactive prompts, distinguishes between email and username using
filter_var, and provides clear error messages when the user is not found.
202-229: Password update logic correctly handles failure cases.The implementation now properly checks the return value from
$repository->update()(lines 212-218) and reports an appropriate error on failure. The lockout clearing (lines 208-210) is a good security practice for password resets.
237-257: Repository factory method is well-designed for testability.The
getUserRepository()method properly handles configuration errors with clear messaging and is protected to allow mocking in tests.tests/Unit/Cms/Cli/Commands/User/ResetPasswordCommandTest.php (13)
23-52: Test setup and teardown are correctly implemented.The virtual filesystem setup, test dependencies initialization, and Registry cleanup in tearDown ensure proper test isolation.
54-67: LGTM - Command metadata test is correct.
69-96: LGTM - Missing config test properly verifies error handling.
98-160: Thorough success path test with good assertions.The test verifies the complete flow including prompt count, repository interactions, and password hash change via the callback assertion.
162-213: LGTM - Email flow test properly exercises the alternative lookup path.
215-242: LGTM - User not found scenario is correctly tested.
244-283: Good test for cancellation flow.The
expects($this->never())->method('update')assertion ensures the database is not modified when the user declines.
285-321: LGTM - Password validation failure test is correct.
323-360: LGTM - Password mismatch test is correct.
362-410: LGTM - Username option test correctly verifies prompt bypass.The assertion that only 3 prompts are shown (vs 4 in interactive mode) validates the option behavior.
412-460: LGTM - Email option test correctly verifies prompt bypass.
462-505: Important test for database failure handling.This test validates that the command properly handles the case when
repository->update()returnsfalse, ensuring the fix for the previously flagged issue is working correctly.
507-566: Good coverage of password policy configuration.The test properly mocks the settings to return custom password requirements and verifies that a password failing those requirements (
abcdefghij- no uppercase, no numbers) results in exit code 1.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Note
Introduces an interactive CLI for resetting user passwords with validation against configurable password policies.
cms:user:reset-passwordcommand (ResetPasswordCommand) and registers it in CLIProviderPasswordHasher(policy loaded fromauth.passwordssettings); clears lockouts on success404view heading; footer branding updated to “NeuronCMS”system.base_pathset to.inconfig/neuron.yaml;.gitignorenow ignores runtimestorage/pathsWritten by Cursor Bugbot for commit 5e5dc98. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
UI
Chores
Tests
Docs
✏️ Tip: You can customize this high-level summary in your review settings.