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" && (