fix: clear pending phone change when the user's email is replaced

Author: fadymak

internal/models/user.go

@@ -328,44 +328,14 @@ func (u *User) UpdateUserEmailFromIdentities(tx *storage.Connection) error {
 	if primaryIdentity == nil {
 		return UserEmailUniqueConflictError{}
 	}
-	previousEmail := u.GetEmail()
 	// default to the first identity's email
 	if terr := u.SetEmail(tx, primaryIdentity.GetEmail()); terr != nil {
 		return terr
 	}
-	if previousEmail != "" {
-		// outstanding tokens were sent to the previous email which is no
-		// longer linked to the user so they can't be trusted anymore
-		u.ConfirmationToken = ""
-		u.ConfirmationSentAt = nil
-		u.RecoveryToken = ""
-		u.RecoverySentAt = nil
-		u.EmailChange = ""
-		u.EmailChangeTokenCurrent = ""
-		u.EmailChangeTokenNew = ""
-		u.EmailChangeSentAt = nil
-		u.EmailChangeConfirmStatus = 0
-		u.ReauthenticationToken = ""
-		u.ReauthenticationSentAt = nil
-		if terr := tx.UpdateOnly(
-			u,
-			"confirmation_token",
-			"confirmation_sent_at",
-			"recovery_token",
-			"recovery_sent_at",
-			"email_change",
-			"email_change_token_current",
-			"email_change_token_new",
-			"email_change_sent_at",
-			"email_change_confirm_status",
-			"reauthentication_token",
-			"reauthentication_sent_at",
-		); terr != nil {
-			return terr
-		}
-		if terr := ClearAllOneTimeTokensForUser(tx, u.ID); terr != nil {
-			return terr
-		}
+	// outstanding tokens and pending account changes were issued before the
+	// primary email transition so they can't be trusted anymore
+	if terr := u.ClearAllPendingTokens(tx); terr != nil {
+		return terr
 	}
 	if primaryIdentity.GetEmail() == "" || !primaryIdentity.IsEmailVerified() {
 		// the promoted email was never verified by the IdP or ourselves,
@@ -383,6 +353,46 @@ func (u *User) UpdateUserEmailFromIdentities(tx *storage.Connection) error {
 	return nil
 }
 
+// ClearAllPendingTokens revokes all outstanding confirmation, recovery,
+// email change, phone change and reauthentication tokens issued for the
+// user, together with their one-time token rows.
+func (u *User) ClearAllPendingTokens(tx *storage.Connection) error {
+	u.ConfirmationToken = ""
+	u.ConfirmationSentAt = nil
+	u.RecoveryToken = ""
+	u.RecoverySentAt = nil
+	u.EmailChange = ""
+	u.EmailChangeTokenCurrent = ""
+	u.EmailChangeTokenNew = ""
+	u.EmailChangeSentAt = nil
+	u.EmailChangeConfirmStatus = 0
+	u.PhoneChange = ""
+	u.PhoneChangeToken = ""
+	u.PhoneChangeSentAt = nil
+	u.ReauthenticationToken = ""
+	u.ReauthenticationSentAt = nil
+	if terr := tx.UpdateOnly(
+		u,
+		"confirmation_token",
+		"confirmation_sent_at",
+		"recovery_token",
+		"recovery_sent_at",
+		"email_change",
+		"email_change_token_current",
+		"email_change_token_new",
+		"email_change_sent_at",
+		"email_change_confirm_status",
+		"phone_change",
+		"phone_change_token",
+		"phone_change_sent_at",
+		"reauthentication_token",
+		"reauthentication_sent_at",
+	); terr != nil {
+		return terr
+	}
+	return ClearAllOneTimeTokensForUser(tx, u.ID)
+}
+
 // SetEmail sets the user's email
 func (u *User) SetEmail(tx *storage.Connection, email string) error {
 	u.Email = storage.NullString(email)

internal/models/user_test.go

@@ -633,6 +633,9 @@ func (ts *UserTestSuite) TestUpdateUserEmailClearsStaleTokens() {
 	userA.EmailChangeTokenNew = "email-change-new-hash"
 	userA.EmailChangeSentAt = &now
 	userA.EmailChangeConfirmStatus = 1
+	userA.PhoneChange = "123456789"
+	userA.PhoneChangeToken = "phone-change-token-hash"
+	userA.PhoneChangeSentAt = &now
 	userA.ReauthenticationToken = "reauthentication-token-hash"
 	userA.ReauthenticationSentAt = &now
 	require.NoError(ts.T(), ts.db.UpdateOnly(
@@ -646,6 +649,9 @@ func (ts *UserTestSuite) TestUpdateUserEmailClearsStaleTokens() {
 		"email_change_token_new",
 		"email_change_sent_at",
 		"email_change_confirm_status",
+		"phone_change",
+		"phone_change_token",
+		"phone_change_sent_at",
 		"reauthentication_token",
 		"reauthentication_sent_at",
 	))
@@ -667,6 +673,9 @@ func (ts *UserTestSuite) TestUpdateUserEmailClearsStaleTokens() {
 	require.Empty(ts.T(), userA.EmailChangeTokenNew)
 	require.Nil(ts.T(), userA.EmailChangeSentAt)
 	require.Equal(ts.T(), 0, userA.EmailChangeConfirmStatus)
+	require.Empty(ts.T(), userA.PhoneChange)
+	require.Empty(ts.T(), userA.PhoneChangeToken)
+	require.Nil(ts.T(), userA.PhoneChangeSentAt)
 	require.Empty(ts.T(), userA.ReauthenticationToken)
 	require.Nil(ts.T(), userA.ReauthenticationSentAt)
 
@@ -675,6 +684,49 @@ func (ts *UserTestSuite) TestUpdateUserEmailClearsStaleTokens() {
 	require.Error(ts.T(), err)
 }
 
+func (ts *UserTestSuite) TestUpdateUserEmailFromEmptyClearsStaleTokens() {
+	// a user without an email or identities, e.g. an anonymous user
+	userA, err := NewUser("", "", "", "authenticated", nil)
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(userA))
+
+	identity, err := NewIdentity(userA, "google", map[string]any{
+		"sub":            userA.ID.String(),
+		"email":          "bar@example.com",
+		"email_verified": true,
+	})
+	require.NoError(ts.T(), err)
+	require.NoError(ts.T(), ts.db.Create(identity))
+
+	// simulate a phone change requested before the identity was linked
+	now := time.Now()
+	userA.PhoneChange = "123456789"
+	userA.PhoneChangeToken = "phone-change-token-hash"
+	userA.PhoneChangeSentAt = &now
+	require.NoError(ts.T(), ts.db.UpdateOnly(
+		userA,
+		"phone_change",
+		"phone_change_token",
+		"phone_change_sent_at",
+	))
+	require.NoError(ts.T(), CreateOneTimeToken(ts.db, userA.ID, userA.PhoneChange, userA.PhoneChangeToken, PhoneChangeToken))
+
+	// promoting an identity's email over an empty one is still a primary
+	// email transition, so outstanding tokens must be revoked
+	require.NoError(ts.T(), userA.UpdateUserEmailFromIdentities(ts.db))
+	require.Equal(ts.T(), identity.GetEmail(), userA.GetEmail())
+
+	userA, err = FindUserByID(ts.db, userA.ID)
+	require.NoError(ts.T(), err)
+	require.Empty(ts.T(), userA.PhoneChange)
+	require.Empty(ts.T(), userA.PhoneChangeToken)
+	require.Nil(ts.T(), userA.PhoneChangeSentAt)
+
+	_, err = FindOneTimeToken(ts.db, "phone-change-token-hash", PhoneChangeToken)
+	require.Error(ts.T(), err)
+	require.True(ts.T(), IsNotFoundError(err))
+}
+
 func (ts *UserTestSuite) TestUpdateUserEmailFailure() {
 	userA, err := NewUser("", "foo@example.com", "", "authenticated", nil)
 	require.NoError(ts.T(), err)