feat: password reset code for users to use in the CLI#8
feat: password reset code for users to use in the CLI#8raz-iacob wants to merge 4 commits intosupo-dev:mainfrom
Conversation
WalkthroughAdds a password-reset flow: actions to send a reset token and perform a reset, a controller with store/update endpoints, validation requests, a queued mail notification carrying the token, API routes, and feature/unit tests covering success and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as API (routes)
participant C as PasswordResetController
participant A1 as SendPasswordResetCode
participant A2 as ResetPassword
participant P as Password Facade
participant N as PasswordResetNotification
participant U as User Model
rect rgb(240,248,255)
User->>API: POST /password/email { email }
API->>C: store(request)
C->>A1: handle(email)
A1->>U: find by email
alt found
A1->>P: createToken(user)
A1->>N: new PasswordResetNotification(token)
A1-->>U: notify(N)
else not found
A1-->>C: return
end
C-->>User: 200 OK
end
rect rgb(245,255,240)
User->>API: PUT /password/reset { token,email,password,password_confirmation }
API->>C: update(request)
C->>A2: handle(token,email,password,password_confirmation)
A2->>P: reset(credentials, callback)
alt success
P-->>A2: PASSWORD_RESET
A2-->>C: PASSWORD_RESET
C-->>User: 200 JSON { message }
else invalid
P-->>A2: INVALID_TOKEN
A2-->>C: INVALID_TOKEN
C-->>User: 400 JSON { message }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/Actions/ResetPassword.php(1 hunks)app/Actions/SendPasswordResetCode.php(1 hunks)app/Http/Controllers/PasswordResetController.php(1 hunks)app/Http/Requests/ResetPasswordRequest.php(1 hunks)app/Http/Requests/SendPasswordResetCodeRequest.php(1 hunks)app/Notifications/PasswordResetNotification.php(1 hunks)routes/api.php(2 hunks)tests/Feature/Http/PasswordResetControllerTest.php(1 hunks)tests/Unit/Actions/ResetPasswordTest.php(1 hunks)tests/Unit/Actions/SendPasswordResetCodeTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
routes/api.php (1)
app/Http/Controllers/PasswordResetController.php (1)
PasswordResetController(15-47)
app/Http/Requests/ResetPasswordRequest.php (1)
app/Http/Requests/SendPasswordResetCodeRequest.php (1)
rules(14-19)
app/Notifications/PasswordResetNotification.php (1)
app/Models/User.php (1)
User(27-82)
app/Actions/SendPasswordResetCode.php (3)
app/Models/User.php (1)
User(27-82)app/Notifications/PasswordResetNotification.php (1)
PasswordResetNotification(13-51)app/Actions/ResetPassword.php (1)
handle(14-33)
app/Actions/ResetPassword.php (2)
app/Models/User.php (1)
User(27-82)app/Actions/SendPasswordResetCode.php (1)
handle(13-22)
tests/Feature/Http/PasswordResetControllerTest.php (2)
app/Models/User.php (1)
User(27-82)app/Notifications/PasswordResetNotification.php (2)
PasswordResetNotification(13-51)toArray(45-50)
app/Http/Requests/SendPasswordResetCodeRequest.php (1)
app/Http/Requests/ResetPasswordRequest.php (1)
rules(14-21)
tests/Unit/Actions/ResetPasswordTest.php (2)
app/Actions/ResetPassword.php (2)
ResetPassword(12-34)handle(14-33)app/Models/User.php (1)
User(27-82)
tests/Unit/Actions/SendPasswordResetCodeTest.php (3)
app/Actions/SendPasswordResetCode.php (2)
SendPasswordResetCode(11-23)handle(13-22)app/Models/User.php (1)
User(27-82)app/Notifications/PasswordResetNotification.php (3)
PasswordResetNotification(13-51)toArray(45-50)toMail(31-40)
app/Http/Controllers/PasswordResetController.php (4)
app/Actions/ResetPassword.php (2)
ResetPassword(12-34)handle(14-33)app/Actions/SendPasswordResetCode.php (2)
SendPasswordResetCode(11-23)handle(13-22)app/Http/Requests/ResetPasswordRequest.php (1)
ResetPasswordRequest(9-22)app/Http/Requests/SendPasswordResetCodeRequest.php (1)
SendPasswordResetCodeRequest(9-20)
🪛 PHPMD (2.15.0)
app/Notifications/PasswordResetNotification.php
26-26: Avoid unused parameters such as '$notifiable'. (undefined)
(UnusedFormalParameter)
45-45: Avoid unused parameters such as '$notifiable'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
tests/Feature/Http/PasswordResetControllerTest.php (1)
13-77: Solid end-to-end coverage.These scenarios give us high confidence in the new flow—nice work keeping the happy path and the key failure modes under test.
app/Http/Controllers/PasswordResetController.php (1)
17-46: Controller flow looks clean.The request casting, action delegation, and status handling are straightforward and match the expectations laid out in the tests.
| 'token' => ['required', 'string'], | ||
| 'email' => ['required', 'email', 'exists:users'], | ||
| 'password' => ['required', 'confirmed', 'min:8'], |
There was a problem hiding this comment.
Remove email existence check to prevent enumeration.
Validating with exists:users here also leaks whether an account exists by returning a 422 before the broker runs. Let the broker handle unknown emails (it already maps failures to an opaque message), and keep the response indistinguishable from the valid flow.
- 'email' => ['required', 'email', 'exists:users'],
+ 'email' => ['required', 'email'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'token' => ['required', 'string'], | |
| 'email' => ['required', 'email', 'exists:users'], | |
| 'password' => ['required', 'confirmed', 'min:8'], | |
| 'token' => ['required', 'string'], | |
| 'email' => ['required', 'email'], | |
| 'password' => ['required', 'confirmed', 'min:8'], |
🤖 Prompt for AI Agents
In app/Http/Requests/ResetPasswordRequest.php around lines 17 to 19, the
validation currently includes an 'exists:users' rule for the email which leaks
account existence; remove the 'exists:users' rule from the email validation
array so only format is validated (e.g., keep 'required' and 'email'), letting
the password broker handle unknown emails and preserve an indistinguishable
response.
| public function via(User $notifiable): array | ||
| { | ||
| return ['mail']; | ||
| } |
There was a problem hiding this comment.
Fix the via signature to match the base class.
Notification::via() declares object $notifiable; narrowing it to User causes a fatal “Declaration ... must be compatible” error. Keep the parameter typed as object (or untyped) and add a docblock if you need stronger hints.
- public function via(User $notifiable): array
+ public function via(object $notifiable): array
{
return ['mail'];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function via(User $notifiable): array | |
| { | |
| return ['mail']; | |
| } | |
| public function via(object $notifiable): array | |
| { | |
| return ['mail']; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
26-26: Avoid unused parameters such as '$notifiable'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In app/Notifications/PasswordResetNotification.php around lines 26 to 29, the
method signature public function via(User $notifiable): array narrows the
parameter type and is incompatible with the base Notification::via(object
$notifiable); change the signature to accept object (e.g. public function
via(object $notifiable): array) and, if you need stronger IDE/type hints, add a
docblock like @param User|object $notifiable above the method to document the
expected User type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Http/Requests/SendPasswordResetCodeRequest.php (1)
14-19: Consider consistent email validation across password reset endpoints.While this request correctly omits
exists:usersto prevent user enumeration, the companionResetPasswordRequest(lines 13-20 in app/Http/Requests/ResetPasswordRequest.php) still includesexists:userson the email field. This creates an inconsistency where the reset endpoint could leak whether an email is registered, even though the request-code endpoint does not.For consistency and security, consider removing
exists:usersfromResetPasswordRequestas well, allowing the action layer to handle missing users gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Http/Requests/SendPasswordResetCodeRequest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Http/Requests/SendPasswordResetCodeRequest.php (1)
app/Http/Requests/ResetPasswordRequest.php (1)
rules(14-21)
🔇 Additional comments (1)
app/Http/Requests/SendPasswordResetCodeRequest.php (1)
17-17: LGTM! Security guidance correctly applied.The validation rules correctly omit
exists:users, preventing account enumeration attacks. This matches the guidance from the previous review and ensures that the endpoint returns the same response regardless of whether the email is registered.
|
You need to remove http layer first than need to think password reset again with new flow. |
|
yeah.. with the new change I'll do that. and I'll use cache for the password reset code |
|
I noticed that the email field was removed from the user. Without an email, there’s no way to reset a password if someone forgets it. I think a better user experience would be to use an OTP (one-time password) flow. Instead of asking users to create a password, they could just provide an email, and we’d send them a one-time password to log in. If they later access Supo with the same SSH key, we can skip the sign-in step entirely. Does that make sense, @nunomaduro, @MrPunyapal ? |
|
I think we will need email and email verification both to check if user is authentic (not spam). cc: @nunomaduro |
2 participants
Added password reset functionality for users using the terminal/CLI
What's new
POST /password/email- Send reset code via notificationPUT /password/reset- Reset password with token / codeSummary by CodeRabbit
New Features
Tests
Documentation