Hotfix 4.4.4 - FinTS bank import review fixes

composer.json

@@ -2,7 +2,7 @@
     "name": "openadministration/stufis",
     "type": "project",
     "description": "Webinterface für das Management und Digitalisierung von Finanzanträgen und deren Buchung für Studierendenschaften nach Deutschem Recht",
-    "version": "4.4.3",
+    "version": "4.4.4",
     "license": "AGPL",
     "require": {
         "php": "^8.4",

docs/feature-changelog.md

@@ -1,3 +1,18 @@
+# v4.4.4
+* Bankimport (FinTS): Das Absenden der Formulare auf den Seiten des Bankzugangs führte zu einer Fehlerseite – betroffen waren das Anlegen eines Zugangs, die Auswahl des TAN-Verfahrens und jede TAN-Eingabe. Die Seiten nehmen ihre eigenen Formulare nun wieder an.
+* Bankimport (FinTS): Die eingegebene Online-Banking-PIN wurde vor dem Login verändert – alle Sonderzeichen und Umlaute wurden stillschweigend entfernt. Wer eine PIN mit Sonderzeichen nutzt, konnte sich dadurch nicht anmelden und riskierte nach drei Versuchen die Sperrung des Bankzugangs. Die PIN wird nun unverändert übernommen. Bei der TAN werden weiterhin nur Leerzeichen entfernt, damit sie sich wie aufgedruckt („123 456“) eingeben lässt.
+* Bankimport (FinTS): In der Bezeichnung eines Bankzugangs und eines Kontos gingen Ziffern und Leerzeichen verloren („Konto 2024“ wurde zu „Konto“). Beide Bezeichnungen werden nun so gespeichert, wie sie eingegeben wurden.
+* Bankimport (FinTS): Beim Abholen der Kontoauszüge konnten bereits importierte Umsätze ein zweites Mal angelegt werden – ohne Hinweis, mit der Erfolgsmeldung „N Einträge importiert“. Ursache war, dass der Anknüpfungspunkt an die bereits vorhandenen Umsätze über die SEPA-Referenz gesucht wurde, die bei vielen Banken leer bleibt. Der Import erkennt die bekannten Umsätze nun zuverlässig und bricht mit einer klaren Meldung ab, wenn er den Anknüpfungspunkt nicht findet, anstatt Buchungen zu verdoppeln.
+* Sicherheit im Bankimport (FinTS): Die Formulare der Bankzugang-Seiten sind nun gegen Anfragen von fremden Seiten abgesichert (CSRF-Schutz). Bisher wurde das mitgesendete Sicherheitsmerkmal auf diesen Seiten nicht geprüft – eine fremde Webseite konnte dadurch im Namen einer angemeldeten kassenverantwortlichen Person Login-Versuche auslösen (drei Fehlversuche sperren den Bankzugang bei der Bank), Zugangsdaten anlegen oder ein beliebiges Konto zur Synchronisation eintragen. Ist ein Formular nicht mehr gültig, wird die Anfrage nun mit einem Hinweis abgelehnt.
+* Bankimport (FinTS): Beim Anlegen eines Kontos für den Import wird die Eingabe nun geprüft, statt sie ungeprüft zu speichern: Die IBAN muss gültig sein und zu einem Konto des gewählten Bankzugangs gehören, das Kürzel muss aus genau zwei Buchstaben bestehen und darf noch nicht vergeben sein, für dieselbe IBAN kann kein zweites Konto angelegt werden, und das Startdatum muss ein gültiges Datum sein. Zuvor konnte ein Konto mit ungültiger IBAN, doppeltem Kürzel oder fremder IBAN entstehen – mit der Folge, dass Umsätze gar nicht oder im falschen Konto landeten.
+* Bankimport (FinTS): Der Aufruf des Umsatzimports für ein noch nicht eingerichtetes Konto führte zu einer Fehlerseite; nun wird auf das Anlegen des Kontos hingewiesen. Ebenso führt ein Konto ohne gesetztes Startdatum nicht mehr zum Abbruch.
+* Bankimport (FinTS): Wurde ein Umsatzabruf durch die TAN-Abfrage unterbrochen und danach ein anderes Konto geöffnet, konnten die Umsätze des ursprünglich abgefragten Kontos dem falschen Konto zugeordnet werden. Ein unterbrochener Abruf wird nun nur noch für genau das Konto und den Zeitraum fortgesetzt, für den er begonnen wurde.
+* Bankimport (FinTS): Eine von der Bank abgelehnte TAN führte zu einer Fehlerseite, sodass der Vorgang abgebrochen war. Nun erscheint der Hinweis „TAN nicht akzeptiert“ und die Eingabe kann wiederholt werden. Ebenso führen gestörte Antworten der Bank beim Abrufen der TAN-Verfahren, der TAN-Medien und beim Abmelden nicht mehr zu einer Fehlerseite. Wird ein Freigabe-Verfahren ohne TAN-Eingabe genutzt (z. B. pushTAN-Freigabe in der Banking-App), wird nun verständlich gemeldet, dass StuFiS dieses Verfahren noch nicht unterstützt.
+* Bankimport (FinTS): War die Sitzung abgelaufen, endete jeder Klick auf den Seiten des Bankzugangs – etwa „Kontostand aktualisieren“ – in einer Fehlerseite. Stattdessen erscheint nun ein Hinweis und die erneute Anmeldung beim Bankzugang wird angeboten.
+* Bankimport (FinTS): Der Kontostand der Bank wird nun tatsächlich gegen den zuletzt gespeicherten Stand geprüft. Diese Prüfung war zwar vorhanden, lief aber nie – Abweichungen (z. B. durch nachträglich geänderte Umsätze) blieben dadurch unbemerkt. Meldungen zu Saldo-Abweichungen und Lücken zwischen zwei Auszügen sind jetzt außerdem verständlich formuliert und nennen die betroffenen Beträge und Daten.
+
+---
+
 # v4.4.3
 * Projekte mit mehreren Posten ließen sich nicht mehr speichern, wenn vor dem Speichern in jeder Postenzeile etwas geändert wurde – das Speichern brach mit einer Fehlerseite ab. Die Beträge gingen dabei auf dem Weg zum Server verloren; sie werden nun wieder zuverlässig als Geldbeträge erkannt.
 * Fehlermeldungen beim Speichern eines Projekts erscheinen jetzt direkt an dem Feld, das sie ausgelöst hat – und zwar alle. Bisher wurde nur die erste Meldung als einzelne Zeile über dem Formular angezeigt, sodass unklar blieb, welche Zeile oder welches Feld gemeint war.

lang/de/general.php

@@ -21,6 +21,7 @@
             'tan-mode' => 'Tan-Modus',
             'sepa' => 'Konten',
             'import-konto' => 'Neu',
+            'import-transactions' => 'Aktualisieren',
         ],
         'sitzung' => 'Sitzung',
         'budget-plan' => 'Haushaltsplan',

