diff --git a/README.md b/README.md index f8076f8..7c78662 100644 --- a/README.md +++ b/README.md @@ -268,7 +268,7 @@ bastion — the last needs jump hosts the connection layer has not got. All thre ### Sharing a vault -**VAULTS** in the nav rail lists every vault you can see, makes new ones, adds and invites people to one, +**VAULTS** in the nav rail lists every vault you can see, makes new ones, adds people to one, shares its key, renames it and hands it over. One distinction runs through the whole screen and is worth having before you use it. @@ -298,40 +298,28 @@ published in a log every other client also reads. **It does not prove the key is Compare the fingerprint with them over something this server does not carry; that is the only step that closes it, and the success message says so every time. -**Somebody with no account here yet can be invited, and nothing is sent.** There is one button — **ADD** — -and it does whichever of the two applies, because which one applies is a fact about the server's account -table rather than about what you are trying to do. If the directory knows the address, that account is added -straight away. If it does not, the address is invited instead, and the status line says which happened, -because the difference decides what you do next. +**Only somebody who already has an account here can be added, and the refusal says what to do about it.** +There is one button — **ADD** — and it takes an email address. If the directory knows it, that account is +added and its public key comes back with it. If the directory does not, the address is still tried against +the server, because the directory lists only accounts that have *published a key* — everybody between +their first sign-in and setting a machine up is missing from it, and reading that silence as "no account +here" would refuse somebody standing right beside you. -An invitation is a standing instruction rather than a message: the next account that signs in with that -address joins this vault, at the role you chose. There is no link and no token, because this server has no -outbound mail path and does not pretend otherwise — telling them to go and sign in is your job, over a -channel this server does not carry, and a link nobody can deliver would be worse than no link. An -invitation lasts fourteen days, so an address handed on to whoever takes the job next does not carry a -standing offer for ever; it can be withdrawn until it is taken up; and like adding a member it grants -nothing readable, so somebody still has to press SHARE KEY afterwards. +What is refused is an address with no account at all. The status line names the address and says the +remedy: **ask them to sign in to this server once — that is what creates the account — and then add +them.** Nothing is held for them in the meantime. -The one thing the merged button costs is worth knowing. Adding an account the directory knows also hands -you the public key you are about to verify and wrap a vault to, and an invitation cannot do that because -there may be no key yet. So when you are adding somebody *in order to* share a vault with them, the useful -sequence is still the same one: add them, see them appear in the members list, then share. +**That is a deliberate limit rather than a missing feature.** There used to be an invitation here: a +standing instruction that the next account signing in with that address joined the vault, taken up on the +strength of the token's `email_verified` claim. It worked, and it was the one place in this product where +access was decided by an assertion about an address rather than by somebody naming an account — so the +whole of its defence was a boolean sent by an identity provider. It is gone. Everything that grants access +now starts with a person who already exists. The reasoning is in +[ADR 0009](docs/adr/0009-team-access-model.md). -Inviting an address that already belongs to a member of the vault is refused and says so. Inviting one that -merely *has* an account here is not — that would make this a way of asking the server which addresses have -accounts, which is not a question anybody willing to create a vault first should be able to put to it. Such -an invitation simply gets claimed sooner: within the hour, on the same sweep that records they were here, -rather than waiting for a first sign-in that has already happened. - -**An invitation is only claimed if your identity provider says the address is verified, and there is no way -to relax that.** The access token has to carry `email_verified` as true. Anything else — false, missing, or -sent under another name — claims nothing at all, and no setting turns that off: an invitation decides what -the server will serve, and one that could be taken by anybody able to obtain a token asserting somebody -else's address is a way into a vault. **If your invitations never activate, this is the first thing to -check.** They sit at *pending* rather than failing, the server logs a warning each time it declines to -claim one, and the two fixes are on your side: set `Oidc:EmailVerifiedClaim` to whatever your provider -calls the claim if it is not `email_verified`, and make sure the provider puts it in the **access** token -rather than only in the ID token or the userinfo response. +Adding somebody still grants nothing readable: press **SHARE KEY** afterwards, or add them from a machine +that holds the vault key and it happens in the same step. A member with no key of their own yet — added +before they finished setting up — says so on their row, and cannot be shared with until they have one. **Ownership is sole, and handing a vault over is one act.** Transferring names an existing active member: they become owner and you become an admin, in a single transaction. Not two role changes — promoting first @@ -513,9 +501,9 @@ rather than in an editor: a tag from inside a host's editor, and a credential fr remember tick, which stores the password just typed and moves the host onto it. Renaming either is still a desktop job. Pins and import have no phone screen either, and importing an `~/.ssh/config` has no meaning on a phone at all. **VAULTS does have one**, behind MORE, and it is there for a reason the design could not -have anticipated: an invitation is claimed by signing in, so somebody being told they have been added to a -vault is at least as likely to be holding a phone as sitting at a desktop, and a membership visible only on -a head they have not installed is a membership they cannot see. +have anticipated: a vault arrives without being asked for — somebody wraps its key to you — so the person +it arrives for is at least as likely to be holding a phone as sitting at a desktop, and a membership +visible only on a head they have not installed is a membership they cannot see. **Port forwarding is not built anywhere**, and the phone's More screen says so in a paragraph rather than leaving a gap. The v2 design draws a whole screen for it; nothing in the SSH layer forwards anything, so @@ -754,8 +742,8 @@ keychain plus a terminal — and the spike that gates all of it. every vault it holds a key for, and a real VAULTS screen replaces the placeholder. The screen is vault-shaped rather than team-shaped: naming a vault makes the membership list that carries it, so the team is behind the vault rather than a thing anybody has to create first. A vault can be renamed, deleted - and handed to another member; a member row carries when that account was last here; and an address with no - account on this deployment can be invited, joining the moment somebody signs in with it. See + and handed to another member; and a member row carries when that account was last here. Only an account + that already exists can be added — an address is not a way into a vault. See [Sharing a vault](#sharing-a-vault) for the one distinction the whole design rests on, and the limits worth knowing before you rely on it; the reasoning is in [ADR 0009](docs/adr/0009-team-access-model.md). diff --git a/docs/adr/0009-team-access-model.md b/docs/adr/0009-team-access-model.md index 3c1a827..aff7fe2 100644 --- a/docs/adr/0009-team-access-model.md +++ b/docs/adr/0009-team-access-model.md @@ -87,39 +87,52 @@ Two smaller choices, recorded because the alternative was written down first and until that exists, the safe direction is the narrow one, and the cost — approving a team host's key once per member per machine — is stated in the README rather than hidden. -### An invitation is membership decided before there is an account to hold it +### Addendum: invitations are gone, and an address is not a way into a team + +There was an invitation here, and it is worth recording what it was before recording why it is not. A membership names an account: `team_membership.user_id` is not nullable and carries a foreign key, so -somebody who has never signed in here has nothing for that row to point at. `MembershipStatus.Invited` -has existed since the first migration and is still never written — not as an oversight, but because a -membership waiting for a person is the one shape this model cannot store. An invitation is therefore its -own record, `team_invitation`, held against an **address**, and it becomes an ordinary active membership -the moment an account with that address signs in. +somebody who has never signed in has nothing for that row to point at. `MembershipStatus.Invited` has +existed since the first migration and has never been written, because a membership waiting for a person +is the one shape this model cannot store. So an invitation was its own record, `team_invitation`, held +against an **address**, and it became an ordinary active membership the moment an account with that +address signed in — provided the access token asserted `email_verified` over it. -That extends the model rather than bending it. An invitation grants nothing readable and cannot be a -step towards sharing, because there is no account and so no public key to wrap a vault to. It moves the -first half of the split earlier and leaves the second half exactly where it was. +**That last clause was the whole feature, and it is why the feature is gone.** Everything else in this +model decides access from something a person did: an admin naming an account, a key holder wrapping a +vault key to a public key they verified. The invitation decided access from an *assertion about an +address*, made by an identity provider, on behalf of somebody the deployment had never seen. It was +guarded as tightly as that can be guarded — the claim was refused outright on an unverified or absent +claim, with no setting to relax it, for the same reason `OidcOptions.AllowEmailLinking` is off and stays +off. But the guard and the risk were the same shape: the server was trusting a token's email claim to +decide who is in a team, and the whole of the defence was one boolean sent by a system this deployment +does not control. -Three decisions inside it belong here, because each had a more convenient alternative: +So the decision now reads: **membership is granted only to an account that already exists, named by +somebody who can already see it.** `POST /api/v1/teams/{id}/members` takes a user id from the directory, +or an address it resolves to an existing account, and an address with no account is refused with +`ProblemCodes.NoSuchAccount`. That refusal is the end of the road rather than a step on it — there is +nothing to fall through to. -- **The claim requires `email_verified` on the access token, and nothing relaxes it.** This is the whole - of the security boundary. Membership is authorisation, so an invitation that could be taken by anybody - able to obtain a token asserting somebody else's address is a way into a team — the same attack - `OidcOptions.AllowEmailLinking` exists to refuse, arriving by another door and deserving the same bar. - An unverified or absent claim claims nothing and logs a warning, which is the only signal an operator - gets that their provider is not sending it. There is deliberately no setting to trust an unverified - address: a flag that exists is a flag somebody turns on for the afternoon their provider is - misconfigured, and this is the one it must not be possible to turn on. -- **Nothing is sent, and the product says so rather than implying a mail path it has not got.** There is - no token and no link — the row is a standing instruction, and telling the invitee to go and sign in - happens over a channel this server does not carry. A link nobody can deliver would be worse than none. - The compensation, such as it is, is real: an invitation that is not a bearer credential is one that - cannot be forwarded, intercepted or replayed. -- **An address that already has an account here is accepted rather than refused.** Refusing and pointing - at the directory would have been tidier, and would have turned the endpoint into an oracle for which - addresses have accounts on this deployment, answerable by anybody willing to create a team first. Only - an address already belonging to a member of *this* team is refused, and that is a fact the caller can - already read off the members table, so naming it leaks nothing. +Three consequences, all of them stated rather than mitigated: + +- **Somebody who has never signed in here cannot be added yet.** The remedy is theirs: they sign in + once, which is what creates the account, and then they can be added. Both clients say exactly that + when the refusal comes back. This is a real loss of convenience, and it is the price of not having a + path where an address is a credential. +- **Nothing reads the email claim for authorisation any more**, anywhere in the server. + `OidcOptions.EmailVerifiedClaim` was read by the claim path and by nothing else, so it went with it — + a setting that changes no outcome is worse than no setting. The address is still recorded and still + resolved by the directory, both for display and for naming an account that exists. +- **Pending invitations were dropped rather than converted.** Converting one would have meant creating a + membership because an address matched, which is the property being removed; and an invitation to an + address that *did* have an account here had already been claimed by the hourly sweep, so what was left + was offers to people who never arrived. See the `DropTeamInvitation` migration. + +The oracle question the old design worried about — whether this endpoint tells a caller which addresses +have accounts here — is answered rather than avoided. `NoSuchAccount` does say so, to an admin or owner +of the team the add names, which is the same fact the member list shows them a moment later. That was +already true of adding by address before invitations existed. ### Addendum: the vault is what the product shows, and the team is behind it @@ -165,9 +178,9 @@ Three consequences of the change belong here: The sharing graph is visible to the operator: who is in which team, which vaults exist, and who holds a grant are all plaintext rows. That was already true of metadata generally (`docs/crypto.md` §10) -and is not made worse here, but it is now a graph rather than a list. Invitations widen it by one -edge — an address that has been invited is on the graph before its owner has ever been here — which is -the same class of fact and worth naming rather than leaving to be noticed. +and is not made worse here, but it is now a graph rather than a list. Every node on it is an account +that exists: there is no longer an edge to an address whose owner has never been here, which is what +invitations added and what removing them took back. A malicious granter can seal garbage. The recipient detects it as a tag failure and the grant's Ed25519 signature names who issued it — detectable and attributable, which is the most that is diff --git a/docs/crypto.md b/docs/crypto.md index c507ce5..3ace35d 100644 --- a/docs/crypto.md +++ b/docs/crypto.md @@ -48,7 +48,6 @@ this specification is not implementable. | --- | --- | --- | --- | --- | | Passphrase → master key | 256 MiB | 4 | 1 | 32 B | | Recovery code → KEK | 64 MiB | 3 | 1 | 32 B | -| Invite secret → KEK | 64 MiB | 3 | 1 | 32 B | Salt is 16 bytes from a CSPRNG, fresh on every passphrase change. diff --git a/docs/design-import-gaps.md b/docs/design-import-gaps.md index c307536..0cae216 100644 --- a/docs/design-import-gaps.md +++ b/docs/design-import-gaps.md @@ -30,11 +30,11 @@ the chrome, hosts and terminals, file transfer, the vault, teams, and preference > SFTP and S3 are one screen over one `TransfersViewModel`, differing only in which picker they offer. > > **A sixth is behind MORE that v2 never drew: VAULTS.** It is the reverse case — a shipped screen the -> design had no slot for — and it is on the phone for a reason the design could not have anticipated, -> because invitations did not exist when it was drawn. An invitation is claimed by *signing in*, and the -> person being invited is at least as likely to be holding a phone as sitting at a desktop; a vault the -> server has just put somebody in, visible only on a head they may not have installed, is a membership -> they cannot see. It runs over the same view model the desktop screen drives, like the other four. +> design had no slot for — and it is on the phone because a vault arrives without being asked for. +> Somebody wraps its key to you from their machine, and the person it arrives for is at least as likely to +> be holding a phone as sitting at a desktop; a vault the server has just put somebody in, visible only on +> a head they may not have installed, is a membership they cannot see. It runs over the same view model +> the desktop screen drives, like the other four. > > | v2 element | What ships instead | > | --- | --- | @@ -195,12 +195,13 @@ unused tables bought. See [Vaults](#vaults). What has *not* changed is the split decides what it will serve, and only a client can decide who can read it — so "shared with" is two facts on this screen, not one. -One table did have to be added, and what it shows is the limit of that reservation. An invitation names an +One table was added and then dropped again, and the round trip is worth a line. An invitation named an **address**, and `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 — which is why `MembershipStatus.Invited` has been -reserved since the first migration and is still never written. `team_invitation` is its own table for that -one reason. The schema was right about the shape of a team and had said nothing about the shape of joining -one. +never signed in had nothing for that row to point at — which is why `MembershipStatus.Invited` has been +reserved since the first migration and has never once been written. `team_invitation` existed for that one +reason and is gone: membership is granted only to an account that exists, so there is no longer a shape the +reservation fails to cover. The status stays reserved, because the column holds it in nobody's database and +a client must not fail on a value a later server might send. See [ADR 0009](adr/0009-team-access-model.md). **The client has no preferences store.** It writes exactly two files — `cache.db` and `device.key` — and the cache has six tables, none of them settings. Nothing on the design's TERMINAL preferences panel can be @@ -394,9 +395,8 @@ drew around them. ## Vaults **Built in M3, and reshaped since.** The screen ships: a vault list, a members table with a real -last-active column, the invitations standing against addresses that have no account here yet, who holds a -key, and the two buttons the whole design was really about — add somebody, and share a vault key. A vault -can also be renamed and handed to another member. +last-active column, who holds a key, and the two buttons the whole design was really about — add somebody, +and share a vault key. A vault can also be renamed and handed to another member. **It lists vaults where it used to list teams, and that is the reshaping.** A team is still what the server authorises against; what went is the requirement that anybody make one. Naming a vault makes the membership @@ -416,15 +416,19 @@ place that changed, exactly as its remark predicted, and no migration was needed What the row did not anticipate is that the interesting half is not the endpoints at all. It is that **membership and readability are different things**, and the screen is arranged around saying so. -**There are two ways into a team and they are not interchangeable.** Adding a member takes a *user id* the -caller has already got from the directory, so that account must have signed in here at least once — and the +**There is one way into a team, and that is the decision.** Adding a member takes a *user id* the caller +has already got from the directory, so that account must have signed in here at least once — and the ordering is deliberate rather than incidental, because whoever adds a member is usually about to wrap a -vault key to the public key that lookup returned. Inviting takes an *address*, grants nothing readable, and -cannot be a step towards sharing: there is no account, so there is no key to wrap to. Inviting an address -that already belongs to a member of this team is refused and says so — that is a fact about a team the -caller can already see. Inviting one that merely has an account somewhere on this deployment is **not** -refused, because answering that would turn the endpoint into an oracle for which addresses have accounts, -answerable by anybody willing to create a team first. It simply gets claimed sooner. +vault key to the public key that lookup returned. An *address* is accepted too, and has to be: the +directory lists only accounts that have published a key, so everybody between a first sign-in and their +enrollment is invisible there and would otherwise be unaddable. What is refused is an address with no +account at all, under `no-such-account`, and that refusal is the end of the road — the remedy is that +person signing in once. + +There was a second way in, and there is not any more. An invitation took an address, waited, and became a +membership the next time somebody signed in asserting it. It is gone because it was the one place in this +product where access followed from an identity provider's claim about an address rather than from somebody +naming an account. See [ADR 0009](adr/0009-team-access-model.md). | Design element | Layer | What ships | | --- | --- | --- | @@ -436,17 +440,16 @@ answerable by anybody willing to create a team first. It simply gets claimed soo | Adding a member shares the team's vaults | client | Adding somebody wraps every team vault the adding machine can open to them, as part of the add rather than as a button to remember. Membership and a key are still two acts on two machines; the client just performs both. A vault this machine holds no key to is skipped and named. | | Removing a member rotates the vaults | server + client | `POST /api/v1/vaults/{id}/rekey` advances the generation and records the caller's new grant in one transaction — the server contributes the moment and no cryptography. The client then wraps the new key to the members who remain. Grants for earlier generations are kept, or the vault's stored items would become unreadable to everybody. See [ADR 0010](adr/0010-vault-key-rotation.md). | | Re-sealing a rotated vault's stored items | client | `VaultResealer`, the last step of a rotation: every item is opened and the same plaintext sealed again under the new key, in batches, against the version the server holds. The plaintext is never decoded, so an item written by a newer client crosses a rotation untouched. Resumable, because a vault at mixed generations is readable — a pass that stops half way leaves a working vault and is re-run. A change queued before the rotation is re-sealed by the push path instead, so nothing reaches the server under a superseded key at all. | -| Pending invites, and withdrawing one | server | A `team_invitation` row per (team, address), listed beside the members it is about and withdrawable until it is taken up. It becomes a membership when an account with that address signs in — **and only if the access token asserts `email_verified`**, because membership is authorisation and an invitation anybody could take by naming somebody else's address is a way in. Fourteen days, because an address that is reassigned would otherwise carry a standing offer to whoever holds the job next. | | Ownership transfer | server | `POST /api/v1/teams/{id}/owner`, owner only. One transaction: the named member becomes owner and the outgoing owner becomes an admin. Not two role changes — ownership is sole, so promoting first leaves the team owned twice and demoting first leaves it owned by nobody. The outgoing owner is demoted rather than removed, because removing them would revoke their vault key grants and rotate every team vault, which is a far larger act than the one being asked for. | | `LAST ACTIVE` | server | Real, and coarse on purpose. `UserAccount.LastSeenAtUtc` is now refreshed on ordinary authenticated requests, at most once per account per hour: writing it per request would put an UPDATE on the hot path of every authenticated call and start losing races on `user_account`'s own concurrency token. So the column answers "this week or not", which is the granularity the question is actually asked at, and is shown coarsely rather than to the minute. | -| Renaming and archiving a team | server | `PUT` and `DELETE /api/v1/teams/{id}`. The slug is deliberately not renameable: it is unique only among *live* teams, so a rename could take a slug an archived team still holds and strand it. Archiving soft-deletes the team, every membership and every pending invitation in one transaction — and is refused outright while the team owns any vault. | +| Renaming and archiving a team | server | `PUT` and `DELETE /api/v1/teams/{id}`. The slug is deliberately not renameable: it is unique only among *live* teams, so a rename could take a slug an archived team still holds and strand it. Archiving soft-deletes the team and every membership in one transaction — and is refused outright while the team owns any vault. | | Design element | Layer | What it would take | What ships instead | | --- | --- | --- | --- | | `CONNECT-ONLY` role | — | Nothing that would be true. Connect is a user-interface hint, not a boundary: SSH terminates on the client, so a session needs the credential's plaintext on that machine. See ADR 0001. | Four roles, all of which are enforceable. `Connect` rides along with `Read` and is documented as a hint. | | `2FA ENFORCED` and the per-member 2FA column | server | No two-factor concept exists anywhere — the only hit in the whole worktree is an aside in `docs/crypto.md`. | Omitted. The member columns carry what *is* known and matters: whether they have published a key a vault can be wrapped to, and when they were last here. | | Avatars | server | No picture is stored anywhere. | Omitted; the row shows a name and an address. | -| The invitation mail, and **resend** | server | An outbound mail path: an SMTP configuration, a template, a bounce story and a deliverability problem, none of which this server has. | **Nothing is sent, and the interface says so.** An invitation is a standing instruction rather than a message — the next account to sign in with that address joins the team — so there is no token, no link, and nothing to resend. Telling somebody to sign in is done over a channel this server does not carry. A link nobody can deliver would be worse than no link. | +| Pending invites, the invitation mail, and **resend** | server | An outbound mail path this server has not got, and — the larger cost — a rule that turns an identity provider's claim about an address into team membership. | **Nothing pending exists, and the screen says why.** Only an account that already exists can be added; an address with no account is refused, naming the address and saying the person has to sign in here once. There was a `team_invitation` table doing the deferred version of this and it has been dropped. See [ADR 0009](adr/0009-team-access-model.md). | | Archiving a team that owns vaults | — | Nothing that would be safe. A team vault resolves through membership, so archiving would take those vaults away from everybody holding a key, silently, including the caller — and nothing in this product deletes a vault, so there is no sequence of calls that turns the refusal into a success. | Refused, with `team-not-empty` and a count of the vaults in the way. A stated limit rather than a coming feature, for the reason the SFTP layer refuses a recursive delete: a refusal is visible and a quiet removal is not. | | `SSO · OIDC · okta.dodotech.dev` | server | Per-team SSO. Authentication is one global JWT scheme bound to one authority. | Omitted. | diff --git a/docs/manual-checks.md b/docs/manual-checks.md index c768081..a679e99 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -1239,86 +1239,60 @@ case, and those two have to move together — the switch mirrors that property b ## Phase 12 — Shared vaults: the operations that span two accounts The server's own rules are covered by the endpoint suite: teams are renamed, an archive is refused while a -vault is in the way, ownership changes hands, and every branch of the invitation claim is driven with -tokens the test mints itself. That last freedom is exactly what puts this phase here. **A test can mint a -token asserting anything it likes, so it can prove the server's rule and can say nothing whatever about -whether *your* identity provider sends the claim that rule depends on** — and an invitation that never -activates fails by sitting still, which is the failure mode nobody notices. What is left needs two real -accounts, a real sign-in, and in two cases a clock. +vault is in the way, ownership changes hands, and an address with no account is refused with its own code. +What is left needs two real accounts, a real sign-in, and in one case a clock — because the half of sharing +this product is built around happens on a machine rather than on the server, and no server test can reach +it. -**Two accounts, and two profiles.** The dev realm ships `alice` and `bob`, both with verified addresses; a -second DodoSSH profile means a second machine, a second OS user, or the same machine after signing out. -Whichever account plays the invitee **must not have signed in to this deployment before** — most of what -follows is about what happens the first time it does. +**Two accounts, and two profiles.** The dev realm ships `alice` and `bob`; a second DodoSSH profile means a +second machine, a second OS user, or the same machine after signing out. Whichever account plays the +newcomer **must not have signed in to this deployment before** — 12.1 is about exactly that boundary. -### 12.1 An invitation becomes a membership at the invitee's first sign-in · **the one worth the most care** +### 12.1 An address with no account is refused, and joins nothing when it later signs in · **the one worth the most care** 1. Sign in as `alice`, make a vault on the VAULTS screen, and select it. -2. Add `bob@example.com` as a Member. **Nothing is sent, and nothing should look as though it was** — - no "invitation emailed", no link to copy, no token anywhere on the screen. -3. **Pass:** the row appears as *pending*, carrying the address, the role and an expiry fourteen days out. - Bob is **not** in the members table, because he has no account here for a membership row to point at. +2. Add `bob@example.com` as a Member, with Bob having never signed in here. +3. **Pass:** it is refused. The status line names the address and says to ask them to sign in to this + server once and then add them. **Nothing on the screen should suggest anything is pending** — no + invited row, no "we will add them when they arrive", and the address stays in the box so it can be + used again in a moment. 4. Sign in as `bob` on the second profile and enroll. -5. **Pass:** the vault is in Bob's list the first time he looks, at Member, with nothing further pressed - on either side. Back on Alice's machine, refresh: the invitation reads *accepted* rather than vanishing, - and Bob is now in the members table. -6. **Pass, and this is the half that is easiest to lose:** the vault is in Bob's list **saying it is - waiting for a key**, and nothing in it is readable. Have Alice press SHARE KEY and Bob sync; now it - opens. This is where an invitation differs from an add and the difference is not a defect: adding an - account the directory already knows wraps the key on the spot, because the adding machine has it and - the recipient has a published key to wrap to. An invitation has neither at the moment it is issued — - there is no account yet — and the claim happens on Bob's machine, which holds nothing. So the key is - still owed, and somebody has to hand it over. +5. **Pass, and this is the half that is easiest to lose:** Bob's vault list holds only his personal vault. + Alice's members table is unchanged. Signing in with an address somebody typed earlier must join nothing + at all. +6. Back on Alice's machine, add `bob@example.com` again. +7. **Pass:** he is added, and — because Alice's machine holds the vault key and Bob has now published one + — the key is wrapped in the same step. Have Bob sync; the vault opens. -**Failure means:** step 5 failing with everything else passing is almost always the `email_verified` claim -— go to 12.2 rather than reading the invitation code, because the server is doing exactly what it should. -Step 6 opening the vault *without* Alice sharing a key would be the far more serious failure: nothing on -the server can wrap a vault key, so an item that decrypts after a membership change alone means a key -reached that machine by a route this architecture says does not exist. +**Failure means:** step 5 is the one to stop on. A vault appearing in Bob's list because he signed in with +an address is the deferred-membership path coming back, and it is the property this phase exists to check: +access is granted to an account somebody named, never to an address. See ADR 0009. -### 12.2 An unverified address claims nothing, and the log is the only place that says so +Step 7 opening the vault *without* Alice's machine having wrapped a key would be the more serious failure: +nothing on the server can wrap a vault key, so an item that decrypts after a membership change alone means +a key reached that machine by a route this architecture says does not exist. -In Keycloak's admin console, clear **Email verified** on the invitee *before* their first DodoSSH sign-in. -Invite that address, then sign in as them. +### 12.2 Somebody who has signed in but not enrolled can still be added -**Pass:** they get an account and a personal vault and no shared one. The invitation stays *pending* on -the inviter's screen rather than turning into anything, and the API log carries a warning naming how many -invitations it declined to claim. Now set **Email verified** back on. The claim happens on the next request -that crosses the hourly last-seen window, so it is **not** immediate and restarting the client will not -hurry it along — the account already exists, so there is no second first-sign-in to trigger it. +Have a third account sign in and stop — no passphrase, no enrollment. Then add its address to a vault. -**Failure means:** if the vault appears while the address is unverified, the one security boundary -invitations have is not being enforced, and anybody able to obtain a token asserting a colleague's address -can walk into their vault. Stop there. If it stays pending after verifying, the claim is not reaching the -**access** token — check the provider's mappers, and set `Oidc:EmailVerifiedClaim` if it sends the claim -under some other name. +**Pass:** accepted. Their row appears saying they hold no key, and SHARE KEY does not offer to wrap one to +them. The directory returns nothing for that address, which is correct: it lists accounts that have +published a key, and this one has not. -### 12.3 An invitation can be withdrawn until it is taken up +**Failure means:** a refusal here is the old bug, and it is the reason ADD does not take the directory's +silence as an answer. Everybody passes through the window between a first sign-in and enrollment, and +being told they have no account while they are standing beside you is the worst moment to say it. -Invite an address, then revoke it before anybody has signed in with it. Then sign in with that address. - -**Pass:** the row reads *revoked* and stays on the list rather than disappearing, and the sign-in produces -an ordinary account in no shared vault. Revoking one that has *already* been accepted answers that there -was nothing to withdraw. - -**Failure means:** a revoked invitation that still lets somebody in is a removal that did not remove. An -accepted one that could be unpicked here would be worse: it is a membership now, and removing a member -revokes their vault key grants and flags the vault for rekey, which is not what "revoke invitation" -should quietly do. - -### 12.4 An address already in the team is refused; an address that merely has an account is not +### 12.3 An address already in the team is refused, and says which With Bob in the vault, add `bob@example.com` to it again. **Pass:** refused, with a sentence saying the address already belongs to a member and to change their role -instead. Now make a **second** vault and invite the same address there. +instead — distinct from the "no account here" refusal in 12.1, because the two lead somewhere different. -**Pass:** accepted. Bob having an account is deliberately not a reason to refuse — it is claimed within the -hour on his next request rather than at a sign-in, so give it that long before deciding it has not worked. - -**Failure means:** if the second invitation is refused because the address already has an account, this -endpoint has become a way of asking the server which addresses have accounts on it, answerable by anybody -willing to create a vault first. See ADR 0009. +**Failure means:** the two refusals reading alike leaves somebody checking what they typed when the answer +is that the person is already in. ### 12.5 LAST ACTIVE is a real time, and a coarse one · **needs a couple of hours** diff --git a/src/DodoSSH.Api/Authorization/CurrentUserContext.cs b/src/DodoSSH.Api/Authorization/CurrentUserContext.cs index 6f0907c..a6d7bbe 100644 --- a/src/DodoSSH.Api/Authorization/CurrentUserContext.cs +++ b/src/DodoSSH.Api/Authorization/CurrentUserContext.cs @@ -17,51 +17,27 @@ public interface ICurrentUserContext Task GetOrProvisionAsync(CancellationToken cancellationToken); } -/// -/// Turns pending team invitations addressed to a verified email into memberships. -/// -/// -/// -/// Declared here, beside its only caller, and implemented in Features/Teams. The direction is -/// deliberate: sign-in is what an invitation waits for, so the sign-in path names the shape it needs -/// and the teams feature supplies it — rather than , which every -/// endpoint in the server depends on, growing a reference into one feature's folder. -/// -/// -public interface ITeamInvitationClaim -{ - /// - /// Claims every live invitation addressed to for this account. - /// - /// The account signing in. - /// The address the token asserted, or null if it asserted none. - /// - /// Whether the provider marked that address verified. False refuses the claim outright and is the - /// whole of what stops an invitation being taken by anybody able to assert somebody else's - /// address. - /// - /// Cancellation. - /// How many invitations became memberships. - Task ClaimAsync( - UserAccount user, - string? email, - bool emailVerified, - CancellationToken cancellationToken); -} - /// /// Request-scoped caller identity with just-in-time provisioning. /// /// +/// /// Identity is keyed on (issuer, subject), never on email. Matching an existing account by /// email means anyone who can obtain a token bearing a victim's email address — from any configured /// provider — inherits that victim's vaults, so it is opt-in configuration and off by default. +/// +/// +/// Nothing here reads the email claim for authorization, and there is deliberately no hook left for +/// anything that would. This class used to claim pending team invitations on the way past, which +/// made a provider's assertion about an address into a decision about who joins a team; invitations are +/// gone and membership is granted only to an account somebody named — see +/// docs/adr/0009-team-access-model.md. The address is still recorded, for display. +/// /// internal sealed class CurrentUserContext( IHttpContextAccessor accessor, DodoDbContext database, IOptions oidcOptions, - ITeamInvitationClaim invitations, TimeProvider clock) : ICurrentUserContext { @@ -73,8 +49,7 @@ internal sealed class CurrentUserContext( /// UPDATE on the hot path of every authenticated call and — because user_account carries /// the xmin concurrency token — would start losing races between a user's own overlapping /// requests. Writing it never is what made the old "last active" column impossible to offer - /// honestly. An hour answers the question a colleague actually asks, which is "this week or not", - /// and it is also the window on which a pending invitation is swept for. + /// honestly. An hour answers the question a colleague actually asks, which is "this week or not". /// private static readonly TimeSpan LastSeenWindow = TimeSpan.FromHours(1); @@ -101,7 +76,6 @@ internal sealed class CurrentUserContext( var options = oidcOptions.Value; var email = principal.FindFirstValue(options.EmailClaim); var displayName = principal.FindFirstValue(options.NameClaim); - var emailVerified = IsVerified(principal, options.EmailVerifiedClaim); var existing = await FindAsync(issuer, subject, cancellationToken).ConfigureAwait(false); @@ -110,52 +84,28 @@ internal sealed class CurrentUserContext( cached = await ProvisionAsync(issuer, subject, email, displayName, cancellationToken) .ConfigureAwait(false); - // A first sign-in is exactly what an invitation is waiting for, so it is claimed at once - // rather than on the next hourly sweep — which would leave somebody staring at a team - // list that does not yet contain the team they were told they had been added to. - await invitations - .ClaimAsync(cached, email, emailVerified, cancellationToken) - .ConfigureAwait(false); - return cached; } cached = existing; - await RefreshLastSeenAsync(existing, email, emailVerified, cancellationToken) - .ConfigureAwait(false); + await RefreshLastSeenAsync(existing, cancellationToken).ConfigureAwait(false); return cached; } /// - /// Records that this account is active, and sweeps for invitations it can now claim. + /// Records that this account is active. /// /// - /// - /// The two are one operation because they want the same rate. Both are housekeeping nobody is - /// waiting on, and doing them together costs one extra round trip per account per hour rather - /// than two. - /// - /// - /// The sweep is what makes claiming recoverable rather than one-shot. A claim that failed at - /// provisioning — or an invitation issued in the window between an account being created and this - /// person next signing in — is picked up here instead of being stranded for ever. - /// - /// /// ExecuteUpdateAsync rather than the change tracker, and the predicate rather than a /// read-then-write: user_account carries the xmin concurrency token, so two overlapping /// requests from one user would each read the row, each set the timestamp, and the second would /// fail on a version that had moved under it. This writes at most one row and cannot conflict. /// The tracked entity is deliberately left alone — a value up to an hour stale in memory changes /// nothing, and marking it modified would enlist the user row in whatever the request saves next. - /// /// - private async Task RefreshLastSeenAsync( - UserAccount user, - string? email, - bool emailVerified, - CancellationToken cancellationToken) + private async Task RefreshLastSeenAsync(UserAccount user, CancellationToken cancellationToken) { var now = clock.GetUtcNow(); @@ -171,20 +121,8 @@ internal sealed class CurrentUserContext( setters => setters.SetProperty(u => u.LastSeenAtUtc, now), cancellationToken) .ConfigureAwait(false); - - await invitations - .ClaimAsync(user, email, emailVerified, cancellationToken) - .ConfigureAwait(false); } - /// - /// A JWT boolean arrives as the string "true", so this parses rather than compares against a - /// constant. Anything else — absent, "false", or a value this does not understand — is false, - /// because the failure that matters is treating an unverified address as verified. - /// - private static bool IsVerified(ClaimsPrincipal principal, string claimType) => - bool.TryParse(principal.FindFirstValue(claimType), out var verified) && verified; - private Task FindAsync(string issuer, string subject, CancellationToken cancellationToken) => database.Users.SingleOrDefaultAsync( u => u.Issuer == issuer && u.Subject == subject && u.DeletedAtUtc == null, diff --git a/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs b/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs index 512f60a..1d68e28 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamEndpoints.cs @@ -495,173 +495,6 @@ internal sealed class RemoveTeamMemberEndpoint(ICurrentUserContext currentUser, } } -/// Lists a team's invitations. -/// -/// Readable by every member, as the members list is: whoever is about to be handed a vault key needs -/// to see who else is on their way in. Accepted and withdrawn invitations are included so the screen -/// can say an invitation was taken up rather than letting it vanish and read as never sent. -/// -internal sealed class ListTeamInvitationsEndpoint( - ICurrentUserContext currentUser, - TeamService teams, - TeamInvitationService invitations) - : EndpointWithoutRequest>, NotFound>> -{ - /// - public override void Configure() - { - Get("/api/v1/teams/{teamId:guid}/invitations"); - - Policies(Auth.AuthenticatedPolicy); - - Description(b => b - .WithName("ListTeamInvitations") - .WithSummary("Lists a team's invitations.") - .WithTags("Teams")); - } - - /// - public override async Task>, NotFound>> ExecuteAsync( - CancellationToken ct) - { - var user = await currentUser.GetOrProvisionAsync(ct).ConfigureAwait(false); - var teamId = Route("teamId"); - - var access = await teams.ResolveAsync(user.Id, teamId, ct).ConfigureAwait(false); - - if (!access.Granted) - { - return TypedResults.NotFound(); - } - - return TypedResults.Ok(await invitations.ListAsync(teamId, ct).ConfigureAwait(false)); - } -} - -/// Invites an address to a team. -/// -/// Authenticated rather than Enrolled, and pointedly so. Every other write that ends in somebody -/// reading a vault needs a key of the caller's own; this one does not, because an invitation grants -/// membership and membership is not readability. Requiring enrollment here would also be requiring it -/// of the wrong person — the invitee is the one with no key, and they have no account yet either. -/// -internal sealed class CreateTeamInvitationEndpoint( - ICurrentUserContext currentUser, - TeamService teams, - TeamInvitationService invitations) - : Endpoint, NotFound, ProblemHttpResult>> -{ - /// - public override void Configure() - { - Post("/api/v1/teams/{teamId:guid}/invitations"); - - Policies(Auth.AuthenticatedPolicy); - - Description(b => b - .WithName("CreateTeamInvitation") - .WithSummary("Invites an email address to a team.") - .WithTags("Teams")); - } - - /// - public override async Task, NotFound, ProblemHttpResult>> ExecuteAsync( - CreateTeamInvitationRequest req, - CancellationToken ct) - { - var user = await currentUser.GetOrProvisionAsync(ct).ConfigureAwait(false); - var teamId = Route("teamId"); - - var access = await teams.ResolveAsync(user.Id, teamId, ct).ConfigureAwait(false); - - if (!access.Granted) - { - return TypedResults.NotFound(); - } - - if (!access.CanAdminister) - { - return Problems.Coded( - StatusCodes.Status403Forbidden, - ProblemCodes.Forbidden, - "Only an admin or the owner of this team can invite people to it."); - } - - try - { - var invitation = await invitations - .CreateAsync(user, teamId, req, ct) - .ConfigureAwait(false); - - return TypedResults.Ok(invitation); - } - catch (TeamInvitationInvalidException exception) - { - return Problems.Coded( - StatusCodes.Status400BadRequest, - ProblemCodes.InvalidTeamInvitation, - exception.Message); - } - } -} - -/// Withdraws an invitation that has not been taken up. -/// -/// 404 for an invitation that is not there, is not this team's, or has already been claimed — the -/// same answer for all three, and for the reason revoking a device grant gives: a caller driving -/// towards "that invitation will not let anybody in" can treat 404 as having arrived. A claimed one -/// is a membership now, and removing a member is a different operation with different consequences. -/// -internal sealed class RevokeTeamInvitationEndpoint( - ICurrentUserContext currentUser, - TeamService teams, - TeamInvitationService invitations) - : EndpointWithoutRequest> -{ - /// - public override void Configure() - { - Delete("/api/v1/teams/{teamId:guid}/invitations/{invitationId:guid}"); - - Policies(Auth.AuthenticatedPolicy); - - Description(b => b - .WithName("RevokeTeamInvitation") - .WithSummary("Withdraws an invitation that has not been taken up.") - .WithTags("Teams")); - } - - /// - public override async Task> ExecuteAsync( - CancellationToken ct) - { - var user = await currentUser.GetOrProvisionAsync(ct).ConfigureAwait(false); - var teamId = Route("teamId"); - var invitationId = Route("invitationId"); - - var access = await teams.ResolveAsync(user.Id, teamId, ct).ConfigureAwait(false); - - if (!access.Granted) - { - return TypedResults.NotFound(); - } - - if (!access.CanAdminister) - { - return Problems.Coded( - StatusCodes.Status403Forbidden, - ProblemCodes.Forbidden, - "Only an admin or the owner of this team can withdraw an invitation."); - } - - var revoked = await invitations - .RevokeAsync(user, teamId, invitationId, ct) - .ConfigureAwait(false); - - return revoked ? TypedResults.NoContent() : TypedResults.NotFound(); - } -} - /// Creates a vault owned by a team. internal sealed class CreateTeamVaultEndpoint( ICurrentUserContext currentUser, diff --git a/src/DodoSSH.Api/Features/Teams/TeamExceptions.cs b/src/DodoSSH.Api/Features/Teams/TeamExceptions.cs index 36651b9..7513295 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamExceptions.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamExceptions.cs @@ -36,20 +36,19 @@ internal sealed class TeamNotEmptyException(string message) : Exception(message) /// The address given to an add has no account on this server. /// -/// Separate from because the caller can act on it without being -/// told to: there is nobody to add, so the address is invited instead. Folded into the general code it -/// would be indistinguishable from a rejected role, and a client would have to guess which it was. +/// Separate from because it is the one refusal on this path that is +/// not about the request: the request was well formed and named somebody who is not here. Folded into +/// the general code it would be indistinguishable from a rejected role, and a client wanting to say +/// "ask them to sign in first" would have to guess which of the two it had. +/// +/// It is the end of the road rather than a step on it. Membership is only ever granted to an account +/// that exists — see docs/adr/0009-team-access-model.md — so there is nothing else for a client +/// to try, and the honest answer is to name the address and say what has to happen before it can be +/// added. +/// /// internal sealed class NoSuchAccountException(string message) : Exception(message); -/// An invitation was rejected. -/// -/// Separate from because its commonest cause has a different -/// remedy: an address that already has an account here should be added through the directory, which -/// is the path that shows the caller the public key they are about to trust. -/// -internal sealed class TeamInvitationInvalidException(string message) : Exception(message); - /// /// A vault key grant was rejected. /// diff --git a/src/DodoSSH.Api/Features/Teams/TeamInvitationService.cs b/src/DodoSSH.Api/Features/Teams/TeamInvitationService.cs deleted file mode 100644 index fda9b93..0000000 --- a/src/DodoSSH.Api/Features/Teams/TeamInvitationService.cs +++ /dev/null @@ -1,488 +0,0 @@ -using DodoSSH.Api.Authorization; -using DodoSSH.Contracts; -using DodoSSH.Domain; -using DodoSSH.Infrastructure; -using Microsoft.EntityFrameworkCore; -using Npgsql; - -namespace DodoSSH.Api.Features.Teams; - -/// -/// Invitations to addresses that have no account here yet, and the sign-in path that claims them. -/// -/// -/// -/// An invitation is a standing instruction, not a message and not a token. 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 to sign in with this address joins this team as this role", and telling -/// them to sign in is the caller's job over a channel this server does not carry. That is a smaller -/// feature than the design drew, and it is the whole of what can be built honestly without a mail -/// path — a link nobody can deliver would be worse than none. -/// -/// -/// Verification is the security boundary, and it is the only one. Membership is authorization -/// (ADR 0009), so an invitation decides what the server will serve. Claiming one on an address the -/// identity provider has not marked verified would let anybody who can get a token asserting somebody -/// else's address walk into their team — which is precisely the attack -/// OidcOptions.AllowEmailLinking exists to refuse. So an unverified address claims nothing, -/// there is no setting that relaxes it, and the refusal is logged rather than silent. -/// -/// -/// What an invitation still cannot do is make anything readable. It creates a membership, and a -/// membership is not a key — somebody has to wrap the vault key to them afterwards, from a machine -/// that holds it. The split ADR 0009 describes is not weakened by this; the invitation simply moves -/// the first half of it earlier. -/// -/// -internal sealed class TeamInvitationService( - DodoDbContext database, - TimeProvider clock, - ILogger logger) - : ITeamInvitationClaim -{ - /// Longest acceptable address. Matches the column, and RFC 5321's own limit. - private const int MaxEmailLength = 320; - - /// - /// How long an invitation stays claimable. - /// - /// - /// Fourteen days, and finite for a reason rather than as a default. An invitation that never - /// expired would be a standing offer against an address, and addresses are reassigned — a - /// company address handed to the next person to hold the job would let them into a team the - /// person who left was invited to. Fourteen days is long enough to survive a holiday and short - /// enough that a forgotten invitation lapses rather than waiting. - /// - private static readonly TimeSpan Lifetime = TimeSpan.FromDays(14); - - /// Lists a team's invitations, including the ones already dealt with. - /// - /// Every member may read this, as with the members list and for the same reason: whoever is about - /// to be handed a vault key needs to see who else is on their way into the team. Accepted and - /// revoked rows are returned too, so the screen can show that an invitation was taken up rather - /// than having it silently vanish and read as never sent. - /// - internal async Task> ListAsync( - Guid teamId, - CancellationToken cancellationToken) - { - var invitations = await database.TeamInvitations - .Where(i => i.TeamId == teamId) - .OrderByDescending(i => i.CreatedAtUtc) - .ToListAsync(cancellationToken) - .ConfigureAwait(false); - - var now = clock.GetUtcNow(); - - return [.. invitations.Select(invitation => Describe(invitation, now))]; - } - - /// Invites an address to a team. - /// - /// - /// An address that already has an account here is accepted rather than refused. The obvious - /// alternative — refusing and pointing at the directory — would turn this endpoint into an oracle - /// for which addresses have accounts, answerable by anybody willing to create a team first. It - /// would also be answering a question the caller did not ask: they want that person in the team, - /// and whether the account exists yet only changes how soon it happens. An existing account picks - /// the invitation up on its next request, within the hour. - /// - /// - /// Idempotent on the client-chosen id, as team and vault creation are: the same id, team and - /// address returns the existing invitation rather than a second one. A different address under an - /// id already in use is refused rather than reinterpreted. - /// - /// - internal async Task CreateAsync( - UserAccount actor, - Guid teamId, - CreateTeamInvitationRequest request, - CancellationToken cancellationToken) - { - ArgumentNullException.ThrowIfNull(request); - - var email = RequireEmail(request.Email); - var role = RequireInvitableRole(request.Role); - - if (request.InvitationId == Guid.Empty) - { - throw new TeamInvitationInvalidException( - "An invitation id is required. Generate a UUIDv7 on the client."); - } - - var now = clock.GetUtcNow(); - - var existing = await database.TeamInvitations - .SingleOrDefaultAsync(i => i.Id == request.InvitationId, cancellationToken) - .ConfigureAwait(false); - - if (existing is not null) - { - return ResolveExisting(existing, teamId, email, now); - } - - await RefuseIfAlreadyAMemberAsync(teamId, email, cancellationToken).ConfigureAwait(false); - - var invitation = new TeamInvitation - { - Id = request.InvitationId, - TeamId = teamId, - Email = email, - Role = role, - InvitedByUserId = actor.Id, - CreatedAtUtc = now, - ExpiresAtUtc = now + Lifetime, - }; - - database.TeamInvitations.Add(invitation); - - try - { - await database.SaveChangesAsync(cancellationToken).ConfigureAwait(false); - } - catch (DbUpdateException exception) when (IsUniqueViolation(exception)) - { - // The partial unique index on (team, email) among live invitations. Reported as its own - // sentence because it is the one failure the caller could not see coming from their own - // input — somebody else may have invited the same person a minute earlier. - throw new TeamInvitationInvalidException( - "There is already an invitation to that address for this team. Withdraw it first if " - + "you want to invite them at a different role."); - } - - TeamLog.InvitationIssued(logger, invitation.Id, teamId, role, actor.Id); - - return Describe(invitation, now); - } - - /// - /// A retry is the same id against the same team and address. Anything else under an id already in - /// use is refused rather than reinterpreted: returning a differently-addressed invitation would - /// tell a client its invite went to somebody it did not. - /// - private static TeamInvitationSummary ResolveExisting( - TeamInvitation existing, - Guid teamId, - string email, - DateTimeOffset now) - { - var isRetry = existing.TeamId == teamId - && string.Equals(existing.Email, email, StringComparison.OrdinalIgnoreCase); - - return isRetry - ? Describe(existing, now) - : throw new TeamInvitationInvalidException( - "That invitation id is already in use. Generate a new UUIDv7 and retry."); - } - - private static TeamRole RequireInvitableRole(TeamMemberRole role) - { - var domain = ToDomain(role); - - return domain is TeamRole.Unspecified or TeamRole.Owner - ? throw new TeamInvitationInvalidException( - "Invite somebody as a viewer, member or admin. Ownership is sole and is handed over " - + "deliberately, never conferred by an address signing in.") - : domain; - } - - /// Withdraws an invitation that has not been taken up. - /// Whether there was a live invitation to withdraw. - /// - /// An invitation that has already been claimed is not withdrawable, and answering false - /// rather than unpicking it is the honest outcome: it is a membership now, and removing a member - /// is a different operation with different consequences — it revokes their vault key grants and - /// flags every team vault for rekey. - /// - internal async Task RevokeAsync( - UserAccount actor, - Guid teamId, - Guid invitationId, - CancellationToken cancellationToken) - { - var invitation = await database.TeamInvitations - .SingleOrDefaultAsync( - i => i.Id == invitationId - && i.TeamId == teamId - && i.AcceptedAtUtc == null - && i.RevokedAtUtc == null, - cancellationToken) - .ConfigureAwait(false); - - if (invitation is null) - { - return false; - } - - invitation.RevokedAtUtc = clock.GetUtcNow(); - - await database.SaveChangesAsync(cancellationToken).ConfigureAwait(false); - - TeamLog.InvitationRevoked(logger, invitation.Id, teamId, actor.Id); - - return true; - } - - /// - public async Task ClaimAsync( - UserAccount user, - string? email, - bool emailVerified, - CancellationToken cancellationToken) - { - ArgumentNullException.ThrowIfNull(user); - - if (string.IsNullOrWhiteSpace(email)) - { - return 0; - } - - var now = clock.GetUtcNow(); - - var pending = await database.TeamInvitations - .Where(i => i.Email == email - && i.AcceptedAtUtc == null - && i.RevokedAtUtc == null - && i.ExpiresAtUtc > now) - .ToListAsync(cancellationToken) - .ConfigureAwait(false); - - if (pending.Count == 0) - { - return 0; - } - - if (!emailVerified) - { - // Logged rather than silent, and this is the only signal an operator gets that their - // provider is not sending the claim. Without it, invitations would simply never work and - // there would be nothing anywhere saying why. - TeamLog.InvitationNotClaimedUnverified(logger, pending.Count, user.Id); - return 0; - } - - return await ApplyAsync(user, pending, now, cancellationToken).ConfigureAwait(false); - } - - /// Turns each claimable invitation into an active membership. - private async Task ApplyAsync( - UserAccount user, - List pending, - DateTimeOffset now, - CancellationToken cancellationToken) - { - var teamIds = pending.Select(i => i.TeamId).ToArray(); - - // Archived teams are excluded here as well as at archive time. An invitation issued moments - // before an archive can still be in flight, and joining a team nobody can see is worse than - // an invitation that quietly lapses. - var liveTeamIds = await database.Teams - .Where(t => teamIds.Contains(t.Id) && t.DeletedAtUtc == null) - .Select(t => t.Id) - .ToListAsync(cancellationToken) - .ConfigureAwait(false); - - var live = liveTeamIds.ToHashSet(); - - var memberships = await database.TeamMemberships - .Where(m => teamIds.Contains(m.TeamId) && m.UserId == user.Id && m.DeletedAtUtc == null) - .ToListAsync(cancellationToken) - .ConfigureAwait(false); - - var claimed = 0; - - foreach (var invitation in pending.Where(i => live.Contains(i.TeamId))) - { - invitation.AcceptedAtUtc = now; - invitation.AcceptedByUserId = user.Id; - - if (Join(user, invitation, memberships.Find(m => m.TeamId == invitation.TeamId), now)) - { - claimed++; - - TeamLog.InvitationClaimed( - logger, user.Id, invitation.Id, invitation.TeamId, invitation.Role); - } - } - - return await SaveClaimAsync(claimed, cancellationToken).ConfigureAwait(false); - } - - /// Adds or reactivates the membership an invitation asks for. - /// Whether the membership changed. False means they were already an active member. - private bool Join( - UserAccount user, - TeamInvitation invitation, - TeamMembership? membership, - DateTimeOffset now) - { - if (membership is null) - { - database.TeamMemberships.Add(new TeamMembership - { - Id = Guid.CreateVersion7(), - TeamId = invitation.TeamId, - UserId = user.Id, - Role = invitation.Role, - Status = MembershipStatus.Active, - InvitedByUserId = invitation.InvitedByUserId, - JoinedAtUtc = now, - CreatedAtUtc = now, - }); - - return true; - } - - if (membership.Status == MembershipStatus.Active) - { - // Already in the team — the invitation is satisfied rather than applied. It must not - // change a role somebody set deliberately in the meantime, which is what re-applying an - // invitation issued weeks ago would silently do. - return false; - } - - // Removed earlier and invited again. The row is reactivated rather than duplicated, exactly - // as TeamService.AddMemberAsync does, so historic audit entries stay resolvable to one - // membership. Their revoked key grants are not restored — those were wrapped to a generation - // the vault has since been flagged to leave behind. - membership.Role = invitation.Role; - membership.Status = MembershipStatus.Active; - membership.JoinedAtUtc = now; - - return true; - } - - /// - /// Its own SaveChanges, never folded into the caller's. CurrentUserContext.ProvisionAsync - /// catches a unique violation and re-reads the account by (issuer, subject); a claim sharing that - /// call would put violations from this table inside a filter written for exactly one race, and - /// its rethrow would stop being correct. - /// - private async Task SaveClaimAsync(int claimed, CancellationToken cancellationToken) - { - try - { - await database.SaveChangesAsync(cancellationToken).ConfigureAwait(false); - } - catch (DbUpdateException exception) when (IsUniqueViolation(exception)) - { - // Two of this account's requests claiming at once. One wins; the other finds nothing - // left to do on the next sweep. Swallowed rather than surfaced because this runs inside - // the authorization middleware, where a throw is a 500 on a request that was otherwise - // fine — and because the outcome the caller wanted has happened either way. - foreach (var entry in database.ChangeTracker.Entries().ToList()) - { - entry.State = EntityState.Detached; - } - - return 0; - } - - return claimed; - } - - /// - /// Refused only for an account that is already in this team — a fact about a team the - /// caller can see, so naming it leaks nothing. Whether an address has an account at all is - /// deliberately not answered here; see . - /// - private async Task RefuseIfAlreadyAMemberAsync( - Guid teamId, - string email, - CancellationToken cancellationToken) - { - var isMember = await database.TeamMemberships - .Where(m => m.TeamId == teamId - && m.Status == MembershipStatus.Active - && m.DeletedAtUtc == null) - .Join( - database.Users.Where(u => u.Email == email && u.DeletedAtUtc == null), - m => m.UserId, - u => u.Id, - (m, u) => m.Id) - .AnyAsync(cancellationToken) - .ConfigureAwait(false); - - if (isMember) - { - throw new TeamInvitationInvalidException( - "That address already belongs to a member of this team. Change their role instead."); - } - } - - /// Derives what has become of an invitation from its timestamps. - /// - /// Computed rather than stored, which is why has no domain twin. - /// Expiry is a fact about the clock: a stored state would need a sweeper to keep it true, and an - /// invitation that read Pending because nothing had run yet would be a lie the interface repeats. - /// - private static TeamInvitationSummary Describe(TeamInvitation invitation, DateTimeOffset now) - { - var state = invitation switch - { - { AcceptedAtUtc: not null } => TeamInvitationState.Accepted, - { RevokedAtUtc: not null } => TeamInvitationState.Revoked, - _ when invitation.ExpiresAtUtc <= now => TeamInvitationState.Expired, - _ => TeamInvitationState.Pending, - }; - - return new TeamInvitationSummary( - invitation.Id, - invitation.Email, - ToContract(invitation.Role), - state, - invitation.InvitedByUserId, - invitation.CreatedAtUtc, - invitation.ExpiresAtUtc, - invitation.AcceptedAtUtc); - } - - /// - /// Deliberately shallow. This checks the shape the column and the claim path need — one at-sign - /// with something either side, no spaces, and inside the length the column holds — and nothing - /// more. A stricter address grammar here would reject addresses that a real identity provider - /// will happily assert, and the only thing that ultimately decides whether an address is that - /// person's is the provider marking it verified. - /// - private static string RequireEmail(string? value) - { - var email = (value ?? string.Empty).Trim(); - - var at = email.IndexOf('@', StringComparison.Ordinal); - - var acceptable = email.Length is > 2 and <= MaxEmailLength - && at > 0 - && at == email.LastIndexOf('@') - && at < email.Length - 1 - && !email.Any(char.IsWhiteSpace); - - return acceptable - ? email - : throw new TeamInvitationInvalidException( - "That does not look like an email address. Invite the address they sign in with."); - } - - private static TeamRole ToDomain(TeamMemberRole role) => role switch - { - TeamMemberRole.Viewer => TeamRole.Viewer, - TeamMemberRole.Member => TeamRole.Member, - TeamMemberRole.Admin => TeamRole.Admin, - TeamMemberRole.Owner => TeamRole.Owner, - _ => TeamRole.Unspecified, - }; - - private static TeamMemberRole ToContract(TeamRole role) => role switch - { - TeamRole.Viewer => TeamMemberRole.Viewer, - TeamRole.Member => TeamMemberRole.Member, - TeamRole.Admin => TeamMemberRole.Admin, - TeamRole.Owner => TeamMemberRole.Owner, - _ => TeamMemberRole.Unspecified, - }; - - private static bool IsUniqueViolation(DbUpdateException exception) => - string.Equals( - (exception.InnerException as PostgresException)?.SqlState, - PostgresErrorCodes.UniqueViolation, - StringComparison.Ordinal); -} diff --git a/src/DodoSSH.Api/Features/Teams/TeamLog.cs b/src/DodoSSH.Api/Features/Teams/TeamLog.cs index d577a73..a6ab644 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamLog.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamLog.cs @@ -134,49 +134,4 @@ internal static partial class TeamLog + "The former owner is now an admin.")] internal static partial void OwnershipTransferred( ILogger logger, Guid teamId, Guid formerOwnerId, Guid newOwnerId); - - /// - /// The invitation id, never the address. TeamLog's rule is ids and outcomes only, and an email is - /// exactly the kind of personal detail a log aggregator would then keep for its whole retention. - /// - [LoggerMessage( - EventId = 2111, - Level = LogLevel.Information, - Message = "Issued invitation {InvitationId} to team {TeamId} as {Role}, by {ActorId}.")] - internal static partial void InvitationIssued( - ILogger logger, Guid invitationId, Guid teamId, Domain.TeamRole role, Guid actorId); - - [LoggerMessage( - EventId = 2112, - Level = LogLevel.Information, - Message = "Revoked invitation {InvitationId} to team {TeamId}, by {ActorId}.")] - internal static partial void InvitationRevoked( - ILogger logger, Guid invitationId, Guid teamId, Guid actorId); - - [LoggerMessage( - EventId = 2113, - Level = LogLevel.Information, - Message = "User {UserId} claimed invitation {InvitationId} and joined team {TeamId} as {Role}.")] - internal static partial void InvitationClaimed( - ILogger logger, Guid userId, Guid invitationId, Guid teamId, Domain.TeamRole role); - - /// - /// - /// Warning, and the one log line an operator will need when invitations appear not to work at all. - /// A provider that does not assert email_verified leaves every invitation pending for ever - /// with nothing else to show for it, and this is the only place that difference is visible. - /// - /// - /// It names the count and the account, never the address — the address is the thing being refused - /// as untrustworthy, and writing it to a log would be keeping a claim the server just rejected. - /// - /// - [LoggerMessage( - EventId = 2114, - Level = LogLevel.Warning, - Message = "Left {InvitationCount} invitation(s) unclaimed for user {UserId}: the access token " - + "does not assert that their email address is verified. Check the identity provider " - + "sends the email_verified claim.")] - internal static partial void InvitationNotClaimedUnverified( - ILogger logger, int invitationCount, Guid userId); } diff --git a/src/DodoSSH.Api/Features/Teams/TeamService.cs b/src/DodoSSH.Api/Features/Teams/TeamService.cs index ff681f3..cf7ea3f 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamService.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamService.cs @@ -277,20 +277,6 @@ internal sealed class TeamService( membership.DeletedAtUtc = now; } - // Pending invitations go too. An invitation that outlived its team would become a - // membership of something nobody can see, on a sign-in weeks later. - var invitations = await database.TeamInvitations - .Where(i => i.TeamId == team.Id - && i.AcceptedAtUtc == null - && i.RevokedAtUtc == null) - .ToListAsync(cancellationToken) - .ConfigureAwait(false); - - foreach (var invitation in invitations) - { - invitation.RevokedAtUtc = now; - } - team.DeletedAtUtc = now; await database.SaveChangesAsync(cancellationToken).ConfigureAwait(false); @@ -577,8 +563,9 @@ internal sealed class TeamService( /// Both misses are specific, and neither is a new oracle. An id confirms nothing the caller did not /// already know from the lookup that produced it. An address is answered only for an admin or owner /// of the team the add names — checked by the endpoint before this runs — and is the same fact the - /// member list would show them a moment later. It carries its own code so the caller can invite the - /// address instead of reporting a failure at somebody who simply is not here yet. + /// member list would show them a moment later. It carries its own code so the caller can say what + /// has to happen next — that person signing in here once — rather than reporting a failure at + /// somebody who simply is not here yet. /// /// private async Task ResolveTargetAsync( @@ -614,8 +601,8 @@ internal sealed class TeamService( .ConfigureAwait(false) ?? throw new NoSuchAccountException( - "No account here uses that address yet. Invite it instead — they join when they " - + "first sign in."); + "No account here uses that address yet. Ask them to sign in to this server once, " + + "which is what creates the account, and then add them."); } /// diff --git a/src/DodoSSH.Api/Program.cs b/src/DodoSSH.Api/Program.cs index d5702d6..650f060 100644 --- a/src/DodoSSH.Api/Program.cs +++ b/src/DodoSSH.Api/Program.cs @@ -37,21 +37,14 @@ builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); -builder.Services.AddScoped(); - -// Registered twice on purpose, resolving to the same scoped instance: the endpoints take the -// concrete service, and CurrentUserContext takes only the claim it needs, so the sign-in path does -// not gain a reference to the whole of a feature it calls one method on. -builder.Services.AddScoped( - provider => provider.GetRequiredService()); builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddSingleton(); // A singleton, because the sockets it holds outlive the requests that opened them. Registered twice -// resolving to the same instance, for the reason the invitation claim above is: the endpoint needs the -// whole hub — admit, remove, count — while the write paths that announce a change need only the two -// methods that announce one, and should not gain a reference to connection management to get them. +// resolving to the same instance: the endpoint needs the whole hub — admit, remove, count — while the +// write paths that announce a change need only the two methods that announce one, and should not gain +// a reference to connection management to get them. builder.Services.AddSingleton(); builder.Services.AddSingleton( provider => provider.GetRequiredService()); diff --git a/src/DodoSSH.Api/Setup/DodoOptions.cs b/src/DodoSSH.Api/Setup/DodoOptions.cs index 31ed25d..6fb14e4 100644 --- a/src/DodoSSH.Api/Setup/DodoOptions.cs +++ b/src/DodoSSH.Api/Setup/DodoOptions.cs @@ -62,28 +62,15 @@ public sealed class OidcOptions public string EmailClaim { get; set; } = "email"; /// Claim type holding the user's display name. - public string NameClaim { get; set; } = "name"; - - /// - /// Claim type asserting that the provider has verified the user's email. - /// /// - /// - /// Read for exactly one purpose: deciding whether a pending team invitation addressed to that - /// email may be claimed. Nothing else in this server trusts the email claim for anything, and - /// records why — a token from any configured provider carrying a - /// victim's address must not confer access to anything of theirs. An invitation is access, so it - /// needs the same bar. - /// - /// - /// Absence is a refusal, not a default. A provider that does not send this claim leaves - /// every invitation pending for ever, which is visible on the teams screen and diagnosable in the - /// log. There is deliberately no option to trust an unverified address instead: a flag that exists - /// is a flag somebody turns on for the afternoon their provider is misconfigured, and this is the - /// one it must not be possible to turn on. - /// + /// The email and the display name are read for display and for the directory's address lookup, and + /// for nothing that decides access. There was an EmailVerifiedClaim beside these, read by + /// exactly one feature — claiming a team invitation addressed to that address — and it went with + /// the invitations. Nothing in this server now trusts the email claim to grant anything, which is + /// the property spends its whole doc comment defending; re-adding a + /// setting here would be the first step back towards a token's address being a way in. /// - public string EmailVerifiedClaim { get; set; } = "email_verified"; + public string NameClaim { get; set; } = "name"; } /// Schema management. diff --git a/src/DodoSSH.Api/Setup/EndpointRegistration.cs b/src/DodoSSH.Api/Setup/EndpointRegistration.cs index 5260569..3fde3ba 100644 --- a/src/DodoSSH.Api/Setup/EndpointRegistration.cs +++ b/src/DodoSSH.Api/Setup/EndpointRegistration.cs @@ -54,9 +54,6 @@ internal static class EndpointRegistration typeof(AddTeamMemberEndpoint), typeof(ChangeTeamMemberRoleEndpoint), typeof(RemoveTeamMemberEndpoint), - typeof(ListTeamInvitationsEndpoint), - typeof(CreateTeamInvitationEndpoint), - typeof(RevokeTeamInvitationEndpoint), typeof(CreateTeamVaultEndpoint), typeof(RenameVaultEndpoint), typeof(DeleteVaultEndpoint), diff --git a/src/DodoSSH.Client.Android/Views/MoreScreen.axaml b/src/DodoSSH.Client.Android/Views/MoreScreen.axaml index 6c42238..0b98980 100644 --- a/src/DodoSSH.Client.Android/Views/MoreScreen.axaml +++ b/src/DodoSSH.Client.Android/Views/MoreScreen.axaml @@ -129,8 +129,8 @@ - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/src/DodoSSH.Client.Api/DodoSshApiClient.cs b/src/DodoSSH.Client.Api/DodoSshApiClient.cs index 8f84985..6261534 100644 --- a/src/DodoSSH.Client.Api/DodoSshApiClient.cs +++ b/src/DodoSSH.Client.Api/DodoSshApiClient.cs @@ -123,30 +123,6 @@ public interface ITeamApi /// Task RemoveTeamMemberAsync(Guid teamId, Guid userId, CancellationToken cancellationToken); - /// Lists a team's invitations, including the ones already dealt with. - Task> ListTeamInvitationsAsync( - Guid teamId, - CancellationToken cancellationToken); - - /// Invites an email address to a team. - Task CreateTeamInvitationAsync( - Guid teamId, - CreateTeamInvitationRequest request, - CancellationToken cancellationToken); - - /// - /// Withdraws an invitation that has not been taken up. - /// - /// - /// Whether there was a live invitation to withdraw. False covers one that was never there and one - /// already claimed — a claimed invitation is a membership now, and removing a member is a different - /// operation with different consequences. - /// - Task RevokeTeamInvitationAsync( - Guid teamId, - Guid invitationId, - CancellationToken cancellationToken); - /// Creates a vault owned by a team, with the creator's key grant. Task CreateTeamVaultAsync( Guid teamId, @@ -506,39 +482,6 @@ public sealed class DodoSshApiClient(HttpClient http, IAccessTokenProvider token string.Create(CultureInfo.InvariantCulture, $"{TeamsPath}/{teamId}/members/{userId}"), cancellationToken); - /// - public Task> ListTeamInvitationsAsync( - Guid teamId, - CancellationToken cancellationToken) => - SendAsync( - HttpMethod.Get, - string.Create(CultureInfo.InvariantCulture, $"{TeamsPath}/{teamId}/invitations"), - null, - DodoSshJsonContext.Default.IReadOnlyListTeamInvitationSummary, - cancellationToken); - - /// - public Task CreateTeamInvitationAsync( - Guid teamId, - CreateTeamInvitationRequest request, - CancellationToken cancellationToken) => - SendAsync( - HttpMethod.Post, - string.Create(CultureInfo.InvariantCulture, $"{TeamsPath}/{teamId}/invitations"), - JsonContent.Create(request, DodoSshJsonContext.Default.CreateTeamInvitationRequest), - DodoSshJsonContext.Default.TeamInvitationSummary, - cancellationToken); - - /// - public Task RevokeTeamInvitationAsync( - Guid teamId, - Guid invitationId, - CancellationToken cancellationToken) => - DeleteAsync( - string.Create( - CultureInfo.InvariantCulture, $"{TeamsPath}/{teamId}/invitations/{invitationId}"), - cancellationToken); - /// public Task CreateTeamVaultAsync( Guid teamId, diff --git a/src/DodoSSH.Client.App/Views/VaultsScreen.axaml b/src/DodoSSH.Client.App/Views/VaultsScreen.axaml index 6fc7ed8..6a8582d 100644 --- a/src/DodoSSH.Client.App/Views/VaultsScreen.axaml +++ b/src/DodoSSH.Client.App/Views/VaultsScreen.axaml @@ -12,7 +12,7 @@ The left column used to list teams; a team owned vaults, and sharing meant creating a team, then a vault in it, then wrapping a key. Two of those three steps were about a concept nobody came here for. So the rows are vaults now: naming one makes the membership list that carries it, and everything on - the right — members, invitations, key holders — is that vault's. The server still authorises against + the right — members, key holders — is that vault's. The server still authorises against a team, because that is what VaultAccessService resolves; what went is the requirement that a person know it exists. The one case where it is still visible is a membership list carrying several vaults, which this screen cannot make and will not hide: see SharedMembershipWarning. @@ -25,11 +25,12 @@ than a checkbox on the member row. What the design asked for and is still not here: two-factor state (no such concept exists anywhere in - this product) and avatars (no picture is stored anywhere). Nothing is sent for an invitation — there - is no outbound mail path and no token, so an invitation is a standing instruction that the next - account signing in with that address joins, and there is consequently nothing to resend. Last-active - is recorded at most once per account per hour, so it is drawn coarsely. Nor is there a way to delete a - vault: the server has no such call, and the screen says so rather than offering a button that refuses. + this product) and avatars (no picture is stored anywhere). There is no INVITED list either, and that + one is a decision rather than a gap — an address is not a way into a vault, so only an account that + already exists can be added and there is nothing pending to draw. See the ADD box below, which says + what to do about somebody who has not signed in here yet. Last-active is recorded at most once per + account per hour, so it is drawn coarsely. Nor is there a way to delete a vault: the server has no + such call, and the screen says so rather than offering a button that refuses. --> @@ -261,11 +262,11 @@ - +