Skip to content

moderateAccount gates reading an account on a transaction permission, so every account-reading view must carry can_see_transaction_this_bank_account #81

Description

@hongwei1

What

ViewExtended.moderateAccount / moderateAccountLegacy / moderateAccountCore (code/model/View.scala:326, :380, :429) all open with the same gate:

if (viewPermissions.exists(_ == CAN_SEE_TRANSACTION_THIS_BANK_ACCOUNT)) { ... }
else Failure(s"$ViewDoesNotPermitAccess You need the `can_see_transaction_this_bank_account` permission on the view(...)")

So whether an account can be read at all is decided by a permission named for, and everywhere else meaning, transaction visibility. A view that grants no transactions and only wants to show a balance still has to carry it.

How it surfaced

SYSTEM_READ_BALANCES_VIEW_PERMISSION was defined as exactly {can_see_bank_account_balance, can_query_available_funds} — the honest set for a view whose whole job is balances. The UK balances endpoints reach the account through NewStyle.function.moderatedBankAccountCore (Http4sUKOBv401AccountInfo.scala:584, Http4sUKOBv310Balances.scala:118), so with that set:

GET /open-banking/v4.0.1/aisp/accounts/{id}/balances
400 OBP-20022: View does not permit the access.
    You need the `can_see_transaction_this_bank_account` permission on the view(ReadBalances)

Worked around by adding the permission to the set, with a comment pointing here. That is a workaround, not the fix: it puts a transaction-named permission into a balances view, which is precisely the kind of over-broad grant the view's permission set was tightened to remove.

Why it stayed invisible

Three checks covered this area and none could catch it:

  • MappedViewsTest asserted each view's allowed_actions equals its defining constant — the constant compared with itself, which is true whatever the constant says.
  • The in-repo UK balances test asserts only 401 (unauthenticated) and 403 (no consent). It never reads a balance, so it never reaches the gate.
  • The out-of-repo probe matrix ran against a database whose ReadBalances still carried the pre-existing 74-permission generic set, so the code-defined set was exercised nowhere.

A scenario that calls the gate directly has been added to MappedViewsTest, so a view an endpoint moderates through can no longer silently lose the ability to do so.

What fixing it properly looks like

Separate "may this caller reach the account" from "may this caller see transactions". Options, roughly in increasing order of correctness and blast radius:

  1. Gate moderateAccountCore on a permission that means what it does — e.g. require any of the account-field permissions the caller is about to be given, rather than one fixed transaction permission.
  2. Drop the gate from moderateAccountCore entirely and let per-field moderation decide, since every field below it is already individually gated (if (viewPermissions.exists(...)) Some(x) else None). The gate then adds nothing except the ability to fail.
  3. Keep a gate but make it explicit — a dedicated CAN_SEE_BANK_ACCOUNT permission added to every view that is meant to expose an account.

(2) looks closest to right and is the smallest code change, but it changes the failure mode for every existing view and caller from "403/400 with a clear message" to "200 with an empty-ish account", which is a behavioural change worth deciding deliberately rather than inferring.

Whichever is chosen, SYSTEM_READ_BALANCES_VIEW_PERMISSION should lose CAN_SEE_TRANSACTION_THIS_BANK_ACCOUNT again as part of it.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions