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

gen/openapi/authorizer.swagger.json

@@ -792,7 +792,7 @@
     },
     "/v1/admin/delete_user": {
       "post": {
-        "summary": "DeleteUser deletes a user (and associated OTP/verification data) by email.\nRequires super-admin auth.",
+        "summary": "DeleteUser deletes a user (and associated OTP/verification data) by id.\nRequires super-admin auth.",
         "operationId": "AuthorizerAdminService_DeleteUser",
         "responses": {
           "200": {
@@ -811,7 +811,7 @@
         "parameters": [
           {
             "name": "body",
-            "description": "DeleteUserRequest mirrors model.DeleteUserRequest.",
+            "description": "DeleteUserRequest mirrors model.DeleteUserRequest.\n\nBREAKING: this took `email` and now takes `id` only. Email was never an\nidentifier every account has — a phone-only signup has none — so an\nemail-keyed delete could not reach those accounts at all, and there was no\nsecond way in.\n\nField 1 is RESERVED rather than reused. `email` was a string and `id` is a\nstring, so reusing tag 1 would let an old client's email decode silently as\nan id: wire-compatible, semantically wrong, and pointed at a delete. Reserving\nmakes an old client fail loudly instead.",
             "in": "body",
             "required": true,
             "schema": {
@@ -4604,11 +4604,11 @@
     "v1DeleteUserRequest": {
       "type": "object",
       "properties": {
-        "email": {
+        "id": {
           "type": "string"
         }
       },
-      "description": "DeleteUserRequest mirrors model.DeleteUserRequest."
+      "description": "DeleteUserRequest mirrors model.DeleteUserRequest.\n\nBREAKING: this took `email` and now takes `id` only. Email was never an\nidentifier every account has — a phone-only signup has none — so an\nemail-keyed delete could not reach those accounts at all, and there was no\nsecond way in.\n\nField 1 is RESERVED rather than reused. `email` was a string and `id` is a\nstring, so reusing tag 1 would let an old client's email decode silently as\nan id: wire-compatible, semantically wrong, and pointed at a delete. Reserving\nmakes an old client fail loudly instead."
     },
     "v1DeleteUserResponse": {
       "type": "object",

internal/authenticators/totp/totp.go

@@ -127,10 +127,27 @@ func (p *provider) Generate(ctx context.Context, id string) (*config.Authenticat
 	if err != nil {
 		return nil, err
 	}
+	// AccountName is the label the authenticator app shows next to the code, and
+	// pquerna/otp REJECTS an empty one outright ("AccountName must be set").
+	// Email alone is therefore wrong: a phone-only signup has no email, so
+	// enrolling TOTP failed with that error surfaced straight to the user —
+	// which, since MFA is on by default, is the first thing a mobile signup
+	// hits after verifying.
+	//
+	// Fall 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.
+	accountName := refs.StringValue(user.Email)
+	if accountName == "" {
+		accountName = refs.StringValue(user.PhoneNumber)
+	}
+	if accountName == "" {
+		accountName = user.ID
+	}
+
 	// Generate totp, Authenticators hash is valid for 30 seconds
 	key, err := totp.Generate(totp.GenerateOpts{
 		Issuer:      "authorizer",
-		AccountName: refs.StringValue(user.Email),
+		AccountName: accountName,
 	})
 	if err != nil {
 		return nil, err

internal/graph/schema.graphqls

@@ -937,7 +937,10 @@ input ResetPasswordRequest {
 }
 
 input DeleteUserRequest {
-  email: String!
+  # BREAKING: was `email: String!`. Email is not an identifier every account
+  # has — a phone-only signup has none — so an email-keyed delete could not
+  # reach those accounts at all.
+  id: String!
 }
 
 input MagicLinkLoginRequest {

internal/grpcsrv/handlers/admin.go

@@ -127,7 +127,7 @@ func (h *AdminHandler) UpdateUser(ctx context.Context, req *authorizerv1.UpdateU
 // DeleteUser delegates to service.DeleteUser. Requires super-admin auth.
 func (h *AdminHandler) DeleteUser(ctx context.Context, req *authorizerv1.DeleteUserRequest) (*authorizerv1.DeleteUserResponse, error) {
 	res, _, err := h.Service.DeleteUser(ctx, transport.MetaFromGRPC(ctx), &model.DeleteUserRequest{
-		Email: req.GetEmail(),
+		ID: req.GetId(),
 	})
 	if err != nil {
 		return nil, err

internal/integration_tests/admin_users_grpc_test.go

@@ -196,26 +196,34 @@ func TestAdminUpdateUserGRPC(t *testing.T) {
 func TestAdminDeleteUserGRPC(t *testing.T) {
 	client, ts := newAdminClientWithSetup(t)
 	cfg := ts.Config
-	_, email := seedUser(t, ts)
+	// BREAKING: DeleteUser takes an id, not an email. Email was never an
+	// identifier every account has — a phone-only signup has none.
+	id, _ := seedUser(t, ts)
 
 	t.Run("fail closed without admin secret", func(t *testing.T) {
-		_, err := client.DeleteUser(context.Background(), &authorizerv1.DeleteUserRequest{Email: email})
+		_, err := client.DeleteUser(context.Background(), &authorizerv1.DeleteUserRequest{Id: id})
 		require.Error(t, err)
 		require.Equal(t, codes.Unauthenticated, status.Code(err))
 	})
 
 	t.Run("deletes user", func(t *testing.T) {
-		resp, err := client.DeleteUser(adminCtx(cfg.AdminSecret), &authorizerv1.DeleteUserRequest{Email: email})
+		resp, err := client.DeleteUser(adminCtx(cfg.AdminSecret), &authorizerv1.DeleteUserRequest{Id: id})
 		require.NoError(t, err)
 		require.Equal(t, "user deleted successfully", resp.Message)
 	})
 
 	t.Run("deleting unknown user is an error", func(t *testing.T) {
 		_, err := client.DeleteUser(adminCtx(cfg.AdminSecret), &authorizerv1.DeleteUserRequest{
-			Email: "does-not-exist-" + uuid.New().String() + "@authorizer.test",
+			Id: "does-not-exist-" + uuid.New().String(),
 		})
 		require.Error(t, err)
 	})
+
+	t.Run("an empty id is rejected by proto validation", func(t *testing.T) {
+		// min_len = 1 on the field, so this never reaches the service.
+		_, err := client.DeleteUser(adminCtx(cfg.AdminSecret), &authorizerv1.DeleteUserRequest{Id: ""})
+		require.Error(t, err)
+	})
 }
 
 // TestAdminVerificationRequestsGRPC exercises

internal/integration_tests/admin_users_rest_test.go

@@ -112,12 +112,14 @@ func TestAdminUsersREST(t *testing.T) {
 	})
 
 	t.Run("delete_user happy path", func(t *testing.T) {
-		_, email := seedUser(t, ts)
+		// BREAKING: delete_user takes an id, not an email. Email was never an
+		// identifier every account has — a phone-only signup has none.
+		id, _ := seedUser(t, ts)
 		var out struct {
 			Message string `json:"message"`
 		}
 		status := adminRESTJSON(t, baseURL, http.MethodPost, "/v1/admin/delete_user", secret,
-			fmt.Sprintf(`{"email":%q}`, email), &out)
+			fmt.Sprintf(`{"id":%q}`, id), &out)
 		require.Equal(t, http.StatusOK, status)
 		require.Equal(t, "user deleted successfully", out.Message)
 	})

internal/integration_tests/delete_user_cascade_test.go

@@ -64,7 +64,7 @@ func TestDeleteUserCascade(t *testing.T) {
 	clearCookies(ts)
 
 	setAdminCookie(t, ts)
-	deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{Email: email})
+	deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{ID: userID})
 	require.NoError(t, err)
 	require.NotNil(t, deleteRes)
 

internal/integration_tests/delete_user_test.go

@@ -3,13 +3,16 @@ package integration_tests
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/authorizerdev/authorizer/internal/constants"
 	"github.com/authorizerdev/authorizer/internal/graph/model"
+	"github.com/authorizerdev/authorizer/internal/refs"
+	"github.com/authorizerdev/authorizer/internal/storage/schemas"
 )
 
 // TestDeleteUser tests the delete user functionality by the admin
@@ -31,9 +34,11 @@ func TestDeleteUser(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, signupRes)
 	require.NotNil(t, signupRes.User)
+	// BREAKING: DeleteUser takes an id, not an email.
+	userID := signupRes.User.ID
 
 	t.Run("should fail without admin cookie", func(t *testing.T) {
-		deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{Email: email})
+		deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{ID: userID})
 		require.Error(t, err)
 		require.Nil(t, deleteRes)
 	})
@@ -43,8 +48,118 @@ func TestDeleteUser(t *testing.T) {
 		assert.Nil(t, err)
 
 		req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h))
-		deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{Email: email})
+		deleteRes, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{ID: userID})
 		require.NoError(t, err)
 		require.NotNil(t, deleteRes)
 	})
 }
+
+// TestDeleteUserByIDIncludingPhoneOnlyAccounts covers a reported gap: admin
+// delete accepted only an email, so a phone-only account — which never gets one
+// — could not be deleted at all. There was no second way in; the account was
+// permanent.
+//
+// id is now the preferred identifier because it is the only one every account
+// has, mirroring the id-or-email shape GetUserRequest already used. Email is
+// kept for existing callers.
+func TestDeleteUserByIDIncludingPhoneOnlyAccounts(t *testing.T) {
+	cfg := getTestConfig()
+	cfg.IsSMSServiceEnabled = true
+	cfg.EnableMobileBasicAuthentication = true
+	ts := initTestSetup(t, cfg)
+	req, ctx := createContext(ts)
+	h, err := newAdminSessionToken(ts)
+	require.NoError(t, err)
+	req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h))
+
+	t.Run("a phone-only account can be deleted by id", func(t *testing.T) {
+		phone := fmt.Sprintf("+1%010d", time.Now().UnixNano()%10000000000)
+		user, err := ts.StorageProvider.AddUser(ctx, &schemas.User{
+			PhoneNumber:   &phone,
+			SignupMethods: constants.AuthRecipeMethodMobileBasicAuth,
+		})
+		require.NoError(t, err)
+		require.Empty(t, refs.StringValue(user.Email), "this account has no email — that is the point")
+
+		res, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{ID: user.ID})
+		require.NoError(t, err, "an account with no email must still be deletable")
+		require.NotNil(t, res)
+
+		_, err = ts.StorageProvider.GetUserByID(ctx, user.ID)
+		assert.Error(t, err, "the account must actually be gone")
+	})
+
+	t.Run("an empty id is rejected", func(t *testing.T) {
+		// The GraphQL schema marks id non-null, but a caller can still send "".
+		// Without this guard that falls through to a lookup on the empty string,
+		// whose result depends on the storage backend rather than on intent.
+		_, err := ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{ID: ""})
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "id is required")
+	})
+}
+
+// TestDeleteUserDoesNotTouchOtherAccountsRows guards against cross-user data
+// deletion in the post-delete cleanup.
+//
+// generateAndStoreOTP writes Email and PhoneNumber as plain strings, so an
+// account holding only one of them stores the OTHER as "" — not NULL. The
+// cleanup's lookups are `WHERE email = ?` / `WHERE phone_number = ?`, so
+// passing "" matches every OTHER account in the same shape, and the row it
+// returns is then DELETED.
+//
+// The scenario below is deterministic on purpose: the deleted account has no
+// OTP of its own, so the empty-phone lookup can only match the bystander's.
+// This half is reachable TODAY for email-only accounts — it does not need the
+// phone-only delete this PR enables.
+func TestDeleteUserDoesNotTouchOtherAccountsRows(t *testing.T) {
+	cfg := getTestConfig()
+	cfg.IsSMSServiceEnabled = true
+	cfg.EnableMobileBasicAuthentication = true
+	ts := initTestSetup(t, cfg)
+	req, ctx := createContext(ts)
+	h, err := newAdminSessionToken(ts)
+	require.NoError(t, err)
+	req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h))
+
+	mkEmailUser := func(t *testing.T) *schemas.User {
+		t.Helper()
+		email := "cross_delete_" + uuid.NewString() + "@authorizer.dev"
+		u, err := ts.StorageProvider.AddUser(ctx, &schemas.User{
+			Email:         &email,
+			SignupMethods: constants.AuthRecipeMethodBasicAuth,
+		})
+		require.NoError(t, err)
+		return u
+	}
+
+	// The bystander holds a live OTP. Its phone_number column is "" because the
+	// account has no phone — that empty value is the collision key.
+	bystander := mkEmailUser(t)
+	_, err = ts.StorageProvider.UpsertOTP(ctx, &schemas.OTP{
+		Email:       refs.StringValue(bystander.Email),
+		PhoneNumber: refs.StringValue(bystander.PhoneNumber),
+		Otp:         "123456",
+		ExpiresAt:   time.Now().Add(5 * time.Minute).Unix(),
+	})
+	require.NoError(t, err)
+
+	// The account actually being deleted has NO OTP, so an empty-phone lookup
+	// cannot match its own row — only the bystander's.
+	doomed := mkEmailUser(t)
+
+	_, err = ts.GraphQLProvider.DeleteUser(ctx, &model.DeleteUserRequest{ID: doomed.ID})
+	require.NoError(t, err)
+
+	require.Eventually(t, func() bool {
+		_, err := ts.StorageProvider.GetUserByID(ctx, doomed.ID)
+		return err != nil
+	}, 3*time.Second, 50*time.Millisecond, "the targeted account should be gone")
+	// The cleanup is asynchronous; give it room to do damage if it is going to.
+	time.Sleep(500 * time.Millisecond)
+
+	otp, err := ts.StorageProvider.GetOTPByEmail(ctx, refs.StringValue(bystander.Email))
+	require.NoError(t, err,
+		"deleting an account with no phone number must not delete a DIFFERENT account's OTP via a `phone_number = \"\"` match")
+	assert.NotNil(t, otp)
+}