diff --git a/internal/core/shares.go b/internal/core/shares.go index 9d91737..cd5bf87 100644 --- a/internal/core/shares.go +++ b/internal/core/shares.go @@ -197,9 +197,9 @@ func (s *Handlers) handleAcceptInvite(w http.ResponseWriter, r *http.Request) { return } - // Consume the link, grant the share, and seed default commands atomically, so a - // failure can never spend the link without granting access. The used_at guard - // inside makes it the single-use gate against concurrent accepts. + // Grant the share, seed its commands, and delete the link atomically, so a + // failure can never delete the link without granting access. A row lock inside + // makes it the single-use gate against concurrent accepts. if _, err := s.Store.AcceptInvite(r.Context(), token, uid); errors.Is(err, store.ErrNotFound) { s.Render(w, r, "invite.html", map[string]any{"State": "invalid"}) return diff --git a/internal/core/templates/share.html b/internal/core/templates/share.html index 227af48..02c2015 100644 --- a/internal/core/templates/share.html +++ b/internal/core/templates/share.html @@ -16,17 +16,21 @@
-
+

Share links

-

+

+ +
+
+
+

Invite specific people with single-use links: the recipient signs in and accepts to control this - repeater from their own browser-connected KISS modem. Each link works once — mint one per person, - label it, and choose what they can do. + repeater from their own browser-connected KISS modem. Each link works once and disappears when it's + used — mint one per person, label it, and choose what they can do.

- {{if .Invites}}
@@ -34,23 +38,21 @@
{{if .Description}}{{.Description}}{{else}}(no label){{end}} - {{if .UsedAt}}Used by {{.UsedByName}}{{else}}Unused{{end}} + Unused
- {{if not .UsedAt}}
{{$.BaseURL}}/invite/{{.Token}}
- {{end}}
- +
{{end}}
{{else}} -

No links yet.

+

No pending links.

