Merge pull request '[BUG] Don't remove builtin OAuth2 applications' (#3067) from gusted/forgejo-db-consistency-oauth2 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3067
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/auth/TestOrphanedOAuth2Applications/oauth2_application.yaml

@@ -0,0 +1,25 @@
+-
+  id: 1000
+  uid: 0
+  name: "Git Credential Manager"
+  client_id: "e90ee53c-94e2-48ac-9358-a874fb9e0662"
+  redirect_uris: '["http://127.0.0.1", "https://127.0.0.1"]'
+  created_unix: 1712358091
+  updated_unix: 1712358091
+-
+  id: 1001
+  uid: 0
+  name: "git-credential-oauth"
+  client_id: "a4792ccc-144e-407e-86c9-5e7d8d9c3269"
+  redirect_uris: '["http://127.0.0.1", "https://127.0.0.1"]'
+  created_unix: 1712358091
+  updated_unix: 1712358091
+
+-
+  id: 1002
+  uid: 1234567890
+  name: "Should be removed"
+  client_id: "deadc0de-badd-dd11-fee1-deaddecafbad"
+  redirect_uris: '["http://127.0.0.1", "https://127.0.0.1"]'
+  created_unix: 1712358091
+  updated_unix: 1712358091

models/auth/oauth2.go

@@ -74,6 +74,13 @@ func BuiltinApplications() map[string]*BuiltinOAuth2Application {
 	return m
 }
 
+func BuiltinApplicationsClientIDs() (clientIDs []string) {
+	for clientID := range BuiltinApplications() {
+		clientIDs = append(clientIDs, clientID)
+	}
+	return clientIDs
+}
+
 func Init(ctx context.Context) error {
 	builtinApps := BuiltinApplications()
 	var builtinAllClientIDs []string
@@ -637,3 +644,27 @@ func DeleteOAuth2RelictsByUserID(ctx context.Context, userID int64) error {
 
 	return nil
 }
+
+// CountOrphanedOAuth2Applications returns the amount of orphaned OAuth2 applications.
+func CountOrphanedOAuth2Applications(ctx context.Context) (int64, error) {
+	return db.GetEngine(ctx).
+		Table("`oauth2_application`").
+		Join("LEFT", "`user`", "`oauth2_application`.`uid` = `user`.`id`").
+		Where(builder.IsNull{"`user`.id"}).
+		Where(builder.NotIn("`oauth2_application`.`client_id`", BuiltinApplicationsClientIDs())).
+		Select("COUNT(`oauth2_application`.`id`)").
+		Count()
+}
+
+// DeleteOrphanedOAuth2Applications deletes orphaned OAuth2 applications.
+func DeleteOrphanedOAuth2Applications(ctx context.Context) (int64, error) {
+	subQuery := builder.Select("`oauth2_application`.id").
+		From("`oauth2_application`").
+		Join("LEFT", "`user`", "`oauth2_application`.`uid` = `user`.`id`").
+		Where(builder.IsNull{"`user`.id"}).
+		Where(builder.NotIn("`oauth2_application`.`client_id`", BuiltinApplicationsClientIDs()))
+
+	b := builder.Delete(builder.In("id", subQuery)).From("`oauth2_application`")
+	_, err := db.GetEngine(ctx).Exec(b)
+	return -1, err
+}

models/auth/oauth2_test.go

@@ -4,11 +4,13 @@
 package auth_test
 
 import (
+	"path/filepath"
 	"testing"
 
 	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -265,3 +267,31 @@ func TestOAuth2AuthorizationCode_Invalidate(t *testing.T) {
 func TestOAuth2AuthorizationCode_TableName(t *testing.T) {
 	assert.Equal(t, "oauth2_authorization_code", new(auth_model.OAuth2AuthorizationCode).TableName())
 }
+
+func TestBuiltinApplicationsClientIDs(t *testing.T) {
+	assert.EqualValues(t, []string{"a4792ccc-144e-407e-86c9-5e7d8d9c3269", "e90ee53c-94e2-48ac-9358-a874fb9e0662", "d57cb8c4-630c-4168-8324-ec79935e18d4"}, auth_model.BuiltinApplicationsClientIDs())
+}
+
+func TestOrphanedOAuth2Applications(t *testing.T) {
+	defer unittest.OverrideFixtures(
+		unittest.FixturesOptions{
+			Dir:  filepath.Join(setting.AppWorkPath, "models/fixtures/"),
+			Base: setting.AppWorkPath,
+			Dirs: []string{"models/auth/TestOrphanedOAuth2Applications/"},
+		},
+	)()
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	count, err := auth_model.CountOrphanedOAuth2Applications(db.DefaultContext)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 1, count)
+	unittest.AssertExistsIf(t, true, &auth_model.OAuth2Application{ID: 1002})
+
+	_, err = auth_model.DeleteOrphanedOAuth2Applications(db.DefaultContext)
+	assert.NoError(t, err)
+
+	count, err = auth_model.CountOrphanedOAuth2Applications(db.DefaultContext)
+	assert.NoError(t, err)
+	assert.EqualValues(t, 0, count)
+	unittest.AssertExistsIf(t, false, &auth_model.OAuth2Application{ID: 1002})
+}

services/doctor/dbconsistency.go

@@ -8,6 +8,7 @@ import (
 
 	actions_model "code.gitea.io/gitea/models/actions"
 	activities_model "code.gitea.io/gitea/models/activities"
+	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/migrations"
@@ -164,6 +165,12 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er
 			Fixer:        repo_model.DeleteOrphanedTopics,
 			FixedMessage: "Removed",
 		},
+		{
+			Name:         "Orphaned OAuth2Application without existing User",
+			Counter:      auth_model.CountOrphanedOAuth2Applications,
+			Fixer:        auth_model.DeleteOrphanedOAuth2Applications,
+			FixedMessage: "Removed",
+		},
 	}
 
 	// TODO: function to recalc all counters
@@ -208,9 +215,6 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er
 		// find OAuth2Grant without existing user
 		genericOrphanCheck("Orphaned OAuth2Grant without existing User",
 			"oauth2_grant", "user", "oauth2_grant.user_id=`user`.id"),
-		// find OAuth2Application without existing user
-		genericOrphanCheck("Orphaned OAuth2Application without existing User",
-			"oauth2_application", "user", "oauth2_application.uid=`user`.id"),
 		// find OAuth2AuthorizationCode without existing OAuth2Grant
 		genericOrphanCheck("Orphaned OAuth2AuthorizationCode without existing OAuth2Grant",
 			"oauth2_authorization_code", "oauth2_grant", "oauth2_authorization_code.grant_id=oauth2_grant.id"),