Commit Graph
3 Commits
Author SHA1 Message Date
jaap-jan 4b706bc3c3 Say when a vault has moved, so nobody waits out the minute
The delta pull was cheap enough to run on a timer and the client did, once a
minute. That is fine for a machine and wrong for two people: an edit a colleague
makes is up to a minute stale, which is long enough for both of them to make it
and produce a conflict neither needed to have. Shortening the interval is the
obvious answer and the wrong one — it costs a request per client per interval
whether or not anything happened, and it converges on a busier server that is
still late.

So the server now says so. A client holds a WebSocket open at GET /api/v1/events,
subprotocol dodossh.events.v1, and gets a line down it when something it can read
has changed. ADR 0012 has the reasoning; three parts of it are worth repeating
here, because they are what everything else rests on.

**What crosses the socket is a notice, never data.** A frame names a vault and
how far its change log has got. No item, no ciphertext, not even which item it
was. The client's answer is the delta pull it would have run anyway, so there is
still exactly one code path that applies a change to a keychain, and it is not
this one. Pushing the items themselves would save a round trip and fork that path
in two, with the cursor, the merge and the tombstone rules duplicated across both
— ADR 0003 put every mutation through one write path for that reason, and this
keeps every read on one for the same one. It also makes a dropped notice
harmless, which is what lets the fan-out below be as simple as it is.

**Polling stays, and is what guarantees a pass.** The minute timer is unchanged.
A network that eats WebSockets, a server with Events:Enabled off, an older
server, a proxy that will not upgrade, a notice dropped under backpressure —
every one of those leaves a client behaving exactly as it did before this commit.
Nothing is reachable only over the socket and nothing is meant to become so;
VaultViewModel's AutoSyncInterval remark now says that where somebody changing it
will read it.

**The bearer token authorises the upgrade, unlike the relay's ticket.** Not an
inconsistency with ADR 0004: the relay's socket is a byte pipe whose whole
authorization decision — which host, which IPs, which port — is made before it
opens and never revisited, and it is the extraction seam for a process that must
hold no ACL code. This one is a view of the caller's own vault list and has to
keep answering "what may this account read" for as long as it is held. A ticket
would carry that answer in a token and be wrong the moment the account's access
changed. The two bounds that arrangement needs are met rather than waved at: the
socket is closed at the token's exp with close code 4401 and the client comes
straight back with a fresh one, and the vault set is re-resolved every few
minutes as well as on the changes known to affect it. Both bound *metadata*,
because a notice contains nothing else and reading a vault still needs a key this
server has never held.

**The fan-out.** VaultEventHub is a singleton holding the sockets this node
accepted; publishing walks them and asks each whether it cares, rather than
keeping a vault-to-subscriber index that every re-subscription would have to move
entries between under a lock publishing also takes. At a few hundred sockets per
node and an event rate bounded by how often people edit keychains, the walk is
not measurable and its races are obvious. Per-connection queues are bounded and
drop the *oldest*: a notice means "pull vault X, which is at least at sequence
N", so the newest subsumes what it displaces and the client's answer is identical
either way — which is what lets the publish path be void, never block, and never
fail.

Announced from the endpoint rather than from SyncService, and that placement is
the point: by then the push has committed and released the per-vault advisory
lock. From inside it would name a sequence no reader can see yet and would hold
the lock that serialises writers across a socket write. Only the highest
*applied* sequence, so a batch of pure conflicts announces nothing, and a
duplicate — already announced when it first landed — announces nothing either.

Grants and membership publish too, and those take the *recipient* rather than the
actor. This is what AdmitNewVaultsAsync has been apologising for since sharing
shipped — "the recipient is handed nothing, there is no push channel" — and the
README with it. A vault shared with somebody now turns up as it is shared. The
comment and the README paragraph both say what is true now, and both keep saying
that the pass is what *discovers* the vault, because a client with no socket has
to arrive at the same place.

