From 02750a7c9a8b5f2504f22f976c26f27b7b9266e6 Mon Sep 17 00:00:00 2001
From: nexxo <nexxo@clupilot.local>
Date: Sat, 25 Jul 2026 22:46:23 +0200
Subject: [PATCH] fix(vpn): lock the owner row while issuing an access
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A revocation committing between the operator check and the insert would find no
peer to remove and still leave the revoked colleague with a brand-new tunnel.
Issuing now runs in a transaction that locks the owner row — the same row
revokeStaff() locks — so the two cannot interleave.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 app/Livewire/Admin/Vpn.php      | 24 ++++++++++++++----------
 tests/Feature/Admin/VpnTest.php | 12 ++++++++++++
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/app/Livewire/Admin/Vpn.php b/app/Livewire/Admin/Vpn.php
index 7835dc1..e208bf2 100644
--- a/app/Livewire/Admin/Vpn.php
+++ b/app/Livewire/Admin/Vpn.php
@@ -13,6 +13,7 @@ use App\Services\Wireguard\Keypair;
 use App\Services\Wireguard\QrCode;
 use App\Services\Wireguard\WireguardHub;
 use Illuminate\Database\QueryException;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Str;
 use Illuminate\Validation\Rule;
@@ -75,13 +76,6 @@ class Vpn extends Component
             'ownerId' => ['required', Rule::exists('users', 'id')],
         ]);
 
-        $owner = User::query()->find($this->ownerId);
-        if ($owner === null || ! $owner->isOperator()) {
-            $this->addError('ownerId', __('vpn.owner_must_be_operator'));
-
-            return;
-        }
-
         // Storing needs a key. Without one we would either write the credential
         // in the clear or pretend we stored it — both worse than saying no.
         if ($this->storeConfig && ! ConfigVault::available()) {
@@ -114,7 +108,16 @@ class Vpn extends Component
         // tables, so neither unique index can catch the other's insert — the
         // shared lock is what keeps a host and an access off the same tunnel IP.
         try {
-            $peer = Cache::lock('wireguard:allocate', 30)->block(10, function () use ($hub, $publicKey) {
+            $peer = Cache::lock('wireguard:allocate', 30)->block(10, fn () => DB::transaction(function () use ($hub, $publicKey) {
+                // The owner row is locked for the whole insert, and revokeStaff()
+                // locks the same row: without that, a revocation could commit
+                // between the check and the insert, find no peer to remove, and
+                // leave the revoked colleague with a brand-new tunnel.
+                $owner = User::query()->whereKey($this->ownerId)->lockForUpdate()->first();
+                if ($owner === null || ! $owner->isOperator()) {
+                    return 'owner_must_be_operator';
+                }
+
                 // Inside the lock: checking before it would let two concurrent
                 // requests both pass and the loser hit the unique index as a
                 // 500. withTrashed, because a revoked peer keeps its key until
@@ -142,7 +145,7 @@ class Vpn extends Component
                     'present' => false,
                     'created_by' => auth()->id(),
                 ]);
-            });
+            }));
         } catch (QueryException) {
             // Backstop: the unique index caught a writer that did not take this
             // lock. Report it like any other duplicate instead of a 500.
@@ -152,7 +155,8 @@ class Vpn extends Component
         }
 
         if (is_string($peer)) {
-            $this->addError('publicKey', __('vpn.'.$peer));
+            $field = $peer === 'owner_must_be_operator' ? 'ownerId' : 'publicKey';
+            $this->addError($field, __('vpn.'.$peer));
 
             return;
         }
diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php
index 8c0d331..44df093 100644
--- a/tests/Feature/Admin/VpnTest.php
+++ b/tests/Feature/Admin/VpnTest.php
@@ -731,3 +731,15 @@ it('does not keep polling while a private config is on screen', function () {
         ->assertSee('wire:poll', false)
         ->assertDontSee('PrivateKey');
 });
+
+it('will not issue an access to someone who is no longer staff', function () {
+    vpnHub();
+    $former = App\Models\User::factory()->create();   // no operator role
+
+    Livewire::actingAs(operator('Owner'))->test(Vpn::class)
+        ->set('name', 'Ex-Kollege')->set('ownerId', $former->id)
+        ->call('create')
+        ->assertHasErrors('ownerId');
+
+    expect(VpnPeer::withTrashed()->count())->toBe(0);
+});