diff --git a/.gitignore b/.gitignore index d83ae0f8..565eb986 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,13 @@ /build-wasm/ /out/ *.db +# Every ladder rung's server writes its FileActionLog into the *current +# directory* -- `std::filesystem::current_path() / "_actions.jsonl"`, see +# each `src/server/main.cpp`. Running one from the repo root (or pointing the +# scenario runner at it) therefore drops a journal here, and such a journal was +# once staged by accident on a working branch. Nothing matching this pattern is +# in this repository's history -- the rule exists so nothing ever is. +*_actions.jsonl *.profraw /.cache/ /compile_commands.json diff --git a/codecov.yml b/codecov.yml index 65a5c6b3..878906fa 100644 --- a/codecov.yml +++ b/codecov.yml @@ -416,17 +416,27 @@ component_management: # ledger's and lims's above, and `informational: true` like every other # ladder component (morph#133). # - # Unlike those two, this entry does NOT yet carry a per-miss audit - # establishing which of the 148 uncovered lines are unreachable-by-design - # rather than merely untested. One thing the measurement already shows is - # not a coverage artifact, and is recorded here rather than rounded past: - # `ContactModel::attachActionLog` and `SavedViewModel::attachActionLog` - # are called by no test at all (both headers score 0/4), so those two - # models' journaling paths are entirely unverified -- while - # AccountModel, LeadModel, OpportunityModel and QuoteModel each have an - # explicit "journals its edits against the attached identity" test. That - # is the same shape of gap ledger's own first measurement turned up - # (a documented obligation nothing executed), and it is open, not closed. + # Re-measured after morph#403 and morph#426, on a profile with zero foreign + # roots: 1488/1616 = 92.08% lines, of which quote_model.cpp is now + # 222/238 93.28% (GetQuote, previously driven by nothing). By Codecov's own + # arithmetic -- which is what this target is checked against, and which + # counts a line with a one-sided branch as a partial rather than a hit -- + # the component reads 87.15%: 1390 hits, 128 misses, 77 partials. + # + # The per-miss audit this entry used to lack is now written, in + # examples/crm/README.md's "What the uncovered lines in crm's models are". + # Its result is that crm is unlike ledger and lims: 92 of the 141 lines it + # classified are the rung's own ValidationError/NotFound rejection paths on + # 25 actions, reachable from the wire and simply untested, and only 4 are + # unreachable-by-design. That is not a ceiling and must not be recorded as + # one (morph#412). + # + # `ContactModel::attachActionLog` and `SavedViewModel::attachActionLog` are + # no longer called by no test: both models gained the "journals its edits + # against the attached identity" case their four siblings had, and no + # defect was found on either -- both journal both mutations, under the + # attached key, with the caller's principal. Recorded as a negative result + # against the lims precedent, rather than left implying an open finding. - component_id: crm name: "application ladder rung 7 (examples/crm models)" paths: diff --git a/examples/TESTING.md b/examples/TESTING.md index 6b1bf883..5b189b3b 100644 --- a/examples/TESTING.md +++ b/examples/TESTING.md @@ -811,6 +811,117 @@ here as though it already existed, but it has never been built (morph#255): (there is no scheduled workflow in this repo yet), so nothing in the ladder should assume a cadence the rest of the project doesn't have. +## What `examples/common`'s coverage number measures (audit, 2026-09) + +`codecov.yml`'s `ladder` component scores `examples/common/**` against a 95% +target. That number was reported as 90.39% on `master` and read as a five-point +regression (morph#411). It is a real figure, it is reproducible, and the +regression is real — but two things about it have to be said before the figure +means anything, because both of them were being read the other way. + +### Two numbers, both correct, four points apart + +`llvm-cov report`'s line coverage and Codecov's component percentage are not +the same measurement. Codecov reads the uploaded LCOV and scores +`hits / (hits + misses + partials)`, where a *partial* is a line that executed +but carries a branch nothing took. `llvm-cov` counts that line as covered. +Measured on the same profile, on the tree this section was written against: + +| | `llvm-cov` lines | Codecov component | +|------------------------------|------------------|-------------------| +| `examples/common` | 96.94% | 93.68% | + +Whenever the two are quoted side by side without saying which is which, the +gap looks like a regression or a contradiction. It is neither: it is 39 lines +with a one-sided branch. **Quote the Codecov figure when arguing about the +component's target, because that is the one the target is checked against.** + +Reproduce either from a coverage run — `cmake --preset clang-coverage` (plus +`-DMORPH_BUILD_{NET,OFFLINE_SQLITE,QT,LADDER}=ON`), `ctest`, then +`bash scripts/coverage.sh`. `llvm-cov`'s figure is in that script's own report; +Codecov's arithmetic runs over `build/clang-coverage/coverage.lcov`. + +A prior local measurement of this component disagreed with the uploaded one by +five points for a *different* reason — a shared compiler cache served objects +built in another worktree, whose absolute paths matched none of +`scripts/coverage.sh`'s relative filters, so the records were dropped rather +than mis-attributed (morph#426). That is fixed at configure time and gated by +`scripts/check_coverage_roots.sh`; run it before trusting any local figure. + +### Where the drop came from: four files that did not exist at the measurement + +`codecov.yml` records `examples/common` at 95.85% when rungs 2-4 entered the +report (morph#142, 2026-08-21). Splitting today's figure by whether a file +existed then answers the question that ticket asked and could not answer: + +| | hits | miss | partial | lines | Codecov | +|------------------------------------|-----:|-----:|--------:|------:|--------:| +| files present at that measurement | 658 | 17 | 12 | 687 | 95.78% | +| `qml_surface.cpp`, `process_pool.hpp`, `journey.hpp`, `step_executor.hpp` | 505 | 49 | 40 | 594 | 85.02% | + +The code that was measured at 95.85% still measures 95.78%. **Nothing +regressed.** The component's denominator nearly doubled, and every line of the +growth is testkit written to serve a later rung and not itself tested to the +standard the rest of the directory holds. `qml_surface.cpp` alone was 493 of +the 1281 lines and 70 of the 118 non-hits. + +### The artifact set, re-derived + +`codecov.yml` names seven lines as measurement artifacts and puts the ceiling +at 478/485 = 98.56%. That list is still correct about the lines it names and is +no longer the whole set — the tree it was written against was a third of +today's size. Re-derived, and split by *why* each line reads uncovered: + +**Measurement artifacts — the statement ran; the counter on it did not.** +llvm-cov puts a "control reached past this block" counter on certain closing +braces, and there is no `LCOV_EXCL_LINE` equivalent to suppress one. + +* `backend_rig.hpp` 218, 225, 255 — the three switch-case `}` after `break;`, + one per `Mode`. The three the current comment names. +* `strand_interleaver.hpp` 71, 97 — `step()` and `runSchedule()`, whose one + statement calls a `std::function`. The two the current comment names; + in today's LCOV the zero lands on the `task();` line itself rather than the + brace below it. +* `step_executor.hpp` 89 — the same shape in `runOne()`. **New**: the file + postdates the comment. +* `qml_surface.cpp` 599 — a switch-case `}` after `break;`. **New**, same + reason. + +**Unreachable by design — the guard cannot fire through the callers it has.** + +* `backend_rig.hpp` 256, 267 — the `default:` label and its `throw`. Present + only because `-Wswitch-default` demands one on a switch that already covers + every enumerator; the file says so at the site. **New.** +* `fault_proxy.cpp` 68-69 — `onClientConnection`'s null guard. The one the + current comment names; `isValidIncomingConnection` is unit-tested directly. +* `qml_surface.cpp` 92-93, 96-97 — `signalNameOf`'s "not in handler shape" + guards. Both call sites match `on[A-Z]\w*`, so the name it is handed can + never be too short, unprefixed, or lower-cased at position 2. **New.** +* `qml_surface.cpp` 353-354 — `record()`'s empty-name early return, + unreachable for exactly that reason. **New.** + +**Untested, and reachable.** These are not artifacts and are not excused: + +* `qml_surface.cpp` 55, 83-84 — the unbalanced-`{` and unterminated-`(` + fallbacks, reachable only from QML that does not parse. +* `qml_surface.cpp` 441-443 — the "cannot read" finding for a `.qml` file + `QDirIterator` lists but `QFile::open` refuses. Needs a permission-denied + file, which does not survive a git checkout and does not reproduce for a + root-running CI container. +* `fault_proxy.cpp` 17-19 — `~FaultProxy`'s client-socket teardown; every + test today closes the leg before the proxy dies. +* `journey.hpp` 69-72 — the two `catch` arms around a journey step. Pinning + them needs a harness that can assert a Catch2 `FAIL` *happened*, which this + suite does not have. +* `process_pool.hpp` 51-53 — `~ClientProcess`'s kill-if-still-running. +* `process_pool.hpp` 129-130 — `spawn()`'s failed-to-start arm. + +**Dead API — no caller anywhere in the tree.** Not a coverage problem; the +lines are uncovered because nothing wants them: + +* `process_pool.hpp` 67 — `ClientProcess::waitForFinished`. +* `process_pool.hpp` 160-166 — `ProcessPool::killAll`. + ## Framework gaps this strategy exposed (all since closed) This list was written as candidate issues to schedule. **Every item on it has diff --git a/examples/common/testkit/test_event_poller.cpp b/examples/common/testkit/test_event_poller.cpp index d2557619..eb6b6c27 100644 --- a/examples/common/testkit/test_event_poller.cpp +++ b/examples/common/testkit/test_event_poller.cpp @@ -27,10 +27,12 @@ // fire, deterministically and without any sleep_for (examples/TESTING.md, // "Pumping discipline -- no sleeps"). +#include #include #include #include #include +#include #include #include #include @@ -699,3 +701,38 @@ TEST_CASE("EventPoller tolerates an empty onFatalError callback on a non-timeout CHECK(poller.fatalErrorReported()); CHECK_FALSE(poller.running()); } + +// ═════════════════════════════════════════════════════════════════════════ +// The two failure-classifier helpers, called directly (morph#411) +// ═════════════════════════════════════════════════════════════════════════ +// +// `isClientTimeout` and `describeFailure` are declared in +// `event_poller.hpp`'s `detail` namespace with a documented answer for a null +// `std::exception_ptr` and for a throw that is not a `std::exception`. Both +// answers were reachable only through `handleError`, which never produces +// either shape, so what the header promises was held up by nothing. They are +// free functions taking an `exception_ptr`, so the promise can simply be +// called. + +TEST_CASE("isClientTimeout: a null exception is not a timeout", "[testkit][event-poller]") { + // The distinction this classifier draws decides whether the poller retries + // or stops for good. "No exception information" must fall on the *stop* + // side: a tick that failed for a reason nobody captured is not evidence + // the transport is merely slow. + CHECK_FALSE(morph::ladder::gui::detail::isClientTimeout(nullptr)); + CHECK_FALSE( + morph::ladder::gui::detail::isClientTimeout(std::make_exception_ptr(std::runtime_error{"not a timeout"}))); + CHECK(morph::ladder::gui::detail::isClientTimeout(std::make_exception_ptr(morph::backend::ClientTimeoutError{}))); +} + +TEST_CASE("describeFailure: every input yields a non-empty message", "[testkit][event-poller]") { + // `onFatalError`'s message is the only account a GUI gets of why polling + // stopped, and the header promises it is never empty. The two arms that + // make that true — a null exception and a throw that is not a + // `std::exception` — are the ones no poller path produces. + CHECK(morph::ladder::gui::detail::describeFailure(nullptr).contains(QStringLiteral("no exception information"))); + CHECK(morph::ladder::gui::detail::describeFailure(std::make_exception_ptr(42)) + .contains(QStringLiteral("non-std::exception"))); + CHECK(morph::ladder::gui::detail::describeFailure(std::make_exception_ptr(std::runtime_error{"boom"})) == + QStringLiteral("boom")); +} diff --git a/examples/common/testkit/test_qml_surface.cpp b/examples/common/testkit/test_qml_surface.cpp index d1e9ed53..5ed2fa27 100644 --- a/examples/common/testkit/test_qml_surface.cpp +++ b/examples/common/testkit/test_qml_surface.cpp @@ -80,6 +80,15 @@ class OtherFixtureBridge : public QObject { Q_INVOKABLE void ping() {} }; +/// @brief A bridge that declares no QML-visible member of its own. +/// +/// `Q_OBJECT` and nothing else: every name on its metaobject comes from +/// `QObject`, so `Scope::Own` sees an empty surface. This is what a controller +/// looks like when the `Q_INVOKABLE`/`Q_PROPERTY` markers were forgotten. +class EmptyFixtureBridge : public QObject { + Q_OBJECT +}; + /// @brief A shared base that publishes one signal — the shape `bank`'s /// `BankController` has, with six controllers inheriting its `error`. class BaseFixtureBridge : public QObject { @@ -861,3 +870,283 @@ TEST_CASE("QmlSurfaceAudit: a guarded read of a member the bridge DOES have stil INFO(describe(findings)); CHECK(findings.isEmpty()); } + +// ═════════════════════════════════════════════════════════════════════════ +// The arms nothing executed — each one a promise the audit's own header +// makes and, until now, kept only by inspection (morph#411) +// ═════════════════════════════════════════════════════════════════════════ + +TEST_CASE("blankCommentsAndStrings blanks single-quoted and template strings, not just double-quoted", + "[testkit][qml-surface]") { + // QML takes all three quote forms, and every case above this one used + // double quotes only. A blanker that handled `"` alone would leave the + // contents of a `'...'` or a `` `...` `` visible to the reference scanner, + // so a member named in a log line or a translated label would be reported + // as a binding site that does not exist. The assertion is the *contents* + // are gone while the surrounding code survives, because a blanker that + // simply erased the line would also pass a length check. + const QString source = QStringLiteral("a 'one' b\nc `two` d\ne \"three\" f\n"); + const QString blanked = blankCommentsAndStrings(source); + REQUIRE(blanked.size() == source.size()); + CHECK(blanked.count(QLatin1Char('\n')) == source.count(QLatin1Char('\n'))); + CHECK_THAT(blanked.toStdString(), !Catch::Matchers::ContainsSubstring("one")); + CHECK_THAT(blanked.toStdString(), !Catch::Matchers::ContainsSubstring("two")); + CHECK_THAT(blanked.toStdString(), !Catch::Matchers::ContainsSubstring("three")); + for (const char* code : {"a", "b", "c", "d", "e", "f"}) { + CHECK_THAT(blanked.toStdString(), Catch::Matchers::ContainsSubstring(code)); + } +} + +TEST_CASE("QmlSurfaceAudit: a member named only inside a single-quoted string is not a reference", + "[testkit][qml-surface]") { + // The end-to-end half of the case above: `blankCommentsAndStrings` runs + // before the scanner, so a ghost member inside `'...'` must reach neither + // the reference list nor the findings. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + writeQml(dir, QStringLiteral("Main.qml"), + cleanQml().append(QStringLiteral("\nItem { property string doc: 'page.fixture.ghost()' }\n"))); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + CHECK(findings.isEmpty()); +} + +TEST_CASE("QmlSurfaceAudit: an identifier merely ending in the bound alias is not that alias", + "[testkit][qml-surface]") { + // `page.subfixture.ghost()` and `page.sub\u00e9fixture.ghost()` both contain + // `fixture.ghost()`. The ASCII one is refused by the scanner's own `\b`; + // the accented one is not, because QRegularExpression's `\b` is ASCII-only + // while the guard beside it asks `QChar::isLetterOrNumber()`, which is not. + // Both are asserted here, because which of the two mechanisms rejects a + // given spelling is exactly the thing that goes wrong silently: a false + // reference on an alias no one addressed is a finding against a bridge + // that is innocent. + const auto spelling = GENERATE(QStringLiteral("subfixture"), QStringLiteral("sub\u00e9fixture")); + + QTemporaryDir dir; + REQUIRE(dir.isValid()); + writeQml(dir, QStringLiteral("Main.qml"), + cleanQml().append(QStringLiteral("\nItem { function stray() { page.%1.ghost() } }\n").arg(spelling))); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(spelling.toStdString() << ": " << describe(findings)); + CHECK(findings.isEmpty()); +} + +TEST_CASE("QmlSurfaceAudit: a property-style Connections handler is scanned like a function one", + "[testkit][qml-surface]") { + // `onListed: expr` is the other handler syntax QML accepts, and every case + // above binds `listed` through `function onListed(...)`. Both patterns run + // over the same `Connections` body, so a rung written in the property + // style was, until this case, audited as if it bound nothing at all — + // silence that reads exactly like agreement. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + const QString propertyStyle = cleanQml().replace(QStringLiteral(R"( function onListed(rows, ok) { + console.log(rows, ok) + })"), + QStringLiteral(" onListed: console.log(1)")); + REQUIRE_THAT(propertyStyle.toStdString(), Catch::Matchers::ContainsSubstring("onListed: console.log(1)")); + writeQml(dir, QStringLiteral("Main.qml"), propertyStyle); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + SECTION("binding the signal this way is silence, not an unbound-member finding") { + const QStringList findings = audit.run(); + INFO(describe(findings)); + CHECK(findings.isEmpty()); + } +} + +TEST_CASE("QmlSurfaceAudit: a property-style handler for a signal the bridge lacks is a finding", + "[testkit][qml-surface]") { + // The other direction of the case above: the property-style scan must be + // able to *fail*, or it would be silencing rather than auditing. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + const QString qml = + cleanQml().replace(QStringLiteral(R"( function onListed(rows, ok) { + console.log(rows, ok) + })"), + QStringLiteral(" onListed: console.log(1)\n onVanished: console.log(2)")); + writeQml(dir, QStringLiteral("Main.qml"), qml); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + REQUIRE(findings.size() == 1); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("onVanished")); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("emits no signal")); +} + +TEST_CASE("QmlSurfaceAudit: a name inside Connections that is not in handler shape binds nothing", + "[testkit][qml-surface]") { + // `onListed` is a handler; `onlisted` is an ordinary property assignment + // that happens to start with the same three letters, and reading it as a + // binding would silence a genuinely unbound signal. Both handler patterns + // require `on[A-Z]`, so it is the pattern rather than `signalNameOf`'s own + // shape guard that rejects this spelling — see the audit in + // examples/TESTING.md, which classifies that guard as unreachable from + // either caller. Asserted through the sweep: the signal must still be + // reported as bound by nothing. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + const QString qml = cleanQml().replace(QStringLiteral(R"( function onListed(rows, ok) { + console.log(rows, ok) + })"), + QStringLiteral(" onlisted: console.log(1)")); + writeQml(dir, QStringLiteral("Main.qml"), qml); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + REQUIRE(findings.size() == 1); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("listed")); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("no scanned .qml")); +} + +TEST_CASE("QmlSurfaceAudit: a Connections block with no target is skipped rather than mis-attributed", + "[testkit][qml-surface]") { + // A `Connections` block whose `target` is set elsewhere (or not at all) + // names no alias, so its handlers belong to nothing this audit knows + // about. Skipping it is what stops those handlers being charged to + // whichever alias happened to be scanned last. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + writeQml(dir, QStringLiteral("Main.qml"), cleanQml().append(QStringLiteral(R"( +Item { + Connections { + function onSomethingElse() {} + } +} +)"))); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + CHECK(findings.isEmpty()); +} + +TEST_CASE("QmlSurfaceAudit: calling a signal as if it were an invokable says so specifically", + "[testkit][qml-surface]") { + // Drift direction 1, in its most confusing shape: the member exists, so + // "no such invokable" would send the reader looking for a typo. The + // audit's own header promises the distinction; nothing executed the arm + // that draws it. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + writeQml(dir, QStringLiteral("Main.qml"), + cleanQml().replace(QStringLiteral("page.fixture.refresh()"), QStringLiteral("page.fixture.listed()"))); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + // Two: the mis-call itself, and `refresh` now bound by nothing. + REQUIRE(findings.size() == 2); + const std::string joined = describe(findings); + CHECK_THAT(joined, Catch::Matchers::ContainsSubstring("declares it as a signal, not an invokable")); + CHECK_THAT(joined, !Catch::Matchers::ContainsSubstring("has no such invokable")); +} + +TEST_CASE("QmlSurfaceAudit: reading an invokable by name counts as binding it, not as a missing property", + "[testkit][qml-surface]") { + // `onClicked: page.fixture.refresh` — a handler assigned the function + // itself rather than calling it — is a *reference* to a method, and the + // scanner classifies it as a read because there is no `(`. Resolving a + // read against the method table before reporting "no such property" is + // what stops that idiom being a false finding, and it must also mark the + // method bound so the unreferenced sweep does not then report it. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + writeQml(dir, QStringLiteral("Main.qml"), + cleanQml().replace(QStringLiteral("page.fixture.refresh()"), QStringLiteral("page.fixture.refresh"))); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + CHECK(findings.isEmpty()); +} + +TEST_CASE("QmlSurfaceAudit: a bound object with no QML-visible members at all is a finding", + "[testkit][qml-surface]") { + // The vacuity family's missing member. An object with nothing to audit + // cannot disagree with any QML, so pointing the guard at one is a + // configuration mistake that would otherwise read as a clean pass — the + // same failure the "no bridge bound" and "no .qml files" cases above + // exist to refuse. + QTemporaryDir dir; + REQUIRE(dir.isValid()); + // The QML *does* address the second alias, so the "referenced by no + // scanned .qml" arm above it stays quiet and this case isolates the one + // it is named for. That is also the realistic shape: the object is wired + // up and used, and the `Q_INVOKABLE`/`Q_PROPERTY` markers were forgotten. + writeQml(dir, QStringLiteral("Main.qml"), + cleanQml().append(QStringLiteral("\nItem { function go() { page.bare.ping() } }\n"))); + + SurfaceFixtureBridge bridge; + EmptyFixtureBridge bare; + QmlSurfaceAudit audit{dir.path()}; + audit.bind(QStringLiteral("fixture"), bridge); + audit.bind(QStringLiteral("bare"), bare); + + const QStringList findings = audit.run(); + INFO(describe(findings)); + REQUIRE(findings.size() == 1); + CHECK_THAT(findings.first().toStdString(), + Catch::Matchers::ContainsSubstring("declares no QML-visible members at all")); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("bare")); +} + +TEST_CASE("QmlSurfaceAudit: addDirectory() audits the second directory too", "[testkit][qml-surface]") { + // The header offers `addDirectory()` for a rung whose QML lives beside a + // shared renderer's, and nothing called it. A second directory that is + // merely stored and never walked would drop every finding in it, which is + // the guard silently covering less than it claims to. + QTemporaryDir first; + QTemporaryDir second; + REQUIRE(first.isValid()); + REQUIRE(second.isValid()); + writeQml(first, QStringLiteral("Main.qml"), cleanQml()); + writeQml(second, QStringLiteral("Extra.qml"), + QStringLiteral("import QtQuick\nItem { id: page\n property var fixture: null\n" + " function go() { page.fixture.ghost() }\n}\n")); + + SurfaceFixtureBridge bridge; + QmlSurfaceAudit audit{first.path()}; + audit.addDirectory(second.path()); + audit.bind(QStringLiteral("fixture"), bridge); + + INFO("scanned: " << audit.scannedFiles().join(QStringLiteral(", ")).toStdString()); + CHECK(audit.scannedFiles().size() == 2); + const QStringList findings = audit.run(); + INFO(describe(findings)); + REQUIRE(findings.size() == 1); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("Extra.qml")); + CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("no such invokable")); +} diff --git a/examples/crm/README.md b/examples/crm/README.md index cd60d718..8378c956 100644 --- a/examples/crm/README.md +++ b/examples/crm/README.md @@ -641,6 +641,112 @@ The two review-added features in the **"7-later" bucket** (duplicate detection on create, record merge) are deferred by the delivery review itself, not by this build, and neither gates 7a or 7b. +## What the uncovered lines in `crm`'s models are (audit, 2026-09) + +`codecov.yml` scores `examples/crm/{src,include/crm}/models/**` and says, of +this rung's entry specifically, that it "does NOT yet carry a per-miss audit +establishing which of the 148 uncovered lines are unreachable-by-design rather +than merely untested" (morph#412). Ledger's and lims's entries carry one; each +ends by naming two or three defensive guards no caller can reach. + +**crm's answer is different, and that is the finding.** Measured over the whole +crm suite on a clean coverage run, the component had 141 uncovered lines, and +four of them are unreachable. Every one is classified below. + +| class | lines | +|---|---:| +| `validate()` rejections — a required field the action was not given | 52 | +| `NotFound` rejections — an id that names no row | 40 | +| other untested branches (enumerated below) | 39 | +| `journalEntries()` — an accessor with no caller | 6 | +| unreachable by design | 4 | + +### The 92 lines that are the rung's rejection contract + +Two thirds of the gap is one shape: every `throw ValidationError{": X +is required"}` and every `throw NotFound{": no such Y"}` in the eight +model translation units. They are reachable from the wire by any client that +sends a malformed action or a stale id — they are, in fact, *the* thing a +buggy or hostile client reaches first — and they are executed by no test. +`UpdateContact`, `GetContact`, `UpdateLead`, `MarkLeadLost`, `GetLead`, +`ConvertLead`, `UpdateAccount`, `SetAccountRole`, `GetAccountRoles`, +`GetAccountHistory`, `UndoLastAccountChange`, `GetAccount`, `UpdateQuote`, +`SendQuote`, `DecideQuote`, `GetQuote`, `UpdateOpportunity`, `GetOpportunity`, +`MoveOpportunityStage`, `QueuedOpportunityUpdate`, `ListConflicts`, +`ResolveConflict`, `DeleteCustomField`, `RunSavedView` and `DeleteSavedView` +each carry at least one, and none is driven. + +This is untested, not unreachable, and it must not be written down as if it +were a ceiling. Nor is it closable by generating one rejection case per guard: +a case that asserts only "it threw" pins the exception type and nothing else, +while the assertion worth having — that the two refusals stay +*distinguishable*, because a caller retries differently for "you sent nothing" +than for "it is gone" — is one case per model, not one per line. The `GetQuote` +pair in `tests/test_quote_model.cpp` is that shape. + +### The 39 other lines, named + +* **`QuoteModel::execute(const GetQuote&)` — the whole action, 13 lines, 9 of + them here and its two guards counted above.** Registered on the wire by + `include/crm/models/quote_model.hpp`'s `BRIDGE_REGISTER_ACTION` and driven by + no test, no presenter and no scenario. Now covered — and removing its + `validate()` guard does not merely change a message, it aborts the process on + an unset `QuoteId`. +* **`ListContactOptions`' unfiltered branch, 6 lines.** Every test passes an + `accountId`, so "options for every contact" — the form's own default — runs + nowhere. +* **`UpdateOpportunity`'s primary-contact lookup, 5 lines,** and its + `expectedCloseValue` set arm, 2 lines; `ResolveConflict`'s clear-the-value + arm, 3 lines. Each is the unexercised side of an optional field. +* **`entryNamesAccount`'s two `return false` arms, 4 lines,** and + `UndoLastAccountChange`'s `continue` for an entry naming a different account, + 2 lines. The undo history filter only ever runs over entries that all match. +* **`decodeChoiceOptions`' two "nothing stored"/"does not decode" arms, 4 + lines,** and `decodeMinRoleToEdit`'s fall-through to `Role::Member` for a + pre-7b field, 2 lines. +* **`parseAccountChoice`'s empty-choice arm, 2 lines** — a contact submitted + with no account at all. + +### The 6 lines that are not a coverage problem + +`journalEntries()` is declared on all seven models and called from exactly one +place in the rung — `tests/test_saved_view.cpp`. The other six have no caller +in tests, presenters, app code or GUI: a rung reads its audit trail through +`GetAccountHistory`, not through this accessor. They are uncovered because +nothing wants them. + +### The 4 that are unreachable by design + +* **`alreadyDecided`'s empty-key guard (`src/models/opportunity_model.cpp`), 2 + lines.** `QueuedOpportunityUpdate::validate()` requires a non-empty + `operationKey`, and `execute` runs it before any path reaches the lookup, so + the guard fires only for a caller that bypassed validation entirely. This is + the same guard, for the same reason, that lims's own audit records for + `lims::SampleModel::alreadyDecided` — which the function's doc comment + already cites as its model. +* **`MoveOpportunityStage`'s `CrmError{"MoveOpportunityStage: corrupt ledger + entry"}`, 2 lines.** It fires only for a stage-ledger row this model wrote + and cannot parse back. It turns an impossible state into a typed error + instead of a bad read; reaching it in a test means writing the corruption + directly. + +### Two claims in `codecov.yml` this audit retires + +* `ContactModel::attachActionLog` and `SavedViewModel::attachActionLog` are no + longer "called by no test at all", and those models' journaling paths are no + longer "entirely unverified" — both gained the case their four siblings had, + and **no defect was found on either**: both journal both of their mutations, + under the attached key, with the caller's principal. That is contrary to what + the same paragraph's lims precedent would predict, and it is worth recording + as a negative result rather than leaving a comment that implies an open one. +* The 148-line figure is 141 as this audit found it, every one accounted for + above. Covering `GetQuote` leaves 128, and moves the component from 86.33% to + 87.15% by Codecov's own arithmetic (`hits / (hits + misses + partials)` over + the uploaded LCOV) and from 91.27% to 92.08% by `llvm-cov`'s line count. Those + are two different measurements of the same profile, not a discrepancy — see + `../TESTING.md`, "What `examples/common`'s coverage number measures", for + which one a `codecov.yml` target is checked against. + ## Definition of done Current state per bullet. The rung is server-complete; the two bullets that diff --git a/examples/crm/tests/test_contact_model.cpp b/examples/crm/tests/test_contact_model.cpp index 444b3b57..e81aa332 100644 --- a/examples/crm/tests/test_contact_model.cpp +++ b/examples/crm/tests/test_contact_model.cpp @@ -167,3 +167,50 @@ TEST_CASE("ListContactOptions serves {id, name} rows for the primary-contact Cho CHECK(options.contacts.front().id == std::to_string(*created.contactId)); CHECK(options.contacts.front().name == "Ada Lovelace"); } + +// The sibling of `AccountModel`/`LeadModel`/`OpportunityModel`/`QuoteModel`'s +// own "journals its edits against the attached identity" case (morph#412). +// `ContactModel::attachActionLog` was called by no test at all, so the claim +// that this model journals what it writes was supported by no executed line -- +// the state lims was in before its own audit trail turned out to be recording +// verifications under an empty key. +// +// Reading the entries back through `log->entries("contacts")` rather than +// through the model is what makes the key load-bearing: entries stamped with a +// different key, or with none, do not come back from that call, so the case +// fails if `attachActionLog` is not called or its key is not the one used. +TEST_CASE("ContactModel journals its edits against the attached identity", "[crm][contact][audit]") { + DbFixture fixture; + const ScopedPrincipal alice{"alice"}; + auto log = std::make_shared(); + crm::AccountModel accounts; + crm::ContactModel contacts; + contacts.attachActionLog(log, std::string{"contacts"}); + + const auto accountId = createAcme(accounts); + const auto created = contacts.execute(ada(crm::AccountChoice{std::to_string(*accountId)})); + contacts.execute(crm::UpdateContact{ + .contactId = created.contactId, + .account = crm::AccountChoice{std::to_string(*accountId)}, + .firstName = "Ada", + .lastName = "King", + .email = "ada.king@example.test", + .phone = "555-0100", + .expectedVersion = 1, + }); + + const auto entries = log->entries("contacts"); + REQUIRE(entries.size() == 2); + CHECK(entries[0].actionType == "CreateContact"); + CHECK(entries[1].actionType == "UpdateContact"); + for (const auto& entry : entries) { + CHECK(entry.modelType == "ContactModel"); + CHECK(entry.entityKey == "contacts"); + CHECK(entry.principal == "alice"); + CHECK(entry.outcome == morph::journal::Outcome::Succeeded); + CHECK_FALSE(entry.schema.empty()); + } + // `AccountModel` shares neither the log nor the key here, so a stray + // account entry cannot be what satisfied the count above. + CHECK(log->entries("accounts").empty()); +} diff --git a/examples/crm/tests/test_quote_model.cpp b/examples/crm/tests/test_quote_model.cpp index 67bd6ca0..51aa2a01 100644 --- a/examples/crm/tests/test_quote_model.cpp +++ b/examples/crm/tests/test_quote_model.cpp @@ -324,3 +324,57 @@ TEST_CASE("QuoteModel journals its edits against the attached identity", "[crm][ CHECK(entry.outcome == morph::journal::Outcome::Succeeded); } } + +// ── GetQuote: registered on the wire, driven by nothing (morph#412) ────── +// +// `BRIDGE_REGISTER_ACTION(crm::QuoteModel, crm::GetQuote, "GetQuote", ...)` +// puts this action on the rung's wire surface, and the per-miss audit of +// crm's uncovered lines (README, "What the uncovered lines in crm's models +// are") found +// its whole body — every line of it — executed by no test, no presenter and +// no scenario. A read action that is never read back is where a wrong answer +// hides longest: `ListQuotes` exercises the same `toView`/`fetchLines` pair +// in aggregate, so a `GetQuote` that fetched the wrong row, or that answered +// an unknown id with a default-constructed `QuoteView` instead of refusing, +// would look identical to a working one from anywhere else in the suite. + +TEST_CASE("GetQuote answers with that quote, its lines and its computed total", "[crm][quote]") { + DbFixture fixture; + const ScopedPrincipal alice{"alice"}; + crm::AccountModel accounts; + crm::OpportunityModel opportunities; + crm::QuoteModel quotes; + + const auto dealA = createDeal(accounts, opportunities); + const auto dealB = createDeal(accounts, opportunities); + const auto wanted = + quotes.execute(crm::CreateQuote{.opportunityId = dealA, .lines = twoLines(), .taxRate = fraction(0, 100, 2)}); + // A second quote exists, on a different opportunity, so "returns a quote" + // and "returns *this* quote" are distinguishable. + quotes.execute(crm::CreateQuote{.opportunityId = dealB, .lines = twoLines(), .taxRate = fraction(0, 100, 2)}); + + const auto fetched = quotes.execute(crm::GetQuote{.quoteId = wanted.quote.id}); + CHECK(fetched.id == wanted.quote.id); + CHECK(fetched.opportunityId == dealA); + CHECK(fetched.status == crm::QuoteStatus::Draft); + CHECK(fetched.lines.size() == wanted.quote.lines.size()); + CHECK(fetched.grandTotal == wanted.quote.grandTotal); + CHECK(fetched.version == wanted.quote.version); +} + +TEST_CASE("GetQuote refuses an id that names no quote rather than answering an empty one", "[crm][quote]") { + DbFixture fixture; + const ScopedPrincipal alice{"alice"}; + crm::AccountModel accounts; + crm::OpportunityModel opportunities; + crm::QuoteModel quotes; + + const auto opportunityId = createDeal(accounts, opportunities); + const auto created = quotes.execute( + crm::CreateQuote{.opportunityId = opportunityId, .lines = twoLines(), .taxRate = fraction(0, 100, 2)}); + + CHECK_THROWS_AS(quotes.execute(crm::GetQuote{.quoteId = crm::QuoteId{*created.quote.id + 1}}), crm::NotFound); + // An unset id is a client error, not a missing row: the two are different + // refusals and a caller that cannot tell them apart cannot retry correctly. + CHECK_THROWS_AS(quotes.execute(crm::GetQuote{}), crm::ValidationError); +} diff --git a/examples/crm/tests/test_saved_view.cpp b/examples/crm/tests/test_saved_view.cpp index 423f6fe3..df819d30 100644 --- a/examples/crm/tests/test_saved_view.cpp +++ b/examples/crm/tests/test_saved_view.cpp @@ -6,6 +6,9 @@ // result. Scoped per-principal, owner-only to delete. #include +#include +#include +#include #include "crm/core/errors.hpp" #include "crm/models/account_model.hpp" @@ -231,6 +234,63 @@ TEST_CASE("DeleteSavedView naming a nonexistent view is NotFound", "[crm][saved_ CHECK_THROWS_AS(model.execute(crm::DeleteSavedView{.savedViewId = crm::SavedViewId{999}}), crm::NotFound); } +// The sibling of `AccountModel`/`LeadModel`/`OpportunityModel`/`QuoteModel`'s +// own "journals its edits against the attached identity" case (morph#412). +// `SavedViewModel::attachActionLog` was called by no test at all, so nothing +// executed proved this model records what it writes. +// +// Both mutations are covered, and the delete deliberately so: a saved view is +// the one crm entity whose row is *removed* rather than versioned, which makes +// its journal entry the only surviving record that it ever existed. Reading +// back through `log->entries("saved-views")` is what makes the attached key +// load-bearing -- an entry stamped with a different key, or none, does not come +// back from that call. +TEST_CASE("SavedViewModel journals its edits against the attached identity", "[crm][saved_view][audit]") { + DbFixture fixture; + const ScopedPrincipal alice{"alice"}; + auto log = std::make_shared(); + crm::SavedViewModel model; + model.attachActionLog(log, std::string{"saved-views"}); + + const auto saved = + model.execute(crm::CreateSavedView{.name = "My negotiations", .stage = crm::OpportunityStage::Negotiation}); + model.execute(crm::DeleteSavedView{.savedViewId = saved.savedViewId}); + + const auto entries = log->entries("saved-views"); + REQUIRE(entries.size() == 2); + CHECK(entries[0].actionType == "CreateSavedView"); + CHECK(entries[1].actionType == "DeleteSavedView"); + for (const auto& entry : entries) { + CHECK(entry.modelType == "SavedViewModel"); + CHECK(entry.entityKey == "saved-views"); + CHECK(entry.principal == "alice"); + CHECK(entry.outcome == morph::journal::Outcome::Succeeded); + CHECK_FALSE(entry.schema.empty()); + } + // The view itself is gone; the journal is what is left of it. + CHECK(model.execute(crm::ListSavedViews{}).views.empty()); + CHECK(model.journalEntries().size() == 2); +} + +// `ListSavedViews` and `RunSavedView` are registered `Loggable::No` and record +// nothing, which this pins so a later reader does not read the case above as +// "every action journals". +TEST_CASE("SavedViewModel's reads journal nothing", "[crm][saved_view][audit]") { + DbFixture fixture; + const ScopedPrincipal alice{"alice"}; + auto log = std::make_shared(); + crm::SavedViewModel model; + model.attachActionLog(log, std::string{"saved-views"}); + + const auto saved = model.execute(crm::CreateSavedView{.name = "Negotiating"}); + const auto afterCreate = log->entries("saved-views").size(); + + model.execute(crm::ListSavedViews{}); + model.execute(crm::RunSavedView{.savedViewId = saved.savedViewId}); + + CHECK(log->entries("saved-views").size() == afterCreate); +} + TEST_CASE("Every mutating SavedView action refuses an empty principal", "[crm][saved_view][audit]") { DbFixture fixture; crm::SavedViewModel model; diff --git a/examples/kanban/README.md b/examples/kanban/README.md index b8c75368..a1a6caa0 100644 --- a/examples/kanban/README.md +++ b/examples/kanban/README.md @@ -109,8 +109,14 @@ Build order: nothing in this rung uses it. `execute(MoveTaskPosition)` re-checks the whole destination inside its own transaction and throws the rung's typed errors — `NotFound` for a column or swimlane that no longer belongs to - the project (`src/models/board_model.cpp:849`, `:858`), `Conflict` for a - target column already at its WIP limit (`:835`). A replay that keeps + the project (`src/models/board_model.cpp:966`, the + `requireColumnBelongsToProject` call, and `:998`, the swimlane half's + inline re-check), `Conflict` for a target column already at its WIP limit + (`:984`, the `throw Conflict`). Each citation names what is on the line as + well as the number: nothing here verifies a line number — + `scripts/check_spec_citations.sh` checks cited paths and section headings + only — so the name is what a reader follows once the line has moved. + A replay that keeps throwing is retried by `SyncWorker` up to its 5-attempt cumulative cap and then dead-lettered, and `BoardBridge`'s `DeadLetterSink` turns that into a `syncStatusChanged(queueDepth, deadLettered)` emission the GUI diff --git a/examples/kanban/include/kanban/models/board_model.hpp b/examples/kanban/include/kanban/models/board_model.hpp index eb9f1440..3df237cb 100644 --- a/examples/kanban/include/kanban/models/board_model.hpp +++ b/examples/kanban/include/kanban/models/board_model.hpp @@ -279,8 +279,29 @@ class BoardModel { /// functionally the same effect `recordIfAttached` gives a /// holder-wrapped instance, achieved without one. /// @param log Sink entries are forwarded to. - /// @param entityKey Stable identity stamped onto every `LogEntry` this - /// instance produces (this rung's project id, as a string). + /// @param entityKey This rung's project id, as a string. Accepted only + /// if it is the complete *canonical* decimal spelling of one -- + /// `"7"`, never `"007"`, because this member is also each entry's + /// `entityKey` and `execute(GetActivity)` reads entries back by that + /// exact string, so a second spelling would silently split one + /// board's activity stream. Anything else is + /// discarded, and the entries this instance stamps then carry the + /// empty key (`logAction` reads `_projectIdStr`, the one member + /// both purposes share). Two keys that are not project ids reach + /// here, and both used to pose as a board (#368): + /// `ModelFactory::create` passes an *empty* key when it hands the + /// process-wide default log to a newly constructed holder, and + /// `Remote::attachLogIfConfigured` forwards the client's + /// `contextKey` *verbatim off the wire*, so it can be any text at + /// all. The `log` itself is attached either way. + /// + /// Consequence worth knowing before relying on the stamp: for a + /// discarded key, the holder still records `_contextKey` verbatim + /// (`morph/core/model.hpp`) while this instance records `""`, so + /// the two writers disagree in one log. Only garbage keys are + /// affected. Giving journal stamping its own member -- the shape + /// `ledger::LedgerModel` and lims/crm already use -- would settle + /// it, and is the larger change #368's triage deferred. void attachActionLog(std::shared_ptr<::morph::journal::IActionLog> log, std::string entityKey); private: @@ -398,13 +419,25 @@ class BoardModel { /// project. void applyTagMutationImpl(const ApplyTagMutation& action); - /// @brief The project this handler is attached to, cached on the first - /// successful `execute(OpenBoard)`. Also set (independently) by - /// `attachActionLog`, whose `entityKey` parameter is the string - /// form of the same project id in every path this rung exercises - /// -- `OpenBoard` overwrites it with the identical value, so the - /// two writers never disagree in practice. Unset until the first - /// of either call. + /// @brief The project this handler is attached to, set on the first + /// successful `execute(OpenBoard)` and, independently, by + /// `attachActionLog` from an `entityKey` that **parses whole as a + /// project id** (the keyed registration path, where + /// `Remote::attachLogIfConfigured` supplies the client's + /// `contextKey` -- the same project id `OpenBoard` would set). + /// Disengaged until one of those happens. + /// + /// The invariant every attach guard in this class relies on: + /// **engaged implies it parses as a project id.** That is what + /// makes `has_value()` a sufficient test before the + /// `std::stoull(*_projectIdStr)` following it in every `execute()` + /// overload -- sufficient for the parse, that is; whether the + /// project still exists is `loadProjectById`'s question, and + /// whether the caller may touch it is `requireRole`'s. It holds + /// only because both writers establish it: `OpenBoard` stores + /// `std::to_string` of a real row id, and `attachActionLog` + /// declines any key that is not one. See #368 for what the + /// fifteen guards did while it did not hold. std::optional _projectIdStr; /// @brief Durable action log this instance appends to, if any -- set by diff --git a/examples/kanban/src/models/board_model.cpp b/examples/kanban/src/models/board_model.cpp index abe26471..41a49cc3 100644 --- a/examples/kanban/src/models/board_model.cpp +++ b/examples/kanban/src/models/board_model.cpp @@ -5,10 +5,15 @@ #include #include #include +#include +#include #include #include #include #include +#include +#include +#include #include "clock.hpp" #include "kanban/db/kanban_entity.hpp" @@ -236,11 +241,58 @@ void requireProjectMatchesAttachedBoard(ProjectId projectId, std::uint64_t attac return result; } +/// @brief Whether @p key is the decimal spelling of a project row id, whole +/// and with nothing else in it. +/// +/// The precondition the fifteen attach guards in this file are written +/// against. Each tests `_projectIdStr.has_value()` and then reaches +/// `std::stoull(*_projectIdStr)`, so `has_value()` is only a sufficient guard +/// while everything stored there parses -- and one of the two writers takes +/// its key straight off the wire (`Remote::attachLogIfConfigured` forwards the +/// client's `contextKey` verbatim, `morph/core/remote.hpp`), so "everything +/// stored there" is a claim about untrusted text. +/// +/// `std::from_chars` rather than a `stoull` in a `try` block, because the +/// question is whether the *whole* string is an id: `stoull` stops at the +/// first non-digit and reports success, so it reads "5x" as project 5 and +/// would silently attach the handler to a board the client did not name. It +/// also accepts leading whitespace and wraps a negative around to a huge +/// id; `from_chars` on an unsigned type does neither. +/// +/// The spelling has to be *canonical*, not merely parseable, because +/// `_projectIdStr` has a second reader: `logAction`/`logFailure` stamp it as +/// each entry's `entityKey`, and `execute(GetActivity)` reads entries back by +/// that exact string. `from_chars` accepts "007" for project 7, so a client +/// registering with `contextKey=007` would pass every attach guard and every +/// `requireRole` while journaling under a key no other client -- and no +/// `GetActivity` -- ever asks for, silently splitting one board's activity +/// stream in two. Comparing against `std::to_string(value)` is what rules that +/// out. +[[nodiscard]] bool namesAProject(std::string_view key) noexcept { + std::uint64_t value = 0; + const char* const end = key.data() + key.size(); + // The empty key needs no case of its own: `from_chars` reports + // `invalid_argument` for an empty range, so it fails the first test. + const auto [stopped, ec] = std::from_chars(key.data(), end, value); + return ec == std::errc{} && stopped == end && key == std::to_string(value); +} + } // namespace void BoardModel::attachActionLog(std::shared_ptr<::morph::journal::IActionLog> log, std::string entityKey) { _log = std::move(log); - _projectIdStr = std::move(entityKey); + // A key that does not name a project does not become one -- this + // condition is load-bearing, not defensive. `_projectIdStr` is also the + // answer to "which project is this handler attached to", which every + // `execute()` overload asks as `has_value()` before dereferencing it into + // `std::stoull`; adopting the key unconditionally made that question + // answer "yes" for strings no `stoull` can parse, so the guards fell + // through and the rung replied with the bare text `stoull` -- an + // `std::invalid_argument` escaping as though it were a domain error + // (#368). See `attachActionLog`'s declaration for the contract this keeps. + if (namesAProject(entityKey)) { + _projectIdStr = std::move(entityKey); + } } template diff --git a/examples/kanban/tests/test_board_unattached.cpp b/examples/kanban/tests/test_board_unattached.cpp new file mode 100644 index 00000000..95546d50 --- /dev/null +++ b/examples/kanban/tests/test_board_unattached.cpp @@ -0,0 +1,316 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// A `BoardModel` that was registered but never pointed at a board. +// +// This is the state a client reaches by registering the handler and +// dispatching anything before `OpenBoard` -- `BoardModel` is keyed by project +// id (`BRIDGE_MODEL_KEY` on `OpenBoard::projectId`, `AllowShared`), so a fresh +// registration is attached to no board at all. Every `execute()` overload +// already opens with a `_projectIdStr.has_value()` guard carrying the right +// message, the one `polls::PollModel` answers with. +// +// Those guards were dead over a server, and the reason is not in this file's +// subject at all. `ModelFactory::create` attaches the process-wide default +// action log to every newly constructed holder *with an empty `entityKey`* +// (`include/morph/core/model.hpp`, and kanban's `App` installs such a log via +// `morph::journal::setActionLog`), and `attachActionLog` used to write that +// key straight into `_projectIdStr` -- the same member that records which +// board the handler is attached to. The optional came out *engaged with an +// empty string*, so `has_value()` was true, every guard fell through, and the +// first `std::stoull(*_projectIdStr)` threw `std::invalid_argument{"stoull"}`, +// which went on the wire as though it were a domain error. +// +// The in-process tests missed it because they all attach before acting, and +// the two writers of `_projectIdStr` only disagree when they do not. So these +// cases enter that state deliberately: `attachActionLog(log, {})` is character +// for character the call `ModelFactory::create` makes. +// +// Filed as morph#368; the out-of-process counterpart is +// `scripts/scenario/scenarios/kanban/a-board-must-be-opened-before-it-answers.scenario`. + +#include +#include +#include +#include +#include +#include +#include + +#include "kanban/core/errors.hpp" +#include "kanban/models/board_model.hpp" +#include "kanban/models/project_admin_model.hpp" +#include "testkit/db_fixture.hpp" + +using morph::ladder::testkit::DbFixture; + +namespace { + +/// @brief See `test_board_model.cpp`'s identical `contextFor`/ +/// `ScopedPrincipal` pair for why this is not a designated +/// initializer. +[[nodiscard]] morph::session::Context contextFor(std::string principal) { + morph::session::Context ctx; + ctx.principal = std::move(principal); + return ctx; +} + +class ScopedPrincipal { +public: + explicit ScopedPrincipal(std::string principal) : _ctx{contextFor(std::move(principal))}, _scope{_ctx} {} + +private: + morph::session::Context _ctx; + morph::session::detail::ScopedContext _scope; +}; + +[[nodiscard]] kanban::ProjectId createProjectAs(const std::string& principal, const std::string& name) { + const ScopedPrincipal p{principal}; + kanban::ProjectAdminModel admin; + return admin.execute(kanban::CreateProject{.name = name}).id; +} + +/// @brief Runs @p call and returns the `what()` of whatever it threw, or the +/// empty string if it returned normally. +/// +/// Catches `std::exception`, not `kanban::NotFound`, on purpose. The defect +/// this file pins threw `std::invalid_argument{"stoull"}` from +/// `std::stoull("")` -- outside kanban's own hierarchy entirely -- so a +/// `CHECK_THROWS_AS(..., kanban::NotFound)` would have reported it as an +/// escaping exception rather than as the wrong message, and the assertion +/// that matters here is *which text a client receives*. Returning the text +/// rather than asserting inside lets each case below read as one comparison. +template +[[nodiscard]] std::string errorTextOf(Fn&& call) { + try { + std::forward(call)(); + } catch (const std::exception& error) { + return std::string{error.what()}; + } + return {}; +} + +/// @brief A handler in exactly the state a fresh registration leaves it in: +/// default-constructed, then handed the process-wide default action +/// log with an empty `entityKey`, the way `ModelFactory::create` does. +class UnattachedBoard { +public: + UnattachedBoard() { _model.attachActionLog(std::make_shared(), {}); } + + [[nodiscard]] kanban::BoardModel& get() { return _model; } + +private: + kanban::BoardModel _model; +}; + +} // namespace + +// The two guards issue #368 singles out: they are written, they are correct, +// and over a server they never fired. Split into their own cases so a +// regression in either is named by the failing test rather than by an +// assertion line. +TEST_CASE("GetActivity on an unattached handler names the action and OpenBoard", "[kanban][model][unattached]") { + DbFixture fixture; + UnattachedBoard board; + const ScopedPrincipal alice{"alice"}; + + // Both halves matter and they are separate claims: the *type* is what a + // caller's `.onError(...)` branches on, and it was `std::invalid_argument` + // -- outside kanban's hierarchy altogether -- while the guard was dead. + CHECK_THROWS_AS(board.get().execute(kanban::GetActivity{}), kanban::NotFound); + CHECK(errorTextOf([&] { return board.get().execute(kanban::GetActivity{}); }) == + "GetActivity: handler was never attached via OpenBoard"); +} + +TEST_CASE("GetRules on an unattached handler names the action and OpenBoard", "[kanban][model][unattached]") { + DbFixture fixture; + const auto projectId = createProjectAs("alice", "Unopened"); + UnattachedBoard board; + const ScopedPrincipal alice{"alice"}; + + // A real, existing projectId in the action: the refusal is about the + // *handler* never having been attached, not about the argument. + CHECK_THROWS_AS(board.get().execute(kanban::GetRules{.projectId = projectId}), kanban::NotFound); + CHECK(errorTextOf([&] { return board.get().execute(kanban::GetRules{.projectId = projectId}); }) == + "GetRules: handler was never attached via OpenBoard"); +} + +// All fifteen guards, in one case: the nine actions issue #368 lists -- every +// one of which answered the bare string "stoull" over a server -- and the six +// it does not. +TEST_CASE("Every action on an unattached handler is refused by name, never with \"stoull\"", + "[kanban][model][unattached]") { + DbFixture fixture; + const auto projectId = createProjectAs("alice", "Unopened"); + UnattachedBoard board; + auto& model = board.get(); + const ScopedPrincipal alice{"alice"}; + + // `NotFound` for the whole surface, spot-checked on one representative + // action -- the per-action assertions below are about the text. + CHECK_THROWS_AS(model.execute(kanban::GetBoardState{}), kanban::NotFound); + + CHECK(errorTextOf([&] { return model.execute(kanban::GetBoardState{}); }) == + "GetBoardState: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::CreateColumn{.name = "Todo", .wipLimit = 0}); }) == + "CreateColumn: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::CreateSwimlane{.name = "Lane"}); }) == + "CreateSwimlane: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { + return model.execute(kanban::CreateTask{ + .columnId = kanban::ColumnId{1}, .swimlaneId = kanban::SwimlaneId{1}, .title = "None"}); + }) == "CreateTask: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { + return model.execute(kanban::MoveTaskPosition{.taskId = kanban::TaskId{1}, + .columnId = kanban::ColumnId{1}, + .swimlaneId = kanban::SwimlaneId{1}, + .position = 0, + .opId = ""}); + }) == "MoveTaskPosition: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { + return model.execute(kanban::AddComment{.taskId = kanban::TaskId{1}, .body = "Into the void"}); + }) == "AddComment: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::GetEventsSince{.lastEventId = {}}); }) == + "GetEventsSince: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::GetActivity{}); }) == + "GetActivity: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::GetRules{.projectId = projectId}); }) == + "GetRules: handler was never attached via OpenBoard"); + + // The rest of the fifteen. Not in issue #368's list and not driven by the + // scenario either -- which is exactly why they are here: a regression that + // re-opened the fall-through in one of these six would otherwise pass + // every file written to prevent it. + CHECK(errorTextOf([&] { + return model.execute(kanban::AddAttachment{.taskId = kanban::TaskId{1}, + .filename = "spec.pdf", + .contentType = "application/pdf", + .sizeBytes = 1, + .storageKey = "blob-1"}); + }) == "AddAttachment: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::GetAttachments{.taskId = kanban::TaskId{1}}); }) == + "GetAttachments: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { + return model.execute(kanban::RemoveAttachment{.attachmentId = kanban::AttachmentId{1}}); + }) == "RemoveAttachment: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { + return model.execute(kanban::CreateRule{.projectId = projectId, + .triggerColumnId = kanban::ColumnId{1}, + .mutationType = kanban::RuleMutationType::AddTag, + .mutationValue = "urgent"}); + }) == "CreateRule: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { return model.execute(kanban::DeleteRule{.ruleId = kanban::RuleId{1}}); }) == + "DeleteRule: handler was never attached via OpenBoard"); + CHECK(errorTextOf([&] { + return model.execute(kanban::ApplyTagMutation{ + .taskId = kanban::TaskId{1}, .mutationType = kanban::RuleMutationType::AddTag, .tag = "urgent"}); + }) == "ApplyTagMutation: handler was never attached via OpenBoard"); +} + +// Validation still runs ahead of the attach guard, exactly as it did before: +// a malformed action is refused for being malformed. Pinned here because the +// fix moves *when* the attach guard bites, and the outermost gate must not +// have moved with it. +TEST_CASE("A malformed action on an unattached handler is refused for being malformed", + "[kanban][model][unattached]") { + DbFixture fixture; + UnattachedBoard board; + auto& model = board.get(); + const ScopedPrincipal alice{"alice"}; + + CHECK(errorTextOf([&] { return model.execute(kanban::CreateColumn{.name = "", .wipLimit = 0}); }) == + "CreateColumn: a bounded, non-empty name is required"); + CHECK(errorTextOf([&] { + return model.execute(kanban::GetEventsSince{.lastEventId = kanban::BoardEventId{-1}}); + }) == "GetEventsSince: malformed request"); +} + +// The keyed attach path is the one `attachActionLog` legitimately sets +// `_projectIdStr` from -- `Remote::attachLogIfConfigured` passes a real +// project id as the `entityKey` when the client registered with a non-empty +// contextKey. Refusing the *empty* key must not have disturbed it. +TEST_CASE("A non-empty entityKey still attaches the handler to that board", "[kanban][model][unattached]") { + DbFixture fixture; + const auto projectId = createProjectAs("alice", "Keyed"); + kanban::BoardModel model; + const ScopedPrincipal alice{"alice"}; + + model.attachActionLog(std::make_shared(), std::to_string(*projectId)); + + const auto state = model.execute(kanban::GetBoardState{}); + CHECK(state.projectId == projectId); + CHECK(state.name == "Keyed"); +} + +// The second key that reaches `attachActionLog` is the client's `contextKey`, +// forwarded verbatim off the wire by `Remote::attachLogIfConfigured` -- so it +// is arbitrary text, not necessarily a project id. Registering with +// `contextKey=foo` used to make `_projectIdStr == "foo"`, pass all fifteen +// guards, and reach `std::stoull("foo")`: the same bare "stoull" reply as the +// empty key, one step over. Reproduced over a live server before this was +// fixed. +TEST_CASE("A contextKey that is not a project id does not attach the handler", "[kanban][model][unattached]") { + DbFixture fixture; + kanban::BoardModel model; + const ScopedPrincipal alice{"alice"}; + + model.attachActionLog(std::make_shared(), "foo"); + + CHECK_THROWS_AS(model.execute(kanban::GetBoardState{}), kanban::NotFound); + CHECK(errorTextOf([&] { return model.execute(kanban::GetBoardState{}); }) == + "GetBoardState: handler was never attached via OpenBoard"); +} + +// A partially-numeric key is the dangerous one, and the reason the check is +// `std::from_chars` over the whole string rather than a `stoull` in a `try`: +// `stoull("5x")` succeeds, returning 5, so the handler would have silently +// attached to project 5 -- a board the client never named. Per-action +// `requireRole` still gates access, so this was a wrong-target bug rather than +// an authorization hole, but a client cannot be told which board it got. +TEST_CASE("A partly-numeric contextKey does not attach to the board its prefix names", "[kanban][model][unattached]") { + DbFixture fixture; + const auto projectId = createProjectAs("alice", "Not yours to guess"); + kanban::BoardModel model; + const ScopedPrincipal alice{"alice"}; + + // `x` -- `stoull` would have parsed the prefix and + // attached to exactly this project. + model.attachActionLog(std::make_shared(), std::to_string(*projectId) + "x"); + + CHECK(errorTextOf([&] { return model.execute(kanban::GetBoardState{}); }) == + "GetBoardState: handler was never attached via OpenBoard"); +} + +// A zero-padded key parses, and that is exactly the problem: `_projectIdStr` +// is read twice -- by `std::stoull` for the board, and verbatim as every +// entry's `entityKey`, which `execute(GetActivity)` looks entries up by. A +// handler attached as "007" would work against project 7 while journaling +// under a key no other client and no `GetActivity` ever asks for, splitting +// one board's activity stream in two. So the spelling has to be canonical, +// not merely parseable. +TEST_CASE("A zero-padded contextKey does not attach the handler", "[kanban][model][unattached]") { + DbFixture fixture; + const auto projectId = createProjectAs("alice", "Padded"); + kanban::BoardModel model; + const ScopedPrincipal alice{"alice"}; + + model.attachActionLog(std::make_shared(), "00" + std::to_string(*projectId)); + + CHECK(errorTextOf([&] { return model.execute(kanban::GetBoardState{}); }) == + "GetBoardState: handler was never attached via OpenBoard"); +} + +// Attaching a log with an empty key after OpenBoard must not un-attach the +// handler either: the old assignment clobbered a live board id with "", so a +// handler that had been working answered "stoull" from then on. +TEST_CASE("Attaching a log with an empty entityKey does not un-attach an open board", "[kanban][model][unattached]") { + DbFixture fixture; + const auto projectId = createProjectAs("alice", "Sprint Board"); + kanban::BoardModel model; + const ScopedPrincipal alice{"alice"}; + model.execute(kanban::OpenBoard{.projectId = projectId}); + + model.attachActionLog(std::make_shared(), {}); + + const auto state = model.execute(kanban::GetBoardState{}); + CHECK(state.projectId == projectId); +} diff --git a/examples/ledger/README.md b/examples/ledger/README.md index 2577dde1..6786fc3f 100644 --- a/examples/ledger/README.md +++ b/examples/ledger/README.md @@ -55,8 +55,72 @@ authenticated principal may call it; a caller with no token is refused principal by the model itself. Added by morph#361: until then a `ledgers` row was created by no registered action at all, and a freshly started `ladder_ledger_server` against a new database served a book nobody could open -(`OpenAccount` refused with `OpenAccount: no such ledger`). There is no -per-book ownership — see morph#382. +(`OpenAccount` refused with `OpenAccount: no such ledger`). + +**Whose book it is.** `CreateLedger` records its caller as the book's owner, +and every action that reaches a book compares that owner against the +authenticated principal before it does anything else — the reads included. +`GetLedger`, `GetBudgetReport` and `GetReportStatus` are gated exactly as the +writes are, because a book's accounts, balances and statements are the thing +worth protecting. A principal that does not own the book is refused +`: this book belongs to another principal`; one that does not exist is +still refused `: no such ledger`, and the two stay distinguishable so +an owner debugging a dead id is not told it is a permissions problem. + +The rule lives in the models, through the relation +(`ledger/db/book_access.hpp`, `examples/IMPLEMENTATION.md` rule 4, the shape +`bank::db::loadOwned` established at rung 1), never at the authorizer: +`LedgerAuthorizer::authorizeInstance` compares one register-time owner against +the caller, and `LedgerModel`'s instances are keyed by `ledgerId` and shared +across every client that opens the same book, so it has no single owning +caller to compare against. `RunReportJob` is the one exception, and it is not +one really — it admits only `kReportRunnerPrincipal`, the server's own runner, +which is a stronger gate than ownership rather than a hole in it. + +One book shape predates all of this: a `ledgers` row written before the +`owner` column existed (migration `20260819000015`) records no owner, and +stays readable and writable by every authenticated principal, exactly as it +was. SQLite cannot add a `NOT NULL` column to a table that may already hold +rows, and there is no principal to attribute those rows to; `NULL` therefore +means "created before ownership existed", the same reading `params_json` +already has on a report job. Nothing writes a new one — `CreateLedger` always +stamps its caller — but the scenario corpus's fixture books are seeded by raw +`INSERT` and are unowned for this reason. Added by morph#382. + +**There are no roles.** Ownership here is one principal per book, not a +membership table: there is no way to share a book with a second principal, and +no `kanban`-style `project_roles` to promote anyone through. A book is its +creator's, and everyone else is refused. + +**Two actions a client cannot drive to a result (morph#362).** Every action +below is registered on the wire, but two of them answer only with a refusal +no matter what a client sends, for two different reasons. Both are recorded +in `scripts/scenario/coverage_allowlist.json` so the workflow-coverage gate +does not chase them. + +- **`RunReportJob` is the report runner's, not a client's.** + `LedgerModel::execute(const RunReportJob&)` refuses every principal but + `kReportRunnerPrincipal` with `RunReportJob: only the report runner may run + a report job`. That is step 7's submit→poll split working as designed — + `ledger::app::App` sweeps for `Pending` rows on a timer and dispatches the + run under its own service principal (see step 7 below). The consequence for + a client author is that the *submit → run → poll* triple has no client-side + middle step: a client submits, gets a job id, polls, and either sees the + runner's own tick land or does not. It cannot advance its own job, and + asking to is a refusal, not a slow success. +- **`UndoTransaction` needs a `journalId` no action hands out.** + `UndoTransaction { ledgerId, journalId }` reverses one journal entry, and + `JournalId` appears in exactly one wire DTO in this rung — that action's own + input. Nothing returns one: `StoreTransaction` and `UndoTransaction` answer + `GetLedgerResult` (accounts and balances), `GetLedger` the same, + `ImportLedgerChunk` answers counts, and there is no `GetJournal` or + `ListTransactions`. So the only outcome a client can reach is the not-found + refusal `UndoTransaction: no such journal`, against an id it guessed; the + in-process tests get the id from the database instead. Undo itself is + implemented and exercised (step 5) — it is the *naming* of an entry that has + no client story. Whether to close that with an action that returns journal + ids, or to record undo as a server-side path deliberately, is open on + morph#362. Build order (status as of rung 5's implementation, see `docs/superpowers/plans/2026-08-19-ledger-rung5.md`): diff --git a/examples/ledger/include/ledger/auth/ledger_authorizer.hpp b/examples/ledger/include/ledger/auth/ledger_authorizer.hpp index 84397831..15e9a757 100644 --- a/examples/ledger/include/ledger/auth/ledger_authorizer.hpp +++ b/examples/ledger/include/ledger/auth/ledger_authorizer.hpp @@ -32,9 +32,16 @@ /// is: `SigningAuthorizer::authorize()` verifying a real signed token on /// every `execute` other than `Login`, `RemoteServer` overwriting /// `Context::principal` with the verified identity before the model runs, -/// and each mutating model action refusing an empty principal +/// each mutating model action refusing an empty principal /// (`EmptyPrincipalError`, design spec §11) or, for `RunReportJob`, refusing -/// any principal but `kReportRunnerPrincipal`. +/// any principal but `kReportRunnerPrincipal` -- and, since morph#382, +/// **per-book ownership**: `CreateLedger` records its caller on the `ledgers` +/// row and every action reaching a book compares that owner against +/// `Context::principal`, reads included. That last one is deliberately *not* +/// here: it is the same reason `authorizeInstance` is permissive above, seen +/// from the other side. The hook cannot express it for shared keyed +/// instances, so the rule lives through the relation instead +/// (`ledger/db/book_access.hpp`, `examples/IMPLEMENTATION.md` rule 4). namespace ledger::auth { diff --git a/examples/ledger/include/ledger/db/book_access.hpp b/examples/ledger/include/ledger/db/book_access.hpp new file mode 100644 index 00000000..aee027cd --- /dev/null +++ b/examples/ledger/include/ledger/db/book_access.hpp @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: Apache-2.0 +#pragma once + +#include +#include +#include +#include +#include + +#include "ledger/core/errors.hpp" +#include "ledger/core/types.hpp" +#include "ledger/db/ledger_entity.hpp" + +/// @file +/// The single home for this rung's "whose book is this?" rule (morph#382). +/// +/// **Where the rule lives, and why not at the authorizer.** +/// `examples/IMPLEMENTATION.md` rule 4 puts ownership authorization *through +/// the relation*, the way `bank::db::loadOwned` +/// (`examples/bank/include/bank/db/ledger_ops.hpp`) does for the rung with the +/// same subject matter. `LedgerAuthorizer`'s `authorizeInstance` hook cannot +/// express it: that hook compares one recorded register-time owner against the +/// caller, and `LedgerModel`'s instances are keyed by `ledgerId` and shared +/// across every client that opens the same book, so there is no single owning +/// caller for it to compare against. Rule 1 says the same thing from the other +/// side -- models re-check their own authorization. +/// +/// So every action that reaches a book calls into this header, either directly +/// on its own `ledgerId` (`requireOwnedBook`) or, when it names a child row +/// instead, on the ledger that row belongs to (`requireOwnedParentBook`). +/// +/// **What a NULL owner means.** A `ledgers` row written before the `owner` +/// column existed (schema migration 20260819000015) carries no owner, and this +/// header lets every authenticated principal through for it -- exactly the +/// behaviour that book had before the column was added. `CreateLedger` stamps +/// every book written from here on, so nothing produces a new unowned book. +/// The scenario corpus's fixture books are seeded by raw `INSERT` and are +/// unowned for this reason. +/// +/// **What it does not hide.** A principal that does not own book 7 learns that +/// book 7 exists, because the refusals are distinguishable (`NotFound` versus +/// `Forbidden`). That is `bank::db::loadOwned`'s own ordering and it is the +/// deliberate choice here too: book ids are dense and sequential, so existence +/// is not a secret this rung could keep, and collapsing the two refusals would +/// make a real "no such ledger" indistinguishable from a permissions problem +/// for the owner debugging it. + +namespace ledger::db { + +/// @brief Whether @p book may be reached by @p principal. +/// +/// True when the book records no owner (see this file's comment on NULL) or +/// records exactly @p principal. Byte comparison, not a case- or +/// whitespace-folding one: `LedgerAuthorizer::isValidPrincipal` already +/// restricts a principal to `[A-Za-z0-9._:-]`, and `RemoteServer` overwrites +/// `Context::principal` with the identity the token verified, so the two sides +/// of this comparison are the same bytes or they are different identities. +/// @param book The loaded `ledgers` row. +/// @param principal The caller's authenticated principal. +/// @return `true` if the caller may read and write this book. +[[nodiscard]] inline bool bookIsReachableBy(const LedgerRecord& book, std::string_view principal) noexcept { + const auto& owner = book.owner.Value(); + if (!owner.has_value()) { + return true; + } + return owner->ToStringView() == principal; +} + +/// @brief Loads the book @p ledgerId names, requiring it to exist and to be +/// reachable by @p principal. +/// +/// The guard every action carrying a `ledgerId` runs. @p action is woven into +/// both refusals so a client is told which action refused, matching the +/// `": no such ledger"` messages these call sites already threw. +/// @param mapper The data mapper to query through. +/// @param ledgerId The book's row id. +/// @param principal The caller's authenticated principal. +/// @param action The calling action's name, prefixed onto both refusals. +/// @return The book's row, so a caller needing it for a `BelongsTo` +/// assignment does not query twice. +/// @throws NotFound if no book has that id. +/// @throws Forbidden if the book belongs to a different principal. +[[nodiscard]] inline LedgerRecord requireOwnedBookById(Lightweight::DataMapper& mapper, std::int64_t ledgerId, + std::string_view principal, std::string_view action) { + auto rows = mapper.Query().Where(::Lightweight::FieldNameOf<&LedgerRecord::id>, "=", ledgerId).All(); + if (rows.empty()) { + throw NotFound{std::string{action} + ": no such ledger"}; + } + if (!bookIsReachableBy(rows.front(), principal)) { + throw Forbidden{std::string{action} + ": this book belongs to another principal"}; + } + return rows.front(); +} + +/// @brief `requireOwnedBookById` for a strong `LedgerId`. +/// @param mapper The data mapper to query through. +/// @param ledgerId The book's id, which must be engaged. +/// @param principal The caller's authenticated principal. +/// @param action The calling action's name, prefixed onto both refusals. +/// @return The book's row. +/// @throws NotFound if no book has that id. +/// @throws Forbidden if the book belongs to a different principal. +[[nodiscard]] inline LedgerRecord requireOwnedBook(Lightweight::DataMapper& mapper, const LedgerId& ledgerId, + std::string_view principal, std::string_view action) { + return requireOwnedBookById(mapper, *ledgerId, principal, action); +} + +/// @brief The same guard for an action that named a *child* row -- an account, +/// a budget, a rule, a report job -- rather than a book. +/// +/// Such an action has already loaded its own row and refused a missing one +/// with its own message; what is left is the book that row belongs to, whose +/// id comes off the child's `BelongsTo`. +/// +/// A child row whose book has vanished is *admitted*, not refused. This guard +/// answers "is this book someone else's?", and a book that does not exist is +/// nobody's -- there is no owner left to wrong, and refusing would turn an +/// orphaned row into a permissions error for the one caller who needs to see +/// it. `test_app.cpp`'s "A job whose ledger no longer exists settles Failed" +/// is exactly that case: the job settles terminally and its status must stay +/// readable, or a poller spins on it forever. +/// @param mapper The data mapper to query through. +/// @param ledgerId The child row's `ledger.Value()`. +/// @param principal The caller's authenticated principal. +/// @param action The calling action's name, prefixed onto the refusal. +/// @throws Forbidden if the book exists and belongs to a different principal. +inline void requireOwnedParentBook(Lightweight::DataMapper& mapper, std::uint64_t ledgerId, std::string_view principal, + std::string_view action) { + auto rows = mapper.Query() + .Where(::Lightweight::FieldNameOf<&LedgerRecord::id>, "=", static_cast(ledgerId)) + .All(); + if (!rows.empty() && !bookIsReachableBy(rows.front(), principal)) { + throw Forbidden{std::string{action} + ": this book belongs to another principal"}; + } +} + +/// @brief The caller's authenticated principal, or an empty view when no +/// session is in scope. +/// +/// An empty principal never matches an owner, so a read that carries no +/// principal at all is refused on an owned book and admitted on an unowned +/// one -- which is what the mutating actions' own `EmptyPrincipalError` gate +/// already achieves for writes, without this header needing to duplicate it. +/// @return The current principal, or `{}`. +[[nodiscard]] inline std::string_view currentPrincipal() noexcept { + const auto* ctx = ::morph::session::current(); + return ctx != nullptr ? std::string_view{ctx->principal} : std::string_view{}; +} + +} // namespace ledger::db diff --git a/examples/ledger/include/ledger/db/ledger_entity.hpp b/examples/ledger/include/ledger/db/ledger_entity.hpp index e64e2c9f..6b4ed9d7 100644 --- a/examples/ledger/include/ledger/db/ledger_entity.hpp +++ b/examples/ledger/include/ledger/db/ledger_entity.hpp @@ -24,6 +24,25 @@ struct LedgerRecord { static constexpr std::string_view TableName = "ledgers"; Light::Field id; // 0 Light::Field, Light::SqlRealName{"name"}> name; // 1 + // The principal that created this book (morph#382). Every action reaching + // this book compares it against `session::current()->principal`; see + // `ledger/db/book_access.hpp`, which is the single home for that rule. + // + // Nullable, and the nullability is load-bearing rather than incidental: + // `AddNotRequiredColumn` is the only way SQLite will add a column to a + // table that may already hold rows, and there is no principal to + // attribute those rows to. `std::nullopt` therefore means "created before + // ownership existed", the same reading `ReportJobRecord::paramsJson` + // already has, and such a book stays open to every authenticated + // principal exactly as it was. `CreateLedger` populates it on every book + // written from now on, so nothing produces a new NULL. + // + // Width matches `ledger::auth::kMaxPrincipalBytes`, the longest principal + // `Login` will mint a token for; `ledger_model.cpp` carries the + // `static_assert` tying the two together, because `SqlFixedString`'s + // constructor truncates rather than throwing and a silently shortened + // owner would be an owner nobody can match. + Light::Field>, Light::SqlRealName{"owner"}> owner; // 2 }; // Declared here, ahead of AccountRecord, rather than in its previous diff --git a/examples/ledger/include/ledger/dto/account_dto.hpp b/examples/ledger/include/ledger/dto/account_dto.hpp index 8e8cdf9d..29199dd0 100644 --- a/examples/ledger/include/ledger/dto/account_dto.hpp +++ b/examples/ledger/include/ledger/dto/account_dto.hpp @@ -35,14 +35,15 @@ inline constexpr std::size_t kMaxLedgerNameBytes = 128; /// (`EmptyPrincipalError`, design spec §11) exactly as every other mutating /// action on this model does, and `LedgerAuthorizer` already requires a /// validly signed token for everything but `AuthModel`/`Login`, so a -/// tokenless caller never reaches the model at all. Beyond that there is no -/// gate, and deliberately so: unlike `kanban::CreateProject` -- whose caller -/// becomes the new project's first `Manager` in the same transaction -- this -/// rung ships no per-book roles at all, no table to write one into, and no -/// action that reads one. Recording an owner here would be the first half of -/// an authorization scheme whose second half does not exist, which reads as a -/// guarantee the rung cannot keep. What this rung *does* enforce is -/// documented in `ledger/auth/ledger_authorizer.hpp`'s file comment. Filed as +/// tokenless caller never reaches the model at all. +/// +/// **What it records.** The caller becomes the book's owner +/// (`LedgerRecord::owner`), and every action that reaches this book afterwards +/// refuses any other principal -- `ledger/db/book_access.hpp` is the single +/// home for that comparison and the full rationale. That is a narrower +/// promise than `kanban::CreateProject`'s, which makes its caller the first +/// `Manager` of a real role table: this rung has no roles and no way to share +/// a book, so the owner is simply the one principal that may use it. /// morph#382. struct CreateLedger { std::string name; diff --git a/examples/ledger/src/db/schema.cpp b/examples/ledger/src/db/schema.cpp index 9fcfea57..2eba70c2 100644 --- a/examples/ledger/src/db/schema.cpp +++ b/examples/ledger/src/db/schema.cpp @@ -204,3 +204,25 @@ LIGHTWEIGHT_SQL_MIGRATION(20260819000014, "Store SubmitReport params with the jo // decodeMonthlyParams already treats as the all-time fallback. plan.AlterTable("ledger_report_jobs").AddNotRequiredColumn("params_json", NVarchar(0)); } + +LIGHTWEIGHT_SQL_MIGRATION(20260819000015, "Record which principal owns each book") { + // Per-book ownership (morph#382). Until this column existed the rung had + // no notion of whose book a `ledgers` row was: the signed-token check and + // the per-action empty-principal gate both held, and neither says *whose* + // book this is, so any authenticated principal could read, write and post + // into any book -- including one another principal had just created with + // `CreateLedger`. + // + // Nullable (AddNotRequiredColumn, not AddColumn) for the same reason + // 20260819000014's `params_json` is: SQLite cannot add a NOT NULL column + // without a default to a table that may already hold rows, and there is + // no principal to attribute a pre-existing book to. `NULL` therefore + // means "created before ownership existed" and such a book stays open to + // everyone, which is exactly the behaviour it had. `CreateLedger` stamps + // every book written from here on, so no new NULL is ever produced. + // + // Varchar(64) matches `ledger::auth::kMaxPrincipalBytes` and + // `LedgerRecord::owner`'s own capacity; `ledger_model.cpp` carries the + // static_assert tying them together. + plan.AlterTable("ledgers").AddNotRequiredColumn("owner", Varchar(64)); +} diff --git a/examples/ledger/src/models/budget_model.cpp b/examples/ledger/src/models/budget_model.cpp index fd982c75..01d7abe4 100644 --- a/examples/ledger/src/models/budget_model.cpp +++ b/examples/ledger/src/models/budget_model.cpp @@ -13,6 +13,7 @@ #include "clock.hpp" #include "ledger/core/errors.hpp" #include "ledger/core/money.hpp" +#include "ledger/db/book_access.hpp" #include "ledger/db/ledger_entity.hpp" namespace ledger { @@ -133,14 +134,9 @@ CategoryId BudgetModel::execute(const CreateCategory& action) { throw ValidationError{"CreateCategory: ledgerId and name are required"}; } Lightweight::DataMapper mapper; - auto ledgerRows = mapper.Query() - .Where(::Lightweight::FieldNameOf<&db::LedgerRecord::id>, "=", *action.ledgerId) - .All(); - if (ledgerRows.empty()) { - throw NotFound{"CreateCategory: no such ledger"}; - } + const auto ledgerRow = db::requireOwnedBook(mapper, action.ledgerId, ctx->principal, "CreateCategory"); db::CategoryRecord categoryRow; - categoryRow.ledger = ledgerRows.front(); + categoryRow.ledger = ledgerRow; categoryRow.name = action.name; mapper.Create(categoryRow); auto result = CategoryId{static_cast(categoryRow.id.Value())}; @@ -171,6 +167,16 @@ AccountId BudgetModel::execute(const LinkAccountToCategory& action) { if (accountRows.empty() || categoryRows.empty()) { throw NotFound{"LinkAccountToCategory: no such account or category"}; } + // Both sides, because this action names two rows and nothing else + // constrains them to the same book. What this refuses is a link across + // an *ownership* boundary -- someone else's account under your + // category, or yours under someone else's (morph#382). Two books the + // same principal owns, or two unowned ones, can still be cross-linked; + // that is a separate integrity gap this gate does not claim to close. + db::requireOwnedParentBook(mapper, accountRows.front().ledger.Value(), ctx->principal, + "LinkAccountToCategory"); + db::requireOwnedParentBook(mapper, categoryRows.front().ledger.Value(), ctx->principal, + "LinkAccountToCategory"); accountRows.front().category = categoryRows.front(); mapper.Update(accountRows.front()); auto result = AccountId{static_cast(accountRows.front().id.Value())}; @@ -201,6 +207,14 @@ BudgetId BudgetModel::execute(const CreateBudget& action) { if (ledgerRows.empty() || categoryRows.empty()) { throw NotFound{"CreateBudget: no such ledger or category"}; } + // The named book, and the category's own book -- a budget joins the + // two, so owning one of them is not enough (morph#382). The existence + // check above keeps its combined message; ownership is a separate + // refusal. + if (!db::bookIsReachableBy(ledgerRows.front(), ctx->principal)) { + throw Forbidden{"CreateBudget: this book belongs to another principal"}; + } + db::requireOwnedParentBook(mapper, categoryRows.front().ledger.Value(), ctx->principal, "CreateBudget"); db::BudgetRecord budgetRow; budgetRow.ledger = ledgerRows.front(); budgetRow.name = action.name; @@ -231,6 +245,7 @@ BudgetId BudgetModel::execute(const SetBudgetLimit& action) { if (budgetRows.empty()) { throw NotFound{"SetBudgetLimit: no such budget"}; } + db::requireOwnedParentBook(mapper, budgetRows.front().ledger.Value(), ctx->principal, "SetBudgetLimit"); // Onto the limit currency's own scale before it is stored, the same way // LedgerModel restates a transaction leg onto its account currency's -- // a limit is compared against a sum of legs, and two values only compare @@ -268,6 +283,10 @@ GetBudgetReportResult BudgetModel::execute(const GetBudgetReport& action) { if (budgetRows.empty()) { throw NotFound{"GetBudgetReport: no such budget"}; } + // A pure read, gated the same way `LedgerModel::execute(GetLedger)` is: + // spent-so-far against a budget is the book's activity, and it was + // readable by every authenticated principal (morph#382). + db::requireOwnedParentBook(mapper, budgetRows.front().ledger.Value(), db::currentPrincipal(), "GetBudgetReport"); auto limitRows = mapper.Query() .Where(::Lightweight::FieldNameOf<&db::BudgetLimitRecord::budget>, "=", *action.budgetId) .Where(::Lightweight::FieldNameOf<&db::BudgetLimitRecord::month>, "=", action.month) diff --git a/examples/ledger/src/models/ledger_model.cpp b/examples/ledger/src/models/ledger_model.cpp index 61d4e1f7..895bb9f8 100644 --- a/examples/ledger/src/models/ledger_model.cpp +++ b/examples/ledger/src/models/ledger_model.cpp @@ -22,10 +22,12 @@ #include #include "clock.hpp" +#include "ledger/auth/ledger_authorizer.hpp" #include "ledger/core/errors.hpp" #include "ledger/core/money.hpp" #include "ledger/core/time_util.hpp" #include "ledger/core/units.hpp" +#include "ledger/db/book_access.hpp" #include "ledger/db/ledger_entity.hpp" namespace ledger { @@ -37,6 +39,12 @@ static_assert(decltype(db::LedgerRecord::name)::ValueType{}.capacity() == kMaxLe "throwing), so the caller is told 'ok' about a book stored under a name they never sent. Same guard, " "same reason, as kanban's own kMaxProjectNameBytes assertion in src/models/board_model.cpp."); +static_assert(decltype(db::LedgerRecord::owner)::ValueType::value_type{}.capacity() == auth::kMaxPrincipalBytes, + "ledger::db::LedgerRecord::owner must be exactly as wide as the longest principal Login will mint a " + "token for (ledger::auth::kMaxPrincipalBytes) -- Light::SqlFixedString truncates rather than throwing, " + "so a narrower column would store a shortened owner that the very principal who created the book can " + "never match, locking them out of it (morph#382)."); + namespace { /// @brief Sums every leg posted against @p accountId into a single @@ -637,6 +645,10 @@ CreateLedgerResult LedgerModel::execute(const CreateLedger& action) { Lightweight::DataMapper mapper; db::LedgerRecord ledgerRow; ledgerRow.name = Light::SqlAnsiString<128>{action.name}; + // The caller owns what it creates (morph#382). This is the only place + // an owner is ever written: every other action reads it, and a book + // whose owner is NULL is one written before this column existed. + ledgerRow.owner = Light::SqlAnsiString<64>{ctx->principal}; mapper.Create(ledgerRow); auto result = CreateLedgerResult{.id = LedgerId{static_cast(ledgerRow.id.Value())}}; logAction(action, result); @@ -663,14 +675,9 @@ AccountInfo LedgerModel::execute(const OpenAccount& action) { // BelongsTo assignment needs the real persisted parent (per // polls::db::OptionRecord's own `opt.poll = poll;` usage, where `poll` // is a row that has actually round-tripped through Create/Query). - auto ledgerRows = mapper.Query() - .Where(::Lightweight::FieldNameOf<&db::LedgerRecord::id>, "=", *action.ledgerId) - .All(); - if (ledgerRows.empty()) { - throw NotFound{"OpenAccount: no such ledger"}; - } + const auto ledgerRow = db::requireOwnedBook(mapper, action.ledgerId, ctx->principal, "OpenAccount"); db::AccountRecord accountRow; - accountRow.ledger = ledgerRows.front(); + accountRow.ledger = ledgerRow; accountRow.name = action.name; accountRow.kind = static_cast(action.kind); accountRow.currencyCode = currencyToCode(action.currency); @@ -708,6 +715,14 @@ GetLedgerResult LedgerModel::execute(const GetLedger& action) { throw ValidationError{"GetLedger: ledgerId is required"}; } Lightweight::DataMapper mapper; + // A read is where the gap was widest: this action had no principal check + // of any kind, so a second authenticated client could ask for -- and get + // -- every account and balance in a book it had nothing to do with + // (morph#382). It carries no EmptyPrincipalError gate even now, because + // it does not need one: an empty principal never matches a recorded + // owner, so it is refused here and admitted only for an unowned book, + // which is exactly what it could always reach. + static_cast(db::requireOwnedBook(mapper, action.ledgerId, db::currentPrincipal(), "GetLedger")); // Real balance per account: the sum of every leg posted against it, // computed in-model via Rational::operator+ (never a raw SQL SUM() -- // see sumAccountLegs's own doc comment), via the shared buildLedgerState @@ -727,6 +742,13 @@ GetLedgerResult LedgerModel::execute(const StoreTransaction& action) { "StoreTransaction: description and at least two legs with engaged accountIds are required"}; } Lightweight::DataMapper mapper; + // Before the opId replay below, and before any account lookup: both + // answer questions about the book. A replay hit returns the stored + // `GetLedgerResult` -- every account and balance in the book -- and the + // account lookups are a "does account N belong to book B" oracle, so a + // gate placed after either would hand a non-owner exactly what the + // `GetLedger` gate exists to withhold (morph#382). + const auto ledgerRow = db::requireOwnedBook(mapper, action.ledgerId, ctx->principal, "StoreTransaction"); // Task 11b, design spec §1 (kanban's execute(MoveTaskPosition) pattern, // ladder-kanban-impl:examples/kanban/src/models/board_model.cpp): @@ -792,13 +814,7 @@ GetLedgerResult LedgerModel::execute(const StoreTransaction& action) { // ImportedOpRecord::appliedAtMs/ReportJobRecord::createdAtMs in later // tasks, not a client-supplied journal date). journalRow.date = action.date.value.has_value() ? (*action.date.value).value.time_since_epoch().count() : 0; - auto ledgerRows = mapper.Query() - .Where(::Lightweight::FieldNameOf<&db::LedgerRecord::id>, "=", *action.ledgerId) - .All(); - if (ledgerRows.empty()) { - throw NotFound{"StoreTransaction: no such ledger"}; - } - journalRow.ledger = ledgerRows.front(); + journalRow.ledger = ledgerRow; mapper.Create(journalRow); for (std::size_t i = 0; i < action.legs.size(); ++i) { @@ -845,7 +861,7 @@ GetLedgerResult LedgerModel::execute(const StoreTransaction& action) { throw LedgerError{"StoreTransaction: failed to serialize result for the applied-ops ledger"}; } db::AppliedOpRecord op; - op.ledger = ledgerRows.front(); + op.ledger = ledgerRow; op.opId = *action.opId; op.resultJson = resultJson; op.createdAtMs = (*morph::ladder::now().value).value.time_since_epoch().count(); @@ -972,6 +988,12 @@ GetLedgerResult LedgerModel::execute(const UndoTransaction& action) { if (originalJournalRow.ledger.Value() != static_cast(*action.ledgerId)) { throw NotFound{"UndoTransaction: journal does not belong to this ledger"}; } + // The book gate runs after the journal is resolved, not before it, so + // the two "no such journal" refusals this action already had keep + // their exact wording and order (morph#382). The journal names its own + // ledger and that has just been verified against the action's, so + // gating on it is gating on the book the action really reaches. + db::requireOwnedParentBook(mapper, originalJournalRow.ledger.Value(), ctx->principal, "UndoTransaction"); // A compensating entry names the entry it reverses, so "has this already // been reversed?" is a query rather than mutable state on the original -- @@ -1091,12 +1113,7 @@ ImportResult LedgerModel::execute(const ImportLedgerChunk& action) { // complete, zero-sum journal entry by `storeJournalImpl` -- see that // method's own doc comment), it just does not roll the whole chunk back // to empty. - auto ledgerRows = mapper.Query() - .Where(::Lightweight::FieldNameOf<&db::LedgerRecord::id>, "=", *action.ledgerId) - .All(); - if (ledgerRows.empty()) { - throw NotFound{"ImportLedgerChunk: no such ledger"}; - } + const auto ledgerRow = db::requireOwnedBook(mapper, action.ledgerId, ctx->principal, "ImportLedgerChunk"); // Scoped to the chunk's own ledger, like every row's account below: a // counter account from another book would otherwise take one leg of @@ -1164,7 +1181,7 @@ ImportResult LedgerModel::execute(const ImportLedgerChunk& action) { morph::time::Timestamp{*parsedDate}, legs, legAccounts); db::ImportedTxnHashRecord hashRow; - hashRow.ledger = ledgerRows.front(); + hashRow.ledger = ledgerRow; hashRow.hash = hash; mapper.Create(hashRow); @@ -1213,15 +1230,10 @@ ReportJobId LedgerModel::execute(const SubmitReport& action) { throw ValidationError{"SubmitReport: ledgerId is required"}; } Lightweight::DataMapper mapper; - auto ledgerRows = mapper.Query() - .Where(::Lightweight::FieldNameOf<&db::LedgerRecord::id>, "=", *action.ledgerId) - .All(); - if (ledgerRows.empty()) { - throw NotFound{"SubmitReport: no such ledger"}; - } + const auto ledgerRow = db::requireOwnedBook(mapper, action.ledgerId, ctx->principal, "SubmitReport"); db::ReportJobRecord jobRow; - jobRow.ledger = ledgerRows.front(); + jobRow.ledger = ledgerRow; jobRow.kind = static_cast(action.kind); jobRow.status = static_cast(ReportStatus::Pending); jobRow.createdAtMs = (*morph::ladder::now().value).value.time_since_epoch().count(); @@ -1422,6 +1434,12 @@ GetReportStatusResult LedgerModel::execute(const GetReportStatus& action) { throw NotFound{"GetReportStatus: no such job"}; } const auto& row = jobRows.front(); + // A job id carries no ledgerId of its own (morph#371), so the book this + // read reaches is the one the job row names. Gated after the job lookup so + // "no such job" keeps its wording, and like `execute(GetLedger)` this pure + // read needs no separate empty-principal gate: an empty principal matches + // no recorded owner. + db::requireOwnedParentBook(mapper, row.ledger.Value(), db::currentPrincipal(), "GetReportStatus"); return GetReportStatusResult{ .status = static_cast(row.status.Value()), .result = row.resultJson.Value().has_value() @@ -1458,6 +1476,20 @@ void LedgerModel::setCategoryImpl(Lightweight::DataMapper& mapper, const SetCate if (accountRows.empty() || categoryRows.empty()) { throw NotFound{"SetCategory: no such account or category"}; } + // Both call sites reach this: the public `execute(SetCategory)` overload, + // where it is the only book gate the action gets, and the rule cascade + // inside `execute(StoreTransaction)`, where the caller has already passed + // the same gate on the same book and this one passes too (morph#382). + // + // Both *rows*, too, the way `BudgetModel::execute(LinkAccountToCategory)` + // checks both of its: this action joins two rows nothing else constrains + // to one book. Gating only the account would let a caller file its own + // account under another principal's category, and `GetBudgetReport` + // selects legs by exactly that link -- so every entry posted against the + // account would land in the other principal's budget report. + const auto principal = db::currentPrincipal(); + db::requireOwnedParentBook(mapper, accountRows.front().ledger.Value(), principal, "SetCategory"); + db::requireOwnedParentBook(mapper, categoryRows.front().ledger.Value(), principal, "SetCategory"); accountRows.front().category = categoryRows.front(); mapper.Update(accountRows.front()); } diff --git a/examples/ledger/src/models/rule_model.cpp b/examples/ledger/src/models/rule_model.cpp index 4dfc8a45..c81dd03d 100644 --- a/examples/ledger/src/models/rule_model.cpp +++ b/examples/ledger/src/models/rule_model.cpp @@ -8,6 +8,7 @@ #include "clock.hpp" #include "ledger/core/errors.hpp" +#include "ledger/db/book_access.hpp" #include "ledger/db/ledger_entity.hpp" namespace ledger { @@ -79,14 +80,9 @@ RuleId RuleModel::execute(const CreateRule& action) { throw ValidationError{"CreateRule: ledgerId and matchText are required"}; } Lightweight::DataMapper mapper; - auto ledgerRows = mapper.Query() - .Where(::Lightweight::FieldNameOf<&db::LedgerRecord::id>, "=", *action.ledgerId) - .All(); - if (ledgerRows.empty()) { - throw NotFound{"CreateRule: no such ledger"}; - } + const auto ledgerRow = db::requireOwnedBook(mapper, action.ledgerId, ctx->principal, "CreateRule"); db::RuleRecord ruleRow; - ruleRow.ledger = ledgerRows.front(); + ruleRow.ledger = ledgerRow; ruleRow.trigger = static_cast(action.trigger); ruleRow.matchText = action.matchText; ruleRow.action = static_cast(action.action); @@ -119,6 +115,7 @@ RuleInfo RuleModel::execute(const UpdateRule& action) { throw NotFound{"UpdateRule: no such rule"}; } auto& ruleRow = ruleRows.front(); + db::requireOwnedParentBook(mapper, ruleRow.ledger.Value(), ctx->principal, "UpdateRule"); // Optimistic concurrency (design spec §10, Scenario B). An engaged // expectedVersion that no longer matches means the row moved on after the // client read it: refuse outright rather than overwrite the change that diff --git a/examples/ledger/tests/test_ledger_book_ownership.cpp b/examples/ledger/tests/test_ledger_book_ownership.cpp new file mode 100644 index 00000000..908bb7a6 --- /dev/null +++ b/examples/ledger/tests/test_ledger_book_ownership.cpp @@ -0,0 +1,360 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Per-book authorization (morph#382). +// +// Until this file existed, `ledger` had no per-book ownership of any kind: +// any principal holding a valid token could read, write and post into any +// book, including one another principal had just created over the wire with +// `CreateLedger`. The signed-token check and the per-action empty-principal +// gate both held, and neither of them says *whose* book this is. +// +// The rule these cases pin: `CreateLedger` records its caller as the book's +// owner, and every action that reaches a book -- through its `ledgerId`, or +// through an account, budget, rule or report job that belongs to it -- refuses +// a different principal with `Forbidden`. Ownership is expressed through the +// relation (`examples/IMPLEMENTATION.md` rule 4, bank's `loadOwned`), not at +// the authorizer: `LedgerModel`'s instances are keyed by `ledgerId` and shared +// across every client that opens the same book, so `authorizeInstance` has no +// single owning caller to compare against. +// +// The last case pins the migration's backfill decision, which is the one place +// the old behaviour survives: a `ledgers` row written before the `owner` column +// existed has no owner, and stays readable and writable by everyone, exactly as +// it was. Nothing creates such a row any more. + +#include +#include +#include +#include +#include +#include + +#include "ledger/core/errors.hpp" +#include "ledger/db/ledger_entity.hpp" +#include "ledger/models/budget_model.hpp" +#include "ledger/models/ledger_model.hpp" +#include "ledger/models/rule_model.hpp" +#include "testkit/db_fixture.hpp" + +namespace { + +/// @brief A `Context` carrying only @p principal -- see +/// `test_ledger_model.cpp`'s own identical `contextFor` for why this +/// is not a designated initializer. +[[nodiscard]] morph::session::Context contextFor(std::string principal) { + morph::session::Context ctx; + ctx.principal = std::move(principal); + return ctx; +} + +class ScopedPrincipal { +public: + explicit ScopedPrincipal(std::string principal) : _ctx{contextFor(std::move(principal))}, _scope{_ctx} {} + +private: + morph::session::Context _ctx; + morph::session::detail::ScopedContext _scope; +}; + +/// @brief A book with no recorded owner -- the shape every `ledgers` row had +/// before morph#382's migration, written the only way one can still be +/// written now that `CreateLedger` always stamps its caller. +[[nodiscard]] ledger::LedgerId unownedBook(Lightweight::DataMapper& mapper, const std::string& name) { + ledger::db::LedgerRecord row; + row.name = Light::SqlAnsiString<128>{name}; + mapper.Create(row); + return ledger::LedgerId{static_cast(row.id.Value())}; +} + +} // namespace + +TEST_CASE("A book records the principal that created it", "[ledger][ownership]") { + morph::ladder::testkit::DbFixture fixture; + Lightweight::DataMapper mapper; + + ledger::LedgerModel model; + ledger::LedgerId book; + { + const ScopedPrincipal alice{"alice"}; + book = model.execute(ledger::CreateLedger{.name = "Alice's private book"}).id; + } + REQUIRE(book.hasValue()); + + auto rows = mapper.Query() + .Where(::Lightweight::FieldNameOf<&ledger::db::LedgerRecord::id>, "=", *book) + .All(); + REQUIRE(rows.size() == 1); + REQUIRE(rows.front().owner.Value().has_value()); + CHECK(std::string{rows.front().owner.Value()->ToStringView()} == "alice"); +} + +TEST_CASE("A second principal can neither read nor write a book it does not own", "[ledger][ownership]") { + morph::ladder::testkit::DbFixture fixture; + + ledger::LedgerModel model; + ledger::LedgerId book; + ledger::AccountId cash; + ledger::AccountId spend; + { + const ScopedPrincipal alice{"alice"}; + book = model.execute(ledger::CreateLedger{.name = "Alice's private book"}).id; + cash = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Alice Cash", + .kind = ledger::AccountKind::Asset, + .currency = ledger::Currency::USD}) + .id; + spend = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Alice Spend", + .kind = ledger::AccountKind::Expense, + .currency = ledger::Currency::USD}) + .id; + } + + const ScopedPrincipal bob{"bob"}; + + // The read half. `GetLedger` had no principal check at all, so this is + // where a second client learned the whole book. + CHECK_THROWS_AS(model.execute(ledger::GetLedger{.ledgerId = book}), ledger::Forbidden); + + // The write half, in the two shapes the reproduction on morph#382 used. + CHECK_THROWS_AS(model.execute(ledger::OpenAccount{.ledgerId = book, + .name = "Bob's account in Alice's book", + .kind = ledger::AccountKind::Liability, + .currency = ledger::Currency::USD}), + ledger::Forbidden); + CHECK_THROWS_AS( + model.execute(ledger::StoreTransaction{ + .ledgerId = book, + .description = "Bob posts into Alice's book", + .date = {}, + .legs = {{.accountId = cash, + .amount = morph::math::Rational{morph::math::Numerator{-100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}, + {.accountId = spend, + .amount = morph::math::Rational{morph::math::Numerator{100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}}, + .opId = {}}), + ledger::Forbidden); + CHECK_THROWS_AS(model.execute(ledger::ImportLedgerChunk{.ledgerId = book, + .counterAccountId = spend, + .csvChunk = "date,description,account_id,amount\n", + .opId = ledger::ImportOpId{"bob-import"}}), + ledger::Forbidden); + CHECK_THROWS_AS(model.execute(ledger::SubmitReport{ + .ledgerId = book, .kind = ledger::ReportKind::MonthlyStatement, .params = "{}"}), + ledger::Forbidden); +} + +TEST_CASE("The owner is still free to work in its own book", "[ledger][ownership]") { + morph::ladder::testkit::DbFixture fixture; + + ledger::LedgerModel model; + const ScopedPrincipal alice{"alice"}; + + const auto book = model.execute(ledger::CreateLedger{.name = "Alice's private book"}).id; + const auto cash = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Alice Cash", + .kind = ledger::AccountKind::Asset, + .currency = ledger::Currency::USD}) + .id; + const auto spend = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Alice Spend", + .kind = ledger::AccountKind::Expense, + .currency = ledger::Currency::USD}) + .id; + + CHECK_NOTHROW(model.execute(ledger::GetLedger{.ledgerId = book})); + CHECK_NOTHROW(model.execute(ledger::StoreTransaction{ + .ledgerId = book, + .description = "groceries", + .date = {}, + .legs = {{.accountId = cash, + .amount = morph::math::Rational{morph::math::Numerator{-100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}, + {.accountId = spend, + .amount = morph::math::Rational{morph::math::Numerator{100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}}, + .opId = {}})); +} + +TEST_CASE("Replaying another principal's opId does not hand back its book", "[ledger][ownership]") { + // `StoreTransaction`'s exactly-once path answers a repeated `opId` with + // the stored `GetLedgerResult` -- every account and balance in the book. + // That return sits early in the action, so a book gate placed after it + // would leak exactly what the `GetLedger` gate withholds: guess the opId + // and read the whole book. The account lookups just below it are the same + // hazard in weaker form, a "does account N belong to book B" oracle. + morph::ladder::testkit::DbFixture fixture; + + ledger::LedgerModel model; + ledger::LedgerId book; + ledger::AccountId cash; + ledger::AccountId spend; + const ledger::ImportOpId opId{"tx-1"}; + { + const ScopedPrincipal alice{"alice"}; + book = model.execute(ledger::CreateLedger{.name = "Alice's private book"}).id; + cash = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Alice Cash", + .kind = ledger::AccountKind::Asset, + .currency = ledger::Currency::USD}) + .id; + spend = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Alice Spend", + .kind = ledger::AccountKind::Expense, + .currency = ledger::Currency::USD}) + .id; + model.execute(ledger::StoreTransaction{ + .ledgerId = book, + .description = "groceries", + .date = {}, + .legs = {{.accountId = cash, + .amount = morph::math::Rational{morph::math::Numerator{-100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}, + {.accountId = spend, + .amount = morph::math::Rational{morph::math::Numerator{100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}}, + .opId = opId}); + } + + const ScopedPrincipal bob{"bob"}; + CHECK_THROWS_AS( + model.execute(ledger::StoreTransaction{ + .ledgerId = book, + .description = "groceries", + .date = {}, + .legs = {{.accountId = cash, + .amount = morph::math::Rational{morph::math::Numerator{-100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}, + {.accountId = spend, + .amount = morph::math::Rational{morph::math::Numerator{100}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}}}, + .opId = opId}), + ledger::Forbidden); +} + +TEST_CASE("SetCategory refuses a category in another principal's book", "[ledger][ownership]") { + // The action joins two rows, and gating only the account would let Bob + // file his own account under Alice's category. `GetBudgetReport` selects + // legs by exactly that link, so every entry Bob posts would then be summed + // into Alice's budget report. + morph::ladder::testkit::DbFixture fixture; + + ledger::LedgerModel ledgerModel; + ledger::BudgetModel budgetModel; + + ledger::CategoryId aliceCategory; + { + const ScopedPrincipal alice{"alice"}; + const auto aliceBook = ledgerModel.execute(ledger::CreateLedger{.name = "Alice's private book"}).id; + aliceCategory = budgetModel.execute(ledger::CreateCategory{.ledgerId = aliceBook, .name = "Alice food"}); + } + + const ScopedPrincipal bob{"bob"}; + const auto bobBook = ledgerModel.execute(ledger::CreateLedger{.name = "Bob's own book"}).id; + const auto bobAccount = ledgerModel + .execute(ledger::OpenAccount{.ledgerId = bobBook, + .name = "Bob Spend", + .kind = ledger::AccountKind::Expense, + .currency = ledger::Currency::USD}) + .id; + + CHECK_THROWS_AS( + ledgerModel.execute(ledger::SetCategory{ + .accountId = bobAccount, .categoryId = aliceCategory, .ruleId = ledger::RuleId{}, .ruleVersion = 0}), + ledger::Forbidden); + CHECK_THROWS_AS( + budgetModel.execute(ledger::LinkAccountToCategory{.accountId = bobAccount, .categoryId = aliceCategory}), + ledger::Forbidden); +} + +TEST_CASE("BudgetModel and RuleModel refuse a book they do not own", "[ledger][ownership]") { + morph::ladder::testkit::DbFixture fixture; + + ledger::LedgerModel ledgerModel; + ledger::BudgetModel budgetModel; + ledger::RuleModel ruleModel; + + ledger::LedgerId book; + ledger::CategoryId category; + ledger::BudgetId budget; + ledger::RuleId rule; + { + const ScopedPrincipal alice{"alice"}; + book = ledgerModel.execute(ledger::CreateLedger{.name = "Alice's private book"}).id; + category = budgetModel.execute(ledger::CreateCategory{.ledgerId = book, .name = "Food"}); + budget = budgetModel.execute( + ledger::CreateBudget{.ledgerId = book, .name = "Monthly food", .categoryId = category}); + rule = ruleModel.execute(ledger::CreateRule{.ledgerId = book, + .trigger = ledger::RuleTrigger::DescriptionContains, + .matchText = "market", + .action = ledger::RuleAction::SetCategory, + .actionValue = std::to_string(*category)}); + } + + const ScopedPrincipal bob{"bob"}; + + CHECK_THROWS_AS(budgetModel.execute(ledger::CreateCategory{.ledgerId = book, .name = "Bob's category"}), + ledger::Forbidden); + CHECK_THROWS_AS( + budgetModel.execute(ledger::CreateBudget{.ledgerId = book, .name = "Bob's budget", .categoryId = category}), + ledger::Forbidden); + CHECK_THROWS_AS(budgetModel.execute(ledger::SetBudgetLimit{ + .budgetId = budget, + .month = "2026-08", + .limit = morph::math::Rational{morph::math::Numerator{10000}, morph::math::Denominator{1}, + morph::math::DecimalPlaces{2}}, + .currency = ledger::Currency::USD}), + ledger::Forbidden); + CHECK_THROWS_AS(budgetModel.execute(ledger::GetBudgetReport{.budgetId = budget, .month = "2026-08"}), + ledger::Forbidden); + CHECK_THROWS_AS(ruleModel.execute(ledger::CreateRule{.ledgerId = book, + .trigger = ledger::RuleTrigger::DescriptionContains, + .matchText = "bob", + .action = ledger::RuleAction::SetCategory, + .actionValue = std::to_string(*category)}), + ledger::Forbidden); + CHECK_THROWS_AS(ruleModel.execute(ledger::UpdateRule{.ruleId = rule, + .matchText = "bob", + .actionValue = std::to_string(*category), + .expectedVersion = std::nullopt}), + ledger::Forbidden); +} + +TEST_CASE("A book written before the owner column stays open to everyone", "[ledger][ownership]") { + // The migration's backfill decision, pinned. `AddNotRequiredColumn` cannot + // populate rows that already exist and there is no principal to attribute + // them to, so a NULL owner means "created before ownership existed" -- the + // same reading `params_json` already has on the report-job row -- and such + // a book behaves exactly as every book did before morph#382. The scenario + // corpus's fixture books are written this way. + morph::ladder::testkit::DbFixture fixture; + Lightweight::DataMapper mapper; + const auto book = unownedBook(mapper, "Scenario book"); + + ledger::LedgerModel model; + ledger::AccountId cash; + { + const ScopedPrincipal alice{"alice"}; + cash = model + .execute(ledger::OpenAccount{.ledgerId = book, + .name = "Shared cash", + .kind = ledger::AccountKind::Asset, + .currency = ledger::Currency::USD}) + .id; + } + + const ScopedPrincipal bob{"bob"}; + CHECK_NOTHROW(model.execute(ledger::GetLedger{.ledgerId = book})); + CHECK_NOTHROW(model.execute(ledger::OpenAccount{.ledgerId = book, + .name = "Also shared", + .kind = ledger::AccountKind::Expense, + .currency = ledger::Currency::USD})); + CHECK(cash.hasValue()); +} diff --git a/examples/pastebin/README.md b/examples/pastebin/README.md index aa400227..05ced8a7 100644 --- a/examples/pastebin/README.md +++ b/examples/pastebin/README.md @@ -103,7 +103,7 @@ must both work unchanged. `Loggable::Yes`), not split.** The recommended split (a pure, unlogged `GetPaste` plus an internally-journaled `RecordRead` mutation) turned out to be structurally unavailable: `IModelHolder::recordIfAttached` - (`include/morph/core/model.hpp:145`) is called only by the two + (`include/morph/core/model.hpp:221`, its definition) is called only by the two built-in dispatch runners, for the one action actually dispatched — there is no seam for a model to author a second, independent `LogEntry` from inside its own `execute()`. `Bridge::modelFactory` diff --git a/scripts/scenario/scenarios/kanban/a-board-must-be-opened-before-it-answers.scenario b/scripts/scenario/scenarios/kanban/a-board-must-be-opened-before-it-answers.scenario index 3b3c92d3..6b86d0bf 100644 --- a/scripts/scenario/scenarios/kanban/a-board-must-be-opened-before-it-answers.scenario +++ b/scripts/scenario/scenarios/kanban/a-board-must-be-opened-before-it-answers.scenario @@ -5,25 +5,25 @@ # attached to no board at all. Every other action needs one, and this file # drives each of them in that state. # -# ── What it answers, which is not what it should ─────────────────────── -# **Every** action answers with the bare text `stoull`. +# ── What it answers ──────────────────────────────────────────────────── +# Every action names itself and names `OpenBoard` -- ": handler was +# never attached via OpenBoard", the shape `polls::PollModel` answers with and +# a message a client can act on: it says which call was missed. # -# Two of them look like they should not. `GetActivity` and `GetRules` each -# open with a `_projectIdStr.has_value()` check whose message is exactly the -# right one -- "handler was never attached via OpenBoard", the shape -# `polls::PollModel` uses and the message a client could act on. That check -# never fires: on an unattached handler the optional is *engaged, with an -# empty string*, so `has_value()` is true and control falls through to -# `requireRole`, which reaches `std::stoull("")` and throws -# `std::invalid_argument`. Its `what()` is the string `stoull`, and that is -# what goes on the wire, as though it were a domain error. +# It did not always. Every action used to answer the bare text `stoull` -- the +# `what()` of the `std::invalid_argument` `std::stoull("")` throws -- even +# though all fifteen `execute()` overloads already opened with a +# `_projectIdStr.has_value()` guard carrying the correct message. The optional +# was *engaged with an empty string* on an unattached handler, so `has_value()` +# was true, every guard fell through to `requireRole`, and `std::stoull("")` +# threw. `_projectIdStr` was written by `attachActionLog` as well as by +# `OpenBoard`, and `ModelFactory::create` calls it with an empty `entityKey` on +# every newly constructed holder. Fixed in morph#368 by declining the empty +# key: an empty string identifies no project, so it no longer poses as one. # -# So the guard is written, is correct, and is dead -- which is worse than not -# having one, because reading the source suggests the good message exists. -# -# Asserted here as it behaves, per action, because that is what a client -# receives today and because fixing it then has to come through this file. -# Filed as morph#368. +# Asserted here per action because this is the wire surface a client sees, and +# because the in-process tests all attach before acting -- the two writers of +# `_projectIdStr` only ever disagreed when they did not. # # Re-runnable: this file creates one project and never touches anyone else's. @@ -43,35 +43,68 @@ client fresh model=BoardModel session principal=$who token=$token do GetBoardState -expect err message == "stoull" +expect err message == "GetBoardState: handler was never attached via OpenBoard" do CreateColumn name="Todo" wipLimit=0 -expect err message == "stoull" +expect err message == "CreateColumn: handler was never attached via OpenBoard" do CreateSwimlane name="Lane" -expect err message == "stoull" +expect err message == "CreateSwimlane: handler was never attached via OpenBoard" do CreateTask columnId=1 swimlaneId=1 title="Nowhere" -expect err message == "stoull" +expect err message == "CreateTask: handler was never attached via OpenBoard" do MoveTaskPosition taskId=1 columnId=1 swimlaneId=1 position=0 -expect err message == "stoull" +expect err message == "MoveTaskPosition: handler was never attached via OpenBoard" do AddComment taskId=1 body="Into the void" -expect err message == "stoull" +expect err message == "AddComment: handler was never attached via OpenBoard" do GetEventsSince lastEventId=0 -expect err message == "stoull" +expect err message == "GetEventsSince: handler was never attached via OpenBoard" -# ...including the two whose own has_value() guard has the right message and -# never reaches it. +# ...including the two morph#368 singled out, whose guard was written, was +# correct, and was the last thing on this rung anyone expected to be dead. do GetActivity -expect err message == "stoull" -expect err message !~ "OpenBoard" +expect err message == "GetActivity: handler was never attached via OpenBoard" +expect err message ~ "OpenBoard" do GetRules projectId=$project -expect err message == "stoull" -expect err message !~ "OpenBoard" +expect err message == "GetRules: handler was never attached via OpenBoard" +expect err message ~ "OpenBoard" + +# Nothing on this surface answers `stoull` any more -- an id-parsing detail +# that named neither the action nor the mistake, and that a client could not +# tell apart from an internal fault. +do GetBoardState +expect err message !~ "stoull" + +# ── ...and neither does a contextKey that names no project ───────────── +# The other way to reach an unattached handler: register *with* a contextKey. +# `Remote::attachLogIfConfigured` forwards it to the model verbatim off the +# wire, so it is arbitrary text. `foo` used to be adopted as the board id and +# answer `stoull` from `std::stoull("foo")` -- the same defect one step over +# from the empty key, and the reason morph#368's guard checks that the key +# parses whole rather than merely that it is non-empty. +client garbage model=BoardModel contextKey=foo +session principal=$who token=$token + +do GetBoardState +expect err message == "GetBoardState: handler was never attached via OpenBoard" + +do GetActivity +expect err message == "GetActivity: handler was never attached via OpenBoard" + +# A key whose *prefix* is a real project id is the dangerous one: `stoull` +# stops at the first non-digit and reports success, so this used to attach the +# handler to project $project -- a board this client never named. +client prefixed model=BoardModel contextKey=${project}x +session principal=$who token=$token + +do GetBoardState +expect err message == "GetBoardState: handler was never attached via OpenBoard" + +use fresh # ── Validation still runs first ──────────────────────────────────────── # A malformed action is refused for being malformed, not for the handler being diff --git a/scripts/scenario/scenarios/kanban/sign-in-create-project-open-board.scenario b/scripts/scenario/scenarios/kanban/sign-in-create-project-open-board.scenario index 29bb6682..a29ffe97 100644 --- a/scripts/scenario/scenarios/kanban/sign-in-create-project-open-board.scenario +++ b/scripts/scenario/scenarios/kanban/sign-in-create-project-open-board.scenario @@ -16,9 +16,10 @@ # * `BoardModel` is keyed by project id and shared, so a freshly registered # handler is attached to no board. It has to be pointed at one with # `OpenBoard` before any other action will answer -- and a handler that -# was never opened fails with the raw text `stoull` rather than a typed -# message, which is asserted in -# a-board-must-be-opened-before-it-answers.scenario; +# was never opened is refused by name, ": handler was never +# attached via OpenBoard", which is asserted per action in +# a-board-must-be-opened-before-it-answers.scenario (it answered the raw +# text `stoull` until morph#368); # * `AuthModel`/`Login` is the one model/action pair a tokenless caller may # reach (`KanbanAuthorizer::kAnonymousModelType`). Everything else needs # the token it mints, installed with a `session` step -- `client`'s own