**On the client**, VaultEventStream is really a reconnection policy wrapped round
a ClientWebSocket: a dropped socket is the ordinary case here — laptops sleep,
proxies time out, tokens expire, servers are redeployed — so nothing in it treats
a failure as exceptional, and every path ends in "wait, then dial again". A
connection that lived long enough to say hello resets the backoff, so a laptop
that woke, worked, and lost its network an hour later does not inherit a
minute-long wait it has already proved it need not take. A 4401 close skips the
backoff entirely and asks the token provider again, which is the whole reason
that close code is distinct. A server that does not advertise the events feature
gets IdleVaultEventStream, which never delivers — so IVaultServer.Events is never
null and every caller stays on one shape, because the correct behaviour without a
socket is the behaviour with a silent one.

The shell's background loop now selects between the timer and a notice, and both
waits are held across iterations. That is load-bearing rather than tidy:
PeriodicTimer permits one outstanding WaitForNextTickAsync and throws on a
second, and an abandoned channel read stays registered and consumes the next
notice written. Either defect leaves the first notice working and every one after
it silently lost, which is why NoticesKeepWakingTheLoop_NotJustTheFirst pushes
three and not one. Notices are coalesced over a quarter of a second, so one
person's save — a host and its log entry are two items — and a colleague clearing
a folder each cost one pass rather than a dozen.

**The kind is a string, not an enum**, and that is a compatibility decision.
UseStringEnumConverter throws on a value it does not know, so a newer server
sending a kind an older client had never heard of would not add an unreadable
frame — it would break that client's socket outright. A string is ignored
instead. ProblemCodes is the same shape for the same reason.

**Tested on both sides, through the real pipeline.** The endpoint suite opens a
genuine socket against TestServer and proves a push produces a notice, that
another account's push does not reach it, that a ping is answered, and that a
frame this server cannot parse does not end the connection. Two of those assert
on *ordering* rather than on absence within a timeout — the stranger's write goes
first, so a socket that leaked would have announced it before the one the test
waits for — because "nothing arrived in two seconds" is a test that passes on a
slow machine for the wrong reason. And ANoticeCarriesNoCiphertext asserts on the
bytes that crossed the wire rather than on the record's fields, since the latter
would only prove that this type has no payload member, which is a tautology; the
former is what catches a field added later without anybody thinking about
disclosure.

The client suite drives VaultEventStream through an injected connector, because
the one thing a test cannot do to a real network is make it fail on cue — and
failure is the entire subject. The shell suite proves a notice produces a pull
inside ten seconds against a sixty-second timer, so the timer cannot be what
caused it.

**Two limits, stated rather than left to be discovered.** Fan-out is in-process,
so a deployment running more than one API replica only pushes for writes its own
replica handled and the rest arrive on the timer. IVaultEventPublisher is the
seam a PostgreSQL LISTEN/NOTIFY backplane implements and it is deliberately not
implemented: an untested backplane is worse than a documented gap, and multiple
replicas degrade to the behaviour before this commit rather than breaking. And a
client is notified of its own writes; it pushed, so it already pulled, and the
extra pass finds nothing. Suppressing that echo correctly needs a per-device
identity on the socket, and the same user's other machines must still be told.

Manual checks phase 15 covers what no test here can reach, which is the network
in between: a proxy that will not upgrade, one that drops an idle socket without
telling either end, a laptop lid, a token expiring. Every one of those is
invisible inside a test host, and every check there passes only if the change
arrives quickly *and* still arrives with the socket taken away.

ADR 0012 also fixes one thing about the shared terminal session this is the
transport for, so it need not be renegotiated later: session data will be binary
frames on this same socket, because base64 in a JSON envelope is the wrong shape
for the one payload here that is continuous rather than occasional. Two questions
it explicitly does not answer by implication — whether those bytes go through the
API at all, and what end-to-end encryption means when the second party watches a
stream rather than holding a key — are ADR 0001 questions and get their own
decision.

1512 tests pass. DodoSSH.SystemTests was not run — it needs the whole compose
stack — so the end-to-end path is unverified for this change beyond what the
manual checks describe.
2026-08-04 16:37:41 +02:00
jaap-janandClaude Opus 5 a43286ece8 Let a team change hands, and be joined by somebody with no account yet
M3 built teams and stopped short of the two operations that decide who
controls one. Both were written down as refusals rather than omissions:
ADR 0009 listed ownership transfer under "deliberately not built", and
design-import-gaps said an invitation needed "a token with a lifetime and an
outbound mail path". One of those reasons had expired and the other never
applied — an invitation does not need a token if it is not a thing anybody
presents.