legacy/config/config.routing.php

@@ -463,13 +463,6 @@
                     'controller' => 'rest',
                     'action' => 'chat',
                 ],
-                [
-                    'path' => 'hibiscus',
-                    'type' => 'path',
-                    'controller' => 'rest',
-                    'groups' => 'ref-finanzen-kv',
-                    'action' => 'update-konto',
-                ],
                 [
                     'path' => 'booking',
                     'type' => 'path',

legacy/lib/booking/konto/FintsConnectionHandler.php

@@ -135,9 +135,12 @@ public function logout(): bool
             $this->finTs->close(); // logout @ server
             $this->forgetCachedCredentials($this->credentialId);
             HTMLPageRenderer::addFlash(BT::TYPE_SUCCESS, 'Erfolgreich ausgeloggt');
-        } catch (ServerException $e) {
+        } catch (CurlException|ServerException|UnexpectedResponseException $e) {
+            // A logout that cannot reach the bank is not worth an error page - the local
+            // session data is dropped either way below.
             $this->logger->error('Logout failed', ['exception' => $e]);
             HTMLPageRenderer::addFlash(BT::TYPE_DANGER, 'Logout fehlgeschlagen', $e->getMessage());
+            $this->forgetCachedCredentials($this->credentialId);
 
             return false;
         }
