fix(repo): require organization owners for team access (#39046)

Require organization ownership before changing repository team
associations when team access is restricted.

---------

Co-authored-by: silverwind <me@silverwind.io>

models/organization/org.go

@@ -91,6 +91,14 @@ func (org *Organization) IsOwnedBy(ctx context.Context, uid int64) (bool, error)
 	return IsOrganizationOwner(ctx, org.ID, uid)
 }
 
+// CanChangeRepoTeamAccess reports whether a repository administrator can change team access.
+func (org *Organization) CanChangeRepoTeamAccess(ctx context.Context, doer *user_model.User) (bool, error) {
+	if org.RepoAdminChangeTeamAccess || doer.IsAdmin {
+		return true, nil
+	}
+	return org.IsOwnedBy(ctx, doer.ID)
+}
+
 // IsOrgAdmin returns true if given user is in the owner team or an admin team.
 func (org *Organization) IsOrgAdmin(ctx context.Context, uid int64) (bool, error) {
 	return IsOrganizationAdmin(ctx, org.ID, uid)

routers/api/v1/org/team.go

@@ -659,16 +659,13 @@ func getRepositoryByParams(ctx *context.APIContext) *repo_model.Repository {
 }
 
 func canChangeTeamRepository(ctx *context.APIContext) bool {
-	if ctx.Org.Organization.RepoAdminChangeTeamAccess {
-		return true
-	}
-	isOwner, err := ctx.Org.Organization.IsOwnedBy(ctx, ctx.Doer.ID)
+	canChange, err := ctx.Org.Organization.CanChangeRepoTeamAccess(ctx, ctx.Doer)
 	if err != nil {
 		ctx.APIErrorInternal(err)
 		return false
 	}
-	if !isOwner {
-		ctx.APIError(http.StatusForbidden, "user is nor repo admin nor owner")
+	if !canChange {
+		ctx.APIError(http.StatusForbidden, "Must be an organization owner")
 		return false
 	}
 	return true

routers/api/v1/repo/teams.go

@@ -137,6 +137,8 @@ func AddTeam(ctx *context.APIContext) {
 	// responses:
 	//   "204":
 	//     "$ref": "#/responses/empty"
+	//   "403":
+	//     "$ref": "#/responses/forbidden"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
 	//   "405":
@@ -173,6 +175,8 @@ func DeleteTeam(ctx *context.APIContext) {
 	// responses:
 	//   "204":
 	//     "$ref": "#/responses/empty"
+	//   "403":
+	//     "$ref": "#/responses/forbidden"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
 	//   "405":
@@ -186,9 +190,9 @@ func DeleteTeam(ctx *context.APIContext) {
 func changeRepoTeam(ctx *context.APIContext, add bool) {
 	if !ctx.Repo.Owner.IsOrganization() {
 		ctx.APIError(http.StatusMethodNotAllowed, "repo is not owned by an organization")
+		return
 	}
-	if !ctx.Repo.Owner.RepoAdminChangeTeamAccess && !ctx.Repo.Permission.IsOwner() {
-		ctx.APIError(http.StatusForbidden, "user is nor repo admin nor owner")
+	if !canChangeRepoTeam(ctx) {
 		return
 	}
 
@@ -220,6 +224,19 @@ func changeRepoTeam(ctx *context.APIContext, add bool) {
 	ctx.Status(http.StatusNoContent)
 }
 
+func canChangeRepoTeam(ctx *context.APIContext) bool {
+	canChange, err := organization.OrgFromUser(ctx.Repo.Owner).CanChangeRepoTeamAccess(ctx, ctx.Doer)
+	if err != nil {
+		ctx.APIErrorInternal(err)
+		return false
+	}
+	if !canChange {
+		ctx.APIError(http.StatusForbidden, "Must be an organization owner")
+		return false
+	}
+	return true
+}
+
 func getTeamByParam(ctx *context.APIContext) *organization.Team {
 	team, err := organization.GetTeam(ctx, ctx.Repo.Owner.ID, ctx.PathParam("team"))
 	if err != nil {

routers/web/repo/setting/collaboration.go

@@ -43,6 +43,13 @@ func Collaboration(ctx *context.Context) {
 	ctx.Data["OrgName"] = ctx.Repo.Repository.OwnerName
 	ctx.Data["Org"] = ctx.Repo.Repository.Owner
 	ctx.Data["Units"] = unit_model.Units
+	if ctx.Repo.Owner.IsOrganization() {
+		ctx.Data["CanChangeRepoTeamAccess"], err = organization.OrgFromUser(ctx.Repo.Owner).CanChangeRepoTeamAccess(ctx, ctx.Doer)
+		if err != nil {
+			ctx.ServerError("CanChangeRepoTeamAccess", err)
+			return
+		}
+	}
 
 	ctx.HTML(http.StatusOK, tplCollaboration)
 }
@@ -155,9 +162,7 @@ func DeleteCollaboration(ctx *context.Context) {
 
 // AddTeamPost response for adding a team to a repository
 func AddTeamPost(ctx *context.Context) {
-	if !ctx.Repo.Owner.RepoAdminChangeTeamAccess && !ctx.Repo.Permission.IsOwner() {
-		ctx.Flash.Error(ctx.Tr("repo.settings.change_team_access_not_allowed"))
-		ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration")
+	if !canChangeRepoTeamAccess(ctx) {
 		return
 	}
 
@@ -201,9 +206,7 @@ func AddTeamPost(ctx *context.Context) {
 
 // DeleteTeam response for deleting a team from a repository
 func DeleteTeam(ctx *context.Context) {
-	if !ctx.Repo.Owner.RepoAdminChangeTeamAccess && !ctx.Repo.Permission.IsOwner() {
-		ctx.Flash.Error(ctx.Tr("repo.settings.change_team_access_not_allowed"))
-		ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration")
+	if !canChangeRepoTeamAccess(ctx) {
 		return
 	}
 
@@ -221,3 +224,16 @@ func DeleteTeam(ctx *context.Context) {
 	ctx.Flash.Success(ctx.Tr("repo.settings.remove_team_success"))
 	ctx.JSONRedirect(ctx.Repo.RepoLink + "/settings/collaboration")
 }
+
+func canChangeRepoTeamAccess(ctx *context.Context) bool {
+	canChange, err := organization.OrgFromUser(ctx.Repo.Owner).CanChangeRepoTeamAccess(ctx, ctx.Doer)
+	if err != nil {
+		ctx.ServerError("CanChangeRepoTeamAccess", err)
+		return false
+	}
+	if !canChange {
+		ctx.Flash.Error(ctx.Tr("repo.settings.change_team_access_not_allowed"))
+		ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration")
+	}
+	return canChange
+}

routers/web/repo/setting/settings_test.go

@@ -213,40 +213,27 @@ func TestAddTeamPost(t *testing.T) {
 
 func TestAddTeamPost_NotAllowed(t *testing.T) {
 	unittest.PrepareTestEnv(t)
-	ctx, _ := contexttest.MockContext(t, "org26/repo43")
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 32})
+	require.NoError(t, repo.LoadOwner(t.Context()))
+	adminTeam := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 12})
+	targetTeam := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2})
+	require.NoError(t, repo_service.TeamAddRepository(t.Context(), adminTeam, repo))
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 28})
+	repoContext := &context.Repository{Owner: repo.Owner, Repository: repo}
+	renderCtx, _ := contexttest.MockContext(t, repo.Link()+"/settings/collaboration")
+	renderCtx.Repo = repoContext
+	renderCtx.Doer = doer
+	Collaboration(renderCtx)
+	assert.Equal(t, false, renderCtx.Data["CanChangeRepoTeamAccess"])
 
-	ctx.Req.Form.Set("team", "team11")
-
-	org := &user_model.User{
-		LowerName: "org26",
-		Type:      user_model.UserTypeOrganization,
-	}
-
-	team := &organization.Team{
-		ID:    11,
-		OrgID: 26,
-	}
-
-	re := &repo_model.Repository{
-		ID:      43,
-		Owner:   org,
-		OwnerID: 26,
-	}
-
-	repo := &context.Repository{
-		Owner: &user_model.User{
-			ID:                        26,
-			LowerName:                 "org26",
-			RepoAdminChangeTeamAccess: false,
-		},
-		Repository: re,
-	}
-
-	ctx.Repo = repo
+	ctx, _ := contexttest.MockContext(t, repo.Link()+"/settings/collaboration")
+	ctx.Req.Form.Set("team", targetTeam.Name)
+	ctx.Repo = repoContext
+	ctx.Doer = doer
 
 	AddTeamPost(ctx)
 
-	assert.False(t, repo_service.HasRepository(t.Context(), team, re.ID))
+	assert.False(t, repo_service.HasRepository(t.Context(), targetTeam, repo.ID))
 	assert.Equal(t, http.StatusSeeOther, ctx.Resp.WrittenStatus())
 	assert.NotEmpty(t, ctx.Flash.ErrorMsg)
 }

templates/repo/settings/collaboration.tmpl

@@ -51,7 +51,7 @@
 			<h4 class="ui top attached header">
 				{{ctx.Locale.Tr "repo.settings.teams"}}
 			</h4>
-			{{$allowedToChangeTeams := (or (.Org.RepoAdminChangeTeamAccess) (.Permission.IsOwner))}}
+			{{$allowedToChangeTeams := .CanChangeRepoTeamAccess}}
 			{{if .Teams}}
 			<div class="ui attached segment">
 				<div class="flex-divided-list items-with-main">

templates/swagger/v1-openapi3.generated.json

@@ -32319,6 +32319,9 @@
           "204": {
             "$ref": "#/components/responses/empty"
           },
+          "403": {
+            "$ref": "#/components/responses/forbidden"
+          },
           "404": {
             "$ref": "#/components/responses/notFound"
           },
@@ -32416,6 +32419,9 @@
           "204": {
             "$ref": "#/components/responses/empty"
           },
+          "403": {
+            "$ref": "#/components/responses/forbidden"
+          },
           "404": {
             "$ref": "#/components/responses/notFound"
           },

templates/swagger/v1-swagger.generated.json

@@ -19773,6 +19773,9 @@
           "204": {
             "$ref": "#/responses/empty"
           },
+          "403": {
+            "$ref": "#/responses/forbidden"
+          },
           "404": {
             "$ref": "#/responses/notFound"
           },
@@ -19820,6 +19823,9 @@
           "204": {
             "$ref": "#/responses/empty"
           },
+          "403": {
+            "$ref": "#/responses/forbidden"
+          },
           "404": {
             "$ref": "#/responses/notFound"
           },

tests/integration/api_repo_teams_test.go

@@ -9,10 +9,12 @@ import (
 	"testing"
 
 	auth_model "gitea.dev/models/auth"
+	"gitea.dev/models/organization"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	api "gitea.dev/modules/structs"
+	repo_service "gitea.dev/services/repository"
 	"gitea.dev/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -61,6 +63,19 @@ func TestAPIRepoTeams(t *testing.T) {
 		AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusForbidden)
 
+	adminTeam := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 12})
+	targetTeam := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2})
+	existingTeam := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 7})
+	assert.NoError(t, repo_service.TeamAddRepository(t.Context(), adminTeam, publicOrgRepo))
+	user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 28})
+	token = getUserToken(t, user.Name, auth_model.AccessTokenScopeWriteRepository)
+	req = NewRequest(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/teams/%s", publicOrgRepo.FullName(), targetTeam.Name)).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusForbidden)
+	assert.False(t, repo_service.HasRepository(t.Context(), targetTeam, publicOrgRepo.ID))
+	req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/repos/%s/teams/%s", publicOrgRepo.FullName(), existingTeam.Name)).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusForbidden)
+	assert.True(t, repo_service.HasRepository(t.Context(), existingTeam, publicOrgRepo.ID))
+
 	// AddTeam with user2
 	user = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 	session = loginUser(t, user.Name)

tests/integration/api_team_test.go

@@ -275,6 +275,15 @@ func TestAPIAddRemoveTeamRepositoryRequiresOrgOwnerOrSetting(t *testing.T) {
 	req = NewRequest(t, "DELETE", url).AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusForbidden)
 	unittest.AssertExistsAndLoadBean(t, &organization.TeamRepo{TeamID: team.ID, RepoID: targetRepo.ID})
+
+	siteAdmin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+	token = getUserToken(t, siteAdmin.Name, auth_model.AccessTokenScopeWriteOrganization)
+	req = NewRequest(t, "DELETE", url).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusNoContent)
+	unittest.AssertNotExistsBean(t, &organization.TeamRepo{TeamID: team.ID, RepoID: targetRepo.ID})
+	req = NewRequest(t, "PUT", url).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusNoContent)
+	unittest.AssertExistsAndLoadBean(t, &organization.TeamRepo{TeamID: team.ID, RepoID: targetRepo.ID})
 }
 
 func TestAPITeamVisibilityAccess(t *testing.T) {