feat: seedphrase account system + bulk-delete offline clients#406
Draft
Ryanmello07 wants to merge 39 commits into
Draft
feat: seedphrase account system + bulk-delete offline clients#406Ryanmello07 wants to merge 39 commits into
Ryanmello07 wants to merge 39 commits into
Conversation
…tions Adds three tables: - network_user_auth_seedphrase: BIP39 seedphrase auth storage - network_name_reclaim: 24h cooldown pool for changed network names - network_create_attempt: 5-per-IP-per-day seedphrase signup rate limit
…rate limit - seedphrase_controller: errors returned in result struct (not fmt.Errorf) - auth_controller: RemoveAuth errors returned in result struct - account_controller: accept both network_name and new_name, return name - auth_model: seedphrase login errors returned in result, not Go error - network_model: seedphrase path skips old auth rate limit, uses new one - db_migrations: add network_create_attempt table - network_create_rate_limit: new file with 5/ip/day limit for seedphrase
… name Seedphrase-only users (or seedphrase + wallet only) can't call claim-name or change-name until they bind an email, phone, or social login. Returns a clear error message guiding them.
requireEmailOrSsoBound now checks verified=true on network_user_auth_password instead of just existence. SSO auths (Apple/Google) are inherently verified by the provider so they still count without a verified column.
Mirrors urnetwork#399 but redesigned for production scale: a sync path for small requests (<=10,000 ids, applied inline) and an async path for larger ones, handed off to a single background task that self-chunks in bounded batches and reschedules its own remainder rather than looping unboundedly or requiring the caller to chunk requests themselves. Also fixes: - missing deactivate_time on the bulk path, which would have let the reap job's 30-day grace period be skipped for bulk-deleted clients - an unbounded per-request size with no batching, which doesn't hold up against real per-network client counts Adds task.ScheduleTaskInTxIfAbsent (task/task.go) for atomic schedule-if-not-already-pending semantics, since the existing RunOnce merge-on-conflict path only updates timing/priority on an existing pending row, not its args -- reusing it naively for duplicate-request rejection would silently drop a racing duplicate's client_ids while still reporting success.
…t gap Two independent review passes (DeepSeek, Gemini 3.1 Pro) confirmed no critical issues; this addresses their medium/low findings: - guard against a nil session.ByJwt in RemoveNetworkClientsTask rather than panicking on a nil dereference inside a MaintenanceTx - clarify why RemoveNetworkClientsTaskPost's reschedule uses plain ScheduleTaskInTx rather than the IfAbsent variant (no possible conflict: the finishing task's own row is deleted in the same tx) - lower MaxRemoveNetworkClientsCount 5M -> 2M: still 2x headroom over known ~1M real-world usage, materially smaller worst-case args_json payload on the request path - assert deactivate_time is stamped in the real-worker lifecycle test, the only test exercising multiple task invocations
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ling - nil session.ByJwt in RemoveNetworkClientsTask now has coverage (the guard added in the prior commit had none) - boundary tests at exactly RemoveNetworkClientsBatchCount (still sync) and exactly MaxRemoveNetworkClientsCount (still accepted) -- previously only the "+1 over" side of each boundary was tested - a fast, isolated test of RemoveNetworkClientsTaskPost's reschedule logic (same run_once key, different network unaffected), as a complement to the slow 205k-row real-worker lifecycle test
…d, index, reduced cap, guest rejection - Fix #1 (DoS): Add http.MaxBytesReader(100MB) to body read + reject guest-mode callers - Fix #3 (abuse): Switch to WrapWithInputRequireAuthNoGuest - Fix #4 (perf): Add network_client_network_id_client_id index migration - Fix urnetwork#5 (correctness): restored original panic-based error propagation (by convention) - Fix urnetwork#6 (quality): Deduplicate client IDs in RemoveNetworkClients input - Fix urnetwork#7 (scale): Lower MaxRemoveNetworkClientsCount from 2M to 1M - Copilot suggestion 1: Return zero server.Id{} when ScheduleTaskInTxIfAbsent is not scheduled - Copilot suggestion 2: Add nil-ByJwt guard to RemoveNetworkClientsTaskPost
router_test.go still referenced WrapRequireAuthNoGuest/WrapWithInputRequireAuthNoGuest and guest-mode JWTs, which no longer exist after guest mode removal. Strip the guest-specific test routes and assertions. network_client_handlers.go's RemoveNetworkClients also called the removed WrapWithInputRequireAuthNoGuest wrapper; switch to WrapWithInputRequireAuth.
Removes stale references to model.UpgradeGuestArgs/UpgradeGuestResult and model.UpgradeGuestExistingArgs (types removed with guest mode). Adds registry entries for the new seedphrase and account-management endpoints: /auth/add-auth, /auth/remove-auth, /auth/regenerate-seedphrase, /auth/generate-seedphrase, /account/change-name, /account/claim-name. Note: the connect/api/bringyour.yml OpenAPI spec (separate repo) still lists the removed /auth/upgrade-guest* routes and doesn't yet list the new seedphrase/account routes. That spec update is out of scope for this PR and should be coordinated separately in the connect repo.
These result/error types (UpgradeGuestNetwork, UpgradeGuestResultVerification, UpgradeGuestError, UpgradeGuestExisting*) had no remaining callers after the UpgradeGuest/UpgradeFromGuestExisting functions were removed. Keeping Network.GuestUpgradeNetworkId and Testing_CreateGuestNetwork, which still reflect real DB state for existing (un-migrated) guest accounts.
AddAuth() was swallowing errors from addUserAuth(), addSsoAuth(), and
addWalletAuth(). If a user tried to add a second email (same auth_type),
SSO provider, or wallet, the function silently returned {} with no
error message despite the DB correctly rejecting the duplicate via PK
constraint. Now each path checks the returned error and surfaces it
as a proper JSON error response.
Contributor
Author
🐛 Fix: Error propagation in AddAuth (cherry-picked 2025-07-17)When a user tried to add a second email (same auth_type), SSO provider, or wallet to an account, the DB-level PK constraint correctly rejected the duplicate — but the error was silently swallowed and the API returned {} with no error message. Fixed by capturing errors from addUserAuth(), addSsoAuth(), and addWalletAuth() inside AddAuth() and returning them as proper JSON error responses. This applies to all auth types: email, phone, Google, Apple, wallet. |
RemoveAuth('solana') only deleted the network_user_auth_wallet row, but the
network_user.wallet_address and wallet_blockchain columns were left set,
preventing that wallet from creating a new account. Now they are cleared so
the wallet can be reused for a fresh signup.
Contributor
Author
🐛 Fix: Wallet can be reused after being removed
|
RemoveAuth only deleted auth rows but never updated the denormalized auth_type column on network_user. After removing wallet auth, the app still showed 'sign in with wallet' because auth_type remained 'solana'. Now every RemoveAuth path re-derives auth_type from whichever auth tables still have rows, in priority order: password > apple > google > solana > seedphrase.
NetworkUser struct now includes: - SeedphraseAuths: list of linked seedphrase auths (mirrors WalletAuths) - AuthTypes: flat string array of all linked auth methods e.g. ['solana', 'seedphrase'] or ['solana', 'seedphrase', 'email'] The app can use this directly instead of guessing from auth_type.
…pe panic countAuthMethods previously ran outside the deletion transaction, so two concurrent RemoveAuth calls for different auth types could both read the same pre-deletion count, both pass the "keep at least one method" guard, and both commit against different rows in different auth tables (no conflict under RepeatableRead) -- leaving the account with zero auth methods and no recovery path short of DB intervention. Move the count query inside the deletion tx behind a per-user `FOR UPDATE` row lock so a concurrent call blocks and retries (via server.Tx's existing serialization-failure retry) against the reduced count instead. Also replace the panic on an unrecognized auth_type with a returned error, matching the existing "last auth method" validation pattern -- user-controlled input reaching the default case previously crashed the request with a 500 and a stack-trace log entry instead of a clean error.
AuthLogin pre-inserts a success=false user_auth_attempt row and relies on each login branch to flip it to success afterward. The password and SSO branches did this; the seedphrase and wallet branches did not. Since seedphrase/wallet logins have a nil userAuth, UserAuthAttempt's rate limiter counts their failures per client IP only (5 in 5 minutes). With success never recorded, 5 successful seedphrase or wallet logins from one IP -- trivial on any shared NAT (household, office, mobile carrier CGNAT) -- exhausts that budget and locks out all subsequent auth from that IP, including unrelated users' SSO/password logins, for the rate-limit window. Seedphrase is now the primary login path (guest mode was removed), so this was reachable in ordinary use, not just adversarially.
jwt.NewByJwt panics on a zero-value networkId. If a seedphrase auth row ever outlives its network_user/network (this branch's RemoveNetwork already deletes it, but the defensive gap remains for any other path that could orphan the row), a subsequent login would pass the hash/salt check and then panic on the network join instead of failing cleanly, turning an edge case into a 500 on a public endpoint.
RemoveNetwork's batch delete covers network_user, network_user_auth_wallet, network_user_auth_password, and network_user_auth_sso, but not network_user_auth_seedphrase -- so deleting a network left the seedphrase auth row (hash, salt, lookup) behind. A later login attempt with that seedphrase would pass the argon2 check, then find no matching network_user on the join and panic on a zero-value networkId in jwt.NewByJwt (guarded separately in LoginWithSeedphrase). Also permanently blocked reusing that seedphrase for a new account since the lookup column is unique. (This fix already existed on the fork's feat/seedphrase-account-system branch -- ported by hand here since a mechanical cherry-pick would have been a no-op against this branch's history.)
…tests 58 assert.Equal/assert.NotEqual call sites had no import for github.com/go-playground/assert/v2, so the whole model test package failed to compile. Once fixed, TestRemoveNetworkClientsRejectsOversizedRequest and TestRemoveNetworkClientsExactlyAtCapIsAccepted would still fail: both seeded their client-id slice with make([]server.Id, N), which is all zero-value UUIDs. RemoveNetworkClients deduplicates ids before checking the length cap, so N identical zero ids collapse to length 1 and never exercise the boundary being tested. Seed with distinct server.NewId() values instead, matching the existing correct idiom in TestRemoveNetworkClientsUUIDArrayBinding.
The second half of TestNetworkUser used bare assert.Equal/assert.NotEqual with no import (undefined: assert) -- this file otherwise uses connect.AssertEqual throughout. It also still asserted guest/seedphrase-shaped values (UserAuth == nil, Verified == false, AuthType == AuthTypeSeedphrase) against a user created by Testing_CreateNetwork, which unconditionally creates a password user (UserAuth set, Verified: true, AuthType: AuthTypePassword). The block was mechanically migrated from a guest-network helper without updating its assertions, so it could never pass. No seedphrase-creating test helper exists to restore the original intent, so this switches to asserting what Testing_CreateNetwork actually produces, matching the pattern already used earlier in the same test.
The cherry-picked fix used connect.AssertEqual, matching the fork branch's convention where this file already imported "connect". This branch's copy of the file was already migrated to go-playground/assert/v2 throughout and never imports "connect" -- adjusting to match.
networkCreateSeedphrase's only caller (NetworkCreate) guards entry on networkCreate.AuthJwt == nil, so the "if AuthJwt provided, bind SSO too" branch inside it could never execute -- dead code that also silently swallowed a bind error via glog.Infof had it ever run, which would have been a latent trap if the outer guard were ever loosened later.
changeNetworkName's reclaim-cooldown INSERT fired whenever an old name existed, with no check that the new name actually differs from it. A no-op rename (idempotent retry, resend, a flaky client re-POST with the same name) would put the user's own current name into network_name_reclaim, and the cooldown lookup has no owner exemption -- so the user would then be locked out of "renaming" back to the name they still hold for 24h. Guard the insert on the name actually changing.
Same class of pre-existing test-compile break found and fixed on the fork's feat/seedphrase-account-system branch, applied to the subset that also exists here (this branch's network_model_test.go, router_test.go, and spec_conformance_test.go were already clean of guest-mode test scaffolding, so only these two needed the fix): - controller/network_controller_test.go: three NetworkCreateArgs struct literals still set a GuestMode field that no longer exists on that type (model/peer_model_test.go:403 also sets GuestMode, but on jwt.ByJwt, which still has that field -- not a compile error, left as-is). - task/task_test.go: a block of assert.Equal calls had no import for github.com/go-playground/assert/v2.
…Auth
The mirroring UPDATE added to sync network_user.wallet_address used
RaisePgResult, which panics on any error -- unlike the sibling INSERT a
few lines above, which returns a clean error. network_user.wallet_address
has a global (not per-user) unique index, so this UPDATE can legitimately
conflict: a legacy account whose network_user.wallet_address was set
before network_user_auth_wallet existed, and never cleared, collides
when a different user links that same address.
Since constraint violations are classified as transient, server.Tx would
retry the whole transaction for up to a minute against the same
permanent conflict before finally re-panicking, surfacing as an opaque
500 instead of the structured AddAuthMethodResult{Error:...} every other
AddAuth failure path returns. Catch it the same way the INSERT does.
A signed wallet challenge proves control of that identity the same way a verified email or SSO login does, so it should count toward requireVerifiedIdentityBound (formerly requireEmailOrSsoBound) just like the other two methods.
…d hung instead of erroring RefreshToken re-signed a new JWT using session.ByJwt.NetworkName -- whatever name was baked into the JWT being refreshed -- instead of re-reading the current name from the DB. A rename via change-name/claim-name would never be picked up until the client happened to get a truly fresh JWT some other way (e.g. logging back in). addWalletAuth attempted the INSERT directly and let a duplicate-wallet unique-constraint violation abort the transaction, then returned the error cleanly (no panic). But server.Tx's default retry options retry any failed COMMIT for up to 60s regardless of whether the underlying cause is retryable -- committing an aborted transaction always fails the same way on every retry, so this stalled every "wallet already taken" request for up to a minute before ever surfacing an error. Added a pre-check SELECT before the INSERT, mirroring the validateUserAuthAvailability pattern addUserAuthInTx already uses for email/phone auth, so the common case fails fast with a clean error instead of ever touching the DB constraint.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent features, cherry-picked cleanly from a beta fork branch onto current
main:1. Seedphrase account system (replaces guest mode)
POST /auth/network-createwith no auth fieldsPOST /auth/login(seedphrasefield)POST /auth/generate-seedphrase,POST /auth/regenerate-seedphrase)POST /auth/add-auth,POST /auth/remove-auth) — cannot remove your last remaining methodPOST /account/change-name(24h cooldown on the old name) andPOST /account/claim-name(no cooldown, for first-time custom names) — both require a verified email or an SSO login bound to the account (wallet-only auth does not qualify, since it can't be used to recover the account)network_create_attempttable), independent of the existing email/password auth-attempt rate limiterguest_modeon/auth/network-create,/auth/upgrade-guest,/auth/upgrade-guest-existing) is removed. Existing guest rows/JWTs are left untouched for backward compatibility (GuestModestays onjwt.ByJwt, alwaysfalsefor new tokens;Network.GuestUpgradeNetworkIdcolumn/field kept).2. Bulk delete for offline network clients
RemoveNetworkClientscalls: small requests (≤10k ids) apply synchronously; larger requests are handed off to a background task (RemoveNetworkClientsTask) that processes up to 1M ids in bounded batches, so a single call can clear a network with hundreds of thousands of offline clients without a long-running request-path transactionrun_oncekey), with reschedule/continuation support for partial batchesnetwork_client_network_id_client_id) to support the batched deactivation query at scaleNew tables
network_user_auth_seedphrase— SHA-256 lookup + Argon2id hash for seedphrase auth (never stores the raw mnemonic)network_name_reclaim— 24h cooldown pool for changed network namesnetwork_create_attempt— 5-per-IP-per-day seedphrase signup rate limitNew endpoints
/auth/network-create/auth/loginseedphrasefield added alongside existing methods)/auth/add-auth/auth/remove-auth/auth/generate-seedphrase/auth/regenerate-seedphrase/account/change-name/account/claim-nameRemoved endpoints
POST /auth/upgrade-guestPOST /auth/upgrade-guest-existingNotes for reviewers
api/spec_conformance_test.gois updated to match the new/removed routes. The OpenAPI spec in the separateconnectrepo (connect/api/bringyour.yml) still lists the removed/auth/upgrade-guest*routes and doesn't yet list the new seedphrase/account routes — that update is out of scope here and should be coordinated in a follow-up PR againstconnect..superpowers/planning docs, BETA.md/beta-setup scripts, and any CI/workflow changes — those are fork/beta-specific and not relevant upstream.Verification
go build ./...passesgo vet ./api(spec conformance package) passes clean