@@ -156,7 +159,7 @@ public function getUserTanModes(): array
         try {
             $this->logger->info('Fetch TAN Modes', ['credId' => $this->credentialId]);
             $tanModes = $this->finTs->getTanModes();
-        } catch (CurlException|ServerException $e) {
+        } catch (CurlException|ServerException|UnexpectedResponseException $e) {
             $this->logger->info('Fetch TAN Modes failed', ['exception' => $e]);
             ErrorHandler::handleException($e, 'TAN Modi können nicht empfangen werden - Verbringung zur Bank gestört');
         }
@@ -185,7 +188,7 @@ public function getTanMedias(int $tanModeId): array
             }
 
             return $tanMediumNames;
-        } catch (CurlException|ServerException $e) {
+        } catch (CurlException|ServerException|UnexpectedResponseException $e) {
             $this->logger->error('Tan kann nicht empfangen werden - Verbindung zur Bank gestört', ['exception' => $e]);
             ErrorHandler::handleException($e, 'TAN Modi können nicht empfangen werden - Verbindung zur Bank gestört');
         }
@@ -284,6 +287,7 @@ private function saveAction(?BaseAction $action = null): void
         } else {
             // delete it from cache otherwise
             $this->setCache('action', null);
+            $this->setCache('action-scope', null);
         }
         // save persist in cache
         $this->setCache('persist', $this->finTs->persist());
@@ -342,11 +346,24 @@ public static function load(int $credentialId): self
 
         $credentials = Credentials::create($username, self::getPassword($credentialId));
 
+        if (trim((string) FINTS_REGNR) === '') {
+            // FinTsOptions::validate() would raise "Product name required!" as an
+            // uncaught InvalidArgumentException, i.e. an error page with no clue.
+            throw new LegacyDieException(
+                500,
+                'Für den Bankzugang fehlt die FinTS-Registrierungsnummer (FINTS_REG_NR in der Konfiguration). '.
+                'Bitte wende dich an die Administration.'
+            );
+        }
+
         $options = new FinTsOptions;
         $options->url = $res['bank.url'];
         $options->bankCode = $res['bank.blz'];
         $options->productName = FINTS_REGNR;
-        $options->productVersion = InstalledVersions::getRootPackage()['version'].DEV ? '-dev' : '';
+        // The concatenation binds tighter than ?:, so this used to evaluate as
+        // (('4.4.3'.DEV) ? '-dev' : '') - an always-truthy string, which reported the
+        // version to the bank as literally "-dev" regardless of what is installed.
+        $options->productVersion = InstalledVersions::getRootPackage()['version'].(DEV ? '-dev' : '');
 
         $tanModeInt = null;
         if ($res['tan_mode'] !== 'null' && ! is_null($res['tan_mode'])) {
@@ -374,7 +391,7 @@ private function execute(BaseAction $action): void
                 // TODO decoupled tan stuff here
                 throw new NeedsTanException($action);
             }
-        } catch (CurlException|ServerException $e) {
+        } catch (CurlException|ServerException|UnexpectedResponseException $e) {
             $this->logger->error('Aktion nicht ausgeführt', ['exception' => $e]);
             ErrorHandler::handleException($e, 'Verbindung zur Bank gestört - Aktion nicht ausgeführt');
         }
@@ -423,10 +440,27 @@ public function submitTan(string $tan): bool
             HTMLPageRenderer::addFlash(BT::TYPE_DANGER, 'Konnte keine Verbindung zum Server aufbauen', $e->getMessage());
 
             return false;
