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 /']);