fix: bind the UK SCA challenge to its consent, and three ownership fixes - #84
Merged
Merged
Conversation
authoriseUKConsent fetched the started challenge and discarded it, so nothing tied the challenge to the consent being authorised. validateChallengeAnswerC4 is handed the consentId but the connector ignores it -- LocalMappedConnector matches on challengeId, supplied answer and expected user alone -- so the consent id travelled the whole way without ever being compared. A PSU could therefore answer a challenge raised for one of their consents against another. The two need not carry the same Permissions, so an OTP the PSU was given for a narrow consent authorised a wider one: measured on a local instance, a challenge minted on a ReadAccountsBasic consent moved a ReadAccountsBasic+ReadAccountsDetail+ReadBalances consent to AUTHORISED and bound the PSU to it, while the consent that was actually challenged stayed AWAITINGAUTHORISATION. UK Open Banking defines the authentication journey as taking place in the context of one consent -- the ConsentId is the intent identifier, carried into the request object so that the token that follows is bound to a specific consent. OBP mints its own OTP challenge instead of using that mechanism, so the binding the profile gets structurally has to be asserted here. The Berlin Group half of this same change already asserts it, getting the binding from its own URL shape; this brings the UK half into line, with the same guard, message and code. It also refuses a transaction-request challenge here for free, since that carries no consentId at all.
… nobody Two ways the shared consent-ownership rule answered "allowed" when it should not have. Matching the PSU ended the enquiry. psuOrLodgingTppRefusal returned None as soon as the two user ids agreed, without ever comparing the Consumer, so a second TPP holding a session for the same person could read and revoke a consent the first TPP had lodged. One TPP's mandate over a consent is not another's: Berlin Group states it for the whole API -- "all methods submitted by a TPP, which are addressing dynamically created resources in this API, may only apply to resources which have been created by the same TPP before" (Implementation Guidelines 4.11) -- and UK reaches the same place per endpoint, scoping GET and DELETE of an account-access-consent to one "that they have created", on a client-credentials token where no PSU is party to the call at all. A PSU session on those endpoints is an OBP extension on top of the standard, so it cannot be the thing that waives the standard's own check. A PSU match is now necessary rather than sufficient, and falls through to the Consumer comparison. The exception is the ASPSP's declared SCA front end, which under Redirect arrives under its own Consumer by definition -- that is why it has to be declared at all -- so it keeps passing, and only on a consent whose PSU it already agrees with. A consent recording no Consumer was addressable by everyone. The check read `owner.forall(...)`, which is vacuously true when there is no owner, so "absent" was being read as "matches every caller" where it means "there is nobody this belongs to". These rows are real: 10 of 566 Berlin Group, 50 of 463 OBP-native and 4 of 753 UK consents record no consumer on a long-lived instance. In Berlin Group they were already unreachable by accident -- the hand-rolled `consent.mConsumerId.get == ...` compare in the five consent reads is false for everyone when the stored value is null -- but the UK pair let anyone in. They are now refused, with consent_allow_legacy_unrecorded_tpp for an operator who needs a migration window and accepts that it means any authenticated caller can read and revoke them. Default false, documented, and it warns on every use. Three existing assertions invert, and say at the assertion why: they pinned the fail-open rather than the rule. UKOpenBankingV401AccountInfoTests.createRealConsent also stopped passing consumerId = None -- that produced a row no production path creates, since createAccountAccessConsents records the calling consumer, and the fixture only got away with it while such a row was readable by anyone.
Five endpoints that read or delete a consent resource each compared the lodging Consumer by hand -- deleteConsent, getConsentAuthorisation, getConsentInformation, getConsentScaStatus and getConsentStatus -- rather than going through checkBerlinGroupConsentAccess like the authorisation pair in the same file. The cost of five copies showed up immediately: the previous commit tightened the shared rule on two axes, and none of it reached these call sites. A consent bound to one PSU stayed readable by the lodging TPP acting for a different one, because a hand-rolled Consumer compare cannot notice that a PSU half exists at all. That is what the new scenario pins, on all five sites at once. They also gain the declared-SCA-front-end exemption, which the reads had no way to express. Under the Redirect approach the PSU authenticates at the ASPSP, so these calls arrive from the ASPSP's own screen rather than the lodging TPP -- and Implementation Guidelines 6.3.3 says the consent read exists precisely for that case, "returning the data for the TPP especially in cases, where the consent was directly managed between ASPSP and PSU e.g. in a re-direct SCA Approach". A front end that may start an authorisation could not read the status of the one it started. The answer these endpoints give is unchanged: ConsentNotFound at 403 whatever the reason, so a caller cannot tell "not yours" from "no such consent". The rule's own more specific reason is logged rather than returned. Whether that is the right code at all is a separate question, decided once for both standards in a later commit rather than drifting here.
… use safely grantAccessToSystemViewForConsumer and grantAccessToCustomViewForConsumer have had no callers since the consent flows stopped using them, and could not have gained one without reintroducing the bug that removed them. Every consent grant is written under Constant.ALL_CONSUMERS, and that is a literal rather than a wildcard: revokeConsentAccountAccess sweeps a revoked consent's rows by asking accessGrantedToUserForConsumer for exactly ALL_CONSUMERS, which matches the column with By(AccountAccess.consumer_id, ...). A row written under a real consumer id is therefore invisible to the sweep, and the access outlives the consent that granted it. Their own comment recommended the opposite -- "keeping it on its own row is what stops a second TPP's consent from rewriting it" -- while the hazard was already documented on grantConsentAccountAccess a few hundred lines away. Two descriptions of the same column, and the wrong one sat on the API a caller would reach for. The remaining ...ForConsumer methods stay: revokeAccessToViewForUserAndConsumer and accessGrantedToUserForConsumer are what the consent flows use, both against ALL_CONSUMERS. The trait comment now says why the pair is absent, so it reads as a decision rather than a gap for someone to fill in.
MappedConsent.revoke calls revokeConsentAccountAccess after conditionalRevoke has already committed the status change, and it was not guarded. A throw there could not undo the revoke -- it could only hide that it had happened. The caller got a 500, and the retry got ConsentAlreadyRevoked, so an AISP was left unable to complete the DELETE both standards require of it, with no correct next step. Reaching it takes less than it sounds. revokeConsentAccountAccess reads the stored JWT through Box.map, which does not catch, and getSignedPayloadAsJson does not verify the signature -- it parses the structure and hands back the claims -- so a payload that no longer extracts to ConsentJWT arrives as a MappingException. The same trap is already documented on applyUKConsentPrincipalFromToken. Wrapped in tryo, matching the Berlin Group sibling twenty lines below, and logged at error rather than swallowed: the consent IS revoked and the API is right to say so, but rows that should have been dropped are still live, and that is the one thing about a revoked consent nobody comes back for.
|
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
1 participant
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.
Raised in review of OpenBankProject#2881. First of four commits on this branch; the rest follow the same test-first, one-issue-per-commit shape.
1. An SCA challenge could authorise a different consent
authoriseUKConsentfetched the started challenge and threw it away:validateChallengeAnswerC4is handed theconsentId, but the connector ignores it —LocalMappedConnectormatches on challenge id, supplied answer and expected user alone. So the consent id travelled the whole way without ever being compared to anything.Measured on a local instance, before the fix: a challenge minted on a
ReadAccountsBasicconsent, answered against aReadAccountsBasic + ReadAccountsDetail + ReadBalancesconsent → 200, the wider consent moved toAUTHORISEDand was bound to the PSU, while the consent that was actually challenged stayedAWAITINGAUTHORISATION. An OTP the PSU was given for a narrow consent authorised a wider one.After: 400
OBP-40022, and neither consent moves.Standard
read-write-data-api-profile.html, Consent Authorisation / Identifying the consent to be authorised: "For the OBL APIs, all authentication journeys take place in the context of a consent"; the ConsentId "is the intent identifier"; it is carried into the request object so that "the access token that is eventually generated [is] bound to a specific consent"; and "the ASPSP must allow at most one bc_authorize request at any time to be associated with a given consent."OBP mints its own OTP challenge rather than using the profile's request-object mechanism, so the binding the profile gets structurally has to be asserted explicitly. The Berlin Group half of this same PR already asserts it (
Http4sBGv13AIS:startedChallenge.consentId.contains(consentId)), getting the binding from its own URL shape — authorisations are nested under the consent. This brings the UK half into line with the same guard, message and code. It also refuses a transaction-request challenge here for free: that carries noconsentId.Verification
UKOpenBankingV401AccountInfoTests, observed failing first (200 did not equal 400), green after. It asserts the refusal and that both consents are left untouched — an authorisation that got this far used to bind the PSU and flip the status.code.api.v5_1_0+code.api.UKOpenBanking+code.api.berlin.group.v1_3, 53 suites — 798 tests, 0 failures.git_commitchecked against branch HEAD):run_probes.py105/0,uk_view_endpoint_matrix.py12/12,stale_revoke_failure.py17/0,uk_direction.py6/0,bg_legacy_payment.py15/15,uk_token_path.pyOK.uk_challenge_binding.pydrives the scenario end to end and was run on both builds: 200-and-authorised before, 400-and-untouched after.Noted, not fixed here
LocalMappedConnector.createChallengesC2hardcodeschallengeType = OBP_TRANSACTION_REQUEST_CHALLENGE, discarding the caller's value — there is not oneOBP_CONSENT_CHALLENGErow in the database despite consent challenges being created constantly. It does not affect this guard, which keys offconsentId, so it is left for its own change rather than widening this one.