Skip to content

Password reset: Show a friendly message when the reset email cannot be sent - #24042

Merged
AndyButland merged 10 commits into
v17/devfrom
v17/bugfix/forgot-password-email-unavailable
Oct 8, 2026
Merged

AndyButland merged 10 commits into
v17/devfrom
v17/bugfix/forgot-password-email-unavailable

Conversation

@AndyButland

@AndyButland AndyButland commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Description

If the backoffice "Forgotten password?" form was submitted while the configured SMTP server was unreachable, the unhandled mail exception reached the login screen. With debug enabled that showed the full stack trace, and without it just a generic "Server Error".

image

With this PR the user instead sees a localised message: "Sorry, password reset is not currently available. Please try again later or contact your administrator." The underlying error is logged.

image

Changes

  • Availability check before sending: UserService.SendResetPasswordEmailAsync now checks up front that the reset email can currently be delivered. If it can't, it returns a new UserOperationStatus.PasswordResetUnavailable, which the endpoint surfaces as a 400 ProblemDetails. The check runs before the user lookup, so the response is the same whatever email address is entered, in line with how the endpoint already behaves.
  • Send failures are handled: if sending still fails after that check, the exception is logged and the endpoint returns its usual non-committal confirmation instead of throwing.
  • New API for checking email configuration and availability (additive, with default interface implementations):
    • IEmailSender: IsEmailConfigured() checks configuration only, with no I/O. IsEmailAvailableAsync() also checks that the transport can currently be reached. CanSendRequiredEmail() is obsoleted in favour of these.
    • IUserForgotPasswordSender: IsPasswordResetConfigured() and IsPasswordResetAvailableAsync() follow the same pattern. CanSend() is obsoleted.
    • IEmailSenderClient.VerifyConnectionAsync(): BasicSmtpEmailSenderClient connects (and authenticates if credentials are configured) without sending. EmailSender uses it whenever SMTP is configured as the transport, including as the fallback when a SendEmailNotification handler is registered, since a handler may only handle some email types. A pickup directory, or a handler with no SMTP configured, is assumed to be available.
    • IUserService.SendResetPasswordEmailAsync(string, CancellationToken): the endpoint now passes the request's cancellation token through. The overload without it is obsoleted.
  • SMTP check is shared and cached: concurrent callers share a single in-flight check, and its result is cached (60 seconds when reachable, 15 seconds when not), so anonymous reset requests don't each open an SMTP connection. The check has its own 10 second timeout, a caller's cancellation only stops that caller waiting, and the cache is cleared when GlobalSettings change.
  • SMTP settings are read per operation: BasicSmtpEmailSenderClient previously captured GlobalSettings when constructed, and as the singleton EmailSender resolves it once, changes to the SMTP configuration weren't picked up until restart. This also applied to sending, not just the new check.
  • Login screen: the forgot password form always shows the localised message on failure (new auth_passwordResetUnavailable key in en, da, de, nb, nl and sv). The other login flows no longer display the detail of 5xx responses. The login view now uses IUserForgotPasswordSender.IsPasswordResetConfigured() to decide whether to offer password reset, so a custom sender is respected there as it already was by the endpoint.

Testing

Configure SMTP to point at a host/port with nothing listening (for example "Smtp": { "From": "noreply@example.com", "Host": "localhost", "Port": 2525 }), then request a password reset from the login screen. Try both a registered and an unregistered email: both should show the localised "not currently available" message, and the log should contain a warning about the SMTP connection. With a working SMTP server (e.g. smtp4dev), the reset email should arrive as before. Note that a successful check is cached for 60 seconds, so if you stop the SMTP server after a successful reset, allow up to a minute before the "not currently available" message appears.

…t password screen when SMTP is not available.
Copilot AI balanced review requested due to automatic review settings September 30, 2026 14:17
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Claude finished @AndyButland's task in 6m 24s —— View job


PR Review

Target: origin/v17/dev · Based on commit: a8bf7022 · Skipped: 5 non-English lang files out of 25 total

Adds SMTP availability probing before password-reset email dispatch, surfaces a localised failure message in the login UI, and prevents 5xx exception details from leaking through other login-screen flows. New IsEmailConfigured() / IsEmailAvailableAsync() / IsPasswordResetConfigured() / IsPasswordResetAvailableAsync() methods replace the obsoleted CanSendRequiredEmail() / CanSend() across IEmailSender and IUserForgotPasswordSender.

  • Modified public API: IEmailSender (+IsEmailConfigured, +IsEmailAvailableAsync, CanSendRequiredEmail obsoleted); IUserForgotPasswordSender (+IsPasswordResetConfigured, +IsPasswordResetAvailableAsync, CanSend obsoleted); IEmailSenderClient (+VerifyConnectionAsync default impl); IUserService.SendResetPasswordEmailAsync (contract/behaviour change documented in XML doc)
  • Affected implementations (outside this PR): Any third-party IEmailSender or IUserForgotPasswordSender implementation (default interface impls provide safe fallbacks; no compile break)
  • Other changes: UserOperationStatus.PasswordResetUnavailable new enum value; password-reset endpoint now returns 400 when SMTP is unreachable (was 500/unhandled); login UI always shows a localised message on reset failure (was raw API error text)

