From 074a3e05f665ad8c635a314f49080f8846e6d315 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 8 Apr 2024 12:13:34 +0800 Subject: [PATCH] Fix oauth2 builtin application logic (#30304) Fix #29074 (allow to disable all builtin apps) and don't make the doctor command remove the builtin apps. By the way, rename refobject and joincond to camel case. --- models/db/consistency.go | 12 +++---- modules/setting/oauth2.go | 4 +++ modules/setting/oauth2_test.go | 18 ++++++++++ services/doctor/dbconsistency.go | 25 +++++++------ services/doctor/dbconsistency_test.go | 51 +++++++++++++++++++++++++++ services/doctor/main_test.go | 14 ++++++++ 6 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 services/doctor/dbconsistency_test.go create mode 100644 services/doctor/main_test.go diff --git a/models/db/consistency.go b/models/db/consistency.go index d19732cf80dce..d0b0ab8315f12 100644 --- a/models/db/consistency.go +++ b/models/db/consistency.go @@ -10,21 +10,21 @@ import ( ) // CountOrphanedObjects count subjects with have no existing refobject anymore -func CountOrphanedObjects(ctx context.Context, subject, refobject, joinCond string) (int64, error) { +func CountOrphanedObjects(ctx context.Context, subject, refObject, joinCond string) (int64, error) { return GetEngine(ctx). Table("`"+subject+"`"). - Join("LEFT", "`"+refobject+"`", joinCond). - Where(builder.IsNull{"`" + refobject + "`.id"}). + Join("LEFT", "`"+refObject+"`", joinCond). + Where(builder.IsNull{"`" + refObject + "`.id"}). Select("COUNT(`" + subject + "`.`id`)"). Count() } // DeleteOrphanedObjects delete subjects with have no existing refobject anymore -func DeleteOrphanedObjects(ctx context.Context, subject, refobject, joinCond string) error { +func DeleteOrphanedObjects(ctx context.Context, subject, refObject, joinCond string) error { subQuery := builder.Select("`"+subject+"`.id"). From("`"+subject+"`"). - Join("LEFT", "`"+refobject+"`", joinCond). - Where(builder.IsNull{"`" + refobject + "`.id"}) + Join("LEFT", "`"+refObject+"`", joinCond). + Where(builder.IsNull{"`" + refObject + "`.id"}) b := builder.Delete(builder.In("id", subQuery)).From("`" + subject + "`") _, err := GetEngine(ctx).Exec(b) return err diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index 1429a7585c9e5..830472db32bf5 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -118,6 +118,10 @@ func loadOAuth2From(rootCfg ConfigProvider) { return } + if sec.HasKey("DEFAULT_APPLICATIONS") && sec.Key("DEFAULT_APPLICATIONS").String() == "" { + OAuth2.DefaultApplications = nil + } + // Handle the rename of ENABLE to ENABLED deprecatedSetting(rootCfg, "oauth2", "ENABLE", "oauth2", "ENABLED", "v1.23.0") if sec.HasKey("ENABLE") && !sec.HasKey("ENABLED") { diff --git a/modules/setting/oauth2_test.go b/modules/setting/oauth2_test.go index d822198619db3..4403f358926f5 100644 --- a/modules/setting/oauth2_test.go +++ b/modules/setting/oauth2_test.go @@ -32,3 +32,21 @@ JWT_SECRET = BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB assert.Len(t, actual, 32) assert.EqualValues(t, expected, actual) } + +func TestOauth2DefaultApplications(t *testing.T) { + cfg, _ := NewConfigProviderFromData(``) + loadOAuth2From(cfg) + assert.Equal(t, []string{"git-credential-oauth", "git-credential-manager", "tea"}, OAuth2.DefaultApplications) + + cfg, _ = NewConfigProviderFromData(`[oauth2] +DEFAULT_APPLICATIONS = tea +`) + loadOAuth2From(cfg) + assert.Equal(t, []string{"tea"}, OAuth2.DefaultApplications) + + cfg, _ = NewConfigProviderFromData(`[oauth2] +DEFAULT_APPLICATIONS = +`) + loadOAuth2From(cfg) + assert.Nil(t, nil, OAuth2.DefaultApplications) +} diff --git a/services/doctor/dbconsistency.go b/services/doctor/dbconsistency.go index e2dcb63f33a7c..dfdf7b547ac0e 100644 --- a/services/doctor/dbconsistency.go +++ b/services/doctor/dbconsistency.go @@ -61,26 +61,20 @@ func asFixer(fn func(ctx context.Context) error) func(ctx context.Context) (int6 } } -func genericOrphanCheck(name, subject, refobject, joincond string) consistencyCheck { +func genericOrphanCheck(name, subject, refObject, joinCond string) consistencyCheck { return consistencyCheck{ Name: name, Counter: func(ctx context.Context) (int64, error) { - return db.CountOrphanedObjects(ctx, subject, refobject, joincond) + return db.CountOrphanedObjects(ctx, subject, refObject, joinCond) }, Fixer: func(ctx context.Context) (int64, error) { - err := db.DeleteOrphanedObjects(ctx, subject, refobject, joincond) + err := db.DeleteOrphanedObjects(ctx, subject, refObject, joinCond) return -1, err }, } } -func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) error { - // make sure DB version is uptodate - if err := db.InitEngineWithMigration(ctx, migrations.EnsureUpToDate); err != nil { - logger.Critical("Model version on the database does not match the current Gitea version. Model consistency will not be checked until the database is upgraded") - return err - } - +func prepareDBConsistencyChecks() []consistencyCheck { consistencyChecks := []consistencyCheck{ { // find labels without existing repo or org @@ -210,7 +204,7 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er "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"), + "oauth2_application", "user", "oauth2_application.uid=0 OR 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"), @@ -224,7 +218,16 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er genericOrphanCheck("Orphaned Redirects without existing redirect user", "user_redirect", "user", "user_redirect.redirect_user_id=`user`.id"), ) + return consistencyChecks +} +func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) error { + // make sure DB version is uptodate + if err := db.InitEngineWithMigration(ctx, migrations.EnsureUpToDate); err != nil { + logger.Critical("Model version on the database does not match the current Gitea version. Model consistency will not be checked until the database is upgraded") + return err + } + consistencyChecks := prepareDBConsistencyChecks() for _, c := range consistencyChecks { if err := c.Run(ctx, logger, autofix); err != nil { return err diff --git a/services/doctor/dbconsistency_test.go b/services/doctor/dbconsistency_test.go new file mode 100644 index 0000000000000..4e4ac535b7b45 --- /dev/null +++ b/services/doctor/dbconsistency_test.go @@ -0,0 +1,51 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package doctor + +import ( + "slices" + "testing" + + "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" + + "github.com/stretchr/testify/assert" +) + +func TestConsistencyCheck(t *testing.T) { + checks := prepareDBConsistencyChecks() + idx := slices.IndexFunc(checks, func(check consistencyCheck) bool { + return check.Name == "Orphaned OAuth2Application without existing User" + }) + if !assert.NotEqual(t, -1, idx) { + return + } + + _ = db.TruncateBeans(db.DefaultContext, &auth.OAuth2Application{}, &user.User{}) + _ = db.TruncateBeans(db.DefaultContext, &auth.OAuth2Application{}, &auth.OAuth2Application{}) + + err := db.Insert(db.DefaultContext, &user.User{ID: 1}) + assert.NoError(t, err) + err = db.Insert(db.DefaultContext, &auth.OAuth2Application{Name: "test-oauth2-app-1", ClientID: "client-id-1"}) + assert.NoError(t, err) + err = db.Insert(db.DefaultContext, &auth.OAuth2Application{Name: "test-oauth2-app-2", ClientID: "client-id-2", UID: 1}) + assert.NoError(t, err) + err = db.Insert(db.DefaultContext, &auth.OAuth2Application{Name: "test-oauth2-app-3", ClientID: "client-id-3", UID: 99999999}) + assert.NoError(t, err) + + unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ClientID: "client-id-1"}) + unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ClientID: "client-id-2"}) + unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ClientID: "client-id-3"}) + + oauth2AppCheck := checks[idx] + err = oauth2AppCheck.Run(db.DefaultContext, log.GetManager().GetLogger(log.DEFAULT), true) + assert.NoError(t, err) + + unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ClientID: "client-id-1"}) + unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ClientID: "client-id-2"}) + unittest.AssertNotExistsBean(t, &auth.OAuth2Application{ClientID: "client-id-3"}) +} diff --git a/services/doctor/main_test.go b/services/doctor/main_test.go new file mode 100644 index 0000000000000..0f365e21d0822 --- /dev/null +++ b/services/doctor/main_test.go @@ -0,0 +1,14 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package doctor + +import ( + "testing" + + "code.gitea.io/gitea/models/unittest" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +}