Conversation
Removing a connection dropped its rows but left the credential it had minted in the provider, unreferenced and invisible. Removal now deletes the items the connection minted, chosen by the row's credential_write marker, after the removal has committed. Referenced items, items another connection still points at, and non-writable providers are left alone; failures never fail the removal. The integrations.remove half of the original fix follows separately.
…n is removed Integration removal cascades every subject's connections under the slug but left all the credentials they had minted in the provider. The doomed rows are now read through the same tenant-reach handle and predicate that deletes them, and their minted items are deleted after the outermost commit with the same best-effort rules as connection removal.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removing a connection dropped its row and left the credential it had minted in the provider. Nothing pointed at that item any more and nothing in the product could show it, so someone who disconnected an account still had that account's tokens held on their behalf with no way left to see them or ask for them to go. Removing an integration is the same leak in bulk. The cascade drops every member's connections under the slug, and every secret those connections minted stayed behind. The refresh token is the one that hurts, long-lived by design and unreachable from any row that survives.
Both removals now delete what the connection minted. The row's
credential_writemarker says which items those are, the same persisted metadata the rest of the code uses to decide what we own, because a minted id carries a per-attempt uuid and cannot be derived from the row. The integration path reads the doomed connection rows before the cascade destroys them, through the same tenant-reach handle that does the delete and with the same predicate, so it sees exactly the rows that are about to go and never one more. In both paths the deletion runs after the outermost transaction commits. A provider does not roll back with the database, and a restored row pointing at a destroyed secret would be worse than the orphan this removes, and unlike the orphan it could not be repaired.Some things are deliberately left alone. An item a connection only referenced stays, because we never wrote it. A minted item another live connection still points at also stays, so removing one connection cannot break another. The OAuth app's own client secret belongs to the client and outlives the integration it was registered against. A v1-migrated
secret_<hash>item is ours but carries no marker, so we still leave its value behind, and closing that needs a schema change rather than a looser rule here. The alias hold-back reads through the bound handle, so it cannot see an alias held by another member, and the removal will delete that item anyway.Fifteen tests cover both paths: the pasted and OAuth mints, another member's connection under a removed integration, the referenced and aliased items that have to survive, a provider whose delete fails, and a rolled-back removal.
This is part of the credential-handling series described in #1585, which lists every change and the reasoning behind it.