From f62304a24586bf6a94e7533448c084524508d3f0 Mon Sep 17 00:00:00 2001 From: nexxo Date: Mon, 3 Aug 2026 23:57:15 +0200 Subject: [PATCH] Jemanden aus einer Gruppe zu nehmen, in der er nicht ist, ist kein Fehler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit K3. `mitarbeiter` und `nur-lesen` legt niemand an — sie entstehen erst, wenn `user:add --group=…` sie zum ersten Mal braucht. `occ group:removeuser` legt nichts an und beendet mit Fehlercode, wenn die Gruppe fehlt, und run() verundet alle Exitcodes. Auf einer frischen Instanz scheiterte damit die erste Rollenaenderung dauerhaft: "fehlgeschlagen — Die Cloud hat die Aenderung nicht angenommen", und Wiederholen waehlte wieder role, also wieder denselben Fehlschlag. `2>/dev/null || true` hinter dem Entfernen, mit derselben Begruendung wie in HostFirewall::releaseMany(): nicht in der Gruppe zu sein IST der gewuenschte Endzustand. Nur fuers Entfernen — ein gescheitertes group:adduser bleibt ein Fehlschlag, denn wer in keiner Gruppe landet, sieht in seiner neuen Cloud nichts. Warum die Suite das nie sah: FakeProxmoxClient liess jeden nicht verskripteten Befehl gelingen, und die Pruefungen belegten die erzeugte Befehlsmenge, nie die Antwort des Gasts. Der Fake beachtet jetzt ein abschliessendes `|| true` — das ist eine Aussage der Shell, nicht des Aufrufers, und ein Fake, der trotzdem einen Fehlercode zurueckgaebe, liesse einen Test beweisen, dass ein Befehl scheitert, den keine echte Shell je scheitern laesst. Damit haelt die Pruefung den Fake ausdruecklich auf Fehlercode und sieht applyRole() trotzdem true liefern. Dazu ein Testkommentar, der den falschen Schutz benannte: bei owner/admin auf denselben Gruppennamen traegt `if ($gruppe !== $ziel)`, nicht array_unique. Co-Authored-By: Claude Opus 5 --- app/Services/Nextcloud/NextcloudUsers.php | 46 ++++++++---- app/Services/Proxmox/FakeProxmoxClient.php | 22 +++++- .../Feature/Nextcloud/NextcloudUsersTest.php | 72 +++++++++++++++---- 3 files changed, 110 insertions(+), 30 deletions(-) diff --git a/app/Services/Nextcloud/NextcloudUsers.php b/app/Services/Nextcloud/NextcloudUsers.php index 39ec794..98b7fa5 100644 --- a/app/Services/Nextcloud/NextcloudUsers.php +++ b/app/Services/Nextcloud/NextcloudUsers.php @@ -17,20 +17,20 @@ use Throwable; * welchen zieht, steht in SyncSeatToNextcloud — dieselbe Trennung wie bei * HostFirewall und BlockAddress. * - * Keine Methode wirft. Ein nicht erreichbarer Gast gibt `false` zurueck, und + * Keine Methode wirft. Ein nicht erreichbarer Gast gibt `false` zurück, und * der Auftrag schreibt das an den Sitz, wo der Inhaber es liest. Eine - * Ausnahme wuerde stattdessen den Bereitstellungs-Arbeiter mitreissen, auf dem - * die bezahlte Kundenbereitstellung laeuft. + * Ausnahme würde stattdessen den Bereitstellungs-Arbeiter mitreissen, auf dem + * die bezahlte Kundenbereitstellung läuft. * * Jeder Benutzername geht vor dem Einsetzen durch `isWellFormed()`. Das ist - * kein doppelter Boden fuer einen ohnehin sauberen Aufrufer, sondern die - * Bedingung dafuer, dass dieser Dienst eine Shell im Gast fuettern darf. + * kein doppelter Boden für einen ohnehin sauberen Aufrufer, sondern die + * Bedingung dafür, dass dieser Dienst eine Shell im Gast füttern darf. */ class NextcloudUsers { public function __construct(private ProxmoxClient $pve) {} - /** Anlegen falls noetig, danach die Willkommensmail — in einem Zug. */ + /** Anlegen falls nötig, danach die Willkommensmail — in einem Zug. */ public function invite(Instance $instance, Seat $seat): bool { $user = (string) $seat->nc_username; @@ -74,18 +74,38 @@ class NextcloudUsers $befehle = []; // Aus jeder anderen bekannten Gruppe heraus, in die eine hinein. + // + // `2>/dev/null || true` hinter dem Entfernen ist kein Wegsehen, + // sondern die richtige Bedeutung — dieselbe Begründung wie bei + // HostFirewall::releaseMany(): jemanden aus einer Gruppe zu + // nehmen, in der er nicht ist, ist kein Fehler, sondern der + // gewünschte Endzustand. + // + // Und ohne das Netz war es der HAEUFIGSTE Fall, nicht ein + // Randfall: `mitarbeiter` und `nur-lesen` legt niemand an. Sie + // entstehen erst, wenn `user:add --group=…` sie zum ersten Mal + // braucht — `group:removeuser` legt nichts an und beendet mit + // Fehlercode, wenn die Gruppe fehlt. Auf einer frischen Instanz + // scheiterte damit die erste Rollenänderung dauerhaft, weil + // run() alle Exitcodes verundet und Wiederholen wieder `role` + // wählt. + // + // Nur fürs Entfernen. Ein gescheitertes `group:adduser` bleibt + // ein Fehlschlag: wer in keiner Gruppe landet, sieht in seiner + // neuen Cloud nichts. foreach (array_unique(array_values(Seat::GROUPS)) as $gruppe) { if ($gruppe !== $ziel) { - $befehle[] = 'group:removeuser '.escapeshellarg($gruppe).' '.escapeshellarg($user); + $befehle[] = 'group:removeuser '.escapeshellarg($gruppe).' ' + .escapeshellarg($user).' 2>/dev/null || true'; } } $befehle[] = 'group:adduser '.escapeshellarg($ziel).' '.escapeshellarg($user); // Der Speicherplatz. Siehe ApplyStorageQuota: ein Konto mit - // EIGENEM Wert folgt der Vorgabe der Instanz nicht mehr. Fuer + // EIGENEM Wert folgt der Vorgabe der Instanz nicht mehr. Für // readonly ist genau das gewollt; beim VERLASSEN der Rolle muss - // der eigene Wert deshalb WEG, nicht ueberschrieben werden. + // der eigene Wert deshalb WEG, nicht überschrieben werden. $befehle[] = $seat->isReadonly() ? 'user:setting '.escapeshellarg($user).' files quota '.escapeshellarg('0 B') : 'user:setting '.escapeshellarg($user).' files quota --delete'; @@ -104,9 +124,9 @@ class NextcloudUsers return $this->run($instance, fn ($pve, $node, $vmid) => [ 'user:disable '.escapeshellarg($user), - // user:disable allein laesst laufende Sitzungen bis zu fuenf + // user:disable allein lässt laufende Sitzungen bis zu fünf // Minuten weiterleben. Bei jemandem, der gerade gegangen ist, - // sind fuenf Minuten fuenf zu viel. + // sind fünf Minuten fünf zu viel. 'user:auth-tokens:delete '.escapeshellarg($user), ]); } @@ -123,7 +143,7 @@ class NextcloudUsers } /** - * Nextcloud laesst Buchstaben, Ziffern und `-_.@` in Kennungen zu. Alles + * Nextcloud lässt Buchstaben, Ziffern und `-_.@` in Kennungen zu. Alles * andere ist entweder ein Fehler weiter oben oder ein Versuch — beides * will man sehen, und keines darf in eine Shell. */ @@ -142,7 +162,7 @@ class NextcloudUsers /** * Der Verbindungsaufbau steht EINMAL hier, nicht in jeder Methode. Der - * Rueckruf bekommt den fertigen Client mit — er braucht ihn, weil `invite()` + * Rückruf bekommt den fertigen Client mit — er braucht ihn, weil `invite()` * erst nachsehen muss, ob es den Benutzer schon gibt, bevor es entscheidet, * welchen Befehl es baut. * diff --git a/app/Services/Proxmox/FakeProxmoxClient.php b/app/Services/Proxmox/FakeProxmoxClient.php index 1b0aa4b..826fede 100644 --- a/app/Services/Proxmox/FakeProxmoxClient.php +++ b/app/Services/Proxmox/FakeProxmoxClient.php @@ -275,13 +275,31 @@ class FakeProxmoxClient implements ProxmoxClient throw $this->guestThrows[$vmid]; } + $ergebnis = ['exitcode' => $this->guestDefaultExit, 'out-data' => $this->guestDefaultOut]; + foreach ($this->guestScripts as $substring => $result) { if (str_contains($command, $substring)) { - return $result; + $ergebnis = $result; + break; } } - return ['exitcode' => $this->guestDefaultExit, 'out-data' => $this->guestDefaultOut]; + // Ein abschliessendes `|| true` ist eine Aussage der SHELL, nicht des + // Aufrufers: der Gastagent startet jede Zeile über `/bin/sh -c`, und + // dash beendet `A || true` immer mit 0 — egal, womit A endete. Ein + // Fake, der hier trotzdem den verskripteten Fehlercode zurückgäbe, + // liesse einen Test „beweisen", dass ein Befehl scheitert, den keine + // echte Shell je scheitern lässt (`group:removeuser` auf eine Gruppe, + // die es im Gast nicht gibt — siehe NextcloudUsers::applyRole()). + // + // Ein geworfener Fehler oben bleibt davon unberührt: ein nicht + // erreichbarer Gastagent führt gar keine Shell aus, da gibt es kein + // `|| true`, das etwas auffangen könnte. + if (str_ends_with(rtrim($command), '|| true')) { + $ergebnis['exitcode'] = 0; + } + + return $ergebnis; } public function guestRan(string $substring): bool diff --git a/tests/Feature/Nextcloud/NextcloudUsersTest.php b/tests/Feature/Nextcloud/NextcloudUsersTest.php index 35ef810..88ced39 100644 --- a/tests/Feature/Nextcloud/NextcloudUsersTest.php +++ b/tests/Feature/Nextcloud/NextcloudUsersTest.php @@ -14,14 +14,14 @@ function gastBereit(): array // Ohne host_id bliebe die host-Beziehung null, und run() wiese jeden // Befehl kommentarlos ab, statt einen zu bauen — die Instanz braucht - // einen echten Host, damit forHost() ueberhaupt greifen kann. + // einen echten Host, damit forHost() überhaupt greifen kann. return [$pve, Instance::factory()->create(['status' => 'active', 'vmid' => 201, 'host_id' => Host::factory()])]; } it('legt einen Benutzer mit erzeugtem Passwort an, das niemand sieht', function () { [$pve, $instance] = gastBereit(); // FakeProxmoxClient antwortet auf einen ungeskripteten Befehl mit Exitcode - // 0 — ohne diese Zeile saehe user:info wie "Benutzer existiert bereits" + // 0 — ohne diese Zeile sähe user:info wie "Benutzer existiert bereits" // aus. Dieselbe Falle, dasselbe Skript wie in CustomerStepsTest. $pve->guestScript('user:info', 1); $sitz = Seat::factory()->create(['email' => 'anna@firma.tld', 'name' => 'Anna', 'nc_username' => 'anna@firma.tld']); @@ -69,7 +69,7 @@ it('setzt bei readonly einen eigenen Speicherplatz von null', function () { it('LOESCHT den eigenen Speicherplatz, wenn readonly verlassen wird', function () { // Die Falle aus ApplyStorageQuota: "An account with an explicit quota stops // following the default". Ein Konto, das mit einem festen Wert aus der - // Rolle herauskommt, waere bei der naechsten Paketaenderung stumm + // Rolle herauskommt, wäre bei der nächsten Paketänderung stumm // ausgenommen — und niemand merkte es, bis der Kunde fragt, warum sein // Mitarbeiter weniger Platz hat als bezahlt. [$pve, $instance] = gastBereit(); @@ -84,10 +84,15 @@ it('LOESCHT den eigenen Speicherplatz, wenn readonly verlassen wird', function ( }); it('nimmt admin nicht kurz aus der eigenen Gruppe, wenn owner und admin auf denselben Namen zeigen', function () { - // owner und admin teilen sich dieselbe Nextcloud-Gruppe (admin). Ohne - // array_unique liefe hier group:removeuser admin VOR group:adduser admin - // — der Benutzer fiele kurz aus genau der Gruppe heraus, in die er - // gerade soll. + // owner und admin teilen sich dieselbe Nextcloud-Gruppe (admin). Tragend + // ist die Bedingung `if ($gruppe !== $ziel)`: ohne sie liefe hier + // group:removeuser admin VOR group:adduser admin — der Benutzer fiele kurz + // aus genau der Gruppe heraus, in die er gerade soll. + // + // NICHT array_unique: das dedupliziert nur die Liste der Gruppennamen und + // spart damit einen wiederholten Befehl, der ohnehin harmlos wäre. Ein + // Kommentar, der den falschen Schutz benennt, hält den Nächsten vom + // Nachsehen ab (R19). [$pve, $instance] = gastBereit(); $sitz = Seat::factory()->create(['role' => 'admin', 'nc_username' => 'anna@firma.tld']); @@ -99,10 +104,47 @@ it('nimmt admin nicht kurz aus der eigenen Gruppe, wenn owner und admin auf dens ->and($befehle)->toContain("group:adduser 'admin'"); }); -it('laesst eine Instanz ohne Host unangetastet', function () { +it('scheitert nicht an einer Gruppe, die es im Gast gar nicht gibt', function () { + // `mitarbeiter` und `nur-lesen` legt niemand an — sie entstehen erst, + // wenn `user:add --group=…` sie zum ersten Mal braucht. `occ + // group:removeuser` legt nichts an und beendet mit Fehlercode, wenn die + // Gruppe fehlt; run() verundet alle Exitcodes. + // + // Ohne das Auffangnetz war damit die erste Rollenänderung auf einer + // frischen Instanz dauerhaft „fehlgeschlagen — Die Cloud hat die Änderung + // nicht angenommen": Wiederholen wählt wieder `role`, und wieder fehlt + // dieselbe Gruppe. + // + // Der Fake wird hier ausdrücklich auf einen Fehlercode gesetzt — genau + // das fehlte bisher, und deshalb belegten die Prüfungen nur die erzeugte + // Befehlsmenge, nie die Antwort des Gasts. + [$pve, $instance] = gastBereit(); + $pve->guestScript('group:removeuser', 1); + $sitz = Seat::factory()->create(['role' => 'member', 'nc_username' => 'anna@firma.tld']); + + expect(app(NextcloudUsers::class)->applyRole($instance, $sitz))->toBeTrue(); + + // Und der Grund, warum es trotzdem gelingt, steht im Befehl selbst. + $befehle = implode("\n", $pve->guestCommands); + + expect($befehle)->toContain("group:removeuser 'admin' 'anna@firma.tld' 2>/dev/null || true"); +}); + +it('meldet ein gescheitertes Zuordnen weiterhin als Fehlschlag', function () { + // Die andere Hälfte derselben Grenze: das Auffangnetz darf nicht zum + // Wegsehen werden. Wer in KEINER Gruppe landet, sieht in seiner neuen + // Cloud nichts — das muss an der Zeile stehen. + [$pve, $instance] = gastBereit(); + $pve->guestScript('group:adduser', 1); + $sitz = Seat::factory()->create(['role' => 'member', 'nc_username' => 'anna@firma.tld']); + + expect(app(NextcloudUsers::class)->applyRole($instance, $sitz))->toBeFalse(); +}); + +it('lässt eine Instanz ohne Host unangetastet', function () { // run() steigt vor dem Verbindungsaufbau aus, wenn es keinen Host gibt — // es darf dabei kein einziger Befehl abgesetzt werden, denn es gibt - // nichts, worueber forHost() eine Verbindung aufbauen koennte. + // nichts, worüber forHost() eine Verbindung aufbauen könnte. $pve = new FakeProxmoxClient; app()->instance(ProxmoxClient::class, $pve); $instance = Instance::factory()->create(['status' => 'active', 'vmid' => 201, 'host_id' => null]); @@ -113,9 +155,9 @@ it('laesst eine Instanz ohne Host unangetastet', function () { }); it('wirft Sitzungen beim Sperren SOFORT hinaus', function () { - // user:disable allein laesst laufende Sitzungen bis zu fuenf Minuten - // weiterleben. Bei einem Mitarbeiter, der gerade gegangen ist, sind fuenf - // Minuten fuenf zu viel. + // user:disable allein lässt laufende Sitzungen bis zu fünf Minuten + // weiterleben. Bei einem Mitarbeiter, der gerade gegangen ist, sind fünf + // Minuten fünf zu viel. [$pve, $instance] = gastBereit(); $sitz = Seat::factory()->create(['nc_username' => 'anna@firma.tld']); @@ -127,7 +169,7 @@ it('wirft Sitzungen beim Sperren SOFORT hinaus', function () { ->and($befehle)->toContain('user:auth-tokens:delete'); }); -it('gibt false zurueck statt zu werfen, wenn der Gast nicht antwortet', function () { +it('gibt false zurück statt zu werfen, wenn der Gast nicht antwortet', function () { [$pve, $instance] = gastBereit(); $pve->guestThrows[201] = new RuntimeException('guest agent unreachable'); $sitz = Seat::factory()->create(['nc_username' => 'anna@firma.tld']); @@ -135,9 +177,9 @@ it('gibt false zurueck statt zu werfen, wenn der Gast nicht antwortet', function expect(app(NextcloudUsers::class)->disable($instance, $sitz))->toBeFalse(); }); -it('fuehrt gar nichts aus, wenn der Benutzername keiner ist', function () { +it('führt gar nichts aus, wenn der Benutzername keiner ist', function () { // Der Name wandert in eine Wurzel-Shell im Gast. Dieselbe Regel wie bei - // HostFirewall: ein Dienst, der eine Shell fuettert, darf sich nicht + // HostFirewall: ein Dienst, der eine Shell füttert, darf sich nicht // darauf verlassen, dass sein Aufrufer sauber war. [$pve, $instance] = gastBereit(); $sitz = Seat::factory()->create(['nc_username' => 'anna; rm -rf /']);