Handing a team over is one write. The member you name becomes owner and you
become an admin, in a single transaction, because ownership is sole: promoting
first leaves the team owned twice, demoting first leaves it owned by nobody,
and there is nobody left with the authority to finish a transfer that stopped
in the middle. That is also why it is not two calls to the role endpoint, which
refuses Owner outright. The outgoing owner is demoted rather than removed —
removing them would revoke their vault key grants and flag every team vault for
rekey, which is a far larger act than the one asked for, and somebody handing
over a team is usually staying in it. It unblocks the thing that was impossible
before: an owner can now leave, by handing the team on first.

An invitation is a standing instruction rather than a message. This server has
no outbound mail path, so nothing is sent and there is nothing for the invitee
to present. The row says the next account signing in with that address joins
this team at this role, and telling them to sign in is the caller's job over a
channel this server does not carry. A link nobody can deliver would be worse
than none. It lives in its own table rather than becoming a membership with
MembershipStatus.Invited, and that member stays unwritten for the reason it
always was: team_membership.user_id is not nullable and carries a foreign key,
so somebody who has never signed in has nothing for that row to point at.
Widening it would make the unique index on (team, user) meaningless, because
PostgreSQL counts every NULL as distinct.

Verification is the security boundary, and nothing in this server read it
before. A claim requires the access token to assert email_verified. An
invitation decides what the server will serve, so one claimable by anybody able
to obtain a token carrying somebody else's address is a way into a team — which
is precisely the attack OidcOptions.AllowEmailLinking exists to refuse, and it
would have been reintroduced by the back door. There is deliberately no setting
that relaxes it: a flag that exists is one somebody turns on for the afternoon
their provider is misconfigured. Absence is refused rather than trusted, and
logged, because a provider that never sends the claim otherwise leaves every
invitation pending with nothing anywhere saying why.

Claiming happens at just-in-time provisioning and again on an hourly sweep. The
sweep is what makes it recoverable rather than one-shot — an invitation issued
between an account being created and that person next signing in would
otherwise be stranded for ever — and it shares its rate with the last-seen
write because both are housekeeping nobody is waiting on.

Archiving is refused while a team owns a vault, and that refusal is the end of
the road rather than a step on it. A team vault is readable because of
membership, so archiving one that still owned vaults would take them away from
everybody holding a key, including the caller, quietly and all at once. Nothing
in this product deletes a vault, so no order of operations gets past it today —
which is stated with a count of what is in the way, for the reason the SFTP
layer refuses a recursive delete: a refusal is visible and a quiet removal is
not. It is owner-only, as handing over is; renaming is not, because a rename is
visible to everybody and reversible by anybody who can do it. The slug is not
renameable at all: it is unique only among live teams, so a rename could take
one an archived team is still holding, and that team could then never be
restored.

LAST ACTIVE is real and coarse on purpose. UserAccount.LastSeenAtUtc is
refreshed on ordinary authenticated requests, at most once per account per
hour, through ExecuteUpdateAsync — user_account carries the xmin concurrency
token, so a read-then-write on the hot path would start losing races between
one user's own overlapping requests. An hour is the granularity the question is
actually asked at, and the interface draws it to the day rather than the minute
so it does not read as a precision that is not there. The remarks in Contracts
and in the view model that argued at length for the column's absence are
rewritten rather than extended; both had become false.

Two endpoints already existed and nothing called them. ChangeTeamMemberRole and
ListVaultGrants have been reachable since M3. The role picker refuses Owner
itself rather than letting the server do it, since the interface already knew
the rule; the key-holder list sits under the vault rather than beside the
member, because a grant is per vault and a count on a member row would imply
per-item sharing, which is M5. It lists withdrawn and stale grants and says
which they are — a list that dropped them would show a departed colleague as
merely absent rather than as somebody whose key was taken away — and staleness
is decided by comparing generations, since a grant can be Active and still open
nothing.

