Skip to content

fix: phone-only accounts could not enrol TOTP or be deleted - #753

Open
lakhansamani wants to merge 1 commit into
mainfrom
fix/phone-only-account-parity
Open

fix: phone-only accounts could not enrol TOTP or be deleted#753
lakhansamani wants to merge 1 commit into
mainfrom
fix/phone-only-account-parity

Conversation

@lakhansamani

@lakhansamani lakhansamani commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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 set

Reported symptom: signing up with a mobile number produced
AccountName must be set.

That is pquerna/otp rejecting an empty AccountName — the label the
authenticator app shows next to the code. Enrolment passed user.Email
verbatim, 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.email was required, and it is the only identifier the
endpoint 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)

  • GraphQL DeleteUserRequest is now { id: String! }email removed.
  • gRPC DeleteUserRequest takes string id = 2 with min_len = 1.
  • REST POST /v1/admin/delete_user takes {"id": "..."}.
  • The dashboard deletes by id.

Field 1 is reserved, not reused. email was a string and id is 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:

message DeleteUserRequest {
  reserved 1;
  reserved "email";
  string id = 2 [(buf.validate.field).string.min_len = 1];
}

Scoped to _delete_user / AuthorizerAdminService.DeleteUser. No public-API
surface changes.

⚠️ Needs the proto-breaking-approved label

buf breaking correctly reports:

Previously present field "1" with name "email" on message "DeleteUserRequest" was deleted.

That is the intended change. The repo already has the right lever for this — a
maintainer applies proto-breaking-approved and the gate becomes
continue-on-error for this PR only. I deliberately did not add a
buf.yaml ignore: that would suppress the check for the whole admin file and
hide the next accidental break.

buf lint passes, and make proto-check confirms gen/ is not stale.

Tests

  • TOTP enrolment across all three identifier shapes — phone-only (the reported
    case), email-only, and neither (falls back to the id). Verified fail-before:
    reverting the fix reproduces AccountName must be set on exactly the
    phone-only and neither cases.
  • Deleting a phone-only account by id — the case that was previously impossible.
  • An empty id is rejected at the service (GraphQL) and by min_len (gRPC);
    without that guard "" falls through to a lookup whose result depends on the
    storage backend rather than on intent.
  • Existing delete tests migrated to id across GraphQL, gRPC and REST.

Verified

make lint (0 issues), go build, go vet, full Go suite, dashboard
typecheck + build — all green. make generate-graphql and make proto-gen
re-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.

@lakhansamani
lakhansamani force-pushed the fix/phone-only-account-parity branch from 52cb1c9 to c5afc04 Compare August 7, 2026 09:47
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
lakhansamani force-pushed the fix/phone-only-account-parity branch from c5afc04 to 5bfc821 Compare August 7, 2026 10:14
@lakhansamani lakhansamani added the proto-breaking-approved Maintainer-approved intentional proto breaking change; relaxes buf breaking gate label Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

proto-breaking-approved Maintainer-approved intentional proto breaking change; relaxes buf breaking gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant