From e5a8ebc0ed2553a16bac4b54a74cc3e5e8036b32 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 6 May 2023 17:39:06 +0800 Subject: [PATCH] Require at least one unit to be enabled (#24189) Don't remember why the previous decision that `Code` and `Release` are non-disable units globally. Since now every unit include `Code` could be disabled, maybe we should have a new rule that the repo should have at least one unit. So any unit could be disabled. Fixes #20960 Fixes #7525 --------- Co-authored-by: delvh Co-authored-by: yp05327 <576951401@qq.com> --- models/repo.go | 4 +- models/unit/unit.go | 47 +++++--------------- models/unit/unit_test.go | 76 +++++++++++++++++++++++++-------- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/repo.go | 8 ++-- routers/web/repo/setting.go | 6 +++ routers/web/web.go | 38 ++++++++++------- templates/explore/navbar.tmpl | 2 +- templates/user/profile.tmpl | 2 +- 9 files changed, 109 insertions(+), 75 deletions(-) diff --git a/models/repo.go b/models/repo.go index 5a1e2e028e8..d2a3311c8b7 100644 --- a/models/repo.go +++ b/models/repo.go @@ -40,7 +40,9 @@ var ItemsPerPage = 40 // Init initialize model func Init(ctx context.Context) error { - unit.LoadUnitConfig() + if err := unit.LoadUnitConfig(); err != nil { + return err + } return system_model.Init(ctx) } diff --git a/models/unit/unit.go b/models/unit/unit.go index 3d5a8842cd6..7cd679116f1 100644 --- a/models/unit/unit.go +++ b/models/unit/unit.go @@ -4,6 +4,7 @@ package unit import ( + "errors" "fmt" "strings" @@ -106,12 +107,6 @@ var ( TypeExternalTracker, } - // MustRepoUnits contains the units could not be disabled currently - MustRepoUnits = []Type{ - TypeCode, - TypeReleases, - } - // DisabledRepoUnits contains the units that have been globally disabled DisabledRepoUnits = []Type{} ) @@ -122,18 +117,13 @@ func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { // Use setting if not empty if len(settingDefaultUnits) > 0 { - // MustRepoUnits required as default - units = make([]Type, len(MustRepoUnits)) - copy(units, MustRepoUnits) + units = make([]Type, 0, len(settingDefaultUnits)) for _, settingUnit := range settingDefaultUnits { if !settingUnit.CanBeDefault() { log.Warn("Not allowed as default unit: %s", settingUnit.String()) continue } - // MustRepoUnits already added - if settingUnit.CanDisable() { - units = append(units, settingUnit) - } + units = append(units, settingUnit) } } @@ -150,30 +140,30 @@ func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { } // LoadUnitConfig load units from settings -func LoadUnitConfig() { +func LoadUnitConfig() error { var invalidKeys []string DisabledRepoUnits, invalidKeys = FindUnitTypes(setting.Repository.DisabledRepoUnits...) if len(invalidKeys) > 0 { log.Warn("Invalid keys in disabled repo units: %s", strings.Join(invalidKeys, ", ")) } - // Check that must units are not disabled - for i, disabledU := range DisabledRepoUnits { - if !disabledU.CanDisable() { - log.Warn("Not allowed to global disable unit %s", disabledU.String()) - DisabledRepoUnits = append(DisabledRepoUnits[:i], DisabledRepoUnits[i+1:]...) - } - } setDefaultRepoUnits, invalidKeys := FindUnitTypes(setting.Repository.DefaultRepoUnits...) if len(invalidKeys) > 0 { log.Warn("Invalid keys in default repo units: %s", strings.Join(invalidKeys, ", ")) } DefaultRepoUnits = validateDefaultRepoUnits(DefaultRepoUnits, setDefaultRepoUnits) + if len(DefaultRepoUnits) == 0 { + return errors.New("no default repository units found") + } setDefaultForkRepoUnits, invalidKeys := FindUnitTypes(setting.Repository.DefaultForkRepoUnits...) if len(invalidKeys) > 0 { log.Warn("Invalid keys in default fork repo units: %s", strings.Join(invalidKeys, ", ")) } DefaultForkRepoUnits = validateDefaultRepoUnits(DefaultForkRepoUnits, setDefaultForkRepoUnits) + if len(DefaultForkRepoUnits) == 0 { + return errors.New("no default fork repository units found") + } + return nil } // UnitGlobalDisabled checks if unit type is global disabled @@ -186,16 +176,6 @@ func (u Type) UnitGlobalDisabled() bool { return false } -// CanDisable checks if this unit type can be disabled. -func (u *Type) CanDisable() bool { - for _, mu := range MustRepoUnits { - if *u == mu { - return false - } - } - return true -} - // CanBeDefault checks if the unit type can be a default repo unit func (u *Type) CanBeDefault() bool { for _, nadU := range NotAllowedDefaultRepoUnits { @@ -216,11 +196,6 @@ type Unit struct { MaxAccessMode perm.AccessMode // The max access mode of the unit. i.e. Read means this unit can only be read. } -// CanDisable returns if this unit could be disabled. -func (u *Unit) CanDisable() bool { - return u.Type.CanDisable() -} - // IsLessThan compares order of two units func (u Unit) IsLessThan(unit Unit) bool { if (u.Type == TypeExternalTracker || u.Type == TypeExternalWiki) && unit.Type != TypeExternalTracker && unit.Type != TypeExternalWiki { diff --git a/models/unit/unit_test.go b/models/unit/unit_test.go index 50d78171977..5c50a106bde 100644 --- a/models/unit/unit_test.go +++ b/models/unit/unit_test.go @@ -12,42 +12,84 @@ import ( ) func TestLoadUnitConfig(t *testing.T) { - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnits = disabledRepoUnits - DefaultRepoUnits = defaultRepoUnits - DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) - defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { - setting.Repository.DisabledRepoUnits = disabledRepoUnits - setting.Repository.DefaultRepoUnits = defaultRepoUnits - setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits - }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) - t.Run("regular", func(t *testing.T) { + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { + DisabledRepoUnits = disabledRepoUnits + DefaultRepoUnits = defaultRepoUnits + DefaultForkRepoUnits = defaultForkRepoUnits + }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { + setting.Repository.DisabledRepoUnits = disabledRepoUnits + setting.Repository.DefaultRepoUnits = defaultRepoUnits + setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits + }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + setting.Repository.DisabledRepoUnits = []string{"repo.issues"} setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls"} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases"} - LoadUnitConfig() + assert.NoError(t, LoadUnitConfig()) assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeCode, TypeReleases}, DefaultForkRepoUnits) + assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) t.Run("invalid", func(t *testing.T) { + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { + DisabledRepoUnits = disabledRepoUnits + DefaultRepoUnits = defaultRepoUnits + DefaultForkRepoUnits = defaultForkRepoUnits + }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { + setting.Repository.DisabledRepoUnits = disabledRepoUnits + setting.Repository.DefaultRepoUnits = defaultRepoUnits + setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits + }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + setting.Repository.DisabledRepoUnits = []string{"repo.issues", "invalid.1"} setting.Repository.DefaultRepoUnits = []string{"repo.code", "invalid.2", "repo.releases", "repo.issues", "repo.pulls"} setting.Repository.DefaultForkRepoUnits = []string{"invalid.3", "repo.releases"} - LoadUnitConfig() + assert.NoError(t, LoadUnitConfig()) assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeCode, TypeReleases}, DefaultForkRepoUnits) + assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) t.Run("duplicate", func(t *testing.T) { + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { + DisabledRepoUnits = disabledRepoUnits + DefaultRepoUnits = defaultRepoUnits + DefaultForkRepoUnits = defaultForkRepoUnits + }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { + setting.Repository.DisabledRepoUnits = disabledRepoUnits + setting.Repository.DefaultRepoUnits = defaultRepoUnits + setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits + }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + setting.Repository.DisabledRepoUnits = []string{"repo.issues", "repo.issues"} setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls", "repo.code"} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"} - LoadUnitConfig() + assert.NoError(t, LoadUnitConfig()) assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) - assert.Equal(t, []Type{TypeCode, TypeReleases}, DefaultForkRepoUnits) + assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) + }) + t.Run("empty_default", func(t *testing.T) { + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { + DisabledRepoUnits = disabledRepoUnits + DefaultRepoUnits = defaultRepoUnits + DefaultForkRepoUnits = defaultForkRepoUnits + }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { + setting.Repository.DisabledRepoUnits = disabledRepoUnits + setting.Repository.DefaultRepoUnits = defaultRepoUnits + setting.Repository.DefaultForkRepoUnits = defaultForkRepoUnits + }(setting.Repository.DisabledRepoUnits, setting.Repository.DefaultRepoUnits, setting.Repository.DefaultForkRepoUnits) + + setting.Repository.DisabledRepoUnits = []string{"repo.issues", "repo.issues"} + setting.Repository.DefaultRepoUnits = []string{} + setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"} + assert.NoError(t, LoadUnitConfig()) + assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) + assert.ElementsMatch(t, []Type{TypeCode, TypePullRequests, TypeReleases, TypeWiki, TypePackages, TypeProjects}, DefaultRepoUnits) + assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index f48ebbdc746..248936e35de 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2004,6 +2004,7 @@ settings.delete_notices_2 = - This operation will permanently delete the 0 { + if err := repo_model.UpdateRepositoryUnits(repo, units, deleteUnitTypes); err != nil { + ctx.Error(http.StatusInternalServerError, "UpdateRepositoryUnits", err) + return err + } } log.Trace("Repository advanced settings updated: %s/%s", owner.Name, repo.Name) diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go index 0c36503b3c4..16b718c919f 100644 --- a/routers/web/repo/setting.go +++ b/routers/web/repo/setting.go @@ -536,6 +536,12 @@ func SettingsPost(ctx *context.Context) { deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests) } + if len(units) == 0 { + ctx.Flash.Error(ctx.Tr("repo.settings.update_settings_no_unit")) + ctx.Redirect(ctx.Repo.RepoLink + "/settings") + return + } + if err := repo_model.UpdateRepositoryUnits(repo, units, deleteUnitTypes); err != nil { ctx.ServerError("UpdateRepositoryUnits", err) return diff --git a/routers/web/web.go b/routers/web/web.go index e904db321d8..5917c93e22c 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -261,6 +261,27 @@ func registerRoutes(m *web.Route) { } } + reqUnitAccess := func(unitType unit.Type, accessMode perm.AccessMode) func(ctx *context.Context) { + return func(ctx *context.Context) { + if unitType.UnitGlobalDisabled() { + ctx.NotFound(unitType.String(), nil) + return + } + + if ctx.ContextUser == nil { + ctx.NotFound(unitType.String(), nil) + return + } + + if ctx.ContextUser.IsOrganization() { + if ctx.Org.Organization.UnitPermission(ctx, ctx.Doer, unitType) < accessMode { + ctx.NotFound(unitType.String(), nil) + return + } + } + } + } + addWebhookAddRoutes := func() { m.Get("/{type}/new", repo.WebhooksNew) m.Post("/gitea/new", web.Bind(forms.NewWebhookForm{}), repo.GiteaHooksNewPost) @@ -334,7 +355,7 @@ func registerRoutes(m *web.Route) { m.Get("/users", explore.Users) m.Get("/users/sitemap-{idx}.xml", sitemapEnabled, explore.Users) m.Get("/organizations", explore.Organizations) - m.Get("/code", explore.Code) + m.Get("/code", reqUnitAccess(unit.TypeCode, perm.AccessModeRead), explore.Code) m.Get("/topics/search", explore.TopicSearch) }, ignExploreSignIn) m.Group("/issues", func() { @@ -649,21 +670,6 @@ func registerRoutes(m *web.Route) { } } - reqUnitAccess := func(unitType unit.Type, accessMode perm.AccessMode) func(ctx *context.Context) { - return func(ctx *context.Context) { - if ctx.ContextUser == nil { - ctx.NotFound(unitType.String(), nil) - return - } - if ctx.ContextUser.IsOrganization() { - if ctx.Org.Organization.UnitPermission(ctx, ctx.Doer, unitType) < accessMode { - ctx.NotFound(unitType.String(), nil) - return - } - } - } - } - // ***** START: Organization ***** m.Group("/org", func() { m.Group("/{org}", func() { diff --git a/templates/explore/navbar.tmpl b/templates/explore/navbar.tmpl index 3a23df69929..3a556812f9b 100644 --- a/templates/explore/navbar.tmpl +++ b/templates/explore/navbar.tmpl @@ -10,7 +10,7 @@ {{svg "octicon-organization"}} {{.locale.Tr "explore.organizations"}} - {{if .IsRepoIndexerEnabled}} + {{if and (not $.UnitTypeCode.UnitGlobalDisabled) .IsRepoIndexerEnabled}} {{svg "octicon-code"}} {{.locale.Tr "explore.code"}} diff --git a/templates/user/profile.tmpl b/templates/user/profile.tmpl index 5463270854e..8d48474441a 100644 --- a/templates/user/profile.tmpl +++ b/templates/user/profile.tmpl @@ -135,7 +135,7 @@ {{svg "octicon-package"}} {{.locale.Tr "packages.title"}} {{end}} - {{if .IsRepoIndexerEnabled}} + {{if and (not $.UnitTypeCode.UnitGlobalDisabled) .IsRepoIndexerEnabled}} {{svg "octicon-code"}} {{.locale.Tr "user.code"}}