Skip to content

feat(banking): persist bank connections and manage consent lifecycle - #542

Open
marianialessandro wants to merge 4 commits into
RIP-Comm:mainfrom
marianialessandro:feature/enable-banking-consent-lifecycle
Open

marianialessandro wants to merge 4 commits into
RIP-Comm:mainfrom
marianialessandro:feature/enable-banking-consent-lifecycle

Conversation

@marianialessandro

Copy link
Copy Markdown
Contributor

Summary

Adds persistence and consent lifecycle management on top of the banking foundation introduced in #536.

  • Persist bank connections, provider identity, and pending authorization state, with migrations and compatibility for existing stored data.
  • Handle authorization callbacks, interrupted-flow recovery, reconnect, and revocation through provider-neutral banking contracts.
  • Integrate credential settings and lifecycle coordination in the application layer, keeping Enable Banking-specific behavior in its adapter.

This is the second PR in the banking integration series for #481. User-facing setup and connection flows will be introduced in the later UI PR.

Commit structure

  1. Validate consent redirects and map connection failures.
  2. Persist bank connections and recovery state.
  3. Integrate the provider-neutral consent lifecycle.

@mikev-cw

Copy link
Copy Markdown
Collaborator

Hey! Thank you so much for this!!!
I thing is very good overall, just some questions/comments/consideration:

1: In BankConsentLifecycleService._rejectStagedConnection, the REAUTH_REQUIRED branch only updates the status and returns.
But BankConnectionRepository.selectAwaitingImport selects rows only by pendingRemoteConnectionId IS NOT NULL.
That means if a staged import fails into REAUTH_REQUIRED, its pending fields remain populated, so it can be selected again on every resume.

Should the REAUTH_REQUIRED path also clear pendingRemoteConnectionId, pendingAuthorizationId, and pendingValidUntil, or should selectAwaitingImport filter by an awaiting/importable status?

2: BankingBackupPolicy.sanitizeForExport converts every exported bankConnection status to REAUTH_REQUIRED.
That seems safe for active sessions because usable session ids are stripped. But for terminal states like REVOKED or DISCONNECTED, this may make intentionally removed connections appear reconnectable after restore.

Should terminal statuses be preserved while only active/pending session states are downgraded to REAUTH_REQUIRED?

3: Consider folding 0009_add_bank_provider_identity into 0008_add_bank_consent_lifecycle. Migration 8 creates non-provider-scoped unique indexes, and migration 9 immediately replaces them with provider-scoped indexes. A single final migration would be simpler and avoid carrying an intermediate schema state that no user should ever need.

4: Can you clarify whether iban is intended to remain on a local account after unlink/disconnect in bank_connection_repository? It is populated from the remote banking account, but _unlinkAccount clears the other provider-derived fields and leaves iban intact. If iban is provider-owned metadata, we should clear it too; if it is local account metadata, keeping it makes sense.

Again, thank you for your time!

@marianialessandro

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the detailed review @mikev-cw !

I addressed all four points in the latest commit:

  1. REAUTH_REQUIRED now clears the pending authorization/session state, so failed staged imports won't be picked up again on resume.
  2. REVOKED and DISCONNECTED are now preserved during backup/restore, while non-terminal states are still downgraded to REAUTH_REQUIRED.
  3. I folded 0009_add_bank_provider_identity into 0008_add_bank_consent_lifecycle, so the schema is created directly with the provider-scoped indexes.
  4. I treated the iban as provider-derived metadata and now clear it when an account is unlinked/disconnected.

I also added/updated tests covering these cases.

Thanks again for catching these!

@theperu theperu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marianialessandro LGTM! ✅

Let's wait for Mike's re-review and then if it's ok for him as well we can merge! 👍🏻

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants