From e04c47a7e4ae1bb4286360505da130319dbda608 Mon Sep 17 00:00:00 2001 From: Ben Herila Date: Sat, 13 Jun 2026 17:19:45 -0700 Subject: [PATCH 1/2] fix: harden audit routes and unify login gate to prevent integration footguns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four integration bugs surfaced in a consuming app (vora); this commit prevents the package-preventable ones: **Finding 1 (P1 — access-control bypass)** - Add `AuthUserPolicy::canLogin()` to the contract with a docblock explaining it is the single gate for account state (active, approved, not disabled). The default implementation duck-types `$user->canLogin()` then falls back to `is_disabled`, matching the previous `canPasskeyLogin()` logic. - `DefaultAuthUserPolicy::canPasskeyLogin()` now delegates to `canLogin()` so both code paths share one implementation. - New `RequireActiveUser` middleware calls `policy->canLogin()` and aborts 403 when the user is not allowed in. Applied automatically on all package audit-log routes (previously only `auth` middleware was applied, letting a pending/disabled admin reach the admin endpoint if their Gate ability only checked role). - Config comment on `audit.admin_ability` warns that the gate definition must check both role and account state. **Finding 2 (P1 — lockout-after-migration)** - Package does not own the `approved_at` column (it is app-side), so there is nothing to migrate-fix. Added prominent README guidance with a code example showing how to backfill the column in the migration that adds it, so integrators do not lock out existing users. **Finding 3 (P2 — onboarding redirect divergence)** - Package does not ship an email-verification controller (app-owned), so the hardcoded `/pending` redirect was not in this repo. - Added README guidance: after email verification the app must call `AuthUserPolicy::redirectAfterLogin()` (not a hardcoded path), so an already-approved user who verifies their email is sent to the correct destination instead of the pending page. **Finding 4 (P3 — dropdown outside-click)** - The package ships no navbar/dropdown/menu components; this bug is entirely app-side. No change. Tests: 7 new assertions covering `canLogin()`, the `is_disabled` fallback, the `canPasskeyLogin()` delegation, and the middleware pass/block/unauthenticated cases. All 40 tests pass. Co-Authored-By: Claude Opus 4.8 --- php/README.md | 64 ++++++++++ php/config/bherila-auth.php | 5 + php/routes/audit.php | 6 +- php/src/Contracts/AuthUserPolicy.php | 12 ++ php/src/Http/Middleware/RequireActiveUser.php | 47 +++++++ php/src/Services/DefaultAuthUserPolicy.php | 25 +++- php/tests/Feature/RequireActiveUserTest.php | 116 ++++++++++++++++++ 7 files changed, 273 insertions(+), 2 deletions(-) create mode 100644 php/src/Http/Middleware/RequireActiveUser.php create mode 100644 php/tests/Feature/RequireActiveUserTest.php diff --git a/php/README.md b/php/README.md index e3df3d5..f887b1f 100644 --- a/php/README.md +++ b/php/README.md @@ -178,6 +178,70 @@ Bind `BWH\Auth\Contracts\AuthUserPolicy` when an app needs custom login gates or Bind `BWH\Auth\Contracts\AuthAuditLogger` only when an app wants to override the built-in audit behavior (for example, to mirror events into its own broader audit table). Most apps should instead use the database driver described below. +### canLogin() — the single gate for account state + +`AuthUserPolicy::canLogin()` is the **single source of truth** for "is this account allowed to proceed through any login flow." The default implementation duck-types `$user->canLogin()` and falls back to checking `$user->is_disabled`. Apps with additional account-state columns (e.g. `approved_at`, `email_verified_at`, or a role whitelist) must bind a custom policy and encode **all** conditions in `canLogin()`: + +```php +// app/Auth/AppUserPolicy.php +class AppUserPolicy extends DefaultAuthUserPolicy +{ + public function canLogin(Authenticatable $user, Request $request): bool + { + return $user->approved_at !== null + && ! $user->is_disabled; + } +} + +// AppServiceProvider::register() +$this->app->bind(AuthUserPolicy::class, AppUserPolicy::class); +``` + +The package calls `canLogin()` automatically from: + +- `RequireActiveUser` middleware, applied to all package audit-log routes +- `canPasskeyLogin()` in the default policy (passkey auth delegates here) +- The 2FA `completeLogin()` path delegates through `redirectAfterLogin()`; if the user should be + blocked at that point, `canLogin()` must return false so the redirect sends them away from the app + +Apps must also call `canLogin()` from their own: + +1. **Primary password-login controller** — before `Auth::attempt()` or after resolving the user. +2. **Email-verification callback** — after marking the email verified, call `canLogin()` and use + `redirectAfterLogin()` (not a hardcoded path) so a just-verified but still-pending user goes to + the pending page rather than into the app. Hardcoding `/pending` in the verification handler + causes approved users who verified their email to be falsely shown the pending page. + +### Protecting admin gates against pending/disabled accounts + +When setting `audit.admin_ability`, the Gate ability definition must verify **both** admin role and active-account state. The package applies `RequireActiveUser` on top, but your Gate definition should be correct independently (it may be called from other locations): + +```php +// AppServiceProvider::boot() +Gate::define('admin-only', function (User $user) { + // WRONG: only checks role — a pending admin bypasses account-state checks + // return $user->is_admin; + + // CORRECT: role AND account state + return $user->is_admin + && $user->approved_at !== null + && ! $user->is_disabled; +}); +``` + +### Backfilling approved_at after adding an approval column + +If you add an `approved_at` (nullable, null = pending) column to your existing `users` table, every pre-existing row will be null after migration, instantly locking out all current users — including the primary admin. Before deploying the migration to production, add a backfill step in your migration (or a separate migration) to grandfather existing rows: + +```php +// In your migration's up() method, after adding the column: +DB::table('users') + ->whereNull('approved_at') + ->update(['approved_at' => now()]); +``` + +Alternatively, make null mean "approved" and use a different sentinel (e.g. a `pending` boolean), but document the convention clearly. + ## Login audit logging The package can own a single append-only audit log for authentication events, so consuming apps no longer hand-roll their own login-audit table and writer. diff --git a/php/config/bherila-auth.php b/php/config/bherila-auth.php index af32471..657bc34 100644 --- a/php/config/bherila-auth.php +++ b/php/config/bherila-auth.php @@ -24,6 +24,11 @@ // null = retain forever (no pruning). Set a positive integer to enable `model:prune`. 'retention_days' => env('BHERILA_AUTH_AUDIT_RETENTION_DAYS'), // Gate ability required for the cross-user admin endpoint; null disables that route. + // IMPORTANT: the ability must verify that the user is active/approved AND is an admin. + // The package enforces its own RequireActiveUser check on top of this gate, but the + // gate should still verify account state independently so your Gate definition is + // correct even when called from other locations. Example: check both ->is_admin and + // ->approved_at, not just the role. 'admin_ability' => env('BHERILA_AUTH_AUDIT_ADMIN_ABILITY'), ], diff --git a/php/routes/audit.php b/php/routes/audit.php index 71f8761..f2f001b 100644 --- a/php/routes/audit.php +++ b/php/routes/audit.php @@ -1,9 +1,13 @@ group(function () { +// The RequireActiveUser middleware ensures a pending or disabled account cannot reach +// these endpoints even when the surrounding app gate only checks role without verifying +// account state (see bherila-auth.audit.admin_ability). +Route::middleware(['auth', RequireActiveUser::class])->group(function () { Route::get('/auth/audit-log', [AuthAuditController::class, 'index'])->name('bherila-auth.audit.index'); Route::post('/auth/audit-log/{id}/suspicious', [AuthAuditController::class, 'markSuspicious'])->name('bherila-auth.audit.suspicious'); Route::get('/auth/audit-log/all', [AuthAuditController::class, 'all'])->name('bherila-auth.audit.all'); diff --git a/php/src/Contracts/AuthUserPolicy.php b/php/src/Contracts/AuthUserPolicy.php index d2bb527..1792d33 100644 --- a/php/src/Contracts/AuthUserPolicy.php +++ b/php/src/Contracts/AuthUserPolicy.php @@ -7,6 +7,18 @@ interface AuthUserPolicy { + /** + * Whether the user is allowed to complete any login (active, not disabled, approved, etc.). + * + * The package's own middleware ({@see \BWH\Auth\Http\Middleware\RequireActiveUser}) calls + * this before serving audit-log endpoints. Implementing apps must also call it from their + * primary password-login controller and from any post-email-verification redirect. + * + * The default implementation duck-types `$user->canLogin()`, then falls back to checking + * `$user->is_disabled`. If neither exists the user is assumed to be allowed. + */ + public function canLogin(Authenticatable $user, Request $request): bool; + public function canPasskeyLogin(Authenticatable $user, Request $request): bool; public function redirectAfterLogin(Authenticatable $user, Request $request): string; diff --git a/php/src/Http/Middleware/RequireActiveUser.php b/php/src/Http/Middleware/RequireActiveUser.php new file mode 100644 index 0000000..85ce377 --- /dev/null +++ b/php/src/Http/Middleware/RequireActiveUser.php @@ -0,0 +1,47 @@ +group(function () { ... }); + * ``` + * + * The check delegates to {@see \BWH\Auth\Contracts\AuthUserPolicy::canLogin()}, which + * by default duck-types `$user->canLogin()` and falls back to `$user->is_disabled`. + * Bind a custom AuthUserPolicy to add approved-at, email-verified, or other checks. + */ +class RequireActiveUser +{ + public function __construct(private readonly AuthUserPolicy $policy) {} + + public function handle(Request $request, Closure $next): Response + { + $user = $request->user(); + + if ($user === null) { + // Let the auth middleware handle unauthenticated requests. + return $next($request); + } + + if (! $this->policy->canLogin($user, $request)) { + abort(403, 'Your account is not active.'); + } + + return $next($request); + } +} diff --git a/php/src/Services/DefaultAuthUserPolicy.php b/php/src/Services/DefaultAuthUserPolicy.php index 79d8369..323387c 100644 --- a/php/src/Services/DefaultAuthUserPolicy.php +++ b/php/src/Services/DefaultAuthUserPolicy.php @@ -8,7 +8,19 @@ class DefaultAuthUserPolicy implements AuthUserPolicy { - public function canPasskeyLogin(Authenticatable $user, Request $request): bool + /** + * Whether the user is allowed to log in. + * + * Duck-types `$user->canLogin()` first (covers approved/active/disabled checks in one call), + * then falls back to `$user->is_disabled`, then defaults to `true` for apps that have no + * such columns. + * + * Apps with an `approved_at` column or a multi-step onboarding state should bind a custom + * {@see \BWH\Auth\Contracts\AuthUserPolicy} implementation that encodes those rules here, + * and call `canLogin()` from their primary password-login controller and from any + * post-email-verification redirect so all entry points share the same gate. + */ + public function canLogin(Authenticatable $user, Request $request): bool { if (method_exists($user, 'canLogin')) { return (bool) $user->canLogin(); @@ -21,6 +33,17 @@ public function canPasskeyLogin(Authenticatable $user, Request $request): bool return true; } + /** + * Whether the user may authenticate via a passkey. + * + * Delegates to {@see canLogin()} — if the user is not allowed to log in at all, passkey + * login is also denied. + */ + public function canPasskeyLogin(Authenticatable $user, Request $request): bool + { + return $this->canLogin($user, $request); + } + public function redirectAfterLogin(Authenticatable $user, Request $request): string { if (method_exists($user, 'getLoginRedirectUrl')) { diff --git a/php/tests/Feature/RequireActiveUserTest.php b/php/tests/Feature/RequireActiveUserTest.php new file mode 100644 index 0000000..90ba965 --- /dev/null +++ b/php/tests/Feature/RequireActiveUserTest.php @@ -0,0 +1,116 @@ + 'Test', 'email' => 'a@example.com', 'password' => bcrypt('x')]); + $policy = new DefaultAuthUserPolicy; + + $this->assertTrue($policy->canLogin($user, Request::create('/'))); + } + + public function test_default_policy_delegates_to_can_login_method_when_present(): void + { + $user = new class extends User + { + public function canLogin(): bool + { + return false; + } + }; + $user->forceFill(['name' => 'Test', 'email' => 'b@example.com', 'password' => bcrypt('x')]); + $user->exists = true; + + $policy = new DefaultAuthUserPolicy; + $this->assertFalse($policy->canLogin($user, Request::create('/'))); + } + + public function test_default_policy_uses_is_disabled_fallback(): void + { + $user = new class extends User + { + public bool $is_disabled = true; + }; + $user->forceFill(['name' => 'Test', 'email' => 'c@example.com', 'password' => bcrypt('x')]); + $user->exists = true; + + $policy = new DefaultAuthUserPolicy; + $this->assertFalse($policy->canLogin($user, Request::create('/'))); + } + + public function test_default_policy_can_passkey_login_delegates_to_can_login(): void + { + $user = new class extends User + { + public function canLogin(): bool + { + return false; + } + }; + $user->forceFill(['name' => 'Test', 'email' => 'd@example.com', 'password' => bcrypt('x')]); + $user->exists = true; + + $policy = new DefaultAuthUserPolicy; + $this->assertFalse($policy->canPasskeyLogin($user, Request::create('/'))); + } + + // --- RequireActiveUser middleware --- + + public function test_middleware_passes_through_when_user_can_login(): void + { + $user = User::create(['name' => 'Test', 'email' => 'e@example.com', 'password' => bcrypt('x')]); + + $request = Request::create('/'); + $request->setUserResolver(fn () => $user); + + $middleware = new RequireActiveUser(app(AuthUserPolicy::class)); + $response = $middleware->handle($request, fn ($r) => response('ok')); + + $this->assertSame('ok', $response->getContent()); + } + + public function test_middleware_aborts_403_when_user_cannot_login(): void + { + $policy = new class extends DefaultAuthUserPolicy + { + public function canLogin(Authenticatable $user, Request $request): bool + { + return false; + } + }; + + $user = User::create(['name' => 'Test', 'email' => 'f@example.com', 'password' => bcrypt('x')]); + + $request = Request::create('/'); + $request->setUserResolver(fn () => $user); + + $this->expectException(\Symfony\Component\HttpKernel\Exception\HttpException::class); + + $middleware = new RequireActiveUser($policy); + $middleware->handle($request, fn ($r) => response('ok')); + } + + public function test_middleware_passes_through_when_no_authenticated_user(): void + { + $request = Request::create('/'); + $request->setUserResolver(fn () => null); + + $middleware = new RequireActiveUser(app(AuthUserPolicy::class)); + $response = $middleware->handle($request, fn ($r) => response('ok')); + + $this->assertSame('ok', $response->getContent()); + } +} From 9549c8f975f73a69a0064d664ef1d77a04d652b5 Mon Sep 17 00:00:00 2001 From: Ben Herila Date: Sat, 13 Jun 2026 17:22:03 -0700 Subject: [PATCH 2/2] docs: flag v0.5.0 breaking contract change and upgrade path Adding canLogin() to the AuthUserPolicy contract is a BC break for direct implementers. Document that this must ship as v0.5.0 (not a 0.4.x patch, which ^0.4 consumers would auto-pull and fatal on) and how integrators migrate. Co-Authored-By: Claude Opus 4.8 --- php/README.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/php/README.md b/php/README.md index f887b1f..c4156d5 100644 --- a/php/README.md +++ b/php/README.md @@ -11,6 +11,35 @@ Includes: - login audit logging: an owned `auth_audit_log` table, a default database logger, binary IP storage, optional read endpoints, and opt-in retention (see "Login audit logging") - policy and audit contracts for app-specific behavior +## Upgrading to v0.5.0 (breaking) + +This release adds a required method to the `AuthUserPolicy` contract: + +```php +public function canLogin(Authenticatable $user, Request $request): bool; +``` + +It is the single gate for account-state checks (active, approved, not disabled) +and is now enforced by the new `RequireActiveUser` middleware on the package's +audit routes, so a role-only admin gate can no longer let a pending or disabled +account through. + +Because the contract gained a required method, this is a **breaking change** and +must be released as **v0.5.0** (not a 0.4.x patch) so consumers opt in. Consuming +apps that **implement `AuthUserPolicy` directly** must add `canLogin()` when they +upgrade — typically delegating to their model: + +```php +public function canLogin(Authenticatable $user, Request $request): bool +{ + return $user instanceof User && $user->canLogin() && $user->hasVerifiedEmail(); +} +``` + +Apps that extend `DefaultAuthUserPolicy` inherit a working `canLogin()` (it +duck-types `$user->canLogin()`, falls back to `is_disabled`, defaults to `true`) +and need no change. + ## Install ```sh