diff --git a/CLAUDE.md b/CLAUDE.md index 3eb78dac0..8003db52e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -128,7 +128,7 @@ One browser tab's content writes to an item are ordered by the server, also **pe - `GET /api/v1/me/invitations`, `POST /api/v1/me/invitations/{id}/accept` — the caller's pending, unexpired invitations to live workspaces they are not already in (PLAN-3002 U4b / TASK-3277), backing the Invitations section of the TopBar "+". Matched on the caller's email ONLY when it is VERIFIED, since an unverified address is just a string typed at signup; every self-hosted account is verified (migration 070 + `CreateUser`'s default), so this bites only on Pad Cloud self-serve accounts that have not confirmed, which get an empty list with `email_verified: false`. Entries carry no code (only its hash is stored), hence the by-id accept, which shares `acceptInvitationCore` with `/invitations/{code}/accept` (membership, role, `accepted_at`, the `workspace_access_changed` gain). It differs in admission only: the caller must already be verified, and it never verifies them. Neither does the code route, since BUG-3348: the inviter also sees the code. What verifies is the mailbox-only proof the invitation EMAIL's link carries in its fragment (`/join/#proof=…`, TASK-3352), sent as `proof` on `POST /invitations/{code}/accept` or `invitation_proof` on register. It is stored hashed, single-use and bound to the invitation, and never appears in the invite response, the members or admin invitation lists, or copy-link; a wrong or spent one is ignored, not refused. An id that is not a pending invitation to the caller answers 404, byte-identical to an unknown id. Both accept routes' success bodies now also carry `workspace_slug` and `owner_username` (additive). An accept by a caller who is ALREADY a member, and the loser of two concurrent accepts, answers 200 and marks the invitation accepted without changing the member's role in either direction; the body's `role` is the role held, and nothing is published (BUG-3281; it used to be a 500). The web client (the TopBar "+" and its count badge) and the CLI (`pad workspace invitations` / `accept ` / `decline `, BUG-2136) use them; not on MCP - `GET /api/v1/auth/session` — auth status (`setup_required`, `setup_method`, `auth_method`, `authenticated`, `email_configured`, `user`) - `POST /api/v1/auth/bootstrap` — create the first admin account from localhost on a fresh instance -- `POST /api/v1/auth/register` — create account (admin-created or invitation-based after setup) +- `POST /api/v1/auth/register` — create account (admin-created or invitation-based after setup). An `invitation_code` signup claims the invitation in the same transaction as the membership (BUG-3438). If the invitation was declined, cancelled or replaced since the code was read: when the code was the signup's ADMISSION (registration is otherwise closed) the account is rolled back and the answer is `404 not_found` (`500` if the rollback itself fails, since the account then still holds the email); where the signup would have been admitted anyway (Pad Cloud self-serve, or an admin creating the account) the account is KEPT, joins nothing, and the 201 carries `invitation_not_joined: {code, message}` (`invitation_gone`, or `invitation_not_applied` for another refused claim) instead of `accepted_invitation`. Additive, omitempty - `POST /api/v1/auth/login` — email/password login (returns session token) - `POST /api/v1/auth/logout` — destroy session - `GET/PATCH /api/v1/auth/me` — current user profile (GET) and update name/password (PATCH) diff --git a/internal/server/bug3438_register_invitation_claim_test.go b/internal/server/bug3438_register_invitation_claim_test.go new file mode 100644 index 000000000..e704ed5d9 --- /dev/null +++ b/internal/server/bug3438_register_invitation_claim_test.go @@ -0,0 +1,368 @@ +package server + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-3438: register-with-code read the invitation, created the account, +// added the membership and then marked the invitation accepted without +// checking that the row was still there. An invitation declined, cancelled +// or replaced inside that window still let the new account join. The signup +// must claim the invitation in the transaction that adds the membership, and +// a claim that finds no row refuses the signup and rolls the account back, +// as the two accept doors do since BUG-2136. +func TestRegisterWithCode_InvitationGoneBeforeClaimJoinsNothing(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + register := func(email, code string) *httptest.ResponseRecorder { + return doRequest(f.srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": email, + "name": "Invitee", + "password": "correct-horse-battery-staple", + "invitation_code": code, + }) + } + joined := func(email string) bool { + t.Helper() + u, err := f.srv.store.GetUserByEmail(email) + if err != nil { + t.Fatalf("GetUserByEmail: %v", err) + } + if u == nil { + return false + } + m, err := f.srv.store.GetWorkspaceMember(f.wsID, u.ID) + if err != nil { + t.Fatalf("GetWorkspaceMember: %v", err) + } + return m != nil + } + + // CONTROL: nothing happens in the window; the signup joins. + ctl := f.invite("control@example.com", "editor") + if rr := register("control@example.com", ctl.Code); rr.Code != http.StatusCreated { + t.Fatalf("control signup: %d %s", rr.Code, rr.Body.String()) + } + if !joined("control@example.com") { + t.Fatal("control signup did not join") + } + + for _, tc := range []struct { + name string + email string + gone func(invID string) + }{ + {"declined or cancelled", "declined@example.com", func(invID string) { + if err := f.srv.store.DeleteInvitation(f.wsID, invID); err != nil { + t.Fatalf("DeleteInvitation: %v", err) + } + }}, + {"replaced by a re-invite", "replaced@example.com", func(string) { + f.invite("replaced@example.com", "viewer") + }}, + } { + t.Run(tc.name, func(t *testing.T) { + inv := f.invite(tc.email, "editor") + f.srv.registerInvitationPreClaimHook = tc.gone + defer func() { f.srv.registerInvitationPreClaimHook = nil }() + + rr := register(tc.email, inv.Code) + if rr.Code != http.StatusNotFound { + t.Errorf("signup with an invitation gone before its claim: %d %s, want 404", rr.Code, rr.Body.String()) + } + if joined(tc.email) { + t.Error("the signup joined the workspace through an invitation that was gone") + } + // Rolled back, so the invitee can sign up again with a live code. + if u, _ := f.srv.store.GetUserByEmail(tc.email); u != nil { + t.Error("the refused signup left its account behind, holding the email") + } + }) + } + }) +} + +// notJoinedBody is the 201 of a signup kept without its invitation. +type notJoinedBody struct { + Token string `json:"token"` + Accepted map[string]any `json:"accepted_invitation"` + NotJoined *struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"invitation_not_joined"` +} + +// Lead ruling on #1834: the account is rolled back only when the invitation +// was the signup's ADMISSION. Where registration is open anyway (Pad Cloud +// with a deliverable verification email, the self-serve path), the account is +// KEPT, nothing is joined, and the response says the invitation is no longer +// valid. The self-hosted legs above are the admission case and roll back. +func TestRegisterWithCode_OpenRegistrationKeepsTheAccountWhenTheInvitationIsGone(t *testing.T) { + srv, mails := newCloudEmailServer(t) + admin, err := srv.store.GetUserByEmail("admin@pad.test") + if err != nil || admin == nil { + t.Fatalf("admin lookup: %v", err) + } + ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Cloud Invite WS", OwnerID: admin.ID}) + if err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + inv, err := srv.store.CreateInvitation(ws.ID, "late@example.com", "editor", admin.ID) + if err != nil { + t.Fatalf("CreateInvitation: %v", err) + } + srv.registerInvitationPreClaimHook = func(invID string) { + if err := srv.store.DeleteInvitation(ws.ID, invID); err != nil { + t.Fatalf("DeleteInvitation: %v", err) + } + } + rr := doRequest(srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "late@example.com", "name": "Late", + "password": "correct-horse-battery-staple", "invitation_code": inv.Code, + }) + if rr.Code != http.StatusCreated { + t.Fatalf("open-registration signup with a gone invitation: %d %s, want 201 (account kept)", rr.Code, rr.Body.String()) + } + var body notJoinedBody + parseJSON(t, rr, &body) + if body.Token == "" { + t.Error("the kept account was not signed in") + } + if body.Accepted != nil { + t.Errorf("a signup that joined nothing reported accepted_invitation %v", body.Accepted) + } + if body.NotJoined == nil || body.NotJoined.Code != "invitation_gone" || body.NotJoined.Message == "" { + t.Errorf("invitation_not_joined = %+v, want code invitation_gone with a message", body.NotJoined) + } + u, _ := srv.store.GetUserByEmail("late@example.com") + if u == nil { + t.Fatal("open registration: the account was rolled back") + } + if member, _ := srv.store.IsWorkspaceMember(ws.ID, u.ID); member { + t.Error("the kept account joined through an invitation that was gone") + } + // Kept like any self-serve signup: unverified, with its link sent. + if u.IsEmailVerified() { + t.Error("the kept account is verified, though nothing proved the address") + } + select { + case m := <-mails: + if m.to != "late@example.com" { + t.Errorf("verification mail went to %q", m.to) + } + case <-time.After(5 * time.Second): + t.Error("the kept account was sent no verification link") + } +} + +// Codex r1: the claim now commits with the membership, so a membership alone no +// longer proves the claim landed. A failed claim beside a membership that came +// from elsewhere must not be reported as a joined signup. +func TestRegisterWithCode_FailedClaimBesideAMembershipIsNotAJoin(t *testing.T) { + e := newAcceptLimitEnv(t) + inv := e.invite(t, "beside@example.com") + // The claim fails (the workspace reaches its cap in the window), and the + // membership read finds a row, as an admin's concurrent add would leave. + e.srv.registerInvitationPreClaimHook = func(string) { e.setMemberCap(t, e.members(t)) } + e.srv.membershipCheck = func(workspaceID, userID string) (*models.WorkspaceMember, error) { + return &models.WorkspaceMember{WorkspaceID: workspaceID, UserID: userID, Role: "editor"}, nil + } + rr := e.register("beside@example.com", inv) + if rr.Code == http.StatusCreated { + t.Fatalf("a signup whose claim failed was reported joined: %s", rr.Body.String()) + } + if !e.stillPending(t, inv) { + t.Error("the invitation was consumed by a claim that failed") + } +} + +// Codex r2/r3: the account exists from CreateUser on, so a sign-in can mint it +// a session, and /forgot-password a reset token, before the claim. The +// rollback must still remove the account. +func TestRegisterWithCode_CredentialsMintedBeforeTheClaimDoNotBlockTheRollback(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + inv := f.invite("signedin@example.com", "editor") + f.srv.registerInvitationPreClaimHook = func(invID string) { + u, err := f.srv.store.GetUserByEmail("signedin@example.com") + if err != nil || u == nil { + t.Fatalf("lookup the new account: %v", err) + } + if _, err := f.srv.store.CreateSession(u.ID, "go-test", "192.0.2.1", "", time.Hour); err != nil { + t.Fatalf("CreateSession: %v", err) + } + // And a public /forgot-password, which mints a reset token (codex r3). + if _, err := f.srv.store.CreatePasswordReset(u.ID); err != nil { + t.Fatalf("CreatePasswordReset: %v", err) + } + if err := f.srv.store.DeleteInvitation(f.wsID, invID); err != nil { + t.Fatalf("DeleteInvitation: %v", err) + } + } + rr := doRequest(f.srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "signedin@example.com", "name": "Invitee", + "password": "correct-horse-battery-staple", "invitation_code": inv.Code, + }) + if rr.Code != http.StatusNotFound { + t.Fatalf("signup: %d %s, want 404", rr.Code, rr.Body.String()) + } + if u, _ := f.srv.store.GetUserByEmail("signedin@example.com"); u != nil { + t.Fatal("a session minted in the window kept the refused signup's account") + } + }) +} + +// Open registration, a claim refused for another reason (the workspace filled +// in the window): the account is kept and verified by its emailed proof, +// nothing is joined, the invitation stays pending, and the kept account +// accepts it from its own list once there is room. +func TestRegisterWithCode_OpenRegistrationKeepsTheAccountWhenTheClaimIsRefused(t *testing.T) { + srv, mails, adminTok, ws := newProofFixture(t) + _, code, proof := inviteByEmail(t, srv, mails, adminTok, ws.Slug, "proved@example.com") + admin, err := srv.store.GetUserByEmail("admin@pad.test") + if err != nil || admin == nil { + t.Fatalf("admin lookup: %v", err) + } + count := func() int { + members, err := srv.store.ListWorkspaceMembers(ws.ID) + if err != nil { + t.Fatal(err) + } + return len(members) + } + setCap := func(n int) { + if err := srv.store.SetUserPlanOverrides(admin.ID, fmt.Sprintf(`{"members_per_workspace":%d}`, n)); err != nil { + t.Fatalf("SetUserPlanOverrides: %v", err) + } + } + srv.registerInvitationPreClaimHook = func(string) { setCap(count()) } + rr := doRequest(srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "proved@example.com", "name": "Invitee", "password": "correct-horse-battery-staple", + "invitation_code": code, "invitation_proof": proof, + }) + srv.registerInvitationPreClaimHook = nil + if rr.Code != http.StatusCreated { + t.Fatalf("open-registration signup refused at the cap: %d %s, want 201 (account kept)", rr.Code, rr.Body.String()) + } + var body notJoinedBody + parseJSON(t, rr, &body) + if body.Accepted != nil || body.NotJoined == nil || body.NotJoined.Code != "invitation_not_applied" { + t.Errorf("accepted_invitation = %v, invitation_not_joined = %+v; want none and code invitation_not_applied", body.Accepted, body.NotJoined) + } + if !verified(t, srv, "proved@example.com") { + t.Error("the kept account is not verified, though its emailed proof was spent on it") + } + pending, err := srv.store.GetInvitationByCode(code) + if err != nil || pending == nil { + t.Fatalf("the invitation should stay pending: %v %v", pending, err) + } + // With room, the kept account accepts it from its own list. + setCap(count() + 10) + rr = doRequestWithCookie(srv, "POST", "/api/v1/me/invitations/"+pending.ID+"/accept", nil, body.Token) + if rr.Code != http.StatusOK { + t.Fatalf("accepting from the list afterwards: %d %s", rr.Code, rr.Body.String()) + } +} + +// Codex r5: the claim keeps a membership that already exists (an admin's add +// in the window) at ITS role, and the signup must report that role, as the +// accept doors do, not the invitation's. +func TestRegisterWithCode_ReportsTheRoleTheAccountHolds(t *testing.T) { + forEachDialect(t, func(t *testing.T, d store.DriverType) { + f := newAccessFixture(t, d) + inv := f.invite("added@example.com", "editor") + f.srv.registerInvitationPreClaimHook = func(string) { + u, err := f.srv.store.GetUserByEmail("added@example.com") + if err != nil || u == nil { + t.Fatalf("lookup the new account: %v", err) + } + if err := f.srv.store.AddWorkspaceMember(f.wsID, u.ID, "viewer"); err != nil { + t.Fatalf("AddWorkspaceMember: %v", err) + } + } + rr := doRequest(f.srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "added@example.com", "name": "Invitee", + "password": "correct-horse-battery-staple", "invitation_code": inv.Code, + }) + if rr.Code != http.StatusCreated { + t.Fatalf("signup: %d %s", rr.Code, rr.Body.String()) + } + var body struct { + Accepted struct { + Role string `json:"role"` + } `json:"accepted_invitation"` + } + parseJSON(t, rr, &body) + if body.Accepted.Role != "viewer" { + t.Errorf("accepted_invitation.role = %q, want the role held (viewer)", body.Accepted.Role) + } + }) +} + +// Codex r9: a claim the store refused BEFORE its commit is definitely not +// landed, so a failing membership read must not turn the open-registration +// outcome (account kept, told) into a 500 that leaves the account behind with +// no session and a retry that meets the duplicate-email 409. +func TestRegisterWithCode_OpenRegistrationRefusedClaimNeedsNoReconcileRead(t *testing.T) { + srv, mails, adminTok, ws := newProofFixture(t) + _, code, _ := inviteByEmail(t, srv, mails, adminTok, ws.Slug, "unread@example.com") + admin, err := srv.store.GetUserByEmail("admin@pad.test") + if err != nil || admin == nil { + t.Fatalf("admin lookup: %v", err) + } + srv.registerInvitationPreClaimHook = func(string) { + members, err := srv.store.ListWorkspaceMembers(ws.ID) + if err != nil { + t.Fatal(err) + } + if err := srv.store.SetUserPlanOverrides(admin.ID, fmt.Sprintf(`{"members_per_workspace":%d}`, len(members))); err != nil { + t.Fatalf("SetUserPlanOverrides: %v", err) + } + } + srv.membershipCheck = func(string, string) (*models.WorkspaceMember, error) { + return nil, fmt.Errorf("injected: membership read failed") + } + rr := doRequest(srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "unread@example.com", "name": "Invitee", + "password": "correct-horse-battery-staple", "invitation_code": code, + }) + if rr.Code != http.StatusCreated { + t.Fatalf("open-registration signup refused before commit, with an unreadable membership: %d %s, want 201", rr.Code, rr.Body.String()) + } + var body notJoinedBody + parseJSON(t, rr, &body) + if body.NotJoined == nil || body.NotJoined.Code != "invitation_not_applied" || body.Token == "" { + t.Errorf("invitation_not_joined = %+v, token set = %v; want invitation_not_applied and a session", body.NotJoined, body.Token != "") + } +} + +// Codex r9: when the invitation was the admission and the account rollback +// FAILS, the signup must not answer as if it had been rolled back: the +// account still holds the email. +func TestRegisterWithCode_FailedRollbackIsNotReportedAsARollback(t *testing.T) { + f := newAccessFixture(t, store.DriverSQLite) + inv := f.invite("stuck@example.com", "editor") + f.srv.registerInvitationPreClaimHook = func(invID string) { + if err := f.srv.store.DeleteInvitation(f.wsID, invID); err != nil { + t.Fatalf("DeleteInvitation: %v", err) + } + } + f.srv.signupRollbackDelete = func(string) error { return fmt.Errorf("injected: rollback failed") } + rr := doRequest(f.srv, "POST", "/api/v1/auth/register", map[string]string{ + "email": "stuck@example.com", "name": "Invitee", + "password": "correct-horse-battery-staple", "invitation_code": inv.Code, + }) + if rr.Code == http.StatusNotFound || rr.Code == http.StatusCreated { + t.Fatalf("a failed rollback answered %d %s; want a server error, not the rolled-back 404 nor a success", rr.Code, rr.Body.String()) + } + if rr.Code != http.StatusInternalServerError { + t.Errorf("status %d, want 500", rr.Code) + } +} diff --git a/internal/server/handlers_auth.go b/internal/server/handlers_auth.go index 24a52cbaa..14cfc423c 100644 --- a/internal/server/handlers_auth.go +++ b/internal/server/handlers_auth.go @@ -598,6 +598,37 @@ func (s *Server) handleBootstrap(w http.ResponseWriter, r *http.Request) { } // handleRegister creates a new user account. +// invitationNotJoined is the additive register response member for a signup +// that was kept without its invitation (BUG-3438): open registration, where +// the invitation was not the admission and failed at its claim. +type invitationNotJoined struct { + Code string `json:"code"` + Message string `json:"message"` +} + +// rollbackSignup removes an account this signup created moments ago and is +// now refusing (BUG-3438). It is the full account deletion, not a bare +// DELETE FROM users: the account exists from CreateUser on, so a sign-in, a +// /forgot-password or the signup's own verification mint can add rows that +// reference it before the refusal, and DeleteAccountAtomic removes or +// detaches every such row and retries one that lands late. +// +// Like every account deletion it then kicks the account's connections +// (TASK-3365): a sign-in in the same window can have opened a stream, which +// would otherwise run until its next revalidation tick (codex r6). The kick +// runs on an error too: a lost commit acknowledgement can land the deletion, +// and a kick for an account that survived only makes its connections +// re-check and keep going (codex r7). +func (s *Server) rollbackSignup(userID string) error { + del := s.store.DeleteAccountAtomic + if s.signupRollbackDelete != nil { + del = s.signupRollbackDelete + } + err := del(userID) + s.invalidateUserAccess(userID) + return err +} + // Registration is restricted to admins or users with a valid invitation code // so invitees can create an account via the /join/[code] flow. func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { @@ -695,9 +726,17 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { // fails SAFE (verified), never write-locked. selfServe := false adminCreated := false + reqUser := currentUser(r) + isAdmin := reqUser != nil && reqUser.Role == "admin" + // inviteIsAdmission: the invitation is what let this signup in, i.e. it + // would have been refused without one. Only then does an invitation that + // fails at its claim take the account with it; where registration is open + // anyway, the account is kept and the invitation reported not applied + // (BUG-3438, lead ruling). + inviteIsAdmission := invitation != nil && + !(isAdmin && (!isAPITokenAuth(r) || s.adminAcceptsAPITokens())) && + !(s.cloudMode && s.emailConfigured()) if invitation == nil { - reqUser := currentUser(r) - isAdmin := reqUser != nil && reqUser.Role == "admin" switch { case isAdmin && isAPITokenAuth(r) && !s.adminAcceptsAPITokens(): // Creating an account is platform administration (BUG-3361): @@ -794,6 +833,12 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { if needsVerification && invitation != nil && input.InvitationProof != "" { ok, perr := s.store.ConsumeInvitationProof(invitation.ID, user.ID, strings.TrimSpace(input.InvitationProof)) if perr != nil { + // NOT counted as spent, even though a lost commit acknowledgement + // could have spent it. Restoring a proof the caller merely SENT + // would install an unproven secret on any pending invitation whose + // proof was already empty (codex r4). The residual is the safe + // direction: a refused signup after such an error leaves the proof + // spent, and the retry is verified by the email link instead. slog.Error("invitation proof: consume failed; continuing unverified", "error", perr, "user_id", user.ID) } else if ok { needsVerification = false @@ -824,7 +869,7 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { vtoken, verr := s.store.CreateEmailVerification(user.ID) if verr != nil { slog.Error("failed to create email verification token; rolling back signup", "error", verr, "user_id", user.ID) - if derr := s.store.DeleteUser(user.ID); derr != nil { + if derr := s.rollbackSignup(user.ID); derr != nil { slog.Error("failed to roll back user after verification-token error", "error", derr, "user_id", user.ID) } writeError(w, http.StatusInternalServerError, "internal_error", "Failed to start email verification") @@ -841,34 +886,66 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { // If registering via invitation, automatically add the user to the // workspace and mark the invitation as accepted. + // The role the account holds in the invited workspace, as the claim + // reports it: an existing membership keeps its own (codex r5). + joinedRole := "" + // notJoined is set when the signup is kept but its invitation was not + // applied: open registration only (inviteIsAdmission false). + var notJoined *invitationNotJoined if invitation != nil { - // THE MEMBERSHIP WRITE IS FATAL, AND THE ORDER IS THE FIX (BUG-2715). - // - // (The ACCEPT below is deliberately not fatal; the reason is with it. - // An earlier revision of this line said "both writes are fatal", which - // its own neighbour contradicted.) - // - // The membership write's error was discarded and the invitation was - // consumed on the next line regardless, so a failure burned the code - // AND left the user with no access — unrecoverable without an admin, - // because the invitation cannot be redeemed twice. + if s.registerInvitationPreClaimHook != nil { + s.registerInvitationPreClaimHook(invitation.ID) + } + // THE MEMBERSHIP WRITE IS FATAL (BUG-2715), AND IT CLAIMS THE + // INVITATION IN THE SAME TRANSACTION (BUG-3438). // // Refusing the registration leaves the invitation OPEN, which is the - // recoverable state: the user retries with the same code. That is the - // whole reason the membership goes first and the accept second. + // recoverable state: the user retries with the same code. A failure + // that burned the code while leaving the user with no access needed an + // admin to undo. // - // This is not a new posture — it is making two doors agree. - // handleAcceptInvitation (handlers_members.go) is the authenticated - // counterpart and refuses the same way: a membership that did not land - // leaves the invitation pending. There the two writes commit in one - // transaction (BUG-3281); this door was the one that diverged. - if addErr := s.store.AddWorkspaceMember(invitation.WorkspaceID, user.ID, invitation.Role, s.workspaceLimitMintOpts()...); addErr != nil { + // The invitation was read before the account was created, and it can + // be declined, cancelled or replaced since. The claim (accepted_at on a + // row still pending) commits with the membership, and a claim that + // finds no row rolls the membership back and answers ErrInvitationGone: + // the account is removed too, so the retry this refusal permits exists + // — when the invitation was the admission. Where registration is open + // anyway the account is kept and the invitation reported not applied. + // This is the store call the two accept doors make + // (acceptInvitationCore), so the three doors agree. + _, heldRole, addErr := s.store.AcceptWorkspaceInvitation(invitation.ID, invitation.WorkspaceID, user.ID, invitation.Role, s.workspaceLimitMintOpts()...) + if errors.Is(addErr, store.ErrInvitationGone) && !inviteIsAdmission { + slog.Info("invitation signup: the invitation was gone at its claim; registration is open, so the "+ + "account is kept and joins nothing", + "workspace_id", invitation.WorkspaceID, "user_id", user.ID, "invitation_id", invitation.ID) + notJoined = &invitationNotJoined{Code: "invitation_gone", + Message: "This invitation is no longer valid. Your account was created, but you were not added to the workspace."} + addErr = nil + } else if errors.Is(addErr, store.ErrInvitationGone) { + slog.Info("invitation signup: the invitation was gone at its claim; rolling back the account", + "workspace_id", invitation.WorkspaceID, "user_id", user.ID, "invitation_id", invitation.ID) + if derr := s.rollbackSignup(user.ID); derr != nil { + // Not rolled back: the account still holds the email, so the + // rolled-back 404 would be false (codex r9). + slog.Error("invitation signup: failed to roll back the account; the email is held by an "+ + "account with no workspace access", + "user_id", user.ID, "error", derr) + writeInternalError(w, derr) + return + } + writeError(w, http.StatusNotFound, "not_found", "Invitation not found or already accepted") + return + } + if notJoined == nil { + joinedRole = heldRole + } + if addErr != nil { // RECONCILE BEFORE DESTROYING — here too (codex round 2). // // Round 1 added a DeleteUser rollback here and got the ORDER of - // reasoning wrong: AddWorkspaceMember returns the raw tx.Commit() - // error, so a lost acknowledgement LANDS THE MEMBERSHIP and reports - // failure. Deleting the account on that error destroys a signup that + // reasoning wrong: the store returns the raw tx.Commit() error, so a + // lost acknowledgement LANDS THE MEMBERSHIP (and, since BUG-3438, the + // claim in the same transaction) and reports failure. Deleting the account on that error destroys a signup that // actually succeeded — the BUG-3026 shape, reintroduced forty lines // from the helper written to prevent it, by a fix for a different // finding in the same unit. @@ -882,7 +959,16 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { if s.membershipCheck != nil { membershipCheck = s.membershipCheck } - member, cerr := membershipCheck(invitation.WorkspaceID, user.ID) + // The store returns the held role only with a COMMIT error; a claim + // it refused before committing (heldRole empty) is definitely not + // landed, so it needs no reconcile read, and a failing read must + // not turn it into a 500 (codex r9). + var member *models.WorkspaceMember + var cerr error + refusedBeforeCommit := heldRole == "" + if !refusedBeforeCommit { + member, cerr = membershipCheck(invitation.WorkspaceID, user.ID) + } switch { case cerr != nil: // UNREADABLE — cannot tell an ack-lost success from a genuine @@ -897,13 +983,42 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { writeInternalError(w, addErr) return case member != nil: - // LANDED despite the reported error. The user HAS access, so the - // signup succeeded and the only thing left is the bookkeeping - // below. Falling through is the honest outcome; refusing here - // would delete a working account. - slog.Warn("invitation signup: the membership write reported an error but the row is present; "+ - "reconciled to success", + // The membership alone does not prove THIS transaction landed: + // it also claimed the invitation, and a membership can come from + // elsewhere (an admin's add in the window). So the claim decides, + // as it does at the accept doors (codex r1). + stored, ierr := s.store.GetInvitation(invitation.ID) + if ierr == nil && stored != nil && stored.AcceptedAt != nil { + joinedRole = member.Role + // LANDED despite the reported error: the user HAS access and + // the signup succeeded; refusing would delete a working account. + slog.Warn("invitation signup: the claim reported an error but the membership and the "+ + "claim are present; reconciled to success", + "workspace_id", invitation.WorkspaceID, "user_id", user.ID, "error", addErr) + break + } + // Not this signup's join. A membership exists, so the account is + // KEPT (deleting it cannot be undone, and it has access through + // whatever added it), and the signup is refused. + slog.Error("invitation signup: the claim failed beside a membership it did not write; "+ + "KEEPING the account and refusing the signup", + "workspace_id", invitation.WorkspaceID, "user_id", user.ID, + "error", addErr, "check_error", ierr) + if s.writeStoreMemberLimitError(w, r, invitation.WorkspaceID, addErr) { + return + } + writeInternalError(w, addErr) + return + case !inviteIsAdmission: + // ABSENT, on open registration: the account did not need the + // invitation, so it is KEPT and joins nothing; the invitation + // stays pending, and the account can accept it from its own + // list once whatever refused it clears (lead ruling). + slog.Warn("invitation signup: the claim was refused; registration is open, so the account is "+ + "kept and joins nothing", "workspace_id", invitation.WorkspaceID, "user_id", user.ID, "error", addErr) + notJoined = &invitationNotJoined{Code: "invitation_not_applied", + Message: "Your account was created, but the invitation could not be applied, so you were not added to the workspace."} default: // ABSENT — no membership. Refuse, and roll the account back so // the retry this refusal exists to permit actually exists: @@ -914,10 +1029,12 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { slog.Error("invitation signup: member was not added; rolling back the account and leaving the "+ "invitation redeemable", "workspace_id", invitation.WorkspaceID, "user_id", user.ID, "error", addErr) - if derr := s.store.DeleteUser(user.ID); derr != nil { + if derr := s.rollbackSignup(user.ID); derr != nil { slog.Error("invitation signup: failed to roll back the account; the email is held by an "+ "account with no workspace access", "user_id", user.ID, "error", derr) + writeInternalError(w, derr) + return } // BUG-3098: a member cap reached after the pre-check is a // refusal the invitee can act on, not a server fault. @@ -928,28 +1045,6 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { return } } - if err := s.store.AcceptInvitation(invitation.ID); err != nil { - // The membership LANDED, so the user has access and the only - // casualty is the invitation row still reading unaccepted. Refusing - // here would report failure for a registration that succeeded, and - // a retry would then hit the duplicate-email path. An invitation - // that stays open alongside a membership that exists is the benign - // direction of this pair — the accept is bookkeeping, the - // membership is the access. - // - // DELIBERATE DIVERGENCE from handleAcceptInvitation, which treats - // this same error as fatal. There the caller is an EXISTING, - // authenticated user, so a 500 is cleanly retryable. Here the - // account was created moments ago by this very request: refusing - // now would report failure for a registration that succeeded, and - // the retry would land on the duplicate-email path with no way - // forward. The ORDER is shared with that door, which is what the - // ruling specified; the second write's disposition differs because - // the rollback cost does. - slog.Error("invitation signup: member added but the invitation was not marked accepted", - "workspace_id", invitation.WorkspaceID, "user_id", user.ID, - "invitation_id", invitation.ID, "error", err) - } } // An admin creating someone else's account is not signing in as them, so @@ -984,8 +1079,13 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { // A signup with an invitation code has already joined the workspace, so // the client can land IN it, as an accept does (BUG-3284, PLAN-3002 Q5). // Additive; absent without an invitation. - if invitation != nil { - resp["accepted_invitation"] = s.invitationAcceptedFields(invitation, invitation.Role) + if notJoined != nil { + resp["invitation_not_joined"] = notJoined + } else if invitation != nil { + if joinedRole == "" { + joinedRole = invitation.Role + } + resp["accepted_invitation"] = s.invitationAcceptedFields(invitation, joinedRole) } writeJSON(w, http.StatusCreated, resp) } diff --git a/internal/server/server.go b/internal/server/server.go index 4732553fa..17a350777 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -458,6 +458,18 @@ type Server struct { // read-then-insert would race (BUG-2792). autoAddPreInsertHook func(requestID, workspaceID string) + // registerInvitationPreClaimHook, when non-nil, runs in handleRegister + // after the invitation was read and validated and before the signup + // claims it (BUG-3438). Test seam only (rowlessPreDeleteHook precedent): + // it lets a test decline, cancel or replace the invitation inside that + // window. + registerInvitationPreClaimHook func(invitationID string) + + // signupRollbackDelete, when non-nil, replaces DeleteAccountAtomic in + // rollbackSignup (BUG-3438). Test seam only: it lets a test make the + // rollback fail. + signupRollbackDelete func(userID string) error + // bg tracks fire-and-forget goroutines spawned by request handlers // (TouchUserActivity in middleware_auth, async email sends, etc.) so // the server can drain them before shutdown / test cleanup. Without diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 3706c9e90..f272be77d 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -2714,7 +2714,7 @@ export const api = { invitation_code?: string, invitation_proof?: string ) => - request<{ user: { id: string; email: string; username: string; name: string; role: string; email_verified?: boolean }; token: string; accepted_invitation?: InvitationAcceptedFields }>('/auth/register', { + request<{ user: { id: string; email: string; username: string; name: string; role: string; email_verified?: boolean }; token: string; accepted_invitation?: InvitationAcceptedFields; invitation_not_joined?: { code: string; message: string } }>('/auth/register', { method: 'POST', body: JSON.stringify({ email, diff --git a/web/src/routes/join/[code]/+page.svelte b/web/src/routes/join/[code]/+page.svelte index b2f93198c..28da8043d 100644 --- a/web/src/routes/join/[code]/+page.svelte +++ b/web/src/routes/join/[code]/+page.svelte @@ -28,7 +28,7 @@ // same-origin relative path — no open redirect. let oauthRedirectTarget = $derived(validateRedirect(`/join/${code}`)); let status = $state< - 'loading' | 'login' | 'register' | 'confirm' | 'accepting' | 'declining' | 'declined' | 'error' | 'setup' | '2fa' + 'loading' | 'login' | 'register' | 'confirm' | 'accepting' | 'declining' | 'declined' | 'not-joined' | 'error' | 'setup' | '2fa' >('loading'); // The invited workspace's name, from the preview, for the accept/decline // card (BUG-2136 U2). @@ -357,6 +357,17 @@ if (restartIfMovedOn(current)) return; // Registration with invitation_code already accepted the invite, // so land directly instead of calling acceptInvitation(). + // Kept without its invitation (BUG-3438): registration was open + // anyway, so the account exists, but the invitation was gone or + // could not be applied. Say so, and land nowhere. + if (registered.invitation_not_joined) { + await authStore.load().catch(() => {}); + if (!current()) return; + submitting = false; + errorMsg = registered.invitation_not_joined.message; + status = 'not-joined'; + return; + } await landInJoinedWorkspace(registered.accepted_invitation, current); return; } else { @@ -476,6 +487,9 @@ actionHref="/login" actionLabel="Go to login" /> + {:else if status === 'not-joined'} + + Go to Pad {:else if status === 'error'} Go to login diff --git a/web/src/routes/join/[code]/joinConsent.svelte.test.ts b/web/src/routes/join/[code]/joinConsent.svelte.test.ts index 3af184134..fc4db4570 100644 --- a/web/src/routes/join/[code]/joinConsent.svelte.test.ts +++ b/web/src/routes/join/[code]/joinConsent.svelte.test.ts @@ -181,6 +181,31 @@ describe('/join/[code] asks before joining (BUG-2136)', () => { expect(mocks.accept).not.toHaveBeenCalled(); }); + it('a registration kept without its invitation says so and lands nowhere (BUG-3438)', async () => { + mocks.session.mockResolvedValue({ authenticated: false }); + mocks.preview.mockResolvedValue({ found: true, email: 'new@example.com', has_account: false, workspace_name: 'Acme' }); + mocks.register.mockResolvedValue({ + user: { id: 'u', email: 'new@example.com' }, + token: 't', + invitation_not_joined: { + code: 'invitation_gone', + message: 'This invitation is no longer valid. Your account was created, but you were not added to the workspace.', + }, + }); + render(JoinPage); + await settle(); + await fireEvent.input(document.querySelector('input[placeholder="Name"]')!, { target: { value: 'New' } }); + const pws = document.querySelectorAll('input[type="password"]'); + await fireEvent.input(pws[0], { target: { value: 'password123' } }); + await fireEvent.input(pws[1], { target: { value: 'password123' } }); + await fireEvent.click(submitBtn()); + await settle(); + expect(mocks.goto).not.toHaveBeenCalled(); + expect(document.querySelector('[role="alert"]')?.textContent).toContain('This invitation is no longer valid'); + const toPad = Array.from(document.querySelectorAll('a')).find((a) => a.getAttribute('href') === '/console'); + expect(toPad).toBeTruthy(); + }); + it('registering a new account through the code still joins in one step', async () => { mocks.session.mockResolvedValue({ authenticated: false }); mocks.preview.mockResolvedValue({ found: true, email: 'new@example.com', has_account: false, workspace_name: 'Acme' });