ADD MEMBER stopped being a dead end. An address the directory did not know used
to end at a sentence telling the user their colleague had to sign in first. It
invites them instead, from the same button, because which of the two applies is
a fact about the server's account table rather than about what the user is
doing; which one happened is reported afterwards, because that decides what
they do next. An address that merely has an account is invited rather than
refused: refusing would have made the endpoint an oracle for which addresses
have accounts here, answerable by anybody willing to create a team first.

The phone has a TEAMS screen, behind MORE, and it is the reverse of every other
row in design-import-gaps: a shipped screen the design had no slot for. It is
there because an invitation is claimed by signing in, so somebody told they are
now in a team is at least as likely to be holding a phone — and a membership
visible only on a head they never installed is one they cannot see. It draws
SHARE KEY and nothing that takes something away: wrapping a key is the one act
on that screen a server cannot perform at all, and the desktop guards its
revocations with a tooltip, which is a control a touch screen cannot show.

Two defects were found by an adversarial pass and both were green against the
whole suite at the time. The owner-only check on archiving and handing over had
been weakened to the admin check while their messages and comments still said
owner — and since nothing behind the archive endpoint re-checks it, an admin
the owner had promoted could have archived the team out from under them. And
the rename endpoint built its response with a hardcoded Owner role, so an admin
who renamed a team was handed a summary claiming they owned it, and a client
trusting that instead of re-listing would have offered them the two owner-only
buttons the server then refuses.

The new table gets its constraints tested rather than merely migrated: live
uniqueness per (team, address), the citext proof that an address typed by a
person matches one cased by a provider, and reissue after both revocation and
acceptance. The teams screen gets its first entries in the layout suite, at the
minimum window with every list populated and with each of the two states that
cover half of it — it had none, and it just grew four sections and a second
line in the member row.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-03 14:31:43 +02:00
jaap-jan 98d29bff37 Add HTTP integration harness and the sync authorization matrix (M1)
ci / build and test (ubuntu) (push) Has been cancelled
ci / build (windows) (push) Has been cancelled
27 end-to-end tests over the real HTTP pipeline, against a PostgreSQL container and a
stubbed identity provider. This closes the gap the previous commit flagged.

Authentication is genuinely exercised, not bypassed. StubIdentityProvider serves real OIDC
discovery and JWKS via WireMock and signs tokens with a real RSA key, so the application's
own JwtBearer pipeline validates issuer, audience, signature, lifetime and claims. A
TestAuthHandler that short-circuits authentication would hide exactly the claim-mapping
mistakes that cause real authorization holes. Proven by rejecting: no token, a foreign
signing key, the wrong audience, the wrong issuer, and an expired token.

Authorization denials — the tests that matter most:
- Another user's vault is 404, not 403, for both pull and push. A distinct
  "exists but forbidden" answer is an existence oracle for other tenants' vault ids.
- A denied push writes nothing: no host row and no change-log entry. A denial that still
  mutated state would be worse than no check at all.
- A team vault is denied until M3 rather than falling through to a permissive default.

Behaviour covered: push/pull round trip, cursor advance (and that an empty pull does not
rewind the cursor, which would replay history), tampered cursor rejection, stale-version
conflict returning server state without overwriting, operation-id replay reported Duplicate
and applied once, a mixed batch applying the good and reporting the bad, relay field
enforcement both ways, delete clearing the relay address, tombstones carrying no payload,
and JIT provisioning happening exactly once.

Two configuration problems found by running it:
- appsettings.json carried empty-string placeholders for the connection string and OIDC
  authority. Under minimal hosting those beat anything a test registers via
  ConfigureAppConfiguration, because Program.cs adds its own sources after that callback
  runs. Removed them outright — an empty placeholder turns "not configured" into
  "configured as empty", which defeats failing fast. Tests now use DODOSSH_ environment
  variables, which Program.cs adds last.
- My first fix for minting an expired test token derived notBefore from the expiry, which
  put nbf fourteen minutes in the future for normal tokens and made every valid token 401.
  It needs the earlier of now-1min and exp-1min.

Verified: 0 warnings on a clean rebuild, 173 tests pass (up from 146), format clean.
2026-07-28 15:11:43 +02:00