From 377a0a20f01a62f15a1504a3bba6cf6cc0c98bea Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 4 May 2023 11:55:35 +0800 Subject: [PATCH 01/13] Merge setting.InitXXX into one function with options (#24389) This PR will merge 3 Init functions on setting packages as 1 and introduce an options struct. --- cmd/actions.go | 3 +- cmd/cmd.go | 3 +- cmd/doctor.go | 3 +- cmd/dump.go | 3 +- cmd/embedded.go | 5 ++- cmd/mailer.go | 3 +- cmd/main_test.go | 6 --- cmd/restore_repo.go | 3 +- cmd/serv.go | 3 +- cmd/web.go | 3 +- models/asymkey/main_test.go | 6 --- models/dbfs/main_test.go | 6 --- models/issues/main_test.go | 6 --- models/main_test.go | 6 --- models/migrations/base/tests.go | 2 +- models/unittest/testdb.go | 22 +++++++++ modules/doctor/doctor.go | 3 +- modules/doctor/paths.go | 3 +- modules/markup/html_test.go | 5 ++- modules/markup/markdown/markdown_test.go | 5 ++- modules/setting/config_provider.go | 45 ++++++++++--------- modules/setting/setting.go | 41 +++-------------- routers/api/v1/repo/main_test.go | 7 +-- routers/install/setting.go | 8 ++-- routers/web/user/setting/account_test.go | 27 ++++++----- services/webhook/main_test.go | 8 ++-- .../migration-test/migration_test.go | 2 +- tests/test_utils.go | 2 +- 28 files changed, 103 insertions(+), 136 deletions(-) diff --git a/cmd/actions.go b/cmd/actions.go index 66ad336da508..346de5b21a6f 100644 --- a/cmd/actions.go +++ b/cmd/actions.go @@ -42,8 +42,7 @@ func runGenerateActionsRunnerToken(c *cli.Context) error { ctx, cancel := installSignals() defer cancel() - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) scope := c.String("scope") diff --git a/cmd/cmd.go b/cmd/cmd.go index 18d5db3987bd..cf2d9ef89e83 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -57,8 +57,7 @@ func confirm() (bool, error) { } func initDB(ctx context.Context) error { - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) setting.LoadDBSetting() setting.InitSQLLog(false) diff --git a/cmd/doctor.go b/cmd/doctor.go index e7baad60c1f9..65c028c5ed19 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -87,8 +87,7 @@ func runRecreateTable(ctx *cli.Context) error { golog.SetPrefix("") golog.SetOutput(log.NewLoggerAsWriter("INFO", log.GetLogger(log.DEFAULT))) - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) setting.LoadDBSetting() setting.Log.EnableXORMLog = ctx.Bool("debug") diff --git a/cmd/dump.go b/cmd/dump.go index 309bd01f6645..32ccc5566c8a 100644 --- a/cmd/dump.go +++ b/cmd/dump.go @@ -185,8 +185,7 @@ func runDump(ctx *cli.Context) error { } fileName += "." + outType } - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) // make sure we are logging to the console no matter what the configuration tells us do to // FIXME: don't use CfgProvider directly diff --git a/cmd/embedded.go b/cmd/embedded.go index cee8928ce08d..3f849bea0a26 100644 --- a/cmd/embedded.go +++ b/cmd/embedded.go @@ -106,8 +106,9 @@ func initEmbeddedExtractor(c *cli.Context) error { log.DelNamedLogger(log.DEFAULT) // Read configuration file - setting.InitProviderAllowEmpty() - setting.LoadCommonSettings() + setting.Init(&setting.Options{ + AllowEmpty: true, + }) patterns, err := compileCollectPatterns(c.Args()) if err != nil { diff --git a/cmd/mailer.go b/cmd/mailer.go index 50ba4b474110..74bae1ab68c7 100644 --- a/cmd/mailer.go +++ b/cmd/mailer.go @@ -16,8 +16,7 @@ func runSendMail(c *cli.Context) error { ctx, cancel := installSignals() defer cancel() - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) if err := argsSet(c, "title"); err != nil { return err diff --git a/cmd/main_test.go b/cmd/main_test.go index ba323af47217..6e20be69451c 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -7,14 +7,8 @@ import ( "testing" "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/setting" ) -func init() { - setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() -} - func TestMain(m *testing.M) { unittest.MainTest(m, &unittest.TestOptions{ GiteaRootPath: "..", diff --git a/cmd/restore_repo.go b/cmd/restore_repo.go index 887b59bba9e1..5a7ede493975 100644 --- a/cmd/restore_repo.go +++ b/cmd/restore_repo.go @@ -51,8 +51,7 @@ func runRestoreRepository(c *cli.Context) error { ctx, cancel := installSignals() defer cancel() - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) var units []string if s := c.String("units"); s != "" { units = strings.Split(s, ",") diff --git a/cmd/serv.go b/cmd/serv.go index 72eb6370711e..a79f314d00b6 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -62,8 +62,7 @@ func setup(ctx context.Context, debug bool) { } else { _ = log.NewLogger(1000, "console", "console", `{"level":"fatal","stacktracelevel":"NONE","stderr":true}`) } - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) if debug { setting.RunMode = "dev" } diff --git a/cmd/web.go b/cmd/web.go index e451cf7dfa4f..3a01d07b05f5 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -177,8 +177,7 @@ func runWeb(ctx *cli.Context) error { log.Info("Global init") // Perform global initialization - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) routers.GlobalInitInstalled(graceful.GetManager().HammerContext()) // We check that AppDataPath exists here (it should have been created during installation) diff --git a/models/asymkey/main_test.go b/models/asymkey/main_test.go index 7f8657189fd1..701722be12ec 100644 --- a/models/asymkey/main_test.go +++ b/models/asymkey/main_test.go @@ -8,14 +8,8 @@ import ( "testing" "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/setting" ) -func init() { - setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() -} - func TestMain(m *testing.M) { unittest.MainTest(m, &unittest.TestOptions{ GiteaRootPath: filepath.Join("..", ".."), diff --git a/models/dbfs/main_test.go b/models/dbfs/main_test.go index 9dce663eeab9..62db3592bed2 100644 --- a/models/dbfs/main_test.go +++ b/models/dbfs/main_test.go @@ -8,14 +8,8 @@ import ( "testing" "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/setting" ) -func init() { - setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() -} - func TestMain(m *testing.M) { unittest.MainTest(m, &unittest.TestOptions{ GiteaRootPath: filepath.Join("..", ".."), diff --git a/models/issues/main_test.go b/models/issues/main_test.go index de84da30ecc0..9fbe294f7067 100644 --- a/models/issues/main_test.go +++ b/models/issues/main_test.go @@ -9,7 +9,6 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/setting" _ "code.gitea.io/gitea/models" _ "code.gitea.io/gitea/models/repo" @@ -18,11 +17,6 @@ import ( "github.com/stretchr/testify/assert" ) -func init() { - setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() -} - func TestFixturesAreConsistent(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) unittest.CheckConsistencyFor(t, diff --git a/models/main_test.go b/models/main_test.go index b5919bb28615..d490507649a3 100644 --- a/models/main_test.go +++ b/models/main_test.go @@ -11,18 +11,12 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/setting" _ "code.gitea.io/gitea/models/system" "github.com/stretchr/testify/assert" ) -func init() { - setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() -} - // TestFixturesAreConsistent assert that test fixtures are consistent func TestFixturesAreConsistent(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/models/migrations/base/tests.go b/models/migrations/base/tests.go index 14f374f1a7c3..124111f51fb2 100644 --- a/models/migrations/base/tests.go +++ b/models/migrations/base/tests.go @@ -150,7 +150,7 @@ func MainTest(m *testing.M) { setting.AppDataPath = tmpDataPath setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() + unittest.InitSettings() if err = git.InitFull(context.Background()); err != nil { fmt.Printf("Unable to InitFull: %v\n", err) os.Exit(1) diff --git a/models/unittest/testdb.go b/models/unittest/testdb.go index cff1489a7c7b..a5b126350d8e 100644 --- a/models/unittest/testdb.go +++ b/models/unittest/testdb.go @@ -6,12 +6,15 @@ package unittest import ( "context" "fmt" + "log" "os" "path/filepath" + "strings" "testing" "code.gitea.io/gitea/models/db" system_model "code.gitea.io/gitea/models/system" + "code.gitea.io/gitea/modules/auth/password/hash" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" @@ -39,6 +42,22 @@ func fatalTestError(fmtStr string, args ...interface{}) { os.Exit(1) } +// InitSettings initializes config provider and load common setttings for tests +func InitSettings(extraConfigs ...string) { + setting.Init(&setting.Options{ + AllowEmpty: true, + ExtraConfig: strings.Join(extraConfigs, "\n"), + }) + + if err := setting.PrepareAppDataPath(); err != nil { + log.Fatalf("Can not prepare APP_DATA_PATH: %v", err) + } + // register the dummy hash algorithm function used in the test fixtures + _ = hash.Register("dummy", hash.NewDummyHasher) + + setting.PasswordHashAlgo, _ = hash.SetDefaultPasswordHashAlgorithm("dummy") +} + // TestOptions represents test options type TestOptions struct { GiteaRootPath string @@ -50,6 +69,9 @@ type TestOptions struct { // MainTest a reusable TestMain(..) function for unit tests that need to use a // test database. Creates the test database, and sets necessary settings. func MainTest(m *testing.M, testOpts *TestOptions) { + setting.SetCustomPathAndConf("", "", "") + InitSettings() + var err error giteaRoot = testOpts.GiteaRootPath diff --git a/modules/doctor/doctor.go b/modules/doctor/doctor.go index b23805bc4c96..32eb5938c307 100644 --- a/modules/doctor/doctor.go +++ b/modules/doctor/doctor.go @@ -44,8 +44,7 @@ func (w *wrappedLevelLogger) Log(skip int, level log.Level, format string, v ... } func initDBDisableConsole(ctx context.Context, disableConsole bool) error { - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) setting.LoadDBSetting() setting.InitSQLLog(disableConsole) if err := db.InitEngine(ctx); err != nil { diff --git a/modules/doctor/paths.go b/modules/doctor/paths.go index 1558efc25b90..957152349c25 100644 --- a/modules/doctor/paths.go +++ b/modules/doctor/paths.go @@ -66,8 +66,7 @@ func checkConfigurationFiles(ctx context.Context, logger log.Logger, autofix boo return err } - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) configurationFiles := []configurationFile{ {"Configuration File Path", setting.CustomConf, false, true, false}, diff --git a/modules/markup/html_test.go b/modules/markup/html_test.go index cb1216ec946e..5e5e4fecbbb7 100644 --- a/modules/markup/html_test.go +++ b/modules/markup/html_test.go @@ -28,8 +28,9 @@ var localMetas = map[string]string{ } func TestMain(m *testing.M) { - setting.InitProviderAllowEmpty() - setting.LoadCommonSettings() + setting.Init(&setting.Options{ + AllowEmpty: true, + }) if err := git.InitSimple(context.Background()); err != nil { log.Fatal("git init failed, err: %v", err) } diff --git a/modules/markup/markdown/markdown_test.go b/modules/markup/markdown/markdown_test.go index 0c7650a5ffab..e81869d7a443 100644 --- a/modules/markup/markdown/markdown_test.go +++ b/modules/markup/markdown/markdown_test.go @@ -33,8 +33,9 @@ var localMetas = map[string]string{ } func TestMain(m *testing.M) { - setting.InitProviderAllowEmpty() - setting.LoadCommonSettings() + setting.Init(&setting.Options{ + AllowEmpty: true, + }) if err := git.InitSimple(context.Background()); err != nil { log.Fatal("git init failed, err: %v", err) } diff --git a/modules/setting/config_provider.go b/modules/setting/config_provider.go index 92c8c97fe952..168595829863 100644 --- a/modules/setting/config_provider.go +++ b/modules/setting/config_provider.go @@ -35,10 +35,9 @@ type ConfigProvider interface { } type iniFileConfigProvider struct { + opts *Options *ini.File - filepath string // the ini file path - newFile bool // whether the file has not existed previously - allowEmpty bool // whether not finding configuration files is allowed (only true for the tests) + newFile bool // whether the file has not existed previously } // NewEmptyConfigProvider create a new empty config provider @@ -66,41 +65,47 @@ func newConfigProviderFromData(configContent string) (ConfigProvider, error) { }, nil } +type Options struct { + CustomConf string // the ini file path + AllowEmpty bool // whether not finding configuration files is allowed (only true for the tests) + ExtraConfig string + DisableLoadCommonSettings bool +} + // newConfigProviderFromFile load configuration from file. // NOTE: do not print any log except error. -func newConfigProviderFromFile(customConf string, allowEmpty bool, extraConfig string) (*iniFileConfigProvider, error) { +func newConfigProviderFromFile(opts *Options) (*iniFileConfigProvider, error) { cfg := ini.Empty() newFile := true - if customConf != "" { - isFile, err := util.IsFile(customConf) + if opts.CustomConf != "" { + isFile, err := util.IsFile(opts.CustomConf) if err != nil { - return nil, fmt.Errorf("unable to check if %s is a file. Error: %v", customConf, err) + return nil, fmt.Errorf("unable to check if %s is a file. Error: %v", opts.CustomConf, err) } if isFile { - if err := cfg.Append(customConf); err != nil { - return nil, fmt.Errorf("failed to load custom conf '%s': %v", customConf, err) + if err := cfg.Append(opts.CustomConf); err != nil { + return nil, fmt.Errorf("failed to load custom conf '%s': %v", opts.CustomConf, err) } newFile = false } } - if newFile && !allowEmpty { + if newFile && !opts.AllowEmpty { return nil, fmt.Errorf("unable to find configuration file: %q, please ensure you are running in the correct environment or set the correct configuration file with -c", CustomConf) } - if extraConfig != "" { - if err := cfg.Append([]byte(extraConfig)); err != nil { + if opts.ExtraConfig != "" { + if err := cfg.Append([]byte(opts.ExtraConfig)); err != nil { return nil, fmt.Errorf("unable to append more config: %v", err) } } cfg.NameMapper = ini.SnackCase return &iniFileConfigProvider{ - File: cfg, - filepath: customConf, - newFile: newFile, - allowEmpty: allowEmpty, + opts: opts, + File: cfg, + newFile: newFile, }, nil } @@ -123,8 +128,8 @@ func (p *iniFileConfigProvider) DeleteSection(name string) error { // Save save the content into file func (p *iniFileConfigProvider) Save() error { - if p.filepath == "" { - if !p.allowEmpty { + if p.opts.CustomConf == "" { + if !p.opts.AllowEmpty { return fmt.Errorf("custom config path must not be empty") } return nil @@ -135,8 +140,8 @@ func (p *iniFileConfigProvider) Save() error { return fmt.Errorf("failed to create '%s': %v", CustomConf, err) } } - if err := p.SaveTo(p.filepath); err != nil { - return fmt.Errorf("failed to save '%s': %v", p.filepath, err) + if err := p.SaveTo(p.opts.CustomConf); err != nil { + return fmt.Errorf("failed to save '%s': %v", p.opts.CustomConf, err) } // Change permissions to be more restrictive diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 9ab55e91c531..b085a7b32149 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -15,7 +15,6 @@ import ( "strings" "time" - "code.gitea.io/gitea/modules/auth/password/hash" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/user" ) @@ -203,44 +202,18 @@ func PrepareAppDataPath() error { return nil } -// InitProviderFromExistingFile initializes config provider from an existing config file (app.ini) -func InitProviderFromExistingFile() { - var err error - CfgProvider, err = newConfigProviderFromFile(CustomConf, false, "") - if err != nil { - log.Fatal("InitProviderFromExistingFile: %v", err) - } -} - -// InitProviderAllowEmpty initializes config provider from file, it's also fine that if the config file (app.ini) doesn't exist -func InitProviderAllowEmpty() { - var err error - CfgProvider, err = newConfigProviderFromFile(CustomConf, true, "") - if err != nil { - log.Fatal("InitProviderAllowEmpty: %v", err) +func Init(opts *Options) { + if opts.CustomConf == "" { + opts.CustomConf = CustomConf } -} - -// InitProviderAndLoadCommonSettingsForTest initializes config provider and load common setttings for tests -func InitProviderAndLoadCommonSettingsForTest(extraConfigs ...string) { var err error - CfgProvider, err = newConfigProviderFromFile(CustomConf, true, strings.Join(extraConfigs, "\n")) + CfgProvider, err = newConfigProviderFromFile(opts) if err != nil { - log.Fatal("InitProviderAndLoadCommonSettingsForTest: %v", err) + log.Fatal("Init[%v]: %v", opts, err) } - loadCommonSettingsFrom(CfgProvider) - if err := PrepareAppDataPath(); err != nil { - log.Fatal("Can not prepare APP_DATA_PATH: %v", err) + if !opts.DisableLoadCommonSettings { + loadCommonSettingsFrom(CfgProvider) } - // register the dummy hash algorithm function used in the test fixtures - _ = hash.Register("dummy", hash.NewDummyHasher) - - PasswordHashAlgo, _ = hash.SetDefaultPasswordHashAlgorithm("dummy") -} - -// LoadCommonSettings loads common configurations from a configuration provider. -func LoadCommonSettings() { - loadCommonSettingsFrom(CfgProvider) } // loadCommonSettingsFrom loads common configurations from a configuration provider. diff --git a/routers/api/v1/repo/main_test.go b/routers/api/v1/repo/main_test.go index c7466c493f01..bc048505f472 100644 --- a/routers/api/v1/repo/main_test.go +++ b/routers/api/v1/repo/main_test.go @@ -13,10 +13,11 @@ import ( ) func TestMain(m *testing.M) { - setting.InitProviderAndLoadCommonSettingsForTest() - setting.LoadQueueSettings() unittest.MainTest(m, &unittest.TestOptions{ GiteaRootPath: filepath.Join("..", "..", "..", ".."), - SetUp: webhook_service.Init, + SetUp: func() error { + setting.LoadQueueSettings() + return webhook_service.Init() + }, }) } diff --git a/routers/install/setting.go b/routers/install/setting.go index dadefa26a293..c14843d8ee4d 100644 --- a/routers/install/setting.go +++ b/routers/install/setting.go @@ -15,8 +15,9 @@ import ( // PreloadSettings preloads the configuration to check if we need to run install func PreloadSettings(ctx context.Context) bool { - setting.InitProviderAllowEmpty() - setting.LoadCommonSettings() + setting.Init(&setting.Options{ + AllowEmpty: true, + }) if !setting.InstallLock { log.Info("AppPath: %s", setting.AppPath) log.Info("AppWorkPath: %s", setting.AppWorkPath) @@ -38,8 +39,7 @@ func PreloadSettings(ctx context.Context) bool { // reloadSettings reloads the existing settings and starts up the database func reloadSettings(ctx context.Context) { - setting.InitProviderFromExistingFile() - setting.LoadCommonSettings() + setting.Init(&setting.Options{}) setting.LoadDBSetting() if setting.InstallLock { if err := common.InitDBEngine(ctx); err == nil { diff --git a/routers/web/user/setting/account_test.go b/routers/web/user/setting/account_test.go index 5fce41f065ba..569d597722ee 100644 --- a/routers/web/user/setting/account_test.go +++ b/routers/web/user/setting/account_test.go @@ -80,19 +80,22 @@ func TestChangePassword(t *testing.T) { PasswordComplexity: pcLU, }, } { - unittest.PrepareTestEnv(t) - ctx := test.MockContext(t, "user/settings/security") - test.LoadUser(t, ctx, 2) - test.LoadRepo(t, ctx, 1) + t.Run(req.OldPassword+"__"+req.NewPassword, func(t *testing.T) { + unittest.PrepareTestEnv(t) + setting.PasswordComplexity = req.PasswordComplexity + ctx := test.MockContext(t, "user/settings/security") + test.LoadUser(t, ctx, 2) + test.LoadRepo(t, ctx, 1) - web.SetForm(ctx, &forms.ChangePasswordForm{ - OldPassword: req.OldPassword, - Password: req.NewPassword, - Retype: req.Retype, - }) - AccountPost(ctx) + web.SetForm(ctx, &forms.ChangePasswordForm{ + OldPassword: req.OldPassword, + Password: req.NewPassword, + Retype: req.Retype, + }) + AccountPost(ctx) - assert.Contains(t, ctx.Flash.ErrorMsg, req.Message) - assert.EqualValues(t, http.StatusSeeOther, ctx.Resp.Status()) + assert.Contains(t, ctx.Flash.ErrorMsg, req.Message) + assert.EqualValues(t, http.StatusSeeOther, ctx.Resp.Status()) + }) } } diff --git a/services/webhook/main_test.go b/services/webhook/main_test.go index 210221b12041..0189e1784016 100644 --- a/services/webhook/main_test.go +++ b/services/webhook/main_test.go @@ -15,13 +15,13 @@ import ( ) func TestMain(m *testing.M) { - setting.InitProviderAndLoadCommonSettingsForTest() - setting.LoadQueueSettings() - // for tests, allow only loopback IPs setting.Webhook.AllowedHostList = hostmatcher.MatchBuiltinLoopback unittest.MainTest(m, &unittest.TestOptions{ GiteaRootPath: filepath.Join("..", ".."), - SetUp: Init, + SetUp: func() error { + setting.LoadQueueSettings() + return Init() + }, }) } diff --git a/tests/integration/migration-test/migration_test.go b/tests/integration/migration-test/migration_test.go index 4152379a9b80..a68d458a0b1e 100644 --- a/tests/integration/migration-test/migration_test.go +++ b/tests/integration/migration-test/migration_test.go @@ -57,7 +57,7 @@ func initMigrationTest(t *testing.T) func() { setting.CustomConf = giteaConf } - setting.InitProviderAndLoadCommonSettingsForTest() + unittest.InitSettings() assert.True(t, len(setting.RepoRootPath) != 0) assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) diff --git a/tests/test_utils.go b/tests/test_utils.go index 0f4aa26a6074..c22b2c356c65 100644 --- a/tests/test_utils.go +++ b/tests/test_utils.go @@ -74,7 +74,7 @@ func InitTest(requireGitea bool) { } setting.SetCustomPathAndConf("", "", "") - setting.InitProviderAndLoadCommonSettingsForTest() + unittest.InitSettings() setting.Repository.DefaultBranch = "master" // many test code still assume that default branch is called "master" _ = util.RemoveAll(repo_module.LocalCopyPath()) From 75ea0d5dba5dbf2f84cef2d12460fdd566d43e62 Mon Sep 17 00:00:00 2001 From: oliverpool <3864879+oliverpool@users.noreply.github.com> Date: Thu, 4 May 2023 07:08:41 +0200 Subject: [PATCH 02/13] Faster git.GetDivergingCommits (#24482) Using `git rev-list --left-right` is almost 2x faster than calling `git rev-list` twice. Co-authored-by: silverwind --- modules/git/repo.go | 37 +++++++++++++++---------------------- modules/git/repo_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/modules/git/repo.go b/modules/git/repo.go index 3637aa47c458..61930ab31db9 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -244,35 +244,28 @@ type DivergeObject struct { Behind int } -func checkDivergence(ctx context.Context, repoPath, baseBranch, targetBranch string) (int, error) { - branches := fmt.Sprintf("%s..%s", baseBranch, targetBranch) - cmd := NewCommand(ctx, "rev-list", "--count").AddDynamicArguments(branches) +// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch +func GetDivergingCommits(ctx context.Context, repoPath, baseBranch, targetBranch string) (do DivergeObject, err error) { + cmd := NewCommand(ctx, "rev-list", "--count", "--left-right"). + AddDynamicArguments(baseBranch + "..." + targetBranch) stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath}) if err != nil { - return -1, err + return do, err } - outInteger, errInteger := strconv.Atoi(strings.Trim(stdout, "\n")) - if errInteger != nil { - return -1, errInteger + left, right, found := strings.Cut(strings.Trim(stdout, "\n"), "\t") + if !found { + return do, fmt.Errorf("git rev-list output is missing a tab: %q", stdout) } - return outInteger, nil -} -// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch -func GetDivergingCommits(ctx context.Context, repoPath, baseBranch, targetBranch string) (DivergeObject, error) { - // $(git rev-list --count master..feature) commits ahead of master - ahead, errorAhead := checkDivergence(ctx, repoPath, baseBranch, targetBranch) - if errorAhead != nil { - return DivergeObject{}, errorAhead + do.Behind, err = strconv.Atoi(left) + if err != nil { + return do, err } - - // $(git rev-list --count feature..master) commits behind master - behind, errorBehind := checkDivergence(ctx, repoPath, targetBranch, baseBranch) - if errorBehind != nil { - return DivergeObject{}, errorBehind + do.Ahead, err = strconv.Atoi(right) + if err != nil { + return do, err } - - return DivergeObject{ahead, behind}, nil + return do, nil } // CreateBundle create bundle content to the target path diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index 044b9d406502..9db78153a102 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -4,6 +4,7 @@ package git import ( + "context" "path/filepath" "testing" @@ -29,3 +30,27 @@ func TestRepoIsEmpty(t *testing.T) { assert.NoError(t, err) assert.True(t, isEmpty) } + +func TestRepoGetDivergingCommits(t *testing.T) { + bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") + do, err := GetDivergingCommits(context.Background(), bareRepo1Path, "master", "branch2") + assert.NoError(t, err) + assert.Equal(t, DivergeObject{ + Ahead: 1, + Behind: 5, + }, do) + + do, err = GetDivergingCommits(context.Background(), bareRepo1Path, "master", "master") + assert.NoError(t, err) + assert.Equal(t, DivergeObject{ + Ahead: 0, + Behind: 0, + }, do) + + do, err = GetDivergingCommits(context.Background(), bareRepo1Path, "master", "test") + assert.NoError(t, err) + assert.Equal(t, DivergeObject{ + Ahead: 0, + Behind: 2, + }, do) +} From 5d77691d428d5302ee4df6c2a936b8e2ea9dca7e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 4 May 2023 14:36:34 +0800 Subject: [PATCH 03/13] Improve template system and panic recovery (#24461) Partially for #24457 Major changes: 1. The old `signedUserNameStringPointerKey` is quite hacky, use `ctx.Data[SignedUser]` instead 2. Move duplicate code from `Contexter` to `CommonTemplateContextData` 3. Remove incorrect copying&pasting code `ctx.Data["Err_Password"] = true` in API handlers 4. Use one unique `RenderPanicErrorPage` for panic error page rendering 5. Move `stripSlashesMiddleware` to be the first middleware 6. Install global panic recovery handler, it works for both `install` and `web` 7. Make `500.tmpl` only depend minimal template functions/variables, avoid triggering new panics Screenshot:
![image](https://user-images.githubusercontent.com/2114189/235444895-cecbabb8-e7dc-4360-a31c-b982d11946a7.png)
--- modules/context/access_log.go | 14 +++-- modules/context/api.go | 13 +---- modules/context/context.go | 73 +++++++++-------------- modules/context/package.go | 3 +- modules/context/private.go | 3 +- modules/templates/base.go | 31 ---------- modules/test/context_tests.go | 2 +- modules/web/middleware/data.go | 62 +++++++++++++++++++- modules/web/middleware/flash.go | 4 +- modules/web/middleware/request.go | 5 -- modules/web/route.go | 4 +- routers/api/v1/admin/user.go | 2 - routers/api/v1/misc/markup_test.go | 3 +- routers/common/errpage.go | 57 ++++++++++++++++++ routers/common/middleware.go | 54 +++++++---------- routers/install/install.go | 30 +++++----- routers/install/routes.go | 66 +-------------------- routers/web/base.go | 66 --------------------- routers/web/web.go | 30 +++++----- services/auth/interface.go | 2 +- services/auth/middleware.go | 4 +- templates/base/head_script.tmpl | 1 - templates/status/500.tmpl | 93 +++++++++++++++++++----------- templates/user/profile.tmpl | 6 +- web_src/js/bootstrap.js | 11 ---- 25 files changed, 276 insertions(+), 363 deletions(-) create mode 100644 routers/common/errpage.go diff --git a/modules/context/access_log.go b/modules/context/access_log.go index 64d204733b8a..b6468d139bf9 100644 --- a/modules/context/access_log.go +++ b/modules/context/access_log.go @@ -5,7 +5,6 @@ package context import ( "bytes" - "context" "fmt" "net" "net/http" @@ -13,8 +12,10 @@ import ( "text/template" "time" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/web/middleware" ) type routerLoggerOptions struct { @@ -26,8 +27,6 @@ type routerLoggerOptions struct { RequestID *string } -var signedUserNameStringPointerKey interface{} = "signedUserNameStringPointerKey" - const keyOfRequestIDInTemplate = ".RequestID" // According to: @@ -60,8 +59,6 @@ func AccessLogger() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { start := time.Now() - identity := "-" - r := req.WithContext(context.WithValue(req.Context(), signedUserNameStringPointerKey, &identity)) var requestID string if needRequestID { @@ -73,9 +70,14 @@ func AccessLogger() func(http.Handler) http.Handler { reqHost = req.RemoteAddr } - next.ServeHTTP(w, r) + next.ServeHTTP(w, req) rw := w.(ResponseWriter) + identity := "-" + data := middleware.GetContextData(req.Context()) + if signedUser, ok := data[middleware.ContextDataKeySignedUser].(*user_model.User); ok { + identity = signedUser.Name + } buf := bytes.NewBuffer([]byte{}) err = logTemplate.Execute(buf, routerLoggerOptions{ req: req, diff --git a/modules/context/api.go b/modules/context/api.go index ae245ec1cb62..e263dcbe8dea 100644 --- a/modules/context/api.go +++ b/modules/context/api.go @@ -222,7 +222,7 @@ func APIContexter() func(http.Handler) http.Handler { ctx := APIContext{ Context: &Context{ Resp: NewResponse(w), - Data: map[string]interface{}{}, + Data: middleware.GetContextData(req.Context()), Locale: locale, Cache: cache.GetCache(), Repo: &Repository{ @@ -250,17 +250,6 @@ func APIContexter() func(http.Handler) http.Handler { ctx.Data["Context"] = &ctx next.ServeHTTP(ctx.Resp, ctx.Req) - - // Handle adding signedUserName to the context for the AccessLogger - usernameInterface := ctx.Data["SignedUserName"] - identityPtrInterface := ctx.Req.Context().Value(signedUserNameStringPointerKey) - if usernameInterface != nil && identityPtrInterface != nil { - username := usernameInterface.(string) - identityPtr := identityPtrInterface.(*string) - if identityPtr != nil && username != "" { - *identityPtr = username - } - } }) } } diff --git a/modules/context/context.go b/modules/context/context.go index cd7fcebe55da..d73a26e5b6f3 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -55,7 +55,7 @@ type Render interface { type Context struct { Resp ResponseWriter Req *http.Request - Data map[string]interface{} // data used by MVC templates + Data middleware.ContextData // data used by MVC templates PageData map[string]interface{} // data used by JavaScript modules in one page, it's `window.config.pageData` Render Render translation.Locale @@ -97,7 +97,7 @@ func (ctx *Context) TrHTMLEscapeArgs(msg string, args ...string) string { } // GetData returns the data -func (ctx *Context) GetData() map[string]interface{} { +func (ctx *Context) GetData() middleware.ContextData { return ctx.Data } @@ -219,6 +219,7 @@ const tplStatus500 base.TplName = "status/500" // HTML calls Context.HTML and renders the template to HTTP response func (ctx *Context) HTML(status int, name base.TplName) { log.Debug("Template: %s", name) + tmplStartTime := time.Now() if !setting.IsProd { ctx.Data["TemplateName"] = name @@ -226,13 +227,19 @@ func (ctx *Context) HTML(status int, name base.TplName) { ctx.Data["TemplateLoadTimes"] = func() string { return strconv.FormatInt(time.Since(tmplStartTime).Nanoseconds()/1e6, 10) + "ms" } - if err := ctx.Render.HTML(ctx.Resp, status, string(name), templates.BaseVars().Merge(ctx.Data)); err != nil { - if status == http.StatusInternalServerError && name == tplStatus500 { - ctx.PlainText(http.StatusInternalServerError, "Unable to find HTML templates, the template system is not initialized, or Gitea can't find your template files.") - return - } + + err := ctx.Render.HTML(ctx.Resp, status, string(name), ctx.Data) + if err == nil { + return + } + + // if rendering fails, show error page + if name != tplStatus500 { err = fmt.Errorf("failed to render template: %s, error: %s", name, templates.HandleTemplateRenderingError(err)) - ctx.ServerError("Render failed", err) + ctx.ServerError("Render failed", err) // show the 500 error page + } else { + ctx.PlainText(http.StatusInternalServerError, "Unable to render status/500 page, the template system is broken, or Gitea can't find your template files.") + return } } @@ -676,7 +683,7 @@ func getCsrfOpts() CsrfOptions { } // Contexter initializes a classic context for a request. -func Contexter(ctx context.Context) func(next http.Handler) http.Handler { +func Contexter() func(next http.Handler) http.Handler { rnd := templates.HTMLRenderer() csrfOpts := getCsrfOpts() if !setting.IsProd { @@ -684,34 +691,30 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler { } return func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - locale := middleware.Locale(resp, req) - startTime := time.Now() - link := setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/") - ctx := Context{ Resp: NewResponse(resp), Cache: mc.GetCache(), - Locale: locale, - Link: link, + Locale: middleware.Locale(resp, req), + Link: setting.AppSubURL + strings.TrimSuffix(req.URL.EscapedPath(), "/"), Render: rnd, Session: session.GetSession(req), Repo: &Repository{ PullRequest: &PullRequest{}, }, - Org: &Organization{}, - Data: map[string]interface{}{ - "CurrentURL": setting.AppSubURL + req.URL.RequestURI(), - "PageStartTime": startTime, - "Link": link, - "RunModeIsProd": setting.IsProd, - }, + Org: &Organization{}, + Data: middleware.GetContextData(req.Context()), } defer ctx.Close() + ctx.Data.MergeFrom(middleware.CommonTemplateContextData()) + ctx.Data["Context"] = &ctx + ctx.Data["CurrentURL"] = setting.AppSubURL + req.URL.RequestURI() + ctx.Data["Link"] = ctx.Link + ctx.Data["locale"] = ctx.Locale + // PageData is passed by reference, and it will be rendered to `window.config.pageData` in `head.tmpl` for JavaScript modules - ctx.PageData = map[string]interface{}{} + ctx.PageData = map[string]any{} ctx.Data["PageData"] = ctx.PageData - ctx.Data["Context"] = &ctx ctx.Req = WithContext(req, &ctx) ctx.Csrf = PrepareCSRFProtector(csrfOpts, &ctx) @@ -755,16 +758,6 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler { ctx.Data["CsrfTokenHtml"] = template.HTML(``) // FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these - ctx.Data["IsLandingPageHome"] = setting.LandingPageURL == setting.LandingPageHome - ctx.Data["IsLandingPageExplore"] = setting.LandingPageURL == setting.LandingPageExplore - ctx.Data["IsLandingPageOrganizations"] = setting.LandingPageURL == setting.LandingPageOrganizations - - ctx.Data["ShowRegistrationButton"] = setting.Service.ShowRegistrationButton - ctx.Data["ShowMilestonesDashboardPage"] = setting.Service.ShowMilestonesDashboardPage - ctx.Data["ShowFooterVersion"] = setting.Other.ShowFooterVersion - - ctx.Data["EnableSwagger"] = setting.API.EnableSwagger - ctx.Data["EnableOpenIDSignIn"] = setting.Service.EnableOpenIDSignIn ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations ctx.Data["DisableStars"] = setting.Repository.DisableStars ctx.Data["EnableActions"] = setting.Actions.Enabled @@ -777,21 +770,9 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler { ctx.Data["UnitProjectsGlobalDisabled"] = unit.TypeProjects.UnitGlobalDisabled() ctx.Data["UnitActionsGlobalDisabled"] = unit.TypeActions.UnitGlobalDisabled() - ctx.Data["locale"] = locale ctx.Data["AllLangs"] = translation.AllLangs() next.ServeHTTP(ctx.Resp, ctx.Req) - - // Handle adding signedUserName to the context for the AccessLogger - usernameInterface := ctx.Data["SignedUserName"] - identityPtrInterface := ctx.Req.Context().Value(signedUserNameStringPointerKey) - if usernameInterface != nil && identityPtrInterface != nil { - username := usernameInterface.(string) - identityPtr := identityPtrInterface.(*string) - if identityPtr != nil && username != "" { - *identityPtr = username - } - } }) } } diff --git a/modules/context/package.go b/modules/context/package.go index 6c418b316466..fe5bdac19d67 100644 --- a/modules/context/package.go +++ b/modules/context/package.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/web/middleware" ) // Package contains owner, access mode and optional the package descriptor @@ -136,7 +137,7 @@ func PackageContexter(ctx gocontext.Context) func(next http.Handler) http.Handle return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { ctx := Context{ Resp: NewResponse(resp), - Data: map[string]interface{}{}, + Data: middleware.GetContextData(req.Context()), Render: rnd, } defer ctx.Close() diff --git a/modules/context/private.go b/modules/context/private.go index 24f50fa4713e..f621dd68390f 100644 --- a/modules/context/private.go +++ b/modules/context/private.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/process" + "code.gitea.io/gitea/modules/web/middleware" ) // PrivateContext represents a context for private routes @@ -62,7 +63,7 @@ func PrivateContexter() func(http.Handler) http.Handler { ctx := &PrivateContext{ Context: &Context{ Resp: NewResponse(w), - Data: map[string]interface{}{}, + Data: middleware.GetContextData(req.Context()), }, } defer ctx.Close() diff --git a/modules/templates/base.go b/modules/templates/base.go index 4254a569764e..ef28cc03f45d 100644 --- a/modules/templates/base.go +++ b/modules/templates/base.go @@ -5,43 +5,12 @@ package templates import ( "strings" - "time" "code.gitea.io/gitea/modules/assetfs" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" ) -// Vars represents variables to be render in golang templates -type Vars map[string]interface{} - -// Merge merges another vars to the current, another Vars will override the current -func (vars Vars) Merge(another map[string]interface{}) Vars { - for k, v := range another { - vars[k] = v - } - return vars -} - -// BaseVars returns all basic vars -func BaseVars() Vars { - startTime := time.Now() - return map[string]interface{}{ - "IsLandingPageHome": setting.LandingPageURL == setting.LandingPageHome, - "IsLandingPageExplore": setting.LandingPageURL == setting.LandingPageExplore, - "IsLandingPageOrganizations": setting.LandingPageURL == setting.LandingPageOrganizations, - - "ShowRegistrationButton": setting.Service.ShowRegistrationButton, - "ShowMilestonesDashboardPage": setting.Service.ShowMilestonesDashboardPage, - "ShowFooterVersion": setting.Other.ShowFooterVersion, - "DisableDownloadSourceArchives": setting.Repository.DisableDownloadSourceArchives, - - "EnableSwagger": setting.API.EnableSwagger, - "EnableOpenIDSignIn": setting.Service.EnableOpenIDSignIn, - "PageStartTime": startTime, - } -} - func AssetFS() *assetfs.LayeredFS { return assetfs.Layered(CustomAssets(), BuiltinAssets()) } diff --git a/modules/test/context_tests.go b/modules/test/context_tests.go index 4af76512505c..35dd920bb92a 100644 --- a/modules/test/context_tests.go +++ b/modules/test/context_tests.go @@ -30,7 +30,7 @@ func MockContext(t *testing.T, path string) *context.Context { resp := &mockResponseWriter{} ctx := context.Context{ Render: &mockRender{}, - Data: make(map[string]interface{}), + Data: make(middleware.ContextData), Flash: &middleware.Flash{ Values: make(url.Values), }, diff --git a/modules/web/middleware/data.go b/modules/web/middleware/data.go index 43189940ee20..c1f0516d7d2f 100644 --- a/modules/web/middleware/data.go +++ b/modules/web/middleware/data.go @@ -3,7 +3,63 @@ package middleware -// DataStore represents a data store -type DataStore interface { - GetData() map[string]interface{} +import ( + "context" + "time" + + "code.gitea.io/gitea/modules/setting" +) + +// ContextDataStore represents a data store +type ContextDataStore interface { + GetData() ContextData +} + +type ContextData map[string]any + +func (ds ContextData) GetData() map[string]any { + return ds +} + +func (ds ContextData) MergeFrom(other ContextData) ContextData { + for k, v := range other { + ds[k] = v + } + return ds +} + +const ContextDataKeySignedUser = "SignedUser" + +type contextDataKeyType struct{} + +var contextDataKey contextDataKeyType + +func WithContextData(c context.Context) context.Context { + return context.WithValue(c, contextDataKey, make(ContextData, 10)) +} + +func GetContextData(c context.Context) ContextData { + if ds, ok := c.Value(contextDataKey).(ContextData); ok { + return ds + } + return nil +} + +func CommonTemplateContextData() ContextData { + return ContextData{ + "IsLandingPageHome": setting.LandingPageURL == setting.LandingPageHome, + "IsLandingPageExplore": setting.LandingPageURL == setting.LandingPageExplore, + "IsLandingPageOrganizations": setting.LandingPageURL == setting.LandingPageOrganizations, + + "ShowRegistrationButton": setting.Service.ShowRegistrationButton, + "ShowMilestonesDashboardPage": setting.Service.ShowMilestonesDashboardPage, + "ShowFooterVersion": setting.Other.ShowFooterVersion, + "DisableDownloadSourceArchives": setting.Repository.DisableDownloadSourceArchives, + + "EnableSwagger": setting.API.EnableSwagger, + "EnableOpenIDSignIn": setting.Service.EnableOpenIDSignIn, + "PageStartTime": time.Now(), + + "RunModeIsProd": setting.IsProd, + } } diff --git a/modules/web/middleware/flash.go b/modules/web/middleware/flash.go index f2d7cc692d28..fa29ddeffc79 100644 --- a/modules/web/middleware/flash.go +++ b/modules/web/middleware/flash.go @@ -18,7 +18,7 @@ var FlashNow bool // Flash represents a one time data transfer between two requests. type Flash struct { - DataStore + DataStore ContextDataStore url.Values ErrorMsg, WarningMsg, InfoMsg, SuccessMsg string } @@ -34,7 +34,7 @@ func (f *Flash) set(name, msg string, current ...bool) { } if isShow { - f.GetData()["Flash"] = f + f.DataStore.GetData()["Flash"] = f } else { f.Set(name, msg) } diff --git a/modules/web/middleware/request.go b/modules/web/middleware/request.go index 34add27214b8..0bb155df7034 100644 --- a/modules/web/middleware/request.go +++ b/modules/web/middleware/request.go @@ -12,8 +12,3 @@ import ( func IsAPIPath(req *http.Request) bool { return strings.HasPrefix(req.URL.Path, "/api/") } - -// IsInternalPath returns true if the specified URL is an internal API path -func IsInternalPath(req *http.Request) bool { - return strings.HasPrefix(req.URL.Path, "/api/internal/") -} diff --git a/modules/web/route.go b/modules/web/route.go index 6fd60f4ca739..d801f1025c94 100644 --- a/modules/web/route.go +++ b/modules/web/route.go @@ -25,12 +25,12 @@ func Bind[T any](_ T) any { } // SetForm set the form object -func SetForm(data middleware.DataStore, obj interface{}) { +func SetForm(data middleware.ContextDataStore, obj interface{}) { data.GetData()["__form"] = obj } // GetForm returns the validate form information -func GetForm(data middleware.DataStore) interface{} { +func GetForm(data middleware.ContextDataStore) interface{} { return data.GetData()["__form"] } diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 8649a982b08e..8afa83aa94fe 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -103,7 +103,6 @@ func CreateUser(ctx *context.APIContext) { if err != nil { log.Error(err.Error()) } - ctx.Data["Err_Password"] = true ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned")) return } @@ -201,7 +200,6 @@ func EditUser(ctx *context.APIContext) { if err != nil { log.Error(err.Error()) } - ctx.Data["Err_Password"] = true ctx.Error(http.StatusBadRequest, "PasswordPwned", errors.New("PasswordPwned")) return } diff --git a/routers/api/v1/misc/markup_test.go b/routers/api/v1/misc/markup_test.go index 32de2956b792..68776613b272 100644 --- a/routers/api/v1/misc/markup_test.go +++ b/routers/api/v1/misc/markup_test.go @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/modules/web/middleware" "github.com/stretchr/testify/assert" ) @@ -36,7 +37,7 @@ func createContext(req *http.Request) (*context.Context, *httptest.ResponseRecor Req: req, Resp: context.NewResponse(resp), Render: rnd, - Data: make(map[string]interface{}), + Data: make(middleware.ContextData), } defer c.Close() diff --git a/routers/common/errpage.go b/routers/common/errpage.go new file mode 100644 index 000000000000..4cf3bf83578a --- /dev/null +++ b/routers/common/errpage.go @@ -0,0 +1,57 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "fmt" + "net/http" + + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/httpcache" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/web/middleware" + "code.gitea.io/gitea/modules/web/routing" +) + +const tplStatus500 base.TplName = "status/500" + +// RenderPanicErrorPage renders a 500 page, and it never panics +func RenderPanicErrorPage(w http.ResponseWriter, req *http.Request, err any) { + combinedErr := fmt.Sprintf("%v\n%s", err, log.Stack(2)) + log.Error("PANIC: %s", combinedErr) + + defer func() { + if err := recover(); err != nil { + log.Error("Panic occurs again when rendering error page: %v", err) + } + }() + + routing.UpdatePanicError(req.Context(), err) + + httpcache.SetCacheControlInHeader(w.Header(), 0, "no-transform") + w.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions) + + data := middleware.GetContextData(req.Context()) + if data["locale"] == nil { + data = middleware.CommonTemplateContextData() + data["locale"] = middleware.Locale(w, req) + } + + // This recovery handler could be called without Gitea's web context, so we shouldn't touch that context too much. + // Otherwise, the 500-page may cause new panics, eg: cache.GetContextWithData, it makes the developer&users couldn't find the original panic. + user, _ := data[middleware.ContextDataKeySignedUser].(*user_model.User) + if !setting.IsProd || (user != nil && user.IsAdmin) { + data["ErrorMsg"] = "PANIC: " + combinedErr + } + + err = templates.HTMLRenderer().HTML(w, http.StatusInternalServerError, string(tplStatus500), data) + if err != nil { + log.Error("Error occurs again when rendering error page: %v", err) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal server error, please collect error logs and report to Gitea issue tracker")) + } +} diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 28ecf934e1cc..c1ee9dd765af 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -10,9 +10,9 @@ import ( "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/context" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/modules/web/routing" "gitea.com/go-chi/session" @@ -20,13 +20,26 @@ import ( chi "github.com/go-chi/chi/v5" ) -// ProtocolMiddlewares returns HTTP protocol related middlewares +// ProtocolMiddlewares returns HTTP protocol related middlewares, and it provides a global panic recovery func ProtocolMiddlewares() (handlers []any) { + // first, normalize the URL path + handlers = append(handlers, stripSlashesMiddleware) + + // prepare the ContextData and panic recovery handlers = append(handlers, func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - // First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL - req.URL.RawPath = req.URL.EscapedPath() + defer func() { + if err := recover(); err != nil { + RenderPanicErrorPage(resp, req, err) // it should never panic + } + }() + req = req.WithContext(middleware.WithContextData(req.Context())) + next.ServeHTTP(resp, req) + }) + }) + handlers = append(handlers, func(next http.Handler) http.Handler { + return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { ctx, _, finished := process.GetManager().AddTypedContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI), process.RequestProcessType, true) defer finished() next.ServeHTTP(context.NewResponse(resp), req.WithContext(cache.WithCacheContext(ctx))) @@ -47,9 +60,6 @@ func ProtocolMiddlewares() (handlers []any) { handlers = append(handlers, proxy.ForwardedHeaders(opt)) } - // Strip slashes. - handlers = append(handlers, stripSlashesMiddleware) - if !setting.Log.DisableRouterLog { handlers = append(handlers, routing.NewLoggerHandler()) } @@ -58,40 +68,18 @@ func ProtocolMiddlewares() (handlers []any) { handlers = append(handlers, context.AccessLogger()) } - handlers = append(handlers, func(next http.Handler) http.Handler { - return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - // Why we need this? The Recovery() will try to render a beautiful - // error page for user, but the process can still panic again, and other - // middleware like session also may panic then we have to recover twice - // and send a simple error page that should not panic anymore. - defer func() { - if err := recover(); err != nil { - routing.UpdatePanicError(req.Context(), err) - combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2)) - log.Error("%v", combinedErr) - if setting.IsProd { - http.Error(resp, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) - } else { - http.Error(resp, combinedErr, http.StatusInternalServerError) - } - } - }() - next.ServeHTTP(resp, req) - }) - }) return handlers } func stripSlashesMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - var urlPath string + // First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL + req.URL.RawPath = req.URL.EscapedPath() + + urlPath := req.URL.RawPath rctx := chi.RouteContext(req.Context()) if rctx != nil && rctx.RoutePath != "" { urlPath = rctx.RoutePath - } else if req.URL.RawPath != "" { - urlPath = req.URL.RawPath - } else { - urlPath = req.URL.Path } sanitizedPath := &strings.Builder{} diff --git a/routers/install/install.go b/routers/install/install.go index c838db65822c..714ddd554845 100644 --- a/routers/install/install.go +++ b/routers/install/install.go @@ -5,7 +5,6 @@ package install import ( - goctx "context" "fmt" "net/http" "os" @@ -53,33 +52,32 @@ func getSupportedDbTypeNames() (dbTypeNames []map[string]string) { return dbTypeNames } -// Init prepare for rendering installation page -func Init(ctx goctx.Context) func(next http.Handler) http.Handler { +// Contexter prepare for rendering installation page +func Contexter() func(next http.Handler) http.Handler { rnd := templates.HTMLRenderer() dbTypeNames := getSupportedDbTypeNames() return func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - locale := middleware.Locale(resp, req) - startTime := time.Now() ctx := context.Context{ Resp: context.NewResponse(resp), Flash: &middleware.Flash{}, - Locale: locale, + Locale: middleware.Locale(resp, req), Render: rnd, + Data: middleware.GetContextData(req.Context()), Session: session.GetSession(req), - Data: map[string]interface{}{ - "locale": locale, - "Title": locale.Tr("install.install"), - "PageIsInstall": true, - "DbTypeNames": dbTypeNames, - "AllLangs": translation.AllLangs(), - "PageStartTime": startTime, - - "PasswordHashAlgorithms": hash.RecommendedHashAlgorithms, - }, } defer ctx.Close() + ctx.Data.MergeFrom(middleware.CommonTemplateContextData()) + ctx.Data.MergeFrom(middleware.ContextData{ + "locale": ctx.Locale, + "Title": ctx.Locale.Tr("install.install"), + "PageIsInstall": true, + "DbTypeNames": dbTypeNames, + "AllLangs": translation.AllLangs(), + + "PasswordHashAlgorithms": hash.RecommendedHashAlgorithms, + }) ctx.Req = context.WithContext(req, &ctx) next.ServeHTTP(resp, ctx.Req) }) diff --git a/routers/install/routes.go b/routers/install/routes.go index 88c7e996952f..52c07cfa26e6 100644 --- a/routers/install/routes.go +++ b/routers/install/routes.go @@ -9,76 +9,14 @@ import ( "html" "net/http" - "code.gitea.io/gitea/modules/httpcache" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/public" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/web" - "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/routers/web/healthcheck" "code.gitea.io/gitea/services/forms" ) -type dataStore map[string]interface{} - -func (d *dataStore) GetData() map[string]interface{} { - return *d -} - -func installRecovery(ctx goctx.Context) func(next http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - defer func() { - // Why we need this? The first recover will try to render a beautiful - // error page for user, but the process can still panic again, then - // we have to just recover twice and send a simple error page that - // should not panic anymore. - defer func() { - if err := recover(); err != nil { - combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2)) - log.Error("%s", combinedErr) - if setting.IsProd { - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) - } else { - http.Error(w, combinedErr, http.StatusInternalServerError) - } - } - }() - - if err := recover(); err != nil { - combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2)) - log.Error("%s", combinedErr) - - lc := middleware.Locale(w, req) - store := dataStore{ - "Language": lc.Language(), - "CurrentURL": setting.AppSubURL + req.URL.RequestURI(), - "locale": lc, - "SignedUserID": int64(0), - "SignedUserName": "", - } - - httpcache.SetCacheControlInHeader(w.Header(), 0, "no-transform") - w.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions) - - if !setting.IsProd { - store["ErrorMsg"] = combinedErr - } - rnd := templates.HTMLRenderer() - err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store)) - if err != nil { - log.Error("%v", err) - } - } - }() - - next.ServeHTTP(w, req) - }) - } -} - // Routes registers the installation routes func Routes(ctx goctx.Context) *web.Route { base := web.NewRoute() @@ -86,9 +24,7 @@ func Routes(ctx goctx.Context) *web.Route { base.RouteMethods("/assets/*", "GET, HEAD", public.AssetsHandlerFunc("/assets/")) r := web.NewRoute() - r.Use(common.Sessioner()) - r.Use(installRecovery(ctx)) - r.Use(Init(ctx)) + r.Use(common.Sessioner(), Contexter()) r.Get("/", Install) // it must be on the root, because the "install.js" use the window.location to replace the "localhost" AppURL r.Post("/", web.Bind(forms.InstallForm{}), SubmitInstall) r.Get("/post-install", InstallDone) diff --git a/routers/web/base.go b/routers/web/base.go index 73607cad0650..b9b59583896f 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -4,7 +4,6 @@ package web import ( - goctx "context" "errors" "fmt" "io" @@ -13,18 +12,12 @@ import ( "path" "strings" - "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/httpcache" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" - "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/util" - "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/modules/web/routing" - "code.gitea.io/gitea/services/auth" - - "gitea.com/go-chi/session" ) func storageHandler(storageSetting setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler { @@ -110,62 +103,3 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor }) } } - -type dataStore map[string]interface{} - -func (d *dataStore) GetData() map[string]interface{} { - return *d -} - -// RecoveryWith500Page returns a middleware that recovers from any panics and writes a 500 and a log if so. -// This error will be created with the gitea 500 page. -func RecoveryWith500Page(ctx goctx.Context) func(next http.Handler) http.Handler { - rnd := templates.HTMLRenderer() - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - defer func() { - if err := recover(); err != nil { - routing.UpdatePanicError(req.Context(), err) - combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, log.Stack(2)) - log.Error("%s", combinedErr) - - sessionStore := session.GetSession(req) - - lc := middleware.Locale(w, req) - store := dataStore{ - "Language": lc.Language(), - "CurrentURL": setting.AppSubURL + req.URL.RequestURI(), - "locale": lc, - } - - // TODO: this recovery handler is usually called without Gitea's web context, so we shouldn't touch that context too much - // Otherwise, the 500 page may cause new panics, eg: cache.GetContextWithData, it makes the developer&users couldn't find the original panic - user := context.GetContextUser(req) // almost always nil - if user == nil { - // Get user from session if logged in - do not attempt to sign-in - user = auth.SessionUser(sessionStore) - } - - httpcache.SetCacheControlInHeader(w.Header(), 0, "no-transform") - w.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions) - - if !setting.IsProd || (user != nil && user.IsAdmin) { - store["ErrorMsg"] = combinedErr - } - - defer func() { - if err := recover(); err != nil { - log.Error("HTML render in Recovery handler panics again: %v", err) - } - }() - err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store)) - if err != nil { - log.Error("HTML render in Recovery handler fails again: %v", err) - } - } - }() - - next.ServeHTTP(w, req) - }) - } -} diff --git a/routers/web/web.go b/routers/web/web.go index 5357c55500fc..8cdc10935efb 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -116,38 +116,34 @@ func Routes(ctx gocontext.Context) *web.Route { _ = templates.HTMLRenderer() - common := []any{ - common.Sessioner(), - RecoveryWith500Page(ctx), - } + var mid []any if setting.EnableGzip { h, err := gziphandler.GzipHandlerWithOpts(gziphandler.MinSize(GzipMinSize)) if err != nil { log.Fatal("GzipHandlerWithOpts failed: %v", err) } - common = append(common, h) + mid = append(mid, h) } if setting.Service.EnableCaptcha { // The captcha http.Handler should only fire on /captcha/* so we can just mount this on that url - routes.RouteMethods("/captcha/*", "GET,HEAD", append(common, captcha.Captchaer(context.GetImageCaptcha()))...) + routes.RouteMethods("/captcha/*", "GET,HEAD", append(mid, captcha.Captchaer(context.GetImageCaptcha()))...) } if setting.HasRobotsTxt { - routes.Get("/robots.txt", append(common, misc.RobotsTxt)...) + routes.Get("/robots.txt", append(mid, misc.RobotsTxt)...) } - // prometheus metrics endpoint - do not need to go through contexter if setting.Metrics.Enabled { prometheus.MustRegister(metrics.NewCollector()) - routes.Get("/metrics", append(common, Metrics)...) + routes.Get("/metrics", append(mid, Metrics)...) } routes.Get("/ssh_info", misc.SSHInfo) routes.Get("/api/healthz", healthcheck.Check) - common = append(common, context.Contexter(ctx)) + mid = append(mid, common.Sessioner(), context.Contexter()) group := buildAuthGroup() if err := group.Init(ctx); err != nil { @@ -155,23 +151,23 @@ func Routes(ctx gocontext.Context) *web.Route { } // Get user from session if logged in. - common = append(common, auth_service.Auth(group)) + mid = append(mid, auth_service.Auth(group)) // GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route - common = append(common, middleware.GetHead) + mid = append(mid, middleware.GetHead) if setting.API.EnableSwagger { // Note: The route is here but no in API routes because it renders a web page - routes.Get("/api/swagger", append(common, misc.Swagger)...) // Render V1 by default + routes.Get("/api/swagger", append(mid, misc.Swagger)...) // Render V1 by default } // TODO: These really seem like things that could be folded into Contexter or as helper functions - common = append(common, user.GetNotificationCount) - common = append(common, repo.GetActiveStopwatch) - common = append(common, goGet) + mid = append(mid, user.GetNotificationCount) + mid = append(mid, repo.GetActiveStopwatch) + mid = append(mid, goGet) others := web.NewRoute() - others.Use(common...) + others.Use(mid...) registerRoutes(others) routes.Mount("", others) return routes diff --git a/services/auth/interface.go b/services/auth/interface.go index f2f1aaf39cb0..c4a8a20d0103 100644 --- a/services/auth/interface.go +++ b/services/auth/interface.go @@ -13,7 +13,7 @@ import ( ) // DataStore represents a data store -type DataStore middleware.DataStore +type DataStore middleware.ContextDataStore // SessionStore represents a session store type SessionStore session.Store diff --git a/services/auth/middleware.go b/services/auth/middleware.go index abeafed9cb46..3b2f883d009c 100644 --- a/services/auth/middleware.go +++ b/services/auth/middleware.go @@ -51,13 +51,11 @@ func authShared(ctx *context.Context, authMethod Method) error { ctx.IsBasicAuth = ctx.Data["AuthedMethod"].(string) == BasicMethodName ctx.IsSigned = true ctx.Data["IsSigned"] = ctx.IsSigned - ctx.Data["SignedUser"] = ctx.Doer + ctx.Data[middleware.ContextDataKeySignedUser] = ctx.Doer ctx.Data["SignedUserID"] = ctx.Doer.ID - ctx.Data["SignedUserName"] = ctx.Doer.Name ctx.Data["IsAdmin"] = ctx.Doer.IsAdmin } else { ctx.Data["SignedUserID"] = int64(0) - ctx.Data["SignedUserName"] = "" } return nil } diff --git a/templates/base/head_script.tmpl b/templates/base/head_script.tmpl index d19fae629bd9..670d146b56a0 100644 --- a/templates/base/head_script.tmpl +++ b/templates/base/head_script.tmpl @@ -6,7 +6,6 @@ If you introduce mistakes in it, Gitea JavaScript code wouldn't run correctly. -{{template "base/footer" .}} + + {{/* When a sub-template triggers an 500 error, its parent template has been partially rendered, then the 500 page + will be rendered after that partially rendered page, the HTML/JS are totally broken. Use this inline script to try to move it to main viewport. + And this page shouldn't include any other JS file, avoid duplicate JS execution (still due to the partial rendering).*/}} + + + diff --git a/templates/user/profile.tmpl b/templates/user/profile.tmpl index 9f454a82f19f..5463270854e7 100644 --- a/templates/user/profile.tmpl +++ b/templates/user/profile.tmpl @@ -5,7 +5,7 @@
- {{if eq .SignedUserName .ContextUser.Name}} + {{if eq .SignedUserID .ContextUser.ID}} {{avatar $.Context .ContextUser 290}} @@ -30,7 +30,7 @@ {{if .ContextUser.Location}}
  • {{svg "octicon-location"}} {{.ContextUser.Location}}
    • {{end}} - {{if (eq .SignedUserName .ContextUser.Name)}} + {{if (eq .SignedUserID .ContextUser.ID)}}
  • {{svg "octicon-mail"}} {{.ContextUser.Email}} @@ -100,7 +100,7 @@
    • {{end}} - {{if and .IsSigned (ne .SignedUserName .ContextUser.Name)}} + {{if and .IsSigned (ne .SignedUserID .ContextUser.ID)}}