Repository: supabase/gotrue
Pull request #1326 (2023-11-30 05:31:22)

feat: unlinking primary identity should update email
## What kind of change does this PR introduce?
* A primary identity is implicitly defined by the first identity created when the user signs up
* Addresses the issue where unlinking a primary identity results in the `auth.users.email` becoming stale. If the other identities do not have the same email, the `auth.users.email` column should be updated to use one of the existing identities emails
* Update the `FindProvidersByUser` method to remove duplicates if there is more than 1 identity that share the same provider

hf: Would prefer if there as a test for this scenario.

Patch:
diff --git a/internal/api/identity.go b/internal/api/identity.go
index b3b69fae..066e18b6 100644
--- a/internal/api/identity.go
+++ b/internal/api/identity.go
@@ -57,6 +57,12 @@ func (a *API) DeleteIdentity(w http.ResponseWriter, r *http.Request) error {
 		if terr := tx.Destroy(identityToBeDeleted); terr != nil {
 			return internalServerError("Database error deleting identity").WithInternalError(terr)
 		}
+		if terr := user.UpdateUserEmail(tx); terr != nil {
+			if models.IsUniqueConstraintViolatedError(terr) {
+				return forbiddenError("Unable to unlink identity due to email conflict").WithInternalError(terr)
+			}
+			return internalServerError("Database error updating user email").WithInternalError(terr)
+		}
 		if terr := user.UpdateAppMetaDataProviders(tx); terr != nil {
 			return internalServerError("Database error updating user providers").WithInternalError(terr)
 		}
diff --git a/internal/models/errors.go b/internal/models/errors.go
index 381517b6..68f302ee 100644
--- a/internal/models/errors.go
+++ b/internal/models/errors.go
@@ -105,3 +105,17 @@ type FlowStateNotFoundError struct{}
 func (e FlowStateNotFoundError) Error() string {
 	return "Flow State not found"
 }
+
+func IsUniqueConstraintViolatedError(err error) bool {
+	switch err.(type) {
+	case UserEmailUniqueConflictError, *UserEmailUniqueConflictError:
+		return true
+	}
+	return false
+}
+
+type UserEmailUniqueConflictError struct{}
+
+func (e UserEmailUniqueConflictError) Error() string {
+	return "User email unique constraint violated"
+}
diff --git a/internal/models/identity.go b/internal/models/identity.go
index 1ada9130..0b7127ce 100644
--- a/internal/models/identity.go
+++ b/internal/models/identity.go
@@ -17,13 +17,14 @@ type Identity struct {
 	ID uuid.UUID `json:"identity_id" db:"id"`
 	// returned as id in JSON for backward compatibility with the interface exposed by the client library
 	// see https://github.com/supabase/gotrue-js/blob/c9296bbc27a2f036af55c1f33fca5930704bd021/src/lib/types.ts#L230-L240
-	ProviderID   string     `json:"id" db:"provider_id"`
-	UserID       uuid.UUID  `json:"user_id" db:"user_id"`
-	IdentityData JSONMap    `json:"identity_data,omitempty" db:"identity_data"`
-	Provider     string     `json:"provider" db:"provider"`
-	LastSignInAt *time.Time `json:"last_sign_in_at,omitempty" db:"last_sign_in_at"`
-	CreatedAt    time.Time  `json:"created_at" db:"created_at"`
-	UpdatedAt    time.Time  `json:"updated_at" db:"updated_at"`
+	ProviderID   string             `json:"id" db:"provider_id"`
+	UserID       uuid.UUID          `json:"user_id" db:"user_id"`
+	IdentityData JSONMap            `json:"identity_data,omitempty" db:"identity_data"`
+	Provider     string             `json:"provider" db:"provider"`
+	LastSignInAt *time.Time         `json:"last_sign_in_at,omitempty" db:"last_sign_in_at"`
+	CreatedAt    time.Time          `json:"created_at" db:"created_at"`
+	UpdatedAt    time.Time          `json:"updated_at" db:"updated_at"`
+	Email        storage.NullString `json:"email,omitempty" db:"email" rw:"r"`
 }
 
 func (Identity) TableName() string {
@@ -31,6 +32,11 @@ func (Identity) TableName() string {
 	return tableName
 }
 
+// GetEmail returns the user's email as a string
+func (i *Identity) GetEmail() string {
+	return string(i.Email)
+}
+
 // NewIdentity returns an identity associated to the user's id.
 func NewIdentity(user *User, provider string, identityData map[string]interface{}) (*Identity, error) {
 	providerId, ok := identityData["sub"]
@@ -46,6 +52,9 @@ func NewIdentity(user *User, provider string, identityData map[string]interface{
 		Provider:     provider,
 		LastSignInAt: &now,
 	}
+	if email, ok := identityData["email"]; ok {
+		identity.Email = storage.NullString(email.(string))
+	}
 
 	return identity, nil
 }
@@ -92,6 +101,7 @@ func FindIdentitiesByUserID(tx *storage.Connection, userID uuid.UUID) ([]*Identi
 // FindProvidersByUser returns all providers associated to a user
 func FindProvidersByUser(tx *storage.Connection, user *User) ([]string, error) {
 	identities := []Identity{}
+	providerExists := map[string]bool{}
 	providers := make([]string, 0)
 	if err := tx.Q().Select("provider").Where("user_id = ?", user.ID).All(&identities); err != nil {
 		if errors.Cause(err) == sql.ErrNoRows {
@@ -100,7 +110,10 @@ func FindProvidersByUser(tx *storage.Connection, user *User) ([]string, error) {
 		return nil, errors.Wrap(err, "error finding providers")
 	}
 	for _, identity := range identities {
-		providers = append(providers, identity.Provider)
+		if _, ok := providerExists[identity.Provider]; !ok {
+			providers = append(providers, identity.Provider)
+			providerExists[identity.Provider] = true
+		}
 	}
 	return providers, nil
 }
diff --git a/internal/models/user.go b/internal/models/user.go
index d52a7047..ed21d632 100644
--- a/internal/models/user.go
+++ b/internal/models/user.go
@@ -227,6 +227,41 @@ func (u *User) UpdateAppMetaDataProviders(tx *storage.Connection) error {
 	return u.UpdateAppMetaData(tx, payload)
 }
 
+// UpdateUserEmail updates the user's email to one of the identity's email
+// if the current email used doesn't match any of the identities email
+func (u *User) UpdateUserEmail(tx *storage.Connection) error {
+	identities, terr := FindIdentitiesByUserID(tx, u.ID)
+	if terr != nil {
+		return terr
+	}
+	for _, i := range identities {
+		if u.GetEmail() == i.GetEmail() {
+			// there's an existing identity that uses the same email
+			// so the user's email can be kept
+			return nil
+		}
+	}
+
+	var primaryIdentity *Identity
+	for _, i := range identities {
+		if _, terr := FindUserByEmailAndAudience(tx, i.GetEmail(), u.Aud); terr != nil {
+			if IsNotFoundError(terr) {
+				primaryIdentity = i
+				break
+			}
+			return terr
+		}
+	}
+	if primaryIdentity == nil {
+		return UserEmailUniqueConflictError{}
+	}
+	// default to the first identity's email
+	if terr := u.SetEmail(tx, primaryIdentity.GetEmail()); terr != nil {
+		return terr
+	}
+	return nil
+}
+
 // SetEmail sets the user's email
 func (u *User) SetEmail(tx *storage.Connection, email string) error {
 	u.Email = storage.NullString(email)


Test patch:
diff --git a/internal/models/user_test.go b/internal/models/user_test.go
index f0b85a43..a09a5b98 100644
--- a/internal/models/user_test.go
+++ b/internal/models/user_test.go
@@ -303,3 +303,66 @@ func (ts *UserTestSuite) TestConfirmPhoneChange() {
 	require.NotNil(ts.T(), identity.IdentityData)
 	require.Equal(ts.T(), identity.IdentityData["phone"], "987654321")
 }
+
+func (ts *UserTestSuite) TestUpdateUserEmailSuccess() {
+	userA, err := NewUser("", "foo@example.com", "", "authenticated", nil)
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(userA))
+
+	primaryIdentity, err := NewIdentity(userA, "email", map[string]interface{}{
+		"sub":   userA.ID.String(),
+		"email": "foo@example.com",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(primaryIdentity))
+
+	secondaryIdentity, err := NewIdentity(userA, "google", map[string]interface{}{
+		"sub":   userA.ID.String(),
+		"email": "bar@example.com",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(secondaryIdentity))
+
+	// UpdateUserEmail should not do anything and the user's email should still use the primaryIdentity
+	require.NoError(ts.T(), userA.UpdateUserEmail(ts.db))
+	require.Equal(ts.T(), primaryIdentity.GetEmail(), userA.GetEmail())
+
+	// remove primary identity
+	require.NoError(ts.T(), ts.db.Destroy(primaryIdentity))
+
+	// UpdateUserEmail should update the user to use the secondary identity's email
+	require.NoError(ts.T(), userA.UpdateUserEmail(ts.db))
+	require.Equal(ts.T(), secondaryIdentity.GetEmail(), userA.GetEmail())
+}
+
+func (ts *UserTestSuite) TestUpdateUserEmailFailure() {
+	userA, err := NewUser("", "foo@example.com", "", "authenticated", nil)
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(userA))
+
+	primaryIdentity, err := NewIdentity(userA, "email", map[string]interface{}{
+		"sub":   userA.ID.String(),
+		"email": "foo@example.com",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(primaryIdentity))
+
+	secondaryIdentity, err := NewIdentity(userA, "google", map[string]interface{}{
+		"sub":   userA.ID.String(),
+		"email": "bar@example.com",
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(secondaryIdentity))
+
+	userB, err := NewUser("", "bar@example.com", "", "authenticated", nil)
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(userB))
+
+	// remove primary identity
+	require.NoError(ts.T(), ts.db.Destroy(primaryIdentity))
+
+	// UpdateUserEmail should fail with the email unique constraint violation error
+	//  since userB is using the secondary identity's email
+	require.EqualError(ts.T(), userA.UpdateUserEmail(ts.db), "User email unique constraint violated")
+	require.Equal(ts.T(), primaryIdentity.GetEmail(), userA.GetEmail())
+}