Implement password reset and email flows with SMTP integration - #1375
Implement password reset and email flows with SMTP integration#1375piciolo wants to merge 1 commit into
Conversation
Geda173
left a comment
Geda173
left a comment
There was a problem hiding this comment.
Hi @piciolo,
thank you for your contribution. Having this in the project will make the user experience better! The core implementation is well-structured: username enumeration protection in ForgotEmailController, correct sendPasswordResetNotification override on the User model, clean Mailable classes, and the throttle configurable via AUTH_PASSWORD_RESET_THROTTLE env is nice to have.
I have added a couple change requests below. Furthermore I would ask that you add some form of test validation for your PR. The forgot-password and forgot-email flows have no feature test coverage. Given the project has thorough tests for other auth-adjacent flows, these should have equivalent coverage. At minimum: happy path (email sent), unknown username/email (silent success, no leak), throttle enforcement, and token expiry/invalid token on reset.
There was a problem hiding this comment.
Both changes in resources/views/ingame/chat/index.blade.php and resources/views/ingame/layouts/main.blade.php are unrelated to your PR and should not be in the diff.
| AUTH_PASSWORD_RESET_THROTTLE=60 | ||
|
|
||
| MAIL_MAILER=smtp | ||
| MAIL_DRIVER=smtp |
There was a problem hiding this comment.
The PR adds MAIL_MAILER=smtp but leaves the pre-existing MAIL_DRIVER=smtp directly below it. MAIL_DRIVER has been deprecated since Laravel 7; MAIL_MAILER is the correct key. The old one should be removed to avoid confusion.
|
|
||
| <!-- Outer wrapper --> | ||
| <table width="100%" border="0" cellpadding="0" cellspacing="0" | ||
| style="background-color:#000000;background-image:url('{{ config('app.url') }}/img/outgame/1867da5b5f8769b547bb91d88bb4f8.jpg');background-repeat:no-repeat;background-position:center top;"> |
There was a problem hiding this comment.
CSS background-image with an external URL is ignored by virtually all major email clients - Gmail strips it entirely, Outlook has never supported it, and Apple Mail blocks it unless the user explicitly loads remote content. This background will not render for most recipients. Since the logo was already embedded as base64 in a follow-up commit, the same approach should be used here, or the background mage should simply be dropped from the email layout.
| <body style="margin:0;padding:0;background-color:#000000;"> | ||
|
|
||
| <table width="100%" border="0" cellpadding="0" cellspacing="0" | ||
| style="background-color:#000000;background-image:url('{{ config('app.url') }}/img/outgame/1867da5b5f8769b547bb91d88bb4f8.jpg');background-repeat:no-repeat;background-position:center top;"> |
There was a problem hiding this comment.
CSS background-image with an external URL is ignored by virtually all major email clients - Gmail strips it entirely, Outlook has never supported it, and Apple Mail blocks it unless the user explicitly loads remote content. This background will not render for most recipients. Since the logo was already embedded as base64 in a follow-up commit, the same approach should be used here, or the background mage should simply be dropped from the email layout.
SummaryFull implementation of SMTP email system for account recovery flows: password reset (via Laravel Fortify) and email retrieval (custom flow by username lookup). New features
Bug fixes
Cleanup
Configuration
Tests (11 tests, 77 assertions — all passing)
|
|
Thanks for addressing the previous feedback. The tests, background image fix, and rate limiter cleanup all look good. However, before this can be merged there are two issues that need to be resolved per the CONTRIBUTING.md:
This branch was not created from
Commit 0b5ec53 (fix: restore chat title styling and fix sidebar player list) is included in this branch but has nothing to do with SMTP. It modifies Once those two things are done the diff should be clean and the PR ready to merge. Please note that repeated violations of the CONTRIBUTING.md guidelines will cause PRs, which do not meet the criteria, to be closed without comment. |
Done |
|
Thanks for the rebase and for addressing all previous feedback. The diff is now clean and all concerns are resolved.
Fix those and this is good to go. |
|
Production smoke test (tiweb442 / space.tiweb-host.com)Tested on a live OGameX deployment (Jul 2026):
Related local fixes in the same batch: #1491–#1494 (tiweb442). Happy to help rebase or re-test if this branch needs an update against current |
Clean rebase onto current
|
|
Hi @Geda173, rebased and green — ready for another look. The branch had gone stale (main advanced ~3 months and it was CONFLICTING), so I rebased it onto current Two adaptations were needed for current
|
|
Hi @Geda173, rebased and green — ready for another look. The branch had gone stale (main advanced ~3 months and it was CONFLICTING), so I rebased it onto current Two adaptations were needed for current
CI is green: |
Geda173
left a comment
Geda173
left a comment
There was a problem hiding this comment.
Thanks for the rebase. The branch is clean now. A few things left before I can approve:
-
The built frontend bundle needs regenerating.The file
public/build/assets/outgame-b0a7e11e.jswas edited directly instead of being rebuilt. The filename contains a hash of the file's contents, so when the contents change the filename has to change too andpublic/build/manifest.jsonneeds to update alongside it. Runningnpm run buildand committing the result should sort it out. -
A couple of test cases from the original request are still missing.
-
Rate limiting isn't tested. The limiter is currently switched off in the test environment (
FortifyServiceProvider.php), so it can't be exercised at all. I confirmed that repeated rapid requests are never blocked during tests. Could you remove that exemption and add a test that the limit actually kicks in? As it stands, if the rate limiting broke or got removed, nothing would notice. -
Expired reset tokens aren't tested. The invalid-token case is covered, but not expiry. The behaviour works correctly, it just isn't protected by a test.
-
One small thing on
testPasswordCanBeResetWithValidToken: it only checks that the request redirects, not that the password actually changed. The reset does work correctly, but as written the test would still pass if it silently did nothing ,which makes it worth tightening, since it's the most important test in the PR. Asserting the new password works afterwards would cover it. -
Minor style:
ResetPasswordMailandRetrieveEmailMailboth declare their traits on one line (use Queueable, SerializesModels;). Everywhere else inapp/uses one trait per line, so please split them to match.
Nothing here is a big lift, the implementation itself is in good shape. Happy to take another look once these are addressed.
Adds two account-recovery flows: - Password reset (Fortify): branded /forgot-password and /reset-password views, ResetPasswordMail, User::sendPasswordResetNotification override, password_reset_tokens migration, and a configurable reset throttle (AUTH_PASSWORD_RESET_THROTTLE). - Forgot email: /forgot-email (ForgotEmailController) looks up a user by username and emails a masked address hint via RetrieveEmailMail, with username-enumeration protection and a named 'forgot-email' rate limiter. Branded HTML email templates, i18n keys (EN/IT/NL), login-modal links wired to the new routes (blade + outgame JS), and feature tests (ForgotPasswordTest, ForgotEmailTest) that cover the reset flow, the forgot-email lookup, the active 'forgot-email' rate limiter (now exercised in the testing environment), reset-token expiry, and that a valid reset actually changes the account password. Rebased onto current main; outgame Vite bundle rebuilt (npm run build) for the new links.
Preview deploymentPreview environment for this PR is ready for functional testing. Use one of the test accounts below to log in.
Test accounts (click to expand)
This preview will be automatically destroyed when the PR is closed. |
|
Thanks for the detailed review @Geda173 — all four points are addressed in the latest push. 1. Rebuilt the outgame bundle 2. Rate limiting is now exercised One wrinkle worth flagging: with the exemption gone, the limiter state leaked across test methods, because 3. Expired-token coverage 4. 5. Trait style CI is green: both docker test jobs, phpstan level 8, Pint, Rector, and the preview deploy. |
3 participants
Description
This PR implements a complete SMTP email system for OGameX, covering two core account-recovery flows: password reset and email retrieval.
Changes by file:
app/Mail/ResetPasswordMail.php(new)Mailable class for the password reset flow. Uses
Envelope/ContentAPI (Laravel 10+), reads the reset URL and username from constructor arguments, and renders the brandedoutgame.mail.reset-passwordBlade view.app/Mail/RetrieveEmailMail.php(new)Mailable class for the "forgot email" flow. Sends a masked version of the user's email address (e.g.
pi***@gmail.com) along with a login link, using the brandedoutgame.mail.retrieve-emailBlade view.app/Http/Controllers/ForgotEmailController.php(new)Controller for the
/forgot-emailroute. Accepts a username, looks up the associated account, and sends the masked email hint viaRetrieveEmailMail. Always returns the same success message regardless of whether the username exists, to prevent user enumeration.app/Models/User.phpAdded
sendPasswordResetNotification($token)override to replace Laravel's default plain-text notification with the brandedResetPasswordMail. Builds the full reset URL (includingemailparameter) before passing it to the Mailable.app/Providers/FortifyServiceProvider.phpRegistered custom Blade views for Fortify's password reset link request page (
outgame.forgot-password) and reset password page (outgame.reset-password), replacing the default Fortify responses which were previously throwingBindingResolutionException.resources/views/outgame/forgot-password.blade.php(new)Branded outgame page for requesting a password reset link. Matches the login page visual style (space background, OGameX card design).
resources/views/outgame/reset-password.blade.php(new)Branded outgame page for submitting a new password after clicking the reset link from email.
resources/views/outgame/forgot-email.blade.php(new)Branded outgame page for the "forgot email" flow — accepts a username and shows a confirmation message after submission.
resources/views/outgame/mail/reset-password.blade.php(new)HTML email template for password reset. Table-based layout for email client compatibility. Matches the login page color palette (
#22303fcard,#619fc8blue headings,#1a2530header). OGameX logo embedded as base64 data URI so it renders correctly in all mail clients (Gmail, Outlook, etc.) without depending onAPP_URLbeing publicly accessible.resources/views/outgame/mail/retrieve-email.blade.php(new)HTML email template for the email retrieval flow. Same visual style as the reset-password template. Displays the masked email address in a highlighted inner box.
resources/views/outgame/layouts/main.blade.phpReplaced
href="#"placeholder links in the login modal with properroute()calls:route('password.request')for forgot password androute('password.email-lookup')for forgot email.resources/js/outgame/b55eb79922e157d28e811c7452ab10.jsRemoved legacy JS handlers that intercepted clicks on
#pwLost/#emailLostand redirected them tohttps://{server}/game/reg/mail.phpandhttps://{server}/game/index.php?page=standalone&component=retrieveEmail(original OGame live server URLs). Replaced with internal route navigation.routes/web.phpAdded two routes for the forgot-email flow:
GET /forgot-email→ForgotEmailController@show(namedpassword.email-lookup)POST /forgot-email→ForgotEmailController@send(withthrottle:5,1middleware)config/auth.phpMade the password reset throttle configurable via
AUTH_PASSWORD_RESET_THROTTLEenv variable (default: 60 seconds) instead of hardcoded value.database/migrations/2026_04_05_193349_create_password_reset_tokens_table.php(new)Creates the
password_reset_tokenstable required by Laravel Fortify's password reset flow (emailPK,token,created_at). This table was missing from the project's migration history..env.exampleAdded SMTP-related environment variables:
MAIL_MAILER=smtp— critical: without this key Laravel defaults to thelogdriver, silently dropping all emailsMAIL_FROM_ADDRESS,MAIL_FROM_NAMEAUTH_PASSWORD_RESET_THROTTLE=60Type of Change:
password_reset_tokenstable, broken forgot-password link, logo not rendering in emails)Related Issues
N/A
Checklist
npm run devafter changes tob55eb79922e157d28e811c7452ab10.js.Additional Information
Email delivery verified: both
ResetPasswordMailandRetrieveEmailMailconfirmed delivered via Gmail SMTP (smtp.gmail.com:587, TLS).Logo rendering fix: the OGameX icon in email templates is now embedded as a base64 data URI. This avoids the issue where external mail servers (Gmail, Outlook) cannot load images from
http://localhostduring development, or from servers with self-signed certificates.No breaking changes. The
password_reset_tokensmigration must be run on existing installations:php artisan migrate.