diff --git a/models/organization/org.go b/models/organization/org.go index 3314e3f65f0..7a4b327fde6 100644 --- a/models/organization/org.go +++ b/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) diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index 7b8e6937cb5..4782dfdb2a4 100644 --- a/routers/api/v1/org/team.go +++ b/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 diff --git a/routers/api/v1/repo/teams.go b/routers/api/v1/repo/teams.go index fc39b1c6166..afa8a3caa46 100644 --- a/routers/api/v1/repo/teams.go +++ b/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 { diff --git a/routers/web/repo/setting/collaboration.go b/routers/web/repo/setting/collaboration.go index 0be463ece8b..a1037dbb5fd 100644 --- a/routers/web/repo/setting/collaboration.go +++ b/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 +} diff --git a/routers/web/repo/setting/settings_test.go b/routers/web/repo/setting/settings_test.go index 12573c11ab3..bd165a89e3e 100644 --- a/routers/web/repo/setting/settings_test.go +++ b/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) } diff --git a/templates/repo/settings/collaboration.tmpl b/templates/repo/settings/collaboration.tmpl index 0324de644ee..62acc053e26 100644 --- a/templates/repo/settings/collaboration.tmpl +++ b/templates/repo/settings/collaboration.tmpl @@ -51,7 +51,7 @@