fix: discard v25 secrets migrations errors instead of failing (#7251)

Failing the migration when a corrupted record is found is problematic because there is no transaction and the database may need to be restored from a backup to attempt the migration again, after deleting the corrupted records.

Each documented case of failed migration was resolved by removing the corrupted records. There is no instance of a failed migration that was caused by non corrupted record.

In the unlikely event of a false negative where a two_factor record is discarded although it is in use, the only consequence is that the user will have to enroll again. Detailed logs are displayed so the Forgejo admin can file a bug report if that happens.

Refs: https://codeberg.org/forgejo/forgejo/issues/6637

<!--start release-notes-assistant-->

## Release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Bug fixes
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/7251): <!--number 7251 --><!--line 0 --><!--description V2hlbiBtaWdyYXRpbmcgdG8gRm9yZ2VqbyB2MTAsIHRoZSBUT1RQIHNlY3JldHMgZm91bmQgdG8gYmUgY29ycnVwdGVkIGFyZSBub3cgdHJhbnNwYXJlbnRseSByZW1vdmVkIGZyb20gdGhlIGRhdGFiYXNlIGluc3RlYWQgb2YgZmFpbGluZyB0aGUgbWlncmF0aW9uLiBUT1RQIGlzIG5vIGxvbmdlciByZXF1aXJlZCB0byBsb2dpbiB3aXRoIHRoZSBhc3NvY2lhdGVkIHVzZXJzLiBUaGV5IHNob3VsZCBiZSBpbmZvcm1lZCBiZWNhdXNlIHRoZXkgd2lsbCBuZWVkIHRvIHZpc2l0IHRoZWlyIHNlY3VyaXR5IHNldHRpbmdzIGFuZCBjb25maWd1cmUgVE9UUCBhZ2Fpbi4gTm8gb3RoZXIgYWN0aW9uIGlzIHJlcXVpcmVkLg==-->When migrating to Forgejo v10, the TOTP secrets found to be corrupted are now transparently removed from the database instead of failing the migration. TOTP is no longer required to login with the associated users. They should be informed because they will need to visit their security settings and configure TOTP again. No other action is required.<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7251
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>

models/forgejo_migrations/v25.go

@@ -7,9 +7,11 @@ import (
 	"context"
 	"crypto/md5"
 	"encoding/base64"
+	"fmt"
 
 	"code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/secret"
 	"code.gitea.io/gitea/modules/setting"
 
@@ -57,19 +59,38 @@ func MigrateTwoFactorToKeying(x *xorm.Engine) error {
 
 	oldEncryptionKey := md5.Sum([]byte(setting.SecretKey))
 
-	return db.Iterate(context.Background(), nil, func(ctx context.Context, bean *auth.TwoFactor) error {
+	messages := make([]string, 0, 100)
+	ids := make([]int64, 0, 100)
+
+	err = db.Iterate(context.Background(), nil, func(ctx context.Context, bean *auth.TwoFactor) error {
 		decodedStoredSecret, err := base64.StdEncoding.DecodeString(string(bean.Secret))
 		if err != nil {
-			return err
+			messages = append(messages, fmt.Sprintf("two_factor.id=%d, two_factor.uid=%d: base64.StdEncoding.DecodeString: %v", bean.ID, bean.UID, err))
+			ids = append(ids, bean.ID)
+			return nil
 		}
 
 		secretBytes, err := secret.AesDecrypt(oldEncryptionKey[:], decodedStoredSecret)
 		if err != nil {
-			return err
+			messages = append(messages, fmt.Sprintf("two_factor.id=%d, two_factor.uid=%d: secret.AesDecrypt: %v", bean.ID, bean.UID, err))
+			ids = append(ids, bean.ID)
+			return nil
 		}
 
 		bean.SetSecret(string(secretBytes))
 		_, err = db.GetEngine(ctx).Cols("secret").ID(bean.ID).Update(bean)
 		return err
 	})
+	if err == nil {
+		if len(ids) > 0 {
+			log.Error("Forgejo migration[25]: The following TOTP secrets were found to be corrupted and removed from the database. TOTP is no longer required to login with the associated users. They should be informed because they will need to visit their security settings and configure TOTP again. No other action is required. See https://codeberg.org/forgejo/forgejo/issues/6637 for more context on the various causes for such a corruption.")
+			for _, message := range messages {
+				log.Error("Forgejo migration[25]: %s", message)
+			}
+
+			_, err = db.GetEngine(context.Background()).In("id", ids).NoAutoCondition().NoAutoTime().Delete(&auth.TwoFactor{})
+		}
+	}
+
+	return err
 }

models/forgejo_migrations/v25_test.go

@@ -36,10 +36,14 @@ func Test_MigrateTwoFactorToKeying(t *testing.T) {
 
 	cnt, err := x.Table("two_factor").Count()
 	require.NoError(t, err)
-	assert.EqualValues(t, 1, cnt)
+	assert.EqualValues(t, 2, cnt)
 
 	require.NoError(t, MigrateTwoFactorToKeying(x))
 
+	cnt, err = x.Table("two_factor").Count()
+	require.NoError(t, err)
+	assert.EqualValues(t, 1, cnt)
+
 	var twofactor auth.TwoFactor
 	_, err = x.Table("two_factor").ID(1).Get(&twofactor)
 	require.NoError(t, err)

models/migrations/fixtures/Test_MigrateTwoFactorToKeying/two_factor.yml

@@ -7,3 +7,12 @@
   last_used_passcode:
   created_unix: 1564253724
   updated_unix: 1564253724
+-
+  id: 2
+  uid: 23
+  secret: badbad
+  scratch_salt: badbad
+  scratch_hash: badbad
+  last_used_passcode:
+  created_unix: 1564253724
+  updated_unix: 1564253724

release-notes/7251.md

@@ -0,0 +1 @@
+When migrating from a Forgejo version lower than v10, the TOTP secrets found to be corrupted are now transparently removed from the database instead of failing the migration. TOTP is no longer required to login with the associated users. They should be informed because they will need to visit their security settings and configure TOTP again. No other action is required.