From 6af8f3a3f2af058a0974b55371533728771bb691 Mon Sep 17 00:00:00 2001
From: Gusted <postmaster@gusted.xyz>
Date: Sat, 6 Apr 2024 00:52:39 +0200
Subject: [PATCH] [BUG] Don't remove builtin OAuth2 applications

- When the database consistency is being run it would check for any
OAuth2 applications that don't have an existing user. However there are
few special OAuth2 applications that don't have an user set, because
they are global applications.
- This was not taken into account by the database consistency checker
and were removed if the database consistency check was being run with
autofix enabled.
- Take into account to ignore these global OAuth2 applications when
running the database consistency check.
- Add unit tests.
- Ref: https://codeberg.org/Codeberg/Community/issues/1530
---
 .../oauth2_application.yaml                   | 25 +++++++++++++++
 models/auth/oauth2.go                         | 31 +++++++++++++++++++
 models/auth/oauth2_test.go                    | 30 ++++++++++++++++++
 services/doctor/dbconsistency.go              | 10 ++++--
 4 files changed, 93 insertions(+), 3 deletions(-)
 create mode 100644 models/auth/TestOrphanedOAuth2Applications/oauth2_application.yaml

diff --git a/models/auth/TestOrphanedOAuth2Applications/oauth2_application.yaml b/models/auth/TestOrphanedOAuth2Applications/oauth2_application.yaml
new file mode 100644
index 0000000000..b188770a30
--- /dev/null
+++ b/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
diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go
index 9d53fffc78..83d60e3abe 100644
--- a/models/auth/oauth2.go
+++ b/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
+}
diff --git a/models/auth/oauth2_test.go b/models/auth/oauth2_test.go
index 122d43098c..9a818a0bb3 100644
--- a/models/auth/oauth2_test.go
+++ b/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})
+}
diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go
index e2dcb63f33..0903ecc2a6 100644
--- a/services/doctor/dbconsistency.go
+++ b/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"),