Skip to content

fix: log when stale consent access cannot be revoked - #75

Merged
hongwei1 merged 1 commit into
develop-obpfrom
fix/log-stale-access-revoke-failures
Aug 9, 2026
Merged

fix: log when stale consent access cannot be revoked#75
hongwei1 merged 1 commit into
develop-obpfrom
fix/log-stale-access-revoke-failures

Conversation

@hongwei1

@hongwei1 hongwei1 commented Aug 9, 2026

Copy link
Copy Markdown
Owner

What

grantAccessToViews reconciles a consent's shadow user against the views the consent declares: grant what is missing, revoke what is no longer declared. The two halves treated failure very differently.

The grant half collects its results and turns a failure into Failure(CouldNotAssignAccountAccess…), so the caller sees it. The revoke half discarded its results:

for {
  staleAccess <- held.filterNot(wantedSet.contains)
} yield {
  Views.views.vend.revokeAccess(staleAccess, user)   // Box[Boolean], thrown away
}

A revoke that failed left the shadow user holding a view the consent had given up, and nothing anywhere recorded it. The only symptom would be data the consent no longer covers still being served.

How it is reached

revokeAccess is not unconditional. canRevokeOwnerAccess (MapperViews.scala:405) refuses to drop an owner row when no other principal holds one on that account. An OBP-native consent can carry owner, because createConsentJWT builds its view list from whatever the PSU already holds. Narrowing such a consent — dropping owner, keeping the rest — is then a silent no-op for that one view.

Why log rather than fail

Returning a Failure would make the whole request fail, which means a consent stuck in this state becomes completely unusable — including every view it still legitimately holds — and the operator has no way back short of editing rows by hand.

The two failure directions are not symmetric. A failed grant denies access the consent asked for, and the caller finds out at once. A failed revoke keeps access the consent gave up, which is narrower and, once logged, actionable. So the request is served and the discrepancy is recorded at warn, naming the consent, the user, the account and the view:

grantAccessToViews: could not revoke owner on gh.29.uk.x1/726b08a5-…, from user 90a84c9e-… ,
which consent <jti> no longer declares. The access is still held: Failure(…)

Scope

One file, one loop. case Full(true) => // gone keeps the successful path silent; every other Box shape logs. No control flow, no signature, no caller changes.

Verification

  • mvn -o -pl obp-api -am compile clean
  • Full local regression suite against a jar built from this branch: 105/105 (see comment)

Found by review of #73, which touched the same function.

grantAccessToViews reconciles a consent's shadow user against the views the
consent declares: it grants what is missing and revokes what is no longer
declared. The grant half collects its results and fails the request when one
of them fails. The revoke half discarded its results entirely, so a revoke
that failed left the shadow user holding a view the consent had given up,
with no trace anywhere.

canRevokeOwnerAccess is the realistic way to reach that: it refuses to drop
an `owner` row when no other principal holds one on the account, and an
OBP-native consent can carry `owner` because createConsentJWT takes its views
from whatever the PSU already holds. Narrowing such a consent then silently
keeps the wider access.

Log rather than fail. Returning a Failure here would make a consent stuck in
this state unusable altogether, including the views it still legitimately
holds, and leave the operator no way back short of editing rows by hand. The
access it should have lost is a smaller problem than the access it should
have kept, so the request is served and the discrepancy is recorded at warn
with the consent, user, account and view named.
@sonarqubecloud

sonarqubecloud Bot commented Aug 9, 2026

Copy link
Copy Markdown

@hongwei1

hongwei1 commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Verification

Built a jar from 5e1cc15f4 and ran it locally (javap on code/api/util/Consent$.class confirms the new string is actually compiled in — strings on the jar gives a false negative, and the class is Consent, not ConsentUtil, since the Scala file name does not name the object).

No regression — full local suite, 5 flows: 105/105.

The branch itself. No-regression says nothing about whether the new path works, so the failure was constructed rather than waited for. canRevokeOwnerAccess refuses to drop an owner row when it is the only owner row on the account, so the setup gives the shadow user owner on an account with no other owner holder and which the consent never names. On the next use that row is stale, the revoke is attempted, and it is refused.

[PASS] the consent works before anything is injected
[PASS] the consent created a shadow user            resourceuser.id=770
[PASS] the scratch account has no account holder to short-circuit the check
[PASS] the scratch account starts with no owner access at all
[PASS] the shadow user now holds owner on an account the consent never named
[PASS] and is the only owner holder, so the revoke must be refused
[PASS] the consent is still served, not failed, despite the stuck view    got 200
[PASS] the stale access really was not revoked
[PASS] the server logged the failed revoke
[PASS] the log names the consent / shadow user / account / view
[PASS] the injected row is cleaned up
14 checks, 0 failed

The log line:

WARN code.api.util.Consent$ -- grantAccessToViews: could not revoke owner on gh.29.uk/1
from user e326edaf-…, which consent 84d5a583-… no longer declares.
The access is still held: Failure(access cannot be revoked)

The response body is the part worth reading. That 200 came back with:

{"accounts":[{"id":"1","bank_id":"gh.29.uk", }]}

The un-revoked access was actively serving an account the consent never named. That is the concrete harm, and until this change it happened without a single line anywhere. It still happens — deliberately, because failing the request would take the consent's legitimate views down with it — but it is now attributable to a specific consent, user, account and view.

The test cleans up in a finally, so the injected row goes even if an assertion above it fails.

@hongwei1
hongwei1 merged commit 15c7094 into develop-obp Aug 9, 2026
25 checks passed
@hongwei1
hongwei1 deleted the fix/log-stale-access-revoke-failures branch August 9, 2026 17:54
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.

1 participant