bank: check a caller-supplied owner against the session principal

examples/bank/README.md

@@ -238,36 +238,25 @@ unvouched-for, `RemoteServer` *clears* it, and every bank action then fails with
 whatever non-empty principal arrives, and anyone who can open a socket can claim
 to be any customer.
 
-What that does and does not leave standing: per-row ownership *is* enforced by
-the models (`db::loadOwned` navigates a row to its owner and compares that with
-the session principal), so cross-customer isolation is real and testable — it is
-what `another-customer-cannot-touch-your-account.scenario` pins. What is absent
-is credential *proof*. A server that needs it swaps in
+What that does and does not leave standing: ownership *is* enforced by the
+models, in both of the two spellings an action can be addressed by. A row id
+goes through `db::loadOwned`, which navigates the row to its owner and compares
+that with the session principal; an owner name goes through
+`bank::resolveOwner`, which compares the caller-supplied `owner` with the
+session principal and refuses on a mismatch. Cross-customer isolation is
+therefore real and testable —
+`another-customer-cannot-touch-your-account.scenario` pins the first spelling
+and `an-owner-named-outright-is-checked-against-the-session.scenario` the
+second. What is absent is credential *proof*. A server that needs it swaps in
 `morph::session::SigningAuthorizer` and issues a token to verify against; bank
 has none to issue.
 
 #### Defects the scenario corpus pins on purpose
 
-Three things the corpus asserts as current behaviour because they are true, not
-because they are right. A scenario passing over any of them is a record, not an
-endorsement; when one is fixed, the file that pins it is meant to fail.
-
-- **[morph#471](https://github.com/LASTRADA-Software/morph/issues/471) — a
-  caller-supplied owner beats the session principal.**
-  `bank::resolveOwner()` ([`include/bank/core/principal.hpp:24`](include/bank/core/principal.hpp))
-  returns `action.owner` whenever it is non-empty and only falls back to the
-  session principal when it is not; nothing compares the two. Ten actions across
-  eight models resolve their scope through it — `ListAccounts`, `ListCards`,
-  `ListPayees`, `ListPayments`, `ListLoans`, `ListBudgets`,
-  `ListNotifications`, `GenerateStatement`, `OpenAccount` and `MarkAllRead` —
-  so a signed-in customer who types another customer's username is served that
-  customer's data, and `MarkAllRead` *writes* to it. `SpendingByKind` consults
-  no owner at all and answers a caller with no session. Actions addressed by
-  row id are unaffected: they load the row and check its owner, which is the
-  pattern the ten should follow.
-  `an-owner-named-outright-is-not-checked-against-the-session.scenario` is the
-  inventory: every cross-owner read and the `MarkAllRead` write are `expect
-  ok` there, beside the id-addressed calls that are correctly refused.
+Two things the corpus asserts as current behaviour because they are true, not
+because they are right. A scenario passing over either of them is a record, not
+an endorsement; when one is fixed, the file that pins it is meant to fail.
+
 - **A DTO's `validate()` shadows the model's own `ValidationError`.** Fifteen
   bank actions carry a `validate()` predicate on the wire DTO *and* open their
   `execute()` with `if (!action.validate()) throw ValidationError{"…"}`. Over

examples/bank/include/bank/core/principal.hpp

@@ -4,6 +4,8 @@
 #include <morph/session/session.hpp>
 #include <string>
 
+#include "bank/core/errors.hpp"
+
 /// @file
 /// Helpers for reading the authenticated principal from the morph session
 /// context. The bridge attaches its default session (set once at login via
@@ -20,9 +22,39 @@ namespace bank {
     return {};
 }
 
-/// @brief Returns @p explicitOwner if non-empty, otherwise the session principal.
+/// @brief Resolves the owner an owner-named action is scoped to: the session
+///        principal, always.
+///
+/// Ten actions across eight models carry an `owner` field on the wire
+/// (`ListAccounts`, `ListCards`, `ListPayees`, `ListPayments`, `ListLoans`,
+/// `ListBudgets`, `ListNotifications`, `GenerateStatement`, `OpenAccount` and
+/// `MarkAllRead`). The field is **verified, not trusted**: it may name the
+/// caller, in which case it is redundant, or it may be left empty, in which
+/// case the session principal stands in. Naming anybody else is refused.
+///
+/// The alternative — ignoring `action.owner` outright — was rejected because
+/// the field is load-bearing on the wire: it is `CustomerModel`'s bridge
+/// routing key (`BRIDGE_MODEL_KEY(CustomerModel, ListAccounts,
+/// &ListAccounts::owner)`), so a request naming another customer would be
+/// routed to that customer's model instance and then quietly served the
+/// caller's own rows. Refusing says what happened instead, and matches
+/// `db::loadOwned`, which is how the id-addressed half of the same models has
+/// always enforced ownership.
+///
+/// An empty session principal matches no name at all, so an anonymous caller
+/// naming a real customer is refused here rather than served. An anonymous
+/// caller naming *nobody* still gets the empty string back: each call site
+/// carries its own `owner.empty()` guard and refuses with a message naming
+/// what it was about to do.
+///
+/// @throws Unauthorized if @p explicitOwner is non-empty and is not the
+///         session principal.
 [[nodiscard]] inline std::string resolveOwner(const std::string& explicitOwner) {
-    return explicitOwner.empty() ? sessionPrincipal() : explicitOwner;
+    std::string principal = sessionPrincipal();
+    if (!explicitOwner.empty() && explicitOwner != principal) {
+        throw Unauthorized{"owner does not match the session principal"};
+    }
+    return principal;
 }
 
 }  // namespace bank

