fix(storage): drop stale unique email/phone constraints on upgrade (#628)

* fix(storage): drop stale unique email/phone constraints on upgrade

v1 declared email and phone_number UNIQUE on authorizer_users and
authorizer_otps; v2 keeps non-unique indexes and enforces uniqueness in
the application layer. On an upgraded SQL database GORM AutoMigrate emits
DROP CONSTRAINT "uni_<table>_<col>" — a name that does not match what the
DB created — and aborts with SQLSTATE 42704, so the server fails to start
("failed to create storage provider").

Clear the legacy uniqueness before AutoMigrate for all four columns. The
old form is a constraint (<table>_<col>_key / uni_<table>_<col>) on some
v1 releases and a standalone unique index (idx_<table>_<col>) on others,
so handle both: drop the named constraints, then drop any single-column
UNIQUE index on email/phone_number via GetIndexes (name-agnostic, leaves
the current non-unique indexes intact). Non-fatal; fresh installs are
unaffected. Generalises the prior phone_number-only handling.

* test(storage): cover email/phone uniqueness and stale-constraint migration

Two SQL provider tests:

- TestProviderEmailPhoneUpdatesAndUniqueness (all SQL backends): a user can
  update their own email and phone number seamlessly, duplicates across
  different users are still rejected (app-layer uniqueness), and OTPs key
  independently on email and phone.
- TestStaleUniqueConstraintMigration (Postgres): seeds a legacy UNIQUE
  constraint on users.email and a legacy UNIQUE index on otps.phone_number,
  then asserts startup drops them (no SQLSTATE 42704 abort), the search index
  is preserved as non-unique, and a post-migration phone update succeeds.

Honors TEST_DBS; defaults to sqlite locally, Postgres path runs in CI.

Author: lakhansamani

internal/storage/db/sql/provider.go

@@ -74,12 +74,69 @@ func NewProvider(
 		return nil, err
 	}
 
-	// For sqlserver, handle uniqueness of phone_number manually via extra db call
-	// during create and update mutation.
-	if sqlDB.Migrator().HasConstraint(&schemas.User{}, "authorizer_users_phone_number_key") {
-		err = sqlDB.Migrator().DropConstraint(&schemas.User{}, "authorizer_users_phone_number_key")
-		if err != nil {
-			deps.Log.Debug().Err(err).Msg("failed to drop unique constraint on phone_number")
+	// v1 declared the email and phone_number columns of authorizer_users and
+	// authorizer_otps as UNIQUE. v2 keeps plain (non-unique) indexes and enforces
+	// uniqueness in the application layer (see AddUser/UpdateUser) so behaviour is
+	// identical across all supported databases and these fields can stay optional
+	// (email-only or phone-only signups).
+	//
+	// On an upgraded SQL database those columns are still unique, so GORM
+	// AutoMigrate tries to DROP CONSTRAINT "uni_<table>_<col>" — a name that does
+	// not match what the database actually created — failing with SQLSTATE 42704
+	// ("constraint does not exist") and aborting the whole migration. Depending on
+	// the v1 release, the old uniqueness exists either as a CONSTRAINT
+	// (<table>_<col>_key) or as a standalone UNIQUE INDEX (idx_<table>_<col>), so
+	// we clear both forms before AutoMigrate. Non-fatal: fresh installs have
+	// nothing to drop.
+	staleUniqueColumns := []struct {
+		model      any
+		table, col string
+	}{
+		{&schemas.User{}, "authorizer_users", "email"},
+		{&schemas.User{}, "authorizer_users", "phone_number"},
+		{&schemas.OTP{}, "authorizer_otps", "email"},
+		{&schemas.OTP{}, "authorizer_otps", "phone_number"},
+	}
+
+	// 1) Drop stale unique CONSTRAINTs by their well-known names.
+	//    "<table>_<col>_key" is the Postgres/MySQL default; "uni_<table>_<col>" is
+	//    GORM's default unique-constraint name.
+	for _, c := range staleUniqueColumns {
+		for _, name := range []string{c.table + "_" + c.col + "_key", "uni_" + c.table + "_" + c.col} {
+			if sqlDB.Migrator().HasConstraint(c.model, name) {
+				if dropErr := sqlDB.Migrator().DropConstraint(c.model, name); dropErr != nil {
+					deps.Log.Debug().Err(dropErr).Str("constraint", name).Msg("failed to drop stale unique constraint")
+				}
+			}
+		}
+	}
+
+	// 2) Drop stale standalone UNIQUE INDEXes on the same columns (e.g.
+	//    idx_authorizer_otps_phone_number created by a v1 gorm:"uniqueIndex").
+	//    This is name-agnostic: only single-column UNIQUE indexes on email /
+	//    phone_number are removed, so the current schema's non-unique indexes are
+	//    left intact and AutoMigrate recreates anything it needs as non-unique.
+	uniqueColTargets := map[string]bool{"email": true, "phone_number": true}
+	for _, model := range []any{&schemas.User{}, &schemas.OTP{}} {
+		indexes, idxErr := sqlDB.Migrator().GetIndexes(model)
+		if idxErr != nil {
+			deps.Log.Debug().Err(idxErr).Msg("failed to list indexes while clearing stale unique indexes")
+			continue
+		}
+		for _, idx := range indexes {
+			unique, ok := idx.Unique()
+			if !ok || !unique {
+				continue
+			}
+			cols := idx.Columns()
+			if len(cols) != 1 || !uniqueColTargets[cols[0]] {
+				continue
+			}
+			if dropErr := sqlDB.Migrator().DropIndex(model, idx.Name()); dropErr != nil {
+				// Constraint-backed indexes (handled in step 1) can't be dropped
+				// directly on Postgres — that's expected and harmless here.
+				deps.Log.Debug().Err(dropErr).Str("index", idx.Name()).Msg("failed to drop stale unique index")
+			}
 		}
 	}
 

internal/storage/db/sql/provider_migration_test.go

@@ -0,0 +1,240 @@
+package sql
+
+import (
+	"context"
+	"net"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/rs/zerolog"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/authorizerdev/authorizer/internal/config"
+	"github.com/authorizerdev/authorizer/internal/constants"
+	"github.com/authorizerdev/authorizer/internal/refs"
+	"github.com/authorizerdev/authorizer/internal/storage/schemas"
+)
+
+func sqlTestDeps(t *testing.T) *Dependencies {
+	t.Helper()
+	logger := zerolog.New(zerolog.NewTestWriter(t)).With().Timestamp().Logger()
+	return &Dependencies{Log: &logger}
+}
+
+// sqlMigrationTestDBTypes returns the SQL backends to exercise. Honors TEST_DBS
+// (comma-separated); defaults to sqlite so local runs stay light. CI sets
+// TEST_DBS=postgres to cover the engine where the original failure was reported.
+func sqlMigrationTestDBTypes() []string {
+	supported := map[string]bool{
+		constants.DbTypePostgres: true,
+		constants.DbTypeSqlite:   true,
+		constants.DbTypeMysql:    true,
+		constants.DbTypeMariaDB:  true,
+	}
+	env := os.Getenv("TEST_DBS")
+	if env == "" {
+		return []string{constants.DbTypeSqlite}
+	}
+	var out []string
+	for _, p := range strings.Split(env, ",") {
+		p = strings.TrimSpace(p)
+		if supported[p] {
+			out = append(out, p)
+		}
+	}
+	return out
+}
+
+// sqlMigrationTestConfig builds a config for dbType, skipping the test when the
+// backend is not reachable in this environment.
+func sqlMigrationTestConfig(t *testing.T, dbType string) *config.Config {
+	t.Helper()
+	cfg := &config.Config{
+		DatabaseType: dbType,
+		DatabaseName: "authorizer_test",
+		DefaultRoles: []string{"user"},
+	}
+	switch dbType {
+	case constants.DbTypeSqlite:
+		cfg.DatabaseURL = filepath.Join(t.TempDir(), "migration_test.db")
+	case constants.DbTypePostgres:
+		cfg.DatabaseURL = "postgres://postgres:postgres@localhost:5434/postgres"
+		skipIfTCPClosed(t, "localhost:5434")
+	case constants.DbTypeMysql, constants.DbTypeMariaDB:
+		cfg.DatabaseURL = "root:password@tcp(localhost:3306)/authorizer_test"
+		skipIfTCPClosed(t, "localhost:3306")
+	default:
+		t.Skipf("unsupported SQL db type for migration test: %s", dbType)
+	}
+	return cfg
+}
+
+func skipIfTCPClosed(t *testing.T, addr string) {
+	t.Helper()
+	conn, err := net.DialTimeout("tcp", addr, 2*time.Second)
+	if err != nil {
+		t.Skipf("skipping: %s not reachable: %v", addr, err)
+	}
+	_ = conn.Close()
+}
+
+// TestProviderEmailPhoneUpdatesAndUniqueness covers v2's application-layer
+// uniqueness for users and OTPs (email and phone_number are non-unique indexes
+// in the DB). It asserts that a user can update their own email/phone number
+// seamlessly, that duplicates across different users are still rejected, and
+// that OTPs key independently on email and phone.
+func TestProviderEmailPhoneUpdatesAndUniqueness(t *testing.T) {
+	for _, dbType := range sqlMigrationTestDBTypes() {
+		t.Run(dbType, func(t *testing.T) {
+			cfg := sqlMigrationTestConfig(t, dbType)
+			p, err := NewProvider(cfg, sqlTestDeps(t))
+			require.NoError(t, err)
+			require.NotNil(t, p)
+			ctx := context.Background()
+
+			uniq := strings.ReplaceAll(uuid.New().String(), "-", "")
+			emailA := "a_" + uniq + "@test.com"
+			phoneA := "+1100" + uniq[:9]
+
+			userA := &schemas.User{
+				ID:            uuid.New().String(),
+				Email:         refs.NewStringRef(emailA),
+				PhoneNumber:   refs.NewStringRef(phoneA),
+				SignupMethods: "basic_auth",
+			}
+			userA, err = p.AddUser(ctx, userA)
+			require.NoError(t, err)
+
+			// Seamless self-update: change A's phone number to a new value.
+			newPhoneA := "+1900" + uniq[:9]
+			userA.PhoneNumber = refs.NewStringRef(newPhoneA)
+			_, err = p.UpdateUser(ctx, userA)
+			require.NoError(t, err, "updating own phone number should succeed")
+			gotByPhone, err := p.GetUserByPhoneNumber(ctx, newPhoneA)
+			require.NoError(t, err)
+			assert.Equal(t, userA.ID, gotByPhone.ID)
+
+			// Seamless self-update: change A's email, then re-save (idempotent).
+			newEmailA := "a2_" + uniq + "@test.com"
+			userA.Email = refs.NewStringRef(newEmailA)
+			_, err = p.UpdateUser(ctx, userA)
+			require.NoError(t, err, "updating own email should succeed")
+			_, err = p.UpdateUser(ctx, userA)
+			require.NoError(t, err, "re-saving the same user should succeed")
+			gotByEmail, err := p.GetUserByEmail(ctx, newEmailA)
+			require.NoError(t, err)
+			assert.Equal(t, userA.ID, gotByEmail.ID)
+
+			// A different user cannot claim A's email or phone via AddUser.
+			_, err = p.AddUser(ctx, &schemas.User{
+				ID: uuid.New().String(), Email: refs.NewStringRef(newEmailA), SignupMethods: "basic_auth",
+			})
+			assert.Error(t, err, "duplicate email should be rejected")
+			_, err = p.AddUser(ctx, &schemas.User{
+				ID: uuid.New().String(), PhoneNumber: refs.NewStringRef(newPhoneA), SignupMethods: "basic_auth",
+			})
+			assert.Error(t, err, "duplicate phone should be rejected")
+
+			// OTPs key on email and phone independently; upsert by the same email
+			// updates in place.
+			otpEmail := &schemas.OTP{Email: newEmailA, Otp: "111111", ExpiresAt: time.Now().Add(5 * time.Minute).Unix()}
+			_, err = p.UpsertOTP(ctx, otpEmail)
+			require.NoError(t, err)
+			otpEmail.Otp = "222222"
+			_, err = p.UpsertOTP(ctx, otpEmail)
+			require.NoError(t, err)
+			fetchedOTP, err := p.GetOTPByEmail(ctx, newEmailA)
+			require.NoError(t, err)
+			assert.Equal(t, "222222", fetchedOTP.Otp)
+
+			otpPhone := &schemas.OTP{PhoneNumber: newPhoneA, Otp: "333333", ExpiresAt: time.Now().Add(5 * time.Minute).Unix()}
+			_, err = p.UpsertOTP(ctx, otpPhone)
+			require.NoError(t, err)
+			fetchedPhoneOTP, err := p.GetOTPByPhoneNumber(ctx, newPhoneA)
+			require.NoError(t, err)
+			assert.Equal(t, "333333", fetchedPhoneOTP.Otp)
+
+			// Cleanup.
+			_ = p.DeleteOTP(ctx, fetchedOTP)
+			_ = p.DeleteOTP(ctx, fetchedPhoneOTP)
+			_ = p.DeleteUser(ctx, userA)
+		})
+	}
+}
+
+// TestStaleUniqueConstraintMigration reproduces an upgraded v1 database whose
+// email/phone columns still carry UNIQUE objects, and asserts the provider
+// clears them on startup instead of aborting with SQLSTATE 42704
+// (regression introduced by the GORM 1.25.10 bump in 2.3.0-rc.1). Postgres only:
+// it has named UNIQUE constraints and standalone unique indexes with stable
+// DDL; sqlite rebuilds tables and has no comparable failure mode.
+func TestStaleUniqueConstraintMigration(t *testing.T) {
+	for _, dbType := range sqlMigrationTestDBTypes() {
+		if dbType != constants.DbTypePostgres {
+			continue
+		}
+		t.Run(dbType, func(t *testing.T) {
+			cfg := sqlMigrationTestConfig(t, dbType)
+			deps := sqlTestDeps(t)
+			ctx := context.Background()
+
+			// First boot creates the v2 schema (non-unique indexes).
+			p1, err := NewProvider(cfg, deps)
+			require.NoError(t, err)
+
+			// Simulate a v1 database: a UNIQUE CONSTRAINT on users.email and a
+			// standalone UNIQUE INDEX on otps.phone_number — the two real-world
+			// forms reported in the field. Idempotent so reruns against the shared
+			// test DB don't fail on pre-existing objects.
+			require.NoError(t, p1.db.Exec(`ALTER TABLE authorizer_users DROP CONSTRAINT IF EXISTS authorizer_users_email_key`).Error)
+			require.NoError(t, p1.db.Exec(`ALTER TABLE authorizer_users ADD CONSTRAINT authorizer_users_email_key UNIQUE (email)`).Error)
+			require.NoError(t, p1.db.Exec(`DROP INDEX IF EXISTS idx_authorizer_otps_phone_number`).Error)
+			require.NoError(t, p1.db.Exec(`CREATE UNIQUE INDEX idx_authorizer_otps_phone_number ON authorizer_otps (phone_number)`).Error)
+			require.True(t, p1.db.Migrator().HasConstraint(&schemas.User{}, "authorizer_users_email_key"),
+				"precondition: stale unique constraint seeded")
+
+			// Second boot must clear the stale uniqueness and complete migration.
+			p2, err := NewProvider(cfg, deps)
+			require.NoError(t, err, "startup must not abort with SQLSTATE 42704")
+
+			assert.False(t, p2.db.Migrator().HasConstraint(&schemas.User{}, "authorizer_users_email_key"),
+				"stale unique constraint should be dropped")
+
+			// The search index is preserved, just no longer unique: AutoMigrate
+			// recreates idx_authorizer_otps_phone_number as a non-unique index.
+			indexes, err := p2.db.Migrator().GetIndexes(&schemas.OTP{})
+			require.NoError(t, err)
+			var phoneIdx string
+			for _, idx := range indexes {
+				if cols := idx.Columns(); len(cols) == 1 && cols[0] == "phone_number" {
+					phoneIdx = idx.Name()
+					if unique, ok := idx.Unique(); ok {
+						assert.False(t, unique, "otps.phone_number index should no longer be unique")
+					}
+				}
+			}
+			assert.NotEmpty(t, phoneIdx, "non-unique search index on otps.phone_number should still exist")
+
+			// After migrating a legacy DB, a user can still update their phone
+			// number seamlessly (no stale constraint in the way).
+			uniq := strings.ReplaceAll(uuid.New().String(), "-", "")
+			u := &schemas.User{
+				ID:            uuid.New().String(),
+				Email:         refs.NewStringRef("m_" + uniq + "@test.com"),
+				PhoneNumber:   refs.NewStringRef("+1700" + uniq[:9]),
+				SignupMethods: "basic_auth",
+			}
+			u, err = p2.AddUser(ctx, u)
+			require.NoError(t, err)
+			u.PhoneNumber = refs.NewStringRef("+1600" + uniq[:9])
+			_, err = p2.UpdateUser(ctx, u)
+			require.NoError(t, err, "updating phone after migration should succeed")
+			_ = p2.DeleteUser(ctx, u)
+		})
+	}
+}