Repository navigation
fix: split guest.capacity_reached into three, and let a player erase their own account - #420
Merged
Merged
Conversation
Three unrelated refusals shared one error code and one warning that named none of them, so a field report of "the deployment is not creating more guests right now" could not be diagnosed from the node. The worst case was a repo error while counting: cached_unlinked_count/0 answers `unknown`, the cap check mapped that to false, and a database fault was reported to the client as a full deployment. They are now three codes - guest.rate_limited (429, with retry_after), guest.capacity_reached (503) and guest.unavailable (503) - and every denial logs guest_create_denied with a reason, plus the count and cap it compared. Still fails closed in all three cases; only the diagnosis changed. An unparseable guest_unlinked_cap now falls back to the default instead of case_clause-ing every guest create. Adds DELETE /api/v1/auth/guest. Until now the only ways to remove a guest were guest_reap_after and the ops erasure route, both operator keys a cloud tenant cannot set, so a cloud deployment that accumulated throwaway guests could not shed a single one. An authenticated, still-unclaimed guest now erases itself through asobi_player_erase, with the same in-transaction re-check the reaper uses so a concurrent upgrade wins. It writes no ops audit row: the subject is the caller, not an operator, and a route any guest can call in a loop must not be able to flood the operator's log. Closes #419
…route Guardian + security review of the previous commit. Four defects, the first of which is the reason this is not a follow-up PR: `is_unclaimed_guest/2` is the *upgrade* predicate: no password AND owns a guest identity. asobi_oauth_controller:link/1 sets no password and leaves the guest identity in place, so a guest who linked Google or Steam still satisfies it - and the delete route would have erased a live account, orphaning its iap_transactions and landing the next provider sign-in on a new empty player. asobi_guest_reaper already had the right predicate (no password AND *every* identity is a guest) and spares that account. Export it and use it, so the sweep and the route cannot drift on who may be erased. The re-check now takes FOR UPDATE on the player row. Without it the re-check was a plain READ COMMITTED select and the DELETE predicates on id, not on hashed_password, so a concurrent upgrade lost rather than won - the opposite of what the comment promised. after_commit/1 moved out of the `try ... of` body. Exceptions raised there escape the catch, so a gen_server timeout evicting the auth cache would have answered 500 for an erasure that had already committed, with nothing logged and a live WS session still running against deleted rows. asobi_client_gate_plugin matched on path alone, so it gated DELETE /api/v1/auth/guest - a pre-auth captcha check, reading its token from a body a DELETE does not have, in front of an authenticated route. On any deployment that configures a gate the new route was a permanent 403, which is exactly the deployment shape it was written for. Also: the route drops `options` (the path already registers it via the POST, and a second insert throws under use_strict_routing), the route-invariant helper now asks whether the path answers a preflight rather than whether each entry lists the method, denial/1 gets a fail-closed catch-all, and guest.rate_limited answers with a retry-after header and a top-level retry_after like every other 429 here.
Replaces the guest-only delete route from the previous commits. Apple App Store guideline 5.1.1(v) has required in-app account deletion from any app offering account creation since June 2022, Google Play asks the same, and GDPR Art. 17 is the second leg - so a backend whose only erasure path is an operator secret cannot ship to a phone. That makes this a shipping gate for every game rather than a guest convenience, and the guest gate was protecting exactly the wrong accounts: a player who upgraded yesterday had no deletion path at all. POST /api/v1/players/me/erase, authenticated, subject is always the caller. An account with a password must echo it; a guest or provider-only account has no credential the client can re-present, so its session is the whole confirmation. Erasing somebody else stays behind the ops plane's `erasure` capability, which is the real asymmetry between the two routes. POST, not DELETE: nova_request_plugin skips JSON decoding entirely on GET and DELETE, so a DELETE could never carry the confirmation this route is gated on. Same shape as the operator's /ops/players/:id/erase one layer up. Erasure gets its own rate-limit bucket. The wrong-password path runs the password KDF, which is the argument /auth/register's dedicated bucket was added on (asobi#157), and /players/* would otherwise sit on the 300/s api limiter. Every erasure writes an audit row. ADR 0007's reason for auditing erasure - the data is gone, so the row is the only surviving evidence - does not care who asked, and this is the only irreversible thing a player can do to themselves. It is not the reaper's no-row case: a self-erasure is one account, asked for by a person, and can succeed at most once per account because the transaction deletes the caller's own tokens. The actor carries a new `subject` source, the one asobi_ops_auth:source() value that names the absence of operator capabilities rather than a way of holding them. ADR 0007 amended with the case and the reasoning. guest.rate_limited now answers with a fixed retry-after hint rather than the limiter's real remaining window. That bucket is keyed on one constant, so it is the whole node's guest-signup budget, and the true figure is the tuning input for a minimum-cost lockout of every new player (measured on this stack in asobi_saas#249). The precise number stays in the log.
…ody/2
A controller that sets a response header builds its own {json, Status,
Headers, Body} and calls asobi_error:legacy_body/2 rather than legacy/2,
which hardcodes empty headers. guest.rate_limited does exactly that to set
retry-after, and the guard's regex did not match it - so a documented code
looked unreachable and the guide read as drifted. Same contract as
legacy/2, same obligation to be documented.
🟡 Code Coverage — 75.6%6922 of 9153 lines covered. |
Surfaced building the client SDKs. Reusing auth.invalid_credentials meant a wrong confirmation password answered 401, and every SDK reads a 401 as "refresh the token pair and replay the request" - so each wrong password burned a refresh rotation and replayed a destructive call, on a session that was never in question. The caller is authenticated; they failed a step-up check. player.confirmation_failed (403) says that, and leaves the SDK machinery alone.
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
1 participant
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.
Reported from the Defold community: a fresh device pair per launch, then
503 guest.capacity_reached, which cleared on its own and came back. Thereporter also asked for a way to remove guests, and there was not one they
could reach.
Three refusals under one code
asobi_guest_controller:create/2collapsed the global limiter, theunlinked-guest cap, and the count query failing into one
guest.capacity_reachedwith a warning that named no reason, no count and nocap. The third is the one that matches the report - a cap does not self-clear
without deletions, and the limiter recovers inside a second - and it was being
reported to the client as a full deployment.
Now
guest.rate_limited(429),guest.capacity_reached(503) andguest.unavailable(503, the node could not count). All three still failclosed; only the diagnosis changed. Every denial logs
guest_create_deniedwith a
reason, and the cap denial logs the numbers it compared. Also fixes acase_clausethat crashed every guest create on an unparseableguest_unlinked_cap.The 429 carries a fixed
retry-afterhint rather than the limiter's realremaining window: that bucket is keyed on one constant, so it is the whole
node's guest-signup budget, and the true figure is the tuning input for a
minimum-cost lockout of every new player. The precise number stays in the log.
Account erasure
POST /api/v1/players/me/erase, authenticated, subject is always the caller.An account with a password must echo it; a guest or provider-only account has
no credential the client can re-present, so its session is the whole
confirmation. Erasing somebody else stays behind the ops plane's
erasurecapability.
Not a guest route, and not primarily about the report. Apple App Store
guideline 5.1.1(v) has required in-app account deletion from any app offering
account creation since June 2022, Google Play asks the same, and GDPR Art. 17
is the second leg - so a backend whose only erasure paths are an operator
retention key and an operator secret cannot ship to a phone. A guest-only gate
would also have protected exactly the wrong accounts, leaving a player who
upgraded yesterday with no deletion path at all.
POST rather than DELETE because
nova_request_pluginskips JSON decodingentirely on GET and DELETE, so a DELETE could not carry the confirmation. Same
shape as
/ops/players/:id/eraseone layer up.Erasure gets its own rate-limit bucket: the wrong-password path runs the
password KDF, which is the argument
/auth/register's dedicated bucket wasadded on (#157), and
/players/*otherwise sits on the 300/s api limiter.Every erasure writes an audit row. ADR 0007's reason for auditing erasure - the
data is gone, so the row is the only surviving evidence - does not depend on who
asked, and this is the only irreversible thing a player can do to themselves. It
is not the reaper's no-row case: one account, asked for by a person, and it can
succeed at most once because the transaction deletes the caller's own tokens.
The actor carries a new
subjectsource, the oneasobi_ops_auth:source()value naming the absence of operator capabilities rather than a way of holding
them. ADR 0007 amended.
From review
The architecture guardian and the security reviewer both ran over the first
draft. What they caught:
is_unclaimed_guest/2, which is the upgradepredicate.
asobi_oauth_controller:link/1sets no password and leaves theguest identity in place, so a guest who linked Google still satisfied it -
the route would have erased a live account and orphaned its
iap_transactions, whileasobi_guest_reaperspares exactly that account.The reaper's predicate is now shared rather than duplicated.
FOR UPDATE. Without it the delete predicates on id, noton the credential, so a concurrent upgrade lost instead of winning.
after_commit/1moved out of thetry ... ofbody, here and inasobi_player_erase(pre-existing): exceptions raised there escape thecatch, so an eviction timeout answered failure for an erasure that hadalready committed, and logged nothing.
Upstream
Taure/seki#17, found alongside: with a negative
erlang:monotonic_time/1(thenormal case for ~18 years of uptime)
window_start/2rounds the wrong way andevery
sliding_windowlimiter denies earlier than configured, soguest_globalallows between 100 and 50/sec rather than 100. Not fixed here.Follow-ups
next SDK sync.
guest_reap_aftertenant-settable. Retention on atime axis and a subject right on demand are different primitives; this route
is not a substitute.
Closes #419