-        } catch (ServerException $e) {
+        } catch (ServerException|UnexpectedResponseException $e) {
+            // A rejected TAN arrives as UnexpectedResponseException("Bank has not accepted
+            // TAN: ...") from FinTs::submitTan(). That extends RuntimeException, while
+            // ServerException extends Exception - two unrelated hierarchies, so catching
+            // only the latter turned a mistyped TAN into an error page.
             $this->logger->error('Wrong Tan', ['exception' => $e]);
             HTMLPageRenderer::addFlash(BT::TYPE_DANGER, 'TAN nicht akzeptiert', $e->getMessage());
 
+            return false;
+        } catch (InvalidArgumentException $e) {
+            // The library refuses to take a TAN for a decoupled TAN mode (confirmation
+            // happens in the banking app instead). Supporting that properly is its own
+            // work package; until then, say so rather than showing an error page.
+            $this->logger->error('TAN submission rejected by the library', ['exception' => $e]);
+            HTMLPageRenderer::addFlash(
+                BT::TYPE_DANGER,
+                'Dieses TAN-Verfahren kann StuFiS derzeit nicht abschließen',
+                'Bei Freigabe-Verfahren ohne TAN-Eingabe (z. B. pushTAN-Freigabe in der Banking-App) '.
+                'fehlt die Unterstützung noch. Bitte wähle ein TAN-Verfahren mit TAN-Eingabe.'
+            );
+
             return false;
         }
 
@@ -447,7 +481,7 @@ public function setTanMode(int $tanModeId, ?string $tanMediumName = null): bool
             }
             $this->saveAction();
             $this->logger->info('Set TAN Mode', ['credId' => $this->credentialId, 'tanMode' => $tanModeId, 'tanMedium' => $tanMediumName]);
-        } catch (CurlException|ServerException  $e) {
+        } catch (CurlException|ServerException|UnexpectedResponseException $e) {
             $this->logger->error('BPB fetch failed', ['exception' => $e]);
             ErrorHandler::handleException($e, 'Kann keine Verbindung zum Bank Server aufbauen', 'BPB fetch failed');
         }
@@ -466,25 +500,45 @@ public function setTanMode(int $tanModeId, ?string $tanMediumName = null): bool
 
     public function getStatements(string $iban, DateTime $start, DateTime $end): StatementOfAccount
     {
+        // What a pending statement request was created for. While it waits for a TAN the
+        // action sits in the session, and it used to be resumed on nothing but its type:
+        // asking for account A, then opening account B's import URL and entering the TAN
+        // there returned A's statements, which the caller then stored under B's konto_id.
+        $scope = $this->statementScope($iban, $start, $end);
         $action = $this->resumableAction();
         if ($action instanceof GetStatementOfAccount) {
-            if ($action->isDone()) {
-                $this->saveAction();
+            if ($this->getCache('action-scope') === $scope) {
+                if ($action->isDone()) {
+                    $this->saveAction();
 
-                return $action->getStatement();
+                    return $action->getStatement();
+                }
+                throw new NeedsTanException($action);
             }
-            throw new NeedsTanException($action);
+            $this->logger->warning('Discarding a pending statement request made for something else', [
+                'credId' => $this->credentialId,
+                'requested' => $scope,
+            ]);
+            $this->saveAction(); // drops the stale action and its scope
         }
         $this->logger->info('Start Get SEPA Statements', ['credId' => $this->credentialId, $iban]);
         $account = $this->getSepaAccount($iban);
         $account = clone $account; // weird fix, without the clone the session var is changed to DateTime object
         // might be a bug in fints TODO: see if minimal example with the same bug can be found
         $action = GetStatementOfAccount::create($account, $start, $end);
+        // Has to be recorded before execute(), which caches the action and then throws
+        // NeedsTanException, ending this request.
+        $this->setCache('action-scope', $scope);
         $this->execute($action);
 
         return $action->getStatement();
     }
 
+    private function statementScope(string $iban, DateTime $start, DateTime $end): string
+    {
+        return $iban.'|'.$start->format('Y-m-d').'|'.$end->format('Y-m-d');
+    }
+
     public function getLogger(): LoggerInterface
     {
         return $this->finTs->getLogger();