{{end}}
diff --git a/internal/store/migrations/0039_drop_invite_used.sql b/internal/store/migrations/0039_drop_invite_used.sql new file mode 100644 index 0000000..c9ea136 --- /dev/null +++ b/internal/store/migrations/0039_drop_invite_used.sql @@ -0,0 +1,13 @@ +-- +goose Up +-- Single-use links are now deleted the instant they're redeemed (see AcceptInvite): +-- a consumed link is redundant with the recipient who then appears in the +-- People-with-access list, so there's no "used" state to track. Remove links that +-- were already consumed (their recipients already hold a share), then drop the +-- consumed-state columns. +DELETE FROM repeater_invites WHERE used_at IS NOT NULL; +ALTER TABLE repeater_invites DROP COLUMN used_at; +ALTER TABLE repeater_invites DROP COLUMN used_by; + +-- +goose Down +ALTER TABLE repeater_invites ADD COLUMN used_at TIMESTAMPTZ; +ALTER TABLE repeater_invites ADD COLUMN used_by BIGINT REFERENCES users(id) ON DELETE SET NULL; diff --git a/internal/store/shares.go b/internal/store/shares.go index 65d4d3e..c200c8d 100644 --- a/internal/store/shares.go +++ b/internal/store/shares.go @@ -170,15 +170,13 @@ func (s *Store) ListStewards(ctx context.Context, repeaterID int64) ([]ShareInfo // --- share links (single-use invites) --- -// Invite is a single-use share link. Used invites retain who consumed them and -// when, as an audit trail. +// Invite is a pending single-use share link. Redeemed links are deleted (see +// AcceptInvite), so every Invite is unused. type Invite struct { ID int64 Token string Description string CreatedAt time.Time - UsedAt *time.Time - UsedByName *string // display name or username of the consumer, if used } // CreateInvite mints a new single-use share link for a repeater, returning its @@ -213,21 +211,19 @@ func (s *Store) CreateInvite(ctx context.Context, repeaterID int64, description return token, nil } -// ListInvites returns all invites for a repeater (pending and used), newest first. +// ListInvites returns a repeater's pending (unredeemed) share links, newest first. func (s *Store) ListInvites(ctx context.Context, repeaterID int64) ([]Invite, error) { rows, err := s.pool.Query(ctx, ` - SELECT i.id, i.token, i.description, i.created_at, i.used_at, - COALESCE(NULLIF(u.display_name, ''), u.username) - FROM repeater_invites i - LEFT JOIN users u ON u.id = i.used_by - WHERE i.repeater_id = $1 - ORDER BY i.created_at DESC`, repeaterID) + SELECT id, token, description, created_at + FROM repeater_invites + WHERE repeater_id = $1 + ORDER BY created_at DESC`, repeaterID) if err != nil { return nil, fmt.Errorf("list invites: %w", err) } return collectRows(rows, func(r pgx.Row) (Invite, error) { var inv Invite - err := r.Scan(&inv.ID, &inv.Token, &inv.Description, &inv.CreatedAt, &inv.UsedAt, &inv.UsedByName) + err := r.Scan(&inv.ID, &inv.Token, &inv.Description, &inv.CreatedAt) return inv, err }) } @@ -243,12 +239,12 @@ func (s *Store) DeleteInvite(ctx context.Context, repeaterID, inviteID int64) er return nil } -// RepeaterByInviteToken resolves a *valid* (unused) share-link token to its -// repeater, or ErrNotFound if the token is unknown, revoked, or already used. -// queryUserID is used only to populate the Shared flag and owner display. +// RepeaterByInviteToken resolves a valid share-link token to its repeater, or +// ErrNotFound if the token is unknown, revoked, or already redeemed (redemption +// deletes the link). queryUserID only populates the Shared flag and owner display. func (s *Store) RepeaterByInviteToken(ctx context.Context, queryUserID int64, token string) (*Repeater, error) { row := s.pool.QueryRow(ctx, repeaterSelect+` - WHERE r.id = (SELECT repeater_id FROM repeater_invites WHERE token = $2 AND used_at IS NULL)`, + WHERE r.id = (SELECT repeater_id FROM repeater_invites WHERE token = $2)`, queryUserID, token) r, err := scanRepeater(row) if err != nil { @@ -257,25 +253,23 @@ func (s *Store) RepeaterByInviteToken(ctx context.Context, queryUserID int64, to return r, nil } -// AcceptInvite redeems a single-use share link for userID, doing all three steps -// in one transaction so they are all-or-nothing: it consumes the link, grants the -// share, and seeds the command set the owner chose for this link (invite_commands). -// If any step fails the whole thing rolls back, so a failure can never spend the -// link without granting access (or grant access without spending the link). +// AcceptInvite redeems a single-use share link for userID in one transaction so +// the steps are all-or-nothing: it grants the share, seeds the command set the +// owner chose for this link (invite_commands), and deletes the link — a redeemed +// link doesn't stick around. If any step fails the whole thing rolls back, so a +// failure can never delete the link without granting access (or grant access +// without deleting the link, leaving it redeemable again). // // It returns whether a new share was created (false if the user already had one) -// and ErrNotFound if the token is unknown, revoked, or already used — the -// single-use guard, safe against concurrent accepts. +// and ErrNotFound if the token is unknown, revoked, or already redeemed. The row +// lock is the single-use gate: concurrent accepts serialize on it, and the winner +// deletes the row, so losers find nothing. func (s *Store) AcceptInvite(ctx context.Context, token string, userID int64) (added bool, err error) { err = s.inTx(ctx, func(tx pgx.Tx) error { var inviteID, repeaterID int64 - // Consume the link. The used_at IS NULL guard makes this the single-use - // gate: only one accept can flip it, so a concurrent (or repeat) accept - // matches no row and falls through to ErrNotFound. if err := tx.QueryRow(ctx, - `UPDATE repeater_invites SET used_at = now(), used_by = $2 - WHERE token = $1 AND used_at IS NULL - RETURNING id, repeater_id`, token, userID).Scan(&inviteID, &repeaterID); err != nil { + `SELECT id, repeater_id FROM repeater_invites WHERE token = $1 FOR UPDATE`, + token).Scan(&inviteID, &repeaterID); err != nil { return notFoundOr(err, "consume invite") } tag, err := tx.Exec(ctx, @@ -285,16 +279,19 @@ func (s *Store) AcceptInvite(ctx context.Context, token string, userID int64) (a return fmt.Errorf("add share: %w", err) } added = tag.RowsAffected() > 0 - if !added { - return nil // already shared: nothing to seed + if added { + // Seed the new share with the command set the owner chose for this link, + // before the delete below cascades invite_commands away. + if _, err := tx.Exec(ctx, ` + INSERT INTO share_commands (repeater_id, user_id, command_id) + SELECT $1, $2, command_id FROM invite_commands WHERE invite_id = $3 + ON CONFLICT DO NOTHING`, repeaterID, userID, inviteID); err != nil { + return fmt.Errorf("seed share commands: %w", err) + } } - // Seed the new share with exactly the command set the owner chose for this - // link; the owner can adjust it afterwards. - if _, err := tx.Exec(ctx, ` - INSERT INTO share_commands (repeater_id, user_id, command_id) - SELECT $1, $2, command_id FROM invite_commands WHERE invite_id = $3 - ON CONFLICT DO NOTHING`, repeaterID, userID, inviteID); err != nil { - return fmt.Errorf("seed share commands: %w", err) + // Consume the single-use link by deleting it (cascades its invite_commands). + if _, err := tx.Exec(ctx, `DELETE FROM repeater_invites WHERE id = $1`, inviteID); err != nil { + return fmt.Errorf("consume invite: %w", err) } return nil }) diff --git a/internal/store/shares_test.go b/internal/store/shares_test.go index a02ce14..8cfdfe7 100644 --- a/internal/store/shares_test.go +++ b/internal/store/shares_test.go @@ -61,15 +61,10 @@ func TestAcceptInvite(t *testing.T) { if len(cmds) != 1 || cmds[0] != grant { t.Fatalf("seeded commands = %v, want exactly [%d] (the chosen grant)", cmds, grant) } + // The redeemed link is deleted, not retained — nothing left to show. invites, err := st.ListInvites(ctx, rep.ID) - if err != nil || len(invites) != 1 { - t.Fatalf("ListInvites = %d invites, %v; want 1", len(invites), err) - } - if invites[0].UsedAt == nil { - t.Fatalf("invite not marked consumed after accept") - } - if invites[0].UsedByName == nil || *invites[0].UsedByName != "invitee" { - t.Fatalf("invite UsedByName = %v, want invitee", invites[0].UsedByName) + if err != nil || len(invites) != 0 { + t.Fatalf("ListInvites = %d invites, %v; want 0 (redeemed link deleted)", len(invites), err) } // Single-use: a second accept (by anyone) must fail and grant nothing. @@ -86,10 +81,10 @@ func TestAcceptInvite(t *testing.T) { } // TestAcceptInviteRollsBack is the atomicity regression: if any step of the -// redemption fails, the whole thing must roll back — the link stays unspent so it +// redemption fails, the whole thing must roll back — the link stays undeleted so it // can still be redeemed, rather than being consumed with no access granted (the // reported bug). We trigger a failure by redeeming for a non-existent user, which -// violates the used_by/user_id foreign keys inside the transaction. +// violates the repeater_shares.user_id foreign key inside the transaction. func TestAcceptInviteRollsBack(t *testing.T) { t.Parallel() st, ctx := orgTestStore(t) @@ -115,13 +110,10 @@ func TestAcceptInviteRollsBack(t *testing.T) { t.Fatalf("AcceptInvite for non-existent user succeeded, want error") } - // The link must NOT have been consumed by the failed attempt. + // The link must NOT have been deleted by the failed attempt — still redeemable. invites, err := st.ListInvites(ctx, rep.ID) if err != nil || len(invites) != 1 { - t.Fatalf("ListInvites = %d, %v; want 1", len(invites), err) - } - if invites[0].UsedAt != nil { - t.Fatalf("link was consumed despite the accept failing (non-atomic)") + t.Fatalf("ListInvites = %d, %v; want 1 (link survives a failed accept)", len(invites), err) } // And a real user can still redeem it afterwards.