diff --git a/app/Livewire/Admin/InstanceAdminAccess.php b/app/Livewire/Admin/InstanceAdminAccess.php index 0115aaf..86a9951 100644 --- a/app/Livewire/Admin/InstanceAdminAccess.php +++ b/app/Livewire/Admin/InstanceAdminAccess.php @@ -2,10 +2,10 @@ namespace App\Livewire\Admin; +use App\Livewire\Concerns\ResolvesOperator; use App\Models\Instance; use App\Provisioning\Jobs\IssueInstanceAdminAccess; use App\Services\Wireguard\ConfigHandoff; -use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Str; @@ -25,6 +25,8 @@ use LivewireUI\Modal\ModalComponent; */ class InstanceAdminAccess extends ModalComponent { + use ResolvesOperator; + public string $uuid; public string $subdomain = ''; @@ -55,9 +57,11 @@ class InstanceAdminAccess extends ModalComponent $this->authorize('instances.adminlogin'); $this->resetErrorBag('password'); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } - $key = 'instance-admin:'.$operator?->id; + $key = 'instance-admin:'.$operator->id; if (RateLimiter::tooManyAttempts($key, 5)) { $this->addError('password', __('instances.too_many_attempts', ['seconds' => RateLimiter::availableIn($key)])); diff --git a/app/Livewire/Admin/Settings.php b/app/Livewire/Admin/Settings.php index 39dfb11..454bf88 100644 --- a/app/Livewire/Admin/Settings.php +++ b/app/Livewire/Admin/Settings.php @@ -4,6 +4,7 @@ namespace App\Livewire\Admin; use App\Http\Middleware\RestrictConsoleNetwork; use App\Livewire\Concerns\ChangesOwnPassword; +use App\Livewire\Concerns\ResolvesOperator; use App\Models\Customer; use App\Models\Operator; use App\Models\VpnPeer; @@ -34,6 +35,7 @@ use Symfony\Component\HttpFoundation\IpUtils; class Settings extends Component { use ChangesOwnPassword; + use ResolvesOperator; /** This page changes the signed-in OPERATOR's password, never a portal one. */ protected function passwordAccountGuard(): string @@ -65,8 +67,12 @@ class Settings extends Component public function mount(): void { - $this->name = Auth::guard('operator')->user()->name; - $this->email = Auth::guard('operator')->user()->email; + if (! $operator = $this->currentOperator()) { + return; + } + + $this->name = $operator->name; + $this->email = $operator->email; $this->requireTwoFactor = AppSettings::bool('console.require_2fa', false); } @@ -89,10 +95,13 @@ class Settings extends Component public function saveAccount(): void { - $user = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } + $data = $this->validate([ 'name' => 'required|string|max:255', - 'email' => 'required|email|max:255|unique:operators,email,'.$user->id, + 'email' => 'required|email|max:255|unique:operators,email,'.$operator->id, ]); // An operator email must never collide with a customer's — that would @@ -105,13 +114,18 @@ class Settings extends Component return; } - $user->update($data); + $operator->update($data); $this->dispatch('notify', message: __('admin_settings.account_saved')); } public function inviteStaff(): void { $this->authorize('staff.manage'); + + if (! $operator = $this->currentOperator()) { + return; + } + $data = $this->validate([ 'staffName' => 'required|string|max:255', 'staffEmail' => 'required|email|max:255|unique:operators,email', @@ -119,7 +133,7 @@ class Settings extends Component ]); // Only an Owner may create another Owner. - if ($data['staffRole'] === 'Owner' && ! Auth::guard('operator')->user()->hasRole('Owner')) { + if ($data['staffRole'] === 'Owner' && ! $operator->hasRole('Owner')) { $this->addError('staffRole', __('admin_settings.owner_only')); return; @@ -135,12 +149,12 @@ class Settings extends Component // password and surface it once to the Owner to share securely — the // account is usable immediately (a proper invite link follows with mail). $temp = Str::password(14); - $operator = Operator::create([ + $newStaff = Operator::create([ 'name' => $data['staffName'], 'email' => $data['staffEmail'], 'password' => Hash::make($temp), ]); - $operator->assignRole($data['staffRole']); + $newStaff->assignRole($data['staffRole']); $this->invitedEmail = $data['staffEmail']; $this->invitedPassword = $temp; @@ -156,7 +170,11 @@ class Settings extends Component return; } - $result = DB::transaction(function () use ($id, $role) { + if (! $operator = $this->currentOperator()) { + return; + } + + $result = DB::transaction(function () use ($id, $role, $operator) { // Serialize on the Owner role so the global owner count can't be // raced to zero by concurrent demotions of different owners. Role::query()->where('name', 'Owner')->lockForUpdate()->first(); @@ -165,7 +183,7 @@ class Settings extends Component if ($target === null) { return 'gone'; } - if ($target->id === Auth::guard('operator')->id()) { + if ($target->id === $operator->id) { return 'self'; } // Only existing operators may be re-roled. The Customer check below @@ -176,7 +194,7 @@ class Settings extends Component return 'not_staff'; } // Granting or revoking Owner is Owner-only. - if (($role === 'Owner' || $target->hasRole('Owner')) && ! Auth::guard('operator')->user()->hasRole('Owner')) { + if (($role === 'Owner' || $target->hasRole('Owner')) && ! $operator->hasRole('Owner')) { return 'owner_only'; } // Never demote the last Owner. @@ -377,7 +395,11 @@ class Settings extends Component { $this->authorize('site.manage'); - $accepted = app(UpdateChannel::class)->request(Auth::guard('operator')->user()->email); + if (! $operator = $this->currentOperator()) { + return; + } + + $accepted = app(UpdateChannel::class)->request($operator->email); $this->dispatch('notify', message: __($accepted ? 'admin_settings.update_requested' @@ -386,6 +408,8 @@ class Settings extends Component public function render() { + $operator = $this->currentOperator(); + $staff = Operator::query() ->whereHas('roles', fn ($q) => $q->whereIn('name', Operator::OPERATOR_ROLES)) ->with('roles') @@ -403,7 +427,7 @@ class Settings extends Component 'update' => app(UpdateChannel::class)->state(), 'updateLog' => app(UpdateChannel::class)->lastLog(), 'sitePublic' => AppSettings::bool('site.public', true), - 'canManageSite' => Auth::guard('operator')->user()?->can('site.manage') ?? false, + 'canManageSite' => $operator?->can('site.manage') ?? false, // Shown so nobody has to guess why they still see the real site. 'viewerOnVpn' => IpUtils::checkIp( (string) request()->ip(), @@ -417,8 +441,8 @@ class Settings extends Component 'viewerIp' => (string) request()->ip(), 'staff' => $staff, 'roles' => Operator::OPERATOR_ROLES, - 'canManageStaff' => Auth::guard('operator')->user()->can('staff.manage'), - 'isOwner' => Auth::guard('operator')->user()->hasRole('Owner'), + 'canManageStaff' => $operator?->can('staff.manage') ?? false, + 'isOwner' => $operator?->hasRole('Owner') ?? false, ]); } } diff --git a/app/Livewire/Admin/TwoFactorSetup.php b/app/Livewire/Admin/TwoFactorSetup.php index 1b9343f..0bb2c97 100644 --- a/app/Livewire/Admin/TwoFactorSetup.php +++ b/app/Livewire/Admin/TwoFactorSetup.php @@ -3,7 +3,7 @@ namespace App\Livewire\Admin; use App\Livewire\Concerns\ConfirmsPassword; -use Illuminate\Support\Facades\Auth; +use App\Livewire\Concerns\ResolvesOperator; use Illuminate\Validation\ValidationException; use Laravel\Fortify\Actions\ConfirmTwoFactorAuthentication; use Laravel\Fortify\Actions\DisableTwoFactorAuthentication; @@ -38,6 +38,7 @@ use Livewire\Component; class TwoFactorSetup extends Component { use ConfirmsPassword; + use ResolvesOperator; /** Two-factor is confirmed against the operator's own record, not a portal one. */ protected function confirmationGuard(): string @@ -61,7 +62,11 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - app(EnableTwoFactorAuthentication::class)(Auth::guard('operator')->user()); + if (! $operator = $this->currentOperator()) { + return; + } + + app(EnableTwoFactorAuthentication::class)($operator); } /** Accept a code from the app, which is what actually turns it on. */ @@ -69,7 +74,9 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } try { app(ConfirmTwoFactorAuthentication::class)( @@ -94,7 +101,10 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } + app(GenerateNewRecoveryCodes::class)($operator); $this->recoveryCodes = json_decode(decrypt($operator->refresh()->two_factor_recovery_codes), true); } @@ -103,7 +113,11 @@ class TwoFactorSetup extends Component { $this->requireConfirmedPassword(); - app(DisableTwoFactorAuthentication::class)(Auth::guard('operator')->user()); + if (! $operator = $this->currentOperator()) { + return; + } + + app(DisableTwoFactorAuthentication::class)($operator); $this->recoveryCodes = null; $this->dispatch('notify', message: __('two_factor_setup.off')); } @@ -122,7 +136,18 @@ class TwoFactorSetup extends Component public function render() { - $operator = Auth::guard('operator')->user(); + // Unlike the actions above, render() has no requireConfirmedPassword() + // gate in front of it — Livewire calls it on every update, including a + // bare property sync, so it is its own reachable path for a session + // that ended after the page loaded. + if (! $operator = $this->currentOperator()) { + return view('livewire.admin.two-factor-setup', [ + 'twoFactorPending' => false, + 'twoFactorOn' => false, + 'twoFactorQr' => null, + 'passwordConfirmed' => false, + ]); + } return view('livewire.admin.two-factor-setup', [ // Never the secret itself — only whether it exists, and the SVG diff --git a/app/Livewire/Admin/VpnConfigAccess.php b/app/Livewire/Admin/VpnConfigAccess.php index 52972fd..310b558 100644 --- a/app/Livewire/Admin/VpnConfigAccess.php +++ b/app/Livewire/Admin/VpnConfigAccess.php @@ -2,11 +2,11 @@ namespace App\Livewire\Admin; +use App\Livewire\Concerns\ResolvesOperator; use App\Models\VpnPeer; use App\Services\Wireguard\ConfigHandoff; use App\Services\Wireguard\ConfigVault; use App\Services\Wireguard\QrCode; -use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Hash; @@ -25,6 +25,8 @@ use LivewireUI\Modal\ModalComponent; */ class VpnConfigAccess extends ModalComponent { + use ResolvesOperator; + public string $uuid; public string $name = ''; @@ -70,10 +72,12 @@ class VpnConfigAccess extends ModalComponent $peer = VpnPeer::query()->where('uuid', $this->uuid)->firstOrFail(); $this->authorize('downloadConfig', $peer); - $operator = Auth::guard('operator')->user(); + if (! $operator = $this->currentOperator()) { + return; + } // Rate-limited per user, so a stolen session cannot grind the password. - $key = 'vpn-config:'.$operator?->id; + $key = 'vpn-config:'.$operator->id; if (RateLimiter::tooManyAttempts($key, 5)) { $this->addError('password', __('vpn.too_many_attempts', ['seconds' => RateLimiter::availableIn($key)])); diff --git a/app/Livewire/Concerns/ResolvesOperator.php b/app/Livewire/Concerns/ResolvesOperator.php new file mode 100644 index 0000000..5e0df8a --- /dev/null +++ b/app/Livewire/Concerns/ResolvesOperator.php @@ -0,0 +1,52 @@ +user() + * comes back null. Four components each dereferenced it unguarded + * ($operator->password, ->id, ->two_factor_recovery_codes, …), turning that + * into a 500 instead of a sign-in prompt. + * + * currentOperator() cannot throw its way out of the null case. Livewire only + * turns $this->redirect() into a browser navigation once the calling action + * returns NORMALLY: the redirect is queued as component state and picked up + * during Livewire's own dehydrate step (Livewire\Features\SupportRedirects), + * which runs strictly after the called method returns — an uncaught + * exception during the call skips it entirely and Livewire ships back a bare + * error response instead (confirmed by reading + * Livewire\Mechanisms\HandleComponents::update() and + * Livewire\Mechanisms\HandleRequests::handleUpdate(), which catches nothing + * but \TypeError). So this returns null and every caller checks for it: + * + * if (! $operator = $this->currentOperator()) { + * return; + * } + * + * That one-line guard is the only thing left for a caller to get wrong; the + * redirect target and the mechanism live here once, so the four cannot drift + * apart the way the original unguarded dereferences did. + */ +trait ResolvesOperator +{ + protected function currentOperator(): ?Operator + { + $operator = Auth::guard('operator')->user(); + + if ($operator === null) { + $this->redirect(route('admin.login')); + } + + return $operator; + } +} diff --git a/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php b/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php index cde5d85..69d6a28 100644 --- a/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php +++ b/tests/Feature/Admin/OperatorTwoFactorEnrollmentTest.php @@ -1,6 +1,7 @@ refresh()->two_factor_confirmed_at)->toBeNull() ->and($op->two_factor_secret)->toBeNull(); }); + +it('sends the operator to sign in instead of a 500 when the session has ended', function () { + // render() has no requireConfirmedPassword() gate in front of it — unlike + // every action above, Livewire calls it on every update including a bare + // property sync, so it is directly reachable with no other guard in the + // way once the session that owned the page has ended. + $op = operator('Support'); + + $page = Livewire::actingAs($op, 'operator')->test(TwoFactorSetup::class); + + // The page sat open long enough for the operator's own session to end. + Auth::guard('operator')->logout(); + + $page->call('$refresh')->assertRedirect(route('admin.login')); +}); diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 7e4bc7c..988ba91 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -4,6 +4,7 @@ use App\Livewire\Admin\ConfirmDeleteVpnPeer; use App\Livewire\Admin\Vpn; use App\Livewire\Admin\VpnConfigAccess; use App\Models\Host; +use App\Models\Operator; use App\Models\VpnPeer; use App\Provisioning\Jobs\ApplyVpnPeer; use App\Provisioning\Jobs\SyncVpnPeers; @@ -12,6 +13,8 @@ use App\Services\Wireguard\Keypair; use App\Services\Wireguard\LocalWireguardHub; use App\Services\Wireguard\PeerSnapshot; use App\Services\Wireguard\WireguardHub; +use Illuminate\Support\Facades\Auth; +use Illuminate\Support\Facades\Gate; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Queue; use Livewire\Livewire; @@ -620,6 +623,31 @@ it('locks out password guessing', function () { expect($modal->get('token'))->toBeNull(); }); +it('sends the operator to sign in instead of a 500 when the session has ended', function () { + // reveal() re-checks $this->authorize('downloadConfig', $peer) on every + // call, which already refuses a guest (Gate auto-denies a policy whose + // first parameter is a non-nullable Operator). That is a separate, + // already-correct guard — not the one this test is for. A Gate::before + // with a NULLABLE $user isolates the one line this regression is about: + // Auth::guard('operator')->user() coming back null past that point, + // which used to be dereferenced unguarded ($operator->password). + Gate::before(fn (?Operator $user, string $ability) => $ability === 'downloadConfig' ? true : null); + + vpnHub(); + $support = operator('Support'); + $peer = VpnPeer::factory()->ownedBy($support)->create([ + 'config_secret' => App\Services\Wireguard\ConfigVault::encrypt("[Interface]\nPrivateKey = XYZ\n"), + ]); + + $modal = Livewire::actingAs($support, 'operator')->test(VpnConfigAccess::class, ['uuid' => $peer->uuid]); + + // The modal sat open long enough for the operator's own session to end. + Auth::guard('operator')->logout(); + + $modal->set('password', 'geheim-1234')->call('reveal') + ->assertRedirect(route('admin.login')); +}); + it('destroys the stored config when the access is revoked', function () { vpnHub(); Queue::fake();