Note

Complexity advisory — This PR may benefit from splitting.

  • Layer spread: Core, Infrastructure, API.Management, Login UI, and tests are all touched (5 layers, 25 files). The interface additions + Infrastructure plumbing could be a separate PR from the API/UI wiring, keeping each diff smaller and easier to revert independently.

This is an observation, not a blocker. The full review follows below.


Important

  • src/Umbraco.Cms.StaticAssets/umbraco/UmbracoLogin/Index.cshtml:32-33: CanSendRequiredEmail() is marked [Obsolete] by this PR, but the login Razor view still calls it on both lines. Per CLAUDE.md §6.4, no internal src/ code should reference obsolete members after they are marked obsolete. Fix this →

    - var allowUserInvite = EmailSender.CanSendRequiredEmail();
    - var allowPasswordReset = SecuritySettings.Value.AllowPasswordReset && EmailSender.CanSendRequiredEmail();
    + var allowUserInvite = EmailSender.IsEmailConfigured();
    + var allowPasswordReset = SecuritySettings.Value.AllowPasswordReset && EmailSender.IsEmailConfigured();

Suggestions

  • src/Umbraco.Cms.Api.Management/Controllers/Security/SecurityControllerBase.cs:29: The Detail text for PasswordResetUnavailable says "because the email could not be sent", but the status is returned from the pre-send availability probe — the email was never attempted. A more accurate phrase: "because the mail server cannot be reached" or "because email delivery is not currently available".

  • src/Umbraco.Infrastructure/Mail/EmailSender.cs:103-106: UsesSmtpTransport() has a subtle condition: a pickup directory with an empty From address falls through to SMTP probing (because SendAsyncInternal also skips the pickup-directory branch when From is empty). This is correct but non-obvious — a one-line comment on the last clause (e.g. // pickup dir requires a non-empty From; without one the SMTP client is used) would save the next reader from having to cross-reference SendAsyncInternal.


Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

The implementation is solid: the availability probe runs before user lookup (preserving email-existence opacity), SMTP connection errors are caught and logged at the right severity, the frontend always shows a friendly message on any failure, and the obsolete/default-impl patterns all follow CLAUDE.md §6 correctly. The one Internal caller issue (the Razor view still using the obsoleted method) should be fixed before merge.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Anonymous requests perform uncancelled SMTP authentication, while selective notification handlers can incorrectly bypass availability checks.

Review effort: Balanced
Findings: 2 Medium severity · 2 Low severity

Open (4)
What changed in this PR

Adds graceful password-reset handling when email delivery is unavailable, spanning Core, SMTP infrastructure, Management API, and login UI.

Changes:

  • Adds email configuration and transport-availability APIs.
  • Maps delivery failures to safe API and localized UI responses.
  • Adds unit and integration coverage for availability and send failures.
File Description
tests/​.../​Security/​EmailUserForgotPasswordSenderTests.cs Tests password-reset availability checks.
tests/​.../​Mail/​EmailSenderTests.cs Tests transport probing and cancellation.
tests/​.../​UserServiceCrudTests.ResetPassword.cs Tests unavailable and failed sends.
tests/​.../​UserServiceCrudTests.cs Adds injectable password-reset dependencies.
src/​Umbraco.Web.UI.Login/​src/​localization/​lang/​sv.ts Adds Swedish message.
src/​Umbraco.Web.UI.Login/​src/​localization/​lang/​nl.ts Adds Dutch message.
src/​Umbraco.Web.UI.Login/​src/​localization/​lang/​nb.ts Adds Norwegian message.
src/​Umbraco.Web.UI.Login/​src/​localization/​lang/​en.ts Adds English message.
src/​Umbraco.Web.UI.Login/​src/​localization/​lang/​de.ts Adds German message.
src/​Umbraco.Web.UI.Login/​src/​localization/​lang/​da.ts Adds Danish message.
src/​Umbraco.Web.UI.Login/​src/​contexts/​auth.repository.ts Sanitizes errors and localizes reset failures.
src/​Umbraco.Infrastructure/​Security/​EmailUserInviteSender.cs Uses configuration-only capability check.
src/​Umbraco.Infrastructure/​Security/​EmailUserForgotPasswordSender.cs Adds reset availability methods.
src/​Umbraco.Infrastructure/​Mail/​Interfaces/​IEmailSenderClient.cs Adds transport verification contract.
src/​Umbraco.Infrastructure/​Mail/​EmailSender.cs Implements availability probing.
src/​Umbraco.Infrastructure/​Mail/​BasicSmtpEmailSenderClient.cs Verifies SMTP connectivity and authentication.
src/​Umbraco.Core/​Services/​UserService.cs Handles unavailable and failed delivery.
src/​Umbraco.Core/​Services/​OperationStatus/​UserOperationStatus.cs Adds unavailable status.
src/​Umbraco.Core/​Services/​IUserService.cs Documents reset failure behavior.
src/​Umbraco.Core/​Security/​IUserForgotPasswordSender.cs Adds compatible availability APIs.
src/​Umbraco.Core/​Mail/​NotImplementedEmailSender.cs Implements new email API members.
src/​Umbraco.Core/​Mail/​IEmailSender.cs Adds and documents availability APIs.
src/​Umbraco.Cms.Api.Management/​Factories/​UserPresentationFactory.cs Uses configuration-only invite check.
src/​Umbraco.Cms.Api.Management/​Controllers/​Security/​SecurityControllerBase.cs Maps unavailable status to ProblemDetails.
src/​Umbraco.Cms.Api.Management/​Controllers/​Security/​ResetPasswordController.cs Surfaces unavailable reset delivery.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Umbraco.Core/Services/UserService.cs
Comment thread src/Umbraco.Infrastructure/Mail/EmailSender.cs
Comment thread src/Umbraco.Core/Mail/IEmailSender.cs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/Umbraco.Core/Services/IUserService.cs Outdated
Comment thread src/Umbraco.Infrastructure/Mail/BasicSmtpEmailSenderClient.cs
Comment thread src/Umbraco.Infrastructure/Mail/EmailSender.cs Outdated

@iOvergaard iOvergaard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, and it also closes the gap where a registered email could get a 500 and an unknown one a 200 when sending failed 👍 Approving, with a few things to consider:

  1. Sites sending through a SendEmailNotification handler: UsesSmtpTransport() only looks at the SMTP settings, not at _notificationHandlerRegistered. So a site that sends everything through a handler but still has an unreachable or placeholder Smtp:Host could reset passwords before, and now always gets "not currently available". I saw this was a deliberate change after the Copilot comment, but since UserService now catches send failures and returns the same 200, the opposite case (a handler covering only some email types, with dead SMTP) already fails safely. So I'd lean towards treating a registered handler as available and skipping the probe. If you'd rather keep it, perhaps a line in the release notes?
  2. The login screen now shows "contact your administrator" for any failure on the reset form, including ApplicationUrlNotConfigured, and ForgotPasswordUriProvider doesn't log that case. Keeping the message generic is fine for an anonymous screen, but then the admin gets no signal at all. Could we log it server-side?
  3. The guarantee that matters most here (a send failure returns the same 200 as an unknown email) is only tested at service level. A controller integration test that makes the send throw and asserts 200 for both a registered and an unknown email would pin it down.

@AndyButland

Copy link
Copy Markdown
Contributor Author

Thanks @iOvergaard - all good points, and worth fixing up I would say. I've applied them in the previous three commits.

Comment thread src/Umbraco.Core/Services/UserService.cs Dismissed
@sonarqubecloud

sonarqubecloud Bot commented Oct 8, 2026

Copy link
Copy Markdown

@iOvergaard iOvergaard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Andy, all three look good, and the unit and integration tests pass locally 👍 The CodeQL alert on the new log line looks like a false positive to me, as it only logs the user Id/Key and the status, not the reset link.

@AndyButland
AndyButland merged commit 20d00de into v17/dev Oct 8, 2026
31 of 32 checks passed
@AndyButland
AndyButland deleted the v17/bugfix/forgot-password-email-unavailable branch October 8, 2026 14:09
iOvergaard added a commit that referenced this pull request Oct 12, 2026
…r lookup and always apply the minimum response time (#24152)

Cherry-picked without the UserServiceCrudTests changes, which build on test helpers from #24042 (not in this release). The unit tests for both fixes are included.

* fix(api): report an unconfigured application URL before the password reset user lookup

ApplicationUrlNotConfigured was only produced once the user had been found,
so the reset endpoint answered a registered email differently from an
unknown one on sites where the application URL could not be resolved.

Add IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync for failures
that don't depend on the user, and check it before the lookup. The default
implementation succeeds, so existing providers are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): always apply the minimum response time on password reset endpoints

The filter checked TimeSpan.Microseconds, which is only the 0-999 µs
component of the remaining wait, so the delay was skipped whenever that
component happened to be zero. Compare the whole TimeSpan instead, and
measure with TimeProvider so the behaviour can be tested deterministically.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Added docs for EnsureMinimumResponseTimeFilter.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Andy Butland <abutland73@gmail.com>
iOvergaard added a commit that referenced this pull request Oct 12, 2026
…r lookup and always apply the minimum response time (#24152)

Cherry-picked without the UserServiceCrudTests changes, which build on test helpers from #24042 (not in this release). The unit tests for both fixes are included.

* fix(api): report an unconfigured application URL before the password reset user lookup

ApplicationUrlNotConfigured was only produced once the user had been found,
so the reset endpoint answered a registered email differently from an
unknown one on sites where the application URL could not be resolved.

Add IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync for failures
that don't depend on the user, and check it before the lookup. The default
implementation succeeds, so existing providers are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): always apply the minimum response time on password reset endpoints

The filter checked TimeSpan.Microseconds, which is only the 0-999 µs
component of the remaining wait, so the delay was skipped whenever that
component happened to be zero. Compare the whole TimeSpan instead, and
measure with TimeProvider so the behaviour can be tested deterministically.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Added docs for EnsureMinimumResponseTimeFilter.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Andy Butland <abutland73@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants