Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add team option to grant rights for all organization repositories #6791

Closed
wants to merge 13 commits into from
Closed
40 changes: 24 additions & 16 deletions integrations/api_team_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,55 +55,63 @@ func TestAPITeam(t *testing.T) {

// Create team.
teamToCreate := &api.CreateTeamOption{
Name: "team1",
Description: "team one",
Permission: "write",
Units: []string{"repo.code", "repo.issues"},
Name: "team1",
Description: "team one",
IsAllRepositories: true,
Permission: "write",
Units: []string{"repo.code", "repo.issues"},
}
req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/orgs/%s/teams?token=%s", org.Name, token), teamToCreate)
resp = session.MakeRequest(t, req, http.StatusCreated)
DecodeJSON(t, resp, &apiTeam)
checkTeamResponse(t, &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.Permission, teamToCreate.Units)
checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.Permission, teamToCreate.Units)
checkTeamResponse(t, &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IsAllRepositories,
teamToCreate.Permission, teamToCreate.Units)
checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IsAllRepositories,
teamToCreate.Permission, teamToCreate.Units)
teamID := apiTeam.ID

// Edit team.
teamToEdit := &api.EditTeamOption{
Name: "teamone",
Description: "team 1",
Permission: "admin",
Units: []string{"repo.code", "repo.pulls", "repo.releases"},
Name: "teamone",
Description: "team 1",
IsAllRepositories: false,
Permission: "admin",
Units: []string{"repo.code", "repo.pulls", "repo.releases"},
}
req = NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/teams/%d?token=%s", teamID, token), teamToEdit)
resp = session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiTeam)
checkTeamResponse(t, &apiTeam, teamToEdit.Name, teamToEdit.Description, teamToEdit.Permission, teamToEdit.Units)
checkTeamBean(t, apiTeam.ID, teamToEdit.Name, teamToEdit.Description, teamToEdit.Permission, teamToEdit.Units)
checkTeamResponse(t, &apiTeam, teamToEdit.Name, teamToEdit.Description, teamToEdit.IsAllRepositories,
teamToEdit.Permission, teamToEdit.Units)
checkTeamBean(t, apiTeam.ID, teamToEdit.Name, teamToEdit.Description, teamToEdit.IsAllRepositories,
teamToEdit.Permission, teamToEdit.Units)

// Read team.
teamRead := models.AssertExistsAndLoadBean(t, &models.Team{ID: teamID}).(*models.Team)
req = NewRequestf(t, "GET", "/api/v1/teams/%d?token="+token, teamID)
resp = session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiTeam)
checkTeamResponse(t, &apiTeam, teamRead.Name, teamRead.Description, teamRead.Authorize.String(), teamRead.GetUnitNames())
checkTeamResponse(t, &apiTeam, teamRead.Name, teamRead.Description, teamRead.IsAllRepositories,
teamRead.Authorize.String(), teamRead.GetUnitNames())

// Delete team.
req = NewRequestf(t, "DELETE", "/api/v1/teams/%d?token="+token, teamID)
session.MakeRequest(t, req, http.StatusNoContent)
models.AssertNotExistsBean(t, &models.Team{ID: teamID})
}

func checkTeamResponse(t *testing.T, apiTeam *api.Team, name, description string, permission string, units []string) {
func checkTeamResponse(t *testing.T, apiTeam *api.Team, name, description string, isAllRepositories bool, permission string, units []string) {
assert.Equal(t, name, apiTeam.Name, "name")
assert.Equal(t, description, apiTeam.Description, "description")
assert.Equal(t, isAllRepositories, apiTeam.IsAllRepositories, "isAllRepositories")
assert.Equal(t, permission, apiTeam.Permission, "permission")
sort.StringSlice(units).Sort()
sort.StringSlice(apiTeam.Units).Sort()
assert.EqualValues(t, units, apiTeam.Units, "units")
}

func checkTeamBean(t *testing.T, id int64, name, description string, permission string, units []string) {
func checkTeamBean(t *testing.T, id int64, name, description string, isAllRepositories bool, permission string, units []string) {
team := models.AssertExistsAndLoadBean(t, &models.Team{ID: id}).(*models.Team)
assert.NoError(t, team.GetUnits(), "GetUnits")
checkTeamResponse(t, convert.ToTeam(team), name, description, permission, units)
checkTeamResponse(t, convert.ToTeam(team), name, description, isAllRepositories, permission, units)
}
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ var migrations = []Migration{
NewMigration("add uploader id for table attachment", addUploaderIDForAttachment),
// v84 -> v85
NewMigration("add table to store original imported gpg keys", addGPGKeyImport),
// v85 -> v86
NewMigration("add is_all_repositories to teams", addTeamIsAllRepositories),
}

// Migrate database to current version
Expand Down
25 changes: 25 additions & 0 deletions models/migrations/v85.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package migrations

import (
"github.com/go-xorm/xorm"
)

func addTeamIsAllRepositories(x *xorm.Engine) error {

type Team struct {
ID int64 `xorm:"pk autoincr"`
IsAllRepositories bool `xorm:"NOT NULL DEFAULT false"`
ngourdon marked this conversation as resolved.
Show resolved Hide resolved
}

if err := x.Sync2(new(Team)); err != nil {
return err
}

_, err := x.Exec("UPDATE team SET is_all_repositories = ? WHERE name=?",
ngourdon marked this conversation as resolved.
Show resolved Hide resolved
true, "Owners")
return err
}
12 changes: 7 additions & 5 deletions models/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func (org *User) GetOwnerTeam() (*Team, error) {
}

func (org *User) getTeams(e Engine) error {
org.Teams = nil
return e.
Where("org_id=?", org.ID).
OrderBy("CASE WHEN name LIKE '" + ownerTeamName + "' THEN '' ELSE name END").
Expand Down Expand Up @@ -151,11 +152,12 @@ func CreateOrganization(org, owner *User) (err error) {

// Create default owner team.
t := &Team{
OrgID: org.ID,
LowerName: strings.ToLower(ownerTeamName),
Name: ownerTeamName,
Authorize: AccessModeOwner,
NumMembers: 1,
OrgID: org.ID,
LowerName: strings.ToLower(ownerTeamName),
Name: ownerTeamName,
Authorize: AccessModeOwner,
NumMembers: 1,
IsAllRepositories: true,
}
if _, err = sess.Insert(t); err != nil {
return fmt.Errorf("insert owner team: %v", err)
Expand Down
62 changes: 51 additions & 11 deletions models/org_team.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@ const ownerTeamName = "Owners"

// Team represents a organization team.
type Team struct {
ID int64 `xorm:"pk autoincr"`
OrgID int64 `xorm:"INDEX"`
LowerName string
Name string
Description string
Authorize AccessMode
Repos []*Repository `xorm:"-"`
Members []*User `xorm:"-"`
NumRepos int
NumMembers int
Units []*TeamUnit `xorm:"-"`
ID int64 `xorm:"pk autoincr"`
OrgID int64 `xorm:"INDEX"`
LowerName string
Name string
Description string
Authorize AccessMode
Repos []*Repository `xorm:"-"`
Members []*User `xorm:"-"`
NumRepos int
NumMembers int
Units []*TeamUnit `xorm:"-"`
IsAllRepositories bool `xorm:"NOT NULL DEFAULT false"`
}

// ColorFormat provides a basic color format for a Team
Expand Down Expand Up @@ -87,6 +88,7 @@ func (t *Team) IsMember(userID int64) bool {
}

func (t *Team) getRepositories(e Engine) error {
t.Repos = nil
ngourdon marked this conversation as resolved.
Show resolved Hide resolved
return e.Join("INNER", "team_repo", "repository.id = team_repo.repo_id").
Where("team_repo.team_id=?", t.ID).
OrderBy("repository.name").
Expand Down Expand Up @@ -158,6 +160,24 @@ func (t *Team) addRepository(e Engine, repo *Repository) (err error) {
return nil
}

// addAllRepositories adds all repositories to the team.
// If the team already has some repositories they will be left unchanged.
func (t *Team) addAllRepositories(e Engine) error {
err := e.Iterate(&Repository{OwnerID: t.OrgID}, func(i int, bean interface{}) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just change includesAllRepository to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All repositories need to be added to the team because access rights management won't work without them.
It is the same behavior as for owners teams.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. And I think we should use a better method to batch insert a slice of team_repo.

Copy link
Contributor Author

@ngourdon ngourdon Jul 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can use a better method to batch insert a slice of team_repo but the method addRepository do a lot more than that.
imho I don't think the batch insert will be worth it compared to the whole processing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume there are 1000 repositories on this organization. And the code here is not work with sqlite database.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still return 500 on my local test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the issue. I must do it wrong.
I build with TAGS="bindata sqlite sqlite_unlock_notify" make clean generate build
I tested on Linux and Windows.
Do the UT pass on your local machine?
Can you please give me the steps which lead to the error 500?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I compiled locally. I'm on macOS. Just grant team to all organization repositories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UT must fail, don't they?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problematic in pgSQL as well (500). After some debugging I found hasTeamRepo get this error: lib/pq#635

repo := bean.(*Repository)
if !t.hasRepository(e, repo.ID) {
if err := t.addRepository(e, repo); err != nil {
return fmt.Errorf("addRepository: %v", err)
}
}
return nil
})
if err != nil {
return fmt.Errorf("Iterate organization repositories: %v", err)
}
return nil
}

// AddRepository adds new repository to team of organization.
func (t *Team) AddRepository(repo *Repository) (err error) {
if repo.OwnerID != t.OrgID {
Expand Down Expand Up @@ -227,6 +247,10 @@ func (t *Team) RemoveRepository(repoID int64) error {
return nil
}

if t.IsAllRepositories {
return nil
}

repo, err := GetRepositoryByID(repoID)
if err != nil {
return err
Expand Down Expand Up @@ -324,6 +348,14 @@ func NewTeam(t *Team) (err error) {
}
}

// Add all repositories to the team if it has access to all of them.
if t.IsAllRepositories {
err = t.addAllRepositories(sess)
ngourdon marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("addAllRepositories: %v", err)
}
}

// Update organization number of teams.
if _, err = sess.Exec("UPDATE `user` SET num_teams=num_teams+1 WHERE id = ?", t.OrgID); err != nil {
sess.Rollback()
Expand Down Expand Up @@ -430,6 +462,14 @@ func UpdateTeam(t *Team, authChanged bool) (err error) {
}
}

// Add all repositories to the team if it has access to all of them.
if t.IsAllRepositories {
err = t.addAllRepositories(sess)
if err != nil {
return fmt.Errorf("addAllRepositories: %v", err)
}
}

lafriks marked this conversation as resolved.
Show resolved Hide resolved
return sess.Commit()
}

Expand Down
115 changes: 115 additions & 0 deletions models/org_team_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@
package models

import (
"fmt"
"strings"
"testing"

"code.gitea.io/gitea/modules/structs"

"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -374,3 +377,115 @@ func TestUsersInTeamsCount(t *testing.T) {
test([]int64{1, 2, 3, 4, 5}, []int64{2, 5}, 2)
test([]int64{1, 2, 3, 4, 5}, []int64{2, 3, 5}, 3)
}

func TestAllRepositoriesTeams(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

// Get an admin user.
user, err := GetUserByID(1)
assert.NoError(t, err, "GetUserByID")

// Create org.
org := &User{
Name: "All repo",
IsActive: true,
Type: UserTypeOrganization,
Visibility: structs.VisibleTypePublic,
}
assert.NoError(t, CreateOrganization(org, user), "CreateOrganization")

// Check Owner team.
ownerTeam, err := org.GetOwnerTeam()
assert.NoError(t, err, "GetOwnerTeam")
assert.True(t, ownerTeam.IsAllRepositories, "Owner team is all repositories")

// Create repos.
repoIds := make([]int64, 0)
for i := 1; i <= 3; i++ {
r, err := CreateRepository(user, org, CreateRepoOptions{Name: fmt.Sprintf("repo-%d", i)})
assert.NoError(t, err, "CreateRepository %d", i)
if r != nil {
repoIds = append(repoIds, r.ID)
}
}

// Create teams and check repo count.
teams := []*Team{
ownerTeam,
{
OrgID: org.ID,
Name: "team one",
Authorize: AccessModeRead,
IsAllRepositories: true,
},
{
OrgID: org.ID,
Name: "team 2",
Authorize: AccessModeRead,
IsAllRepositories: false,
},
{
OrgID: org.ID,
Name: "team three",
Authorize: AccessModeWrite,
IsAllRepositories: true,
},
{
OrgID: org.ID,
Name: "team 4",
Authorize: AccessModeWrite,
IsAllRepositories: false,
},
}
repoCounts := []int{3, 3, 0, 3, 0}
for i, team := range teams {
if i > 0 { // first team is Owner.
assert.NoError(t, NewTeam(team), "team %d: NewTeam", i)
}
assert.NoError(t, team.GetRepositories(), "team %d: GetRepositories", i)
assert.Equal(t, repoCounts[i], len(team.Repos), "team %d: repo count", i)
}

// Update teams and check repo count.
teams[3].IsAllRepositories = false
teams[4].IsAllRepositories = true
repoCounts[4] = 3
for i, team := range teams {
assert.NoError(t, UpdateTeam(team, false), "team %d: UpdateTeam", i)
assert.NoError(t, team.GetRepositories(), "team %d: GetRepositories", i)
assert.Equal(t, repoCounts[i], len(team.Repos), "team %d: repo count", i)
}

// Create repo and check teams repo count.
r, err := CreateRepository(user, org, CreateRepoOptions{Name: "repo-last"})
assert.NoError(t, err, "CreateRepository last")
if r != nil {
repoIds = append(repoIds, r.ID)
}
repoCounts[0] = 4
repoCounts[1] = 4
repoCounts[4] = 4
for i, team := range teams {
assert.NoError(t, team.GetRepositories(), "team %d: GetRepositories", i)
assert.Equal(t, repoCounts[i], len(team.Repos), "team %d: repo count", i)
}

// Remove repo and check teams repo count.
assert.NoError(t, DeleteRepository(user, org.ID, repoIds[0]), "DeleteRepository")
repoCounts[0] = 3
repoCounts[1] = 3
repoCounts[3] = 2
repoCounts[4] = 3
for i, team := range teams {
assert.NoError(t, team.GetRepositories(), "team %d: GetRepositories", i)
assert.Equal(t, repoCounts[i], len(team.Repos), "team %d: repo count", i)
}

// Wipe created items.
for i, rid := range repoIds {
if i > 0 { // first repo already deleted.
assert.NoError(t, DeleteRepository(user, org.ID, rid), "DeleteRepository %d", i)
}
}
assert.NoError(t, DeleteOrganization(org), "DeleteOrganization")
}
Loading