fix: report the revokes the consent revocation sweep could not perform, and document both cases - #76
Merged
Conversation
revokeConsentAccountAccess counted its revoke attempts, not its successes:
.map { access => revokeAccessToViewForUserAndConsumer(...) }.size
so "dropped N account access rows" named rows that were still in the table.
revokeAccessToViewForUserAndConsumer applies the same canRevokeOwnerAccess rule
as revokeAccess, and will not drop the last `owner` row on an account, so the
refusal is reachable here too.
This is the worse place for it to happen. When grantAccessToViews cannot revoke
a stale view, the consent is live and revoking it would still clean up. Here the
consent is already revoked and the access it created outlives it, with nothing
left in the system that will ever come back for that row -- while the log
asserted it had been removed.
Count only the successes, and warn once per row that stayed, naming the account,
the view and the consent so it can be cleared by hand.
Introduced in 01826ce.
Both warnings mean access is still held that a consent no longer covers, and neither self-heals. The runbook says what the line means, why the request is still served, how to tell the two cases apart, and what to do. The diagnosis leans on one point that is easy to miss: for a shadow user, a refusal means no other principal holds `owner` on that account -- not even the PSU. That is a finding about the account, not just the consent, and restoring the PSU's own access lets the next use of the consent revoke the stale row by itself, which is the only resolution that does not involve editing rows by hand.
|
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.



Follow-up to #75. Writing the runbook for #75's warning turned up its sibling one function away, so this carries the fix and the runbook together.
The fix
revokeConsentAccountAccess(ConsentUtil.scala:920) counted attempts, not successes:.map { access => revokeAccessToViewForUserAndConsumer(access, shadowUser, ALL_CONSUMERS) }.sizeso
dropped N account access rowsnamed rows that were still in the table.revokeAccessToViewForUserAndConsumercalls the samecanRevokeOwnerAccessAsBoxasrevokeAccess, so it refuses in exactly the cases #75 describes.This is the worse of the two. When
grantAccessToViewscannot revoke a stale view, the consent is live and revoking it would still clean up. Here the consent is already revoked and the access it created outlives it — nothing left in the system will ever come back for that row — while the log asserted it had been removed. The one signal an operator had was actively wrong.Now: count only successes, and warn once per row that stayed.
Introduced in
01826cede— my own, from the UK consent principal work.The runbook
docs/operations/CONSENT_STALE_ACCESS.md, covering both warnings: what they mean, why the request is still served, how to tell the two cases apart, diagnosis SQL, resolution, verification, alerting.The point worth surfacing from it — for a shadow user, a refusal can only mean no other principal holds
owneron that account, not even the PSU. That is a finding about the account, not just the consent. It also gives the best resolution: restore the PSU's own access and the count exceeds 1, at which point the next use of the consent revokes the stale row by itself. That is the only fix that does not involve deleting rows by hand.Verification
Built and ran locally;
javapconfirms the new string is incode/api/util/Consent$.class.Extended the #75 reproduction to drive the sweep as well — same injected row, then revoke the consent through the real UK endpoint:
The fifth and sixth lines are the pair that matters: one row dropped, one refused. The old code would have logged
dropped 2.Full local suite across all five flows: 105/105, unchanged.
Every line number cited in the runbook was checked against
develop-obpafter the code edit rather than carried over from drafting.