From e77969b8c5ec48044cc461c8a58a7ab05703f889 Mon Sep 17 00:00:00 2001 From: Lars Nolden Date: Fri, 11 Sep 2026 11:46:59 +0200 Subject: [PATCH] Report failed bank connections and stop resurrecting deleted accounts Three defects made new connections silently vanish while removed accounts returned: - A single shared account the journal cannot represent (securities or card entries without IBAN, stable identification or currency) aborted the entire consent. Usable accounts are now linked and the rest counted and reported. - A consent that linked nothing was stored, redirected as success and later reaped by session recovery. It now fails with the reason. - Callback failures rendered a bare JSON error page and were never logged. They now log and redirect into the app with the reason shown. - Deleting an account left its session binding, so the next connect or sync recovered the binding and re-added the account. Account deletion now releases bindings, consents and cursors before committing. --- internal/app/banking_settings_test.go | 12 +-- internal/app/consent_test.go | 101 ++++++++++++++++++++++++- internal/app/import.go | 39 ++++++---- internal/app/manage.go | 44 +++++++++++ internal/app/sync_test.go | 4 +- internal/banking/enablebanking.go | 10 ++- internal/banking/enablebanking_test.go | 21 +++++ internal/server/server.go | 17 ++++- web/src/main.tsx | 28 +++++-- 9 files changed, 242 insertions(+), 34 deletions(-) diff --git a/internal/app/banking_settings_test.go b/internal/app/banking_settings_test.go index 0c8db5e..3f5c8e7 100644 --- a/internal/app/banking_settings_test.go +++ b/internal/app/banking_settings_test.go @@ -116,7 +116,7 @@ func TestBankingRuntimeRotationPreservesConsentAndRejectsPendingCallback(t *test t.Fatal(err) } pending := bankingAuthorization(t, a, key, "app-one", bankingCallback) - if err := a.Callback(ctx, "code", pending); err != nil { + if _, err := a.Callback(ctx, "code", pending); err != nil { t.Fatal(err) } before, err := a.Snapshot(ctx) @@ -138,7 +138,7 @@ func TestBankingRuntimeRotationPreservesConsentAndRejectsPendingCallback(t *test if !reflect.DeepEqual(before.Sessions, after.Sessions) || !reflect.DeepEqual(before.Data, after.Data) || a.ops.AccountSync[accountID] == "" { t.Fatal("same-application rotation discarded consent, cursor or canonical data") } - if err := a.Callback(ctx, "code", pending); err == nil { + if _, err := a.Callback(ctx, "code", pending); err == nil { t.Fatal("rotation accepted a stale pending callback") } bankingAuthorization(t, a, rotated, "app-one", callback) @@ -168,7 +168,7 @@ func TestBankingAppSwitchAndDisableNeverReuseOldSessions(t *testing.T) { t.Fatal(err) } pending := bankingAuthorization(t, a, key, "app-one", bankingCallback) - if err := a.Callback(ctx, "code", pending); err != nil { + if _, err := a.Callback(ctx, "code", pending); err != nil { t.Fatal(err) } before, _ := a.Snapshot(ctx) @@ -195,7 +195,7 @@ func TestBankingAppSwitchAndDisableNeverReuseOldSessions(t *testing.T) { if !remove { bankingAuthorization(t, a, key, "app-two", bankingCallback) } - if err := a.Callback(ctx, "code", pending); err == nil { + if _, err := a.Callback(ctx, "code", pending); err == nil { t.Fatal("old pending authorization accepted after app change") } if _, err := a.Balances(ctx, accountID); err == nil { @@ -286,7 +286,7 @@ func TestBankingRejectedSettingsAndFailedWritePreserveActiveProvider(t *testing. if _, err := a.RemoveBankingSettings(ctx); err == nil { t.Fatal("failed removal reported success") } - if err := a.Callback(ctx, "code", pending); err != nil { + if _, err := a.Callback(ctx, "code", pending); err != nil { t.Fatal("failed credential write invalidated active authorization", err) } bankingAuthorization(t, a, key, "active-app", bankingCallback) @@ -367,7 +367,7 @@ func TestBankingEnvironmentAppChangeInvalidatesBoundSessions(t *testing.T) { a = reopenBankingApp(t, a) ctx := context.Background() pending := bankingAuthorization(t, a, key, "env-one", bankingCallback) - if err := a.Callback(ctx, "code", pending); err != nil { + if _, err := a.Callback(ctx, "code", pending); err != nil { t.Fatal(err) } before, _ := a.Snapshot(ctx) diff --git a/internal/app/consent_test.go b/internal/app/consent_test.go index 4ab790a..94fd8af 100644 --- a/internal/app/consent_test.go +++ b/internal/app/consent_test.go @@ -2,6 +2,8 @@ package app import ( "context" + "slices" + "strings" "testing" "time" @@ -95,7 +97,7 @@ func TestAuthorizedHistorySurvivesReopenAndRespectsIncrementalCursor(t *testing. if _, err := a.Authorize(ctx, "N26", "DE", 24); err != nil { t.Fatal(err) } - if err := a.Callback(ctx, "one_time_code", b.authState); err != nil { + if _, err := a.Callback(ctx, "one_time_code", b.authState); err != nil { t.Fatal(err) } a = reopenBankingApp(t, a) @@ -170,7 +172,7 @@ func TestRenewedConsentWakesSchedulerAndAutomaticallyImports(t *testing.T) { done := make(chan struct{}) go func() { defer close(done); a.RunScheduler(ctx) }() defer func() { cancel(); <-done }() - if err := a.Callback(context.Background(), "one_time_code", "state"); err != nil { + if _, err := a.Callback(context.Background(), "one_time_code", "state"); err != nil { t.Fatal(err) } select { @@ -216,3 +218,98 @@ func TestInterruptedRenewalDiscardsSupersededConsentDuringRecovery(t *testing.T) t.Fatal("recovered replacement did not resume imports") } } + +// A removed bank account must stay removed. Session recovery reconstructs +// bindings for interrupted connects, so a stale binding silently resurrects +// the account on the next connect or sync. +func TestDeletedBankAccountIsNotResurrectedByLaterConnectOrSync(t *testing.T) { + a, s := testApp(t) + ctx := context.Background() + kept := s.Data.Accounts[0] + kept.ExternalAccountID = "kept_uid" + removed := domain.Account{ID: "removed_acct", DisplayName: "Mwst", Institution: "N26", Currency: "EUR", ExternalAccountID: "removed_uid", Active: true} + s, err := a.Mutate(ctx, s.Revision, func(d *domain.Dataset) error { + d.Accounts = []domain.Account{kept, removed} + return nil + }) + if err != nil { + t.Fatal(err) + } + session := banking.Session{ID: "n26_session", ValidUntil: time.Now().Add(24 * time.Hour).Format(time.RFC3339), Accounts: []domain.Account{kept, removed}} + b := &historyBank{bankScenario: bankScenario{session: session}} + a.bank = b + a.ops.Sessions = []banking.Session{session} + a.ops.Consents[session.ID] = Consent{Institution: "N26", Country: "DE", HistoryMonths: 12} + a.ops.AccountSync[removed.ID] = time.Now().UTC().Format(time.RFC3339) + if err := a.saveOps(); err != nil { + t.Fatal(err) + } + s, err = a.ManageRegistry(ctx, s.Revision, "account", "delete", removed.ID, "") + if err != nil { + t.Fatal(err) + } + if slices.ContainsFunc(s.Data.Accounts, func(v domain.Account) bool { return v.ID == removed.ID }) { + t.Fatal("account deletion did not remove the account") + } + if _, tracked := a.ops.AccountSync[removed.ID]; tracked { + t.Fatal("deleted account kept its sync cursor") + } + // A later connect for a different bank triggers binding recovery. + other := domain.Account{ID: "ing_acct", DisplayName: "ING Giro", Institution: "ING", Currency: "EUR", ExternalAccountID: "ing_uid", Active: true} + b.session = banking.Session{ID: "ing_session", ValidUntil: time.Now().Add(24 * time.Hour).Format(time.RFC3339), Accounts: []domain.Account{other}} + if _, err := a.Authorize(ctx, "ING", "DE", 12); err != nil { + t.Fatal(err) + } + if _, err := a.Callback(ctx, "one_time_code", b.authState); err != nil { + t.Fatal(err) + } + a = reopenBankingApp(t, a) + a.bank = b + after, err := a.Sync(ctx) + if err != nil { + t.Fatal(err) + } + names := map[string]bool{} + for _, account := range after.Data.Accounts { + names[account.ID] = true + } + if names[removed.ID] { + t.Fatalf("deleted bank account reappeared: %+v", after.Data.Accounts) + } + if !names[kept.ID] || !names[other.ID] { + t.Fatalf("connecting another bank lost existing accounts: %+v", after.Data.Accounts) + } + for _, connection := range after.Connections { + if connection.AccountID == removed.ID { + t.Fatal("deleted account still reported as connected") + } + } +} + +// A consent that shares no accounts cannot sync and its session is reaped by +// recovery, so the user must be told instead of being silently redirected. +func TestConnectWithoutSharedAccountsFailsVisibly(t *testing.T) { + a, _ := testApp(t) + ctx := context.Background() + b := &historyBank{bankScenario: bankScenario{session: banking.Session{ID: "empty_session", ValidUntil: time.Now().Add(24 * time.Hour).Format(time.RFC3339)}}} + a.bank = b + if _, err := a.Authorize(ctx, "Kontist", "DE", 12); err != nil { + t.Fatal(err) + } + _, err := a.Callback(ctx, "one_time_code", b.authState) + if err == nil || !strings.Contains(err.Error(), "no accounts") { + t.Fatalf("empty consent reported success: %v", err) + } + after, e := a.Snapshot(ctx) + if e != nil { + t.Fatal(e) + } + if len(after.Sessions) != 0 { + t.Fatalf("unusable consent was stored: %+v", after.Sessions) + } + for _, connection := range after.Connections { + if connection.Status != "local" { + t.Fatalf("unusable consent produced a bank connection: %+v", connection) + } + } +} diff --git a/internal/app/import.go b/internal/app/import.go index 3fe8e26..a50d13e 100644 --- a/internal/app/import.go +++ b/internal/app/import.go @@ -249,29 +249,40 @@ func connectAccounts(d *domain.Dataset, session *banking.Session, reconnect bool } } } -func (a *App) Callback(ctx context.Context, code, state string) error { + +// Callback completes a bank authorization and returns how many accounts the +// bank shared that could not be linked to a journal account. +func (a *App) Callback(ctx context.Context, code, state string) (int, error) { a.mu.Lock() defer a.mu.Unlock() auth, ok := a.authStates[state] delete(a.authStates, state) if !ok || time.Now().After(auth.Expires) { - return errors.New("authorization state expired or invalid; reconnect again") + return 0, errors.New("authorization state expired or invalid; reconnect again") } if a.bank == nil || code == "" { - return errors.New("authorization did not provide a code") + return 0, errors.New("authorization did not provide a code") } session, err := a.bank.Exchange(ctx, code) if err != nil { - return err + return 0, err + } + // A consent without linkable accounts can never sync and its stored + // session would be reaped silently. Fail visibly instead. + if len(session.Accounts) == 0 { + if session.Unlinkable > 0 { + return 0, fmt.Errorf("the bank shared %d account(s), but none could be linked: they lack an IBAN or stable identification, or use an unsupported currency", session.Unlinkable) + } + return 0, errors.New("the bank authorized the connection but shared no accounts, so nothing was linked; accounts of another type (for example business) may need a separate consent") } a.ops.Sessions = append(a.ops.Sessions, session) a.ops.Consents[session.ID] = Consent{Institution: auth.Institution, Country: auth.Country, HistoryMonths: auth.HistoryMonths} if err = a.saveOps(); err != nil { - return err + return 0, err } s, err := a.snapshot(ctx) if err != nil { - return err + return 0, err } connectAccounts(&s.Data, &session, true) // Remove superseded account bindings, not unrelated bank consents. @@ -295,16 +306,16 @@ func (a *App) Callback(ctx context.Context, code, state string) error { // Save once-only provider details before the canonical commit. Sync can recover // the account bindings if a crash or external edit interrupts that commit. if err = a.saveOps(); err != nil { - return err + return 0, err } - _, err = a.commit(ctx, s.Revision, s.Data) - if err == nil { - select { - case a.syncRequested <- struct{}{}: - default: - } + if _, err = a.commit(ctx, s.Revision, s.Data); err != nil { + return 0, err } - return err + select { + case a.syncRequested <- struct{}{}: + default: + } + return session.Unlinkable, nil } func (a *App) Balances(ctx context.Context, id string) ([]banking.Balance, error) { a.mu.Lock() diff --git a/internal/app/manage.go b/internal/app/manage.go index 9de270e..39256e6 100644 --- a/internal/app/manage.go +++ b/internal/app/manage.go @@ -1,14 +1,58 @@ package app import ( + "context" "errors" "fmt" "slices" "strings" + "finance-duck/internal/banking" "finance-duck/internal/domain" ) +// ManageRegistry applies registry management and, for account deletions, also +// releases the account's bank bindings first: a deleted account must never be +// resurrected by the session recovery that reconstructs interrupted connects. +func (a *App) ManageRegistry(ctx context.Context, rev, entity, action, id, target string) (State, error) { + a.mu.Lock() + defer a.mu.Unlock() + s, err := a.snapshot(ctx) + if err != nil { + return State{}, err + } + if rev != s.Revision { + return State{}, errors.New("revision conflict: reload before editing") + } + if err = Manage(&s.Data, entity, action, id, target); err != nil { + return State{}, err + } + // Prune bindings before the canonical commit: an interruption then leaves + // an unbound local account rather than a resurrected bank connection. + if entity == "account" { + changed := false + sessions := make([]banking.Session, 0, len(a.ops.Sessions)) + for _, session := range a.ops.Sessions { + accounts := slices.DeleteFunc(slices.Clone(session.Accounts), func(account domain.Account) bool { return account.ID == id }) + changed = changed || len(accounts) != len(session.Accounts) + session.Accounts = accounts + if len(accounts) == 0 { + delete(a.ops.Consents, session.ID) + continue + } + sessions = append(sessions, session) + } + if _, tracked := a.ops.AccountSync[id]; tracked || changed { + a.ops.Sessions = sessions + delete(a.ops.AccountSync, id) + if err = a.saveOps(); err != nil { + return State{}, err + } + } + } + return a.commit(ctx, rev, s.Data) +} + func SaveAccount(d *domain.Dataset, v domain.Account) error { v.DisplayName = strings.TrimSpace(v.DisplayName) if v.ID == "" { diff --git a/internal/app/sync_test.go b/internal/app/sync_test.go index 58e2af0..3ba1244 100644 --- a/internal/app/sync_test.go +++ b/internal/app/sync_test.go @@ -102,7 +102,7 @@ func TestReconnectReplacesOldConsentWithoutDuplicatingLocalAccount(t *testing.T) renewed.DisplayName = "Bank-generated name" a.bank = &bankScenario{session: banking.Session{ID: "new_session", ValidUntil: time.Now().Add(24 * time.Hour).Format(time.RFC3339), Accounts: []domain.Account{renewed}}} a.authStates["one_time_state"] = authorization{Expires: time.Now().Add(time.Minute), Institution: "N26", Country: "DE", HistoryMonths: 24} - if err = a.Callback(context.Background(), "bank_code", "one_time_state"); err != nil { + if _, err = a.Callback(context.Background(), "bank_code", "one_time_state"); err != nil { t.Fatal(err) } after, err := a.Snapshot(context.Background()) @@ -118,7 +118,7 @@ func TestReconnectReplacesOldConsentWithoutDuplicatingLocalAccount(t *testing.T) if a.ops.AccountSync[account.ID] != cursor || after.Connections[0].HistoryMonths != 24 { t.Fatal("reconnect reset the account cursor or lost the history choice") } - if err = a.Callback(context.Background(), "bank_code", "one_time_state"); err == nil { + if _, err = a.Callback(context.Background(), "bank_code", "one_time_state"); err == nil { t.Fatal("authorization state replay was accepted") } } diff --git a/internal/banking/enablebanking.go b/internal/banking/enablebanking.go index 086bd78..04980c8 100644 --- a/internal/banking/enablebanking.go +++ b/internal/banking/enablebanking.go @@ -37,6 +37,10 @@ type Session struct { ID string `json:"session_id"` ValidUntil string `json:"valid_until"` Accounts []domain.Account `json:"accounts"` + // Unlinkable counts accounts the bank shared that cannot be represented + // as a journal account (no IBAN or stable identification, or an + // unsupported currency). They are excluded, never silently merged. + Unlinkable int `json:"unlinkable,omitempty"` } // SessionStatus contains only the current consent expiry and external account @@ -585,10 +589,14 @@ func (p *EnableBanking) Exchange(ctx context.Context, code string) (Session, err return Session{}, fmt.Errorf("Enable Banking returned invalid session expiry") } result := Session{ID: response.ID, ValidUntil: response.Access.ValidUntil, Accounts: []domain.Account{}} + // One account the journal cannot represent (securities or card entries + // commonly lack an IBAN, stable identification or a currency) must not + // discard the whole consent: link the usable accounts and count the rest. for _, a := range response.Accounts { account, err := a.account(response.ASPSP.Name) if err != nil { - return Session{}, err + result.Unlinkable++ + continue } result.Accounts = append(result.Accounts, account) } diff --git a/internal/banking/enablebanking_test.go b/internal/banking/enablebanking_test.go index f89c511..a5a2299 100644 --- a/internal/banking/enablebanking_test.go +++ b/internal/banking/enablebanking_test.go @@ -283,6 +283,27 @@ func TestEnableBankingInstitutionsListsOnlyConnectableBanksWithSafeLogos(t *test t.Fatal("accepted an invalid country code") } } +func TestEnableBankingLinksUsableAccountsAndCountsTheRest(t *testing.T) { + p, _ := testProvider(t, func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `{"session_id":"session-1","access":{"valid_until":"2099-01-01T00:00:00Z"},"aspsp":{"name":"ING","country":"DE"},"accounts":[ + {"uid":"giro","identification_hash":"hash-giro","account_id":{"iban":"DE02120300000000202051"},"details":"Girokonto","currency":"EUR"}, + {"uid":"depot","identification_hash":"hash-depot","details":"Direkt-Depot"}, + {"uid":"card","account_id":{},"details":"Credit card","currency":"EUR"}, + {"uid":"extra","identification_hash":"hash-extra","details":"Extra-Konto","currency":"EUR"} + ]}`) + }) + session, err := p.Exchange(context.Background(), "code") + if err != nil { + t.Fatalf("one unusable shared account discarded the whole consent: %v", err) + } + var names []string + for _, account := range session.Accounts { + names = append(names, account.DisplayName) + } + if !reflect.DeepEqual(names, []string{"Girokonto", "Extra-Konto"}) || session.Unlinkable != 2 { + t.Fatalf("wrong linked accounts or unlinkable count: %v %d", names, session.Unlinkable) + } +} func TestEnableBankingSurfacesOnlyDocumentedErrorCodes(t *testing.T) { body := "" p, _ := testProvider(t, func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/server/server.go b/internal/server/server.go index 70bce6a..9e9515a 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -6,10 +6,12 @@ import ( "errors" "io" "io/fs" + "log" "mime" "net" "net/http" "net/url" + "strconv" "strings" "time" @@ -304,7 +306,7 @@ func (s *Server) manage(w http.ResponseWriter, r *http.Request) { if !decode(w, r, &b) { return } - v, e := s.app.Mutate(r.Context(), b.Revision, func(d *domain.Dataset) error { return app.Manage(d, b.Entity, b.Action, b.ID, b.TargetID) }) + v, e := s.app.ManageRegistry(r.Context(), b.Revision, b.Entity, b.Action, b.ID, b.TargetID) respond(w, v, e) } func (s *Server) importCSV(w http.ResponseWriter, r *http.Request) { @@ -410,12 +412,19 @@ func (s *Server) authorize(w http.ResponseWriter, r *http.Request) { respond(w, map[string]string{"url": v}, e) } func (s *Server) callback(w http.ResponseWriter, r *http.Request) { - e := s.app.Callback(r.Context(), r.URL.Query().Get("code"), r.URL.Query().Get("state")) + unlinkable, e := s.app.Callback(r.Context(), r.URL.Query().Get("code"), r.URL.Query().Get("state")) if e != nil { - respond(w, nil, e) + // Callback errors are locally generated and sanitized. Landing on the + // app with the reason visible beats a bare JSON error page. + log.Printf("bank connection callback failed: %v", e) + http.Redirect(w, r, "/?connect_error="+url.QueryEscape(e.Error()), http.StatusSeeOther) return } - http.Redirect(w, r, "/?connected=1", http.StatusSeeOther) + target := "/?connected=1" + if unlinkable > 0 { + target += "&unlinkable=" + strconv.Itoa(unlinkable) + } + http.Redirect(w, r, target, http.StatusSeeOther) } func (s *Server) preview(w http.ResponseWriter, r *http.Request) { var b app.PreviewRequest diff --git a/web/src/main.tsx b/web/src/main.tsx index c0d677f..b09669e 100644 --- a/web/src/main.tsx +++ b/web/src/main.tsx @@ -79,14 +79,20 @@ function App() { const timeout = window.setTimeout(() => setNotice(""), 7000); return () => window.clearTimeout(timeout); }, [notice]); + const [connectError, setConnectError] = useState(""); useEffect(() => { - const connected = new URLSearchParams(window.location.search).get( - "connected", - ); - if (connected === "1") { + const params = new URLSearchParams(window.location.search); + if (params.get("connected") === "1") { + const unlinkable = Number(params.get("unlinkable") ?? "0"); setNotice( - "Bank authorization completed. Your connection is available in Accounts.", + unlinkable > 0 + ? `Bank authorization completed. ${unlinkable} shared account${unlinkable === 1 ? "" : "s"} could not be linked (no IBAN or stable identification, or an unsupported currency); the rest are available in Accounts.` + : "Bank authorization completed. Your connection is available in Accounts.", ); + } + const failed = params.get("connect_error"); + if (failed) setConnectError(failed); + if (params.get("connected") || failed) { window.history.replaceState( null, "", @@ -275,6 +281,18 @@ function App() { )} + {connectError && ( +
+ + Bank connection failed: {connectError} + +
+ )} {state && (state.status.sync_error || state.status.index_error) && page !== "settings" && (