examples/bank/src/models/budget_model.cpp

@@ -85,12 +85,21 @@ dto::BudgetList BudgetModel::execute(const dto::ListBudgets& action) {
 }
 
 dto::SpendingReport BudgetModel::execute(const dto::SpendingByKind& action) {
+    // This action is addressed by account id and carries no `owner` field, so
+    // `resolveOwner` never sees it and cannot be what scopes it. `db::loadOwned`
+    // is: it navigates the row to its owner and compares that with the session
+    // principal, the same guard every other id-addressed action uses. Without
+    // it the report consulted no owner at all and read any account in the
+    // database, including for a caller with no session.
+    const auto account = db::loadOwned<db::AccountRecord>(mapper(), action.accountId, sessionPrincipal(), "account");
+    const auto accountId = static_cast<std::int64_t>(account.id.Value());
+
     // Push the account/direction/time filters into the query so only the rows we
     // aggregate cross the wire; the by-kind rollup stays in code (no GROUP BY SQL).
     auto rows =
         mapper()
             .Query<db::TxnRecord>()
-            .Where(Lightweight::FieldNameOf<&db::TxnRecord::account>, "=", action.accountId)
+            .Where(Lightweight::FieldNameOf<&db::TxnRecord::account>, "=", accountId)
             .Where(Lightweight::FieldNameOf<&db::TxnRecord::direction>, "=", static_cast<int>(TxnDirection::Debit))
             .Where(Lightweight::FieldNameOf<&db::TxnRecord::createdAtMs>, ">=", action.sinceMs)
             .All();
@@ -107,7 +116,7 @@ dto::SpendingReport BudgetModel::execute(const dto::SpendingByKind& action) {
     }
 
     dto::SpendingReport report;
-    report.accountId = action.accountId;
+    report.accountId = accountId;
     report.totalDebitsMinor = totalDebits;
     report.byKind.reserve(byKind.size());
     for (const auto& [kind, spend] : byKind) {

scripts/scenario/README.md

@@ -125,20 +125,22 @@ scenarios. `--rung bank` and the `bank` column below are therefore spellings of
 | `bank/` | `ladder_bank_server` | registration and password-only sign-in, accounts and the ledger behind them, overdraft boundaries, transfers, bills and scheduled/standing instructions, cards, loans and amortisation, budgets, notifications, statements; two customers, the anonymous session, and the stateful account cache |
 
 Some of the bank files assert behaviour that is **wrong**, deliberately, and
-say so in their own header comments. The largest is
-[morph#471](https://github.com/LASTRADA-Software/morph/issues/471):
-`bank::resolveOwner()` prefers a caller-supplied owner name over the session
-principal, so ten actions serve a signed-in customer another customer's data
-and `MarkAllRead` writes to it —
-`bank/an-owner-named-outright-is-not-checked-against-the-session.scenario` is
-the inventory, its cross-owner steps all `expect ok`. `SpendingByKind`
-answering a caller
-with no session at all, and `GenerateStatement` reporting a live balance in a
-field named `closingBalanceMinor`, are pinned the same way. An `expect ok` over
-a defect records what the server does today so the day it changes is not a
-silent one; it is not an endorsement, and the fix is meant to turn those
-assertions red. That is the opposite arrangement from
-`broken-on-purpose.scenario`, which fails today by design.
+say so in their own header comments: `GenerateStatement` reporting a live
+balance in a field named `closingBalanceMinor` is one, and a DTO's `validate()`
+shadowing the model's own error message is another. An `expect ok` over a defect
+records what the server does today so the day it changes is not a silent one; it
+is not an endorsement, and the fix is meant to turn those assertions red. That
+is the opposite arrangement from `broken-on-purpose.scenario`, which fails today
+by design.
+
+`bank/an-owner-named-outright-is-checked-against-the-session.scenario` is what
+that arrangement looks like after the fix lands. It was
+[morph#471](https://github.com/LASTRADA-Software/morph/issues/471)'s inventory,
+written entirely `expect ok` because `bank::resolveOwner()` preferred a
+caller-supplied owner name over the session principal and ten actions therefore
+served a signed-in customer another customer's data. Flipping those assertions
+to `expect err` was that issue's regression test, and the file now pins the
+enforcement in the same shape it once pinned the defect.
 
 The rung a scenario belongs to is its parent directory name — that is how
 per-rung action coverage is attributed, so a file loose in `scenarios/` is

scripts/scenario/scenarios/bank/a-statement-covers-every-account-an-owner-has.scenario

@@ -89,18 +89,30 @@ expect ok field lines ~ "\"accountId\":$busy,[^}]*\"entryCount\":0"
 
 # ── An owner with no accounts at all ────────────────────────────────────────
 # A statement with no lines, rather than a refusal: the owner exists and simply
-# holds nothing.
+# holds nothing. Asserted from that owner's own session, because
+# `GenerateStatement` resolves its scope through `bank::resolveOwner`, which
+# refuses any `owner` that is not the session principal — see
+# an-owner-named-outright-is-checked-against-the-session.scenario.
 client other model=AuthModel
 do RegisterUser username=perlis-no-accounts password=none-pass displayName="No Accounts"
 expect ok field message ~ "^(registered|username already taken)$"
+do LoginRequest username=perlis-no-accounts password=none-pass
+expect ok capture nobody=$.principal
+expect ok field ok == true
 
-use paper
-do GenerateStatement owner=perlis-no-accounts fromMs=0 toMs=0
+client empty-paper model=StatementModel principal=$nobody
+do GenerateStatement owner=$nobody fromMs=0 toMs=0
 expect ok field owner == "perlis-no-accounts"
 expect ok field lines == []
 expect ok field totalCreditsMinor == 0
 
-# An owner who was never registered has no `users` row to resolve, and that is
-# reported as an authorization failure rather than as an empty statement.
+# Naming somebody else is refused, and refused *before* the `users` row is
+# looked up — so a name that exists and one that never did are answered
+# identically. That is the point: resolving first and reporting "unknown user:
+# …" told an unauthorized caller whether a username existed.
+use paper
+do GenerateStatement owner=perlis-no-accounts fromMs=0 toMs=0
+expect err message == "owner does not match the session principal"
+
 do GenerateStatement owner=nobody-ever fromMs=0 toMs=0
-expect err message == "unknown user: nobody-ever"
+expect err message == "owner does not match the session principal"