fix: phone-only accounts could not enrol TOTP or be deleted - #753
Open
lakhansamani wants to merge 1 commit into
Open
fix: phone-only accounts could not enrol TOTP or be deleted#753lakhansamani wants to merge 1 commit into
lakhansamani wants to merge 1 commit into
Conversation
lakhansamani
force-pushed
the
fix/phone-only-account-parity
branch
from
August 7, 2026 09:47
52cb1c9 to
c5afc04
Compare
Two bugs from local testing, same root cause: paths that treat email as if every account has one. A mobile signup never gets an email, so anything keyed on it is unreachable for those users. TOTP enrolment failed outright with pquerna/otp's "AccountName must be set". AccountName is the label the authenticator app shows and the enrolment passed user.Email verbatim, which is empty for a phone-only account; the library rejects an empty value. MFA is on by default, so this is the first thing a mobile signup hits after verifying - the account could be created but never finish setup. Falls back to the phone number, then the user id, so enrolment cannot fail on a missing identifier. Admin delete accepted ONLY an email (DeleteUserRequest.email was required), so a phone-only account could not be deleted at all - there was no second way in and the account was permanent. Now id-or-email with id preferred, mirroring the shape GetUserRequest already used. Email is kept for existing callers; the handler rejects a request carrying neither, since "exactly one of" is not expressible in the schema or in protobuf. BREAKING (admin API, agreed): DeleteUserRequest.email is no longer required. Callers sending email are unaffected. gRPC gains DeleteUserRequest.id = 2 and drops the min_len constraint on email. The dashboard now deletes by id. Tests cover TOTP enrolment across all three identifier shapes (phone-only, email-only, neither), deleting a phone-only account by id, email still working, and the neither-supplied rejection.
lakhansamani
force-pushed
the
fix/phone-only-account-parity
branch
from
August 7, 2026 10:14
c5afc04 to
5bfc821
Compare
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
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.
Two bugs found in local testing, with the same root cause: paths that treat
email as if every account has one. A mobile signup never gets an email, so
anything keyed on it is unreachable for those users.
1. TOTP enrolment failed —
AccountName must be setReported symptom: signing up with a mobile number produced
AccountName must be set.That is pquerna/otp rejecting an empty
AccountName— the label theauthenticator app shows next to the code. Enrolment passed
user.Emailverbatim, which is empty for a phone-only account.
MFA is on by default, so this is the first thing a mobile signup hits after
verifying: the account could be created but never finish MFA setup.
Falls back to the phone number, then the user id. The id is a poor label but it
is never empty, so enrolment cannot fail on a missing identifier.
2. A phone-only account could not be deleted at all
DeleteUserRequest.emailwas required, and it is the only identifier theendpoint accepted. A phone-only account has no email, so there was no second way
in — the account was permanent.
Now id only, across GraphQL, gRPC and REST.
Breaking change (admin API, agreed in review)
DeleteUserRequestis now{ id: String! }—emailremoved.DeleteUserRequesttakesstring id = 2withmin_len = 1.POST /v1/admin/delete_usertakes{"id": "..."}.Field 1 is
reserved, not reused.emailwas a string andidis a string,so reusing tag 1 would let an old client's email decode silently as an id —
wire-compatible, semantically wrong, and pointed at a delete. Reserving makes an
old client fail loudly instead:
Scoped to
_delete_user/AuthorizerAdminService.DeleteUser. No public-APIsurface changes.
proto-breaking-approvedlabelbuf breakingcorrectly reports:That is the intended change. The repo already has the right lever for this — a
maintainer applies
proto-breaking-approvedand the gate becomescontinue-on-errorfor this PR only. I deliberately did not add abuf.yamlignore: that would suppress the check for the whole admin file andhide the next accidental break.
buf lintpasses, andmake proto-checkconfirmsgen/is not stale.Tests
case), email-only, and neither (falls back to the id). Verified fail-before:
reverting the fix reproduces
AccountName must be seton exactly thephone-only and neither cases.
min_len(gRPC);without that guard
""falls through to a lookup whose result depends on thestorage backend rather than on intent.
Verified
make lint(0 issues),go build,go vet, full Go suite, dashboardtypecheck + build — all green.
make generate-graphqlandmake proto-genre-run and
gen/committed.Note on scope
These are pre-existing bugs, not regressions from the audit work — but they were
surfaced by testing it, and both sit in the same email-centric assumption. Worth
a sweep for other places that key on email where an account may not have one;
this PR fixes the two that were actually hit.