Skip to content

Commit 4f879a0

Browse files
lunnywxiaoguang
lunny
and
wxiaoguang
authored
Refactor find forks and fix possible bugs that weak permissions check (#32528)
- Move models/GetForks to services/FindForks - Add doer as a parameter of FindForks to check permissions - Slight performance optimization for get forks API with batch loading of repository units - Add tests for forking repository to organizations --------- Co-authored-by: wxiaoguang <[email protected]>
1 parent f122aaf commit 4f879a0

File tree

8 files changed

+202
-41
lines changed

8 files changed

+202
-41
lines changed

‎models/repo/fork.go

-15
Original file line numberDiff line numberDiff line change
@@ -54,21 +54,6 @@ func GetUserFork(ctx context.Context, repoID, userID int64) (*Repository, error)
5454
return &forkedRepo, nil
5555
}
5656

57-
// GetForks returns all the forks of the repository
58-
func GetForks(ctx context.Context, repo *Repository, listOptions db.ListOptions) ([]*Repository, error) {
59-
sess := db.GetEngine(ctx)
60-
61-
var forks []*Repository
62-
if listOptions.Page == 0 {
63-
forks = make([]*Repository, 0, repo.NumForks)
64-
} else {
65-
forks = make([]*Repository, 0, listOptions.PageSize)
66-
sess = db.SetSessionPagination(sess, &listOptions)
67-
}
68-
69-
return forks, sess.Find(&forks, &Repository{ForkID: repo.ID})
70-
}
71-
7257
// IncrementRepoForkNum increment repository fork number
7358
func IncrementRepoForkNum(ctx context.Context, repoID int64) error {
7459
_, err := db.GetEngine(ctx).Exec("UPDATE `repository` SET num_forks=num_forks+1 WHERE id=?", repoID)

‎models/repo/repo_list.go

+18-8
Original file line numberDiff line numberDiff line change
@@ -98,19 +98,14 @@ func (repos RepositoryList) IDs() []int64 {
9898
return repoIDs
9999
}
100100

101-
// LoadAttributes loads the attributes for the given RepositoryList
102-
func (repos RepositoryList) LoadAttributes(ctx context.Context) error {
101+
func (repos RepositoryList) LoadOwners(ctx context.Context) error {
103102
if len(repos) == 0 {
104103
return nil
105104
}
106105

107106
userIDs := container.FilterSlice(repos, func(repo *Repository) (int64, bool) {
108107
return repo.OwnerID, true
109108
})
110-
repoIDs := make([]int64, len(repos))
111-
for i := range repos {
112-
repoIDs[i] = repos[i].ID
113-
}
114109

115110
// Load owners.
116111
users := make(map[int64]*user_model.User, len(userIDs))
@@ -123,12 +118,19 @@ func (repos RepositoryList) LoadAttributes(ctx context.Context) error {
123118
for i := range repos {
124119
repos[i].Owner = users[repos[i].OwnerID]
125120
}
121+
return nil
122+
}
123+
124+
func (repos RepositoryList) LoadLanguageStats(ctx context.Context) error {
125+
if len(repos) == 0 {
126+
return nil
127+
}
126128

127129
// Load primary language.
128130
stats := make(LanguageStatList, 0, len(repos))
129131
if err := db.GetEngine(ctx).
130132
Where("`is_primary` = ? AND `language` != ?", true, "other").
131-
In("`repo_id`", repoIDs).
133+
In("`repo_id`", repos.IDs()).
132134
Find(&stats); err != nil {
133135
return fmt.Errorf("find primary languages: %w", err)
134136
}
@@ -141,10 +143,18 @@ func (repos RepositoryList) LoadAttributes(ctx context.Context) error {
141143
}
142144
}
143145
}
144-
145146
return nil
146147
}
147148

149+
// LoadAttributes loads the attributes for the given RepositoryList
150+
func (repos RepositoryList) LoadAttributes(ctx context.Context) error {
151+
if err := repos.LoadOwners(ctx); err != nil {
152+
return err
153+
}
154+
155+
return repos.LoadLanguageStats(ctx)
156+
}
157+
148158
// SearchRepoOptions holds the search options
149159
type SearchRepoOptions struct {
150160
db.ListOptions

‎routers/api/v1/repo/fork.go

+12-3
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,20 @@ func ListForks(ctx *context.APIContext) {
5555
// "404":
5656
// "$ref": "#/responses/notFound"
5757

58-
forks, err := repo_model.GetForks(ctx, ctx.Repo.Repository, utils.GetListOptions(ctx))
58+
forks, total, err := repo_service.FindForks(ctx, ctx.Repo.Repository, ctx.Doer, utils.GetListOptions(ctx))
5959
if err != nil {
60-
ctx.Error(http.StatusInternalServerError, "GetForks", err)
60+
ctx.Error(http.StatusInternalServerError, "FindForks", err)
6161
return
6262
}
63+
if err := repo_model.RepositoryList(forks).LoadOwners(ctx); err != nil {
64+
ctx.Error(http.StatusInternalServerError, "LoadOwners", err)
65+
return
66+
}
67+
if err := repo_model.RepositoryList(forks).LoadUnits(ctx); err != nil {
68+
ctx.Error(http.StatusInternalServerError, "LoadUnits", err)
69+
return
70+
}
71+
6372
apiForks := make([]*api.Repository, len(forks))
6473
for i, fork := range forks {
6574
permission, err := access_model.GetUserRepoPermission(ctx, fork, ctx.Doer)
@@ -70,7 +79,7 @@ func ListForks(ctx *context.APIContext) {
7079
apiForks[i] = convert.ToRepo(ctx, fork, permission)
7180
}
7281

73-
ctx.SetTotalCountHeader(int64(ctx.Repo.Repository.NumForks))
82+
ctx.SetTotalCountHeader(total)
7483
ctx.JSON(http.StatusOK, apiForks)
7584
}
7685

‎routers/web/repo/view.go

+11-12
Original file line numberDiff line numberDiff line change
@@ -1151,26 +1151,25 @@ func Forks(ctx *context.Context) {
11511151
if page <= 0 {
11521152
page = 1
11531153
}
1154+
pageSize := setting.ItemsPerPage
11541155

1155-
pager := context.NewPagination(ctx.Repo.Repository.NumForks, setting.ItemsPerPage, page, 5)
1156-
ctx.Data["Page"] = pager
1157-
1158-
forks, err := repo_model.GetForks(ctx, ctx.Repo.Repository, db.ListOptions{
1159-
Page: pager.Paginater.Current(),
1160-
PageSize: setting.ItemsPerPage,
1156+
forks, total, err := repo_service.FindForks(ctx, ctx.Repo.Repository, ctx.Doer, db.ListOptions{
1157+
Page: page,
1158+
PageSize: pageSize,
11611159
})
11621160
if err != nil {
1163-
ctx.ServerError("GetForks", err)
1161+
ctx.ServerError("FindForks", err)
11641162
return
11651163
}
11661164

1167-
for _, fork := range forks {
1168-
if err = fork.LoadOwner(ctx); err != nil {
1169-
ctx.ServerError("LoadOwner", err)
1170-
return
1171-
}
1165+
if err := repo_model.RepositoryList(forks).LoadOwners(ctx); err != nil {
1166+
ctx.ServerError("LoadAttributes", err)
1167+
return
11721168
}
11731169

1170+
pager := context.NewPagination(int(total), pageSize, page, 5)
1171+
ctx.Data["Page"] = pager
1172+
11741173
ctx.Data["Forks"] = forks
11751174

11761175
ctx.HTML(http.StatusOK, tplForks)

‎services/repository/fork.go

+24
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"code.gitea.io/gitea/models/db"
1313
git_model "code.gitea.io/gitea/models/git"
1414
repo_model "code.gitea.io/gitea/models/repo"
15+
"code.gitea.io/gitea/models/unit"
1516
user_model "code.gitea.io/gitea/models/user"
1617
"code.gitea.io/gitea/modules/git"
1718
"code.gitea.io/gitea/modules/gitrepo"
@@ -20,6 +21,8 @@ import (
2021
"code.gitea.io/gitea/modules/structs"
2122
"code.gitea.io/gitea/modules/util"
2223
notify_service "code.gitea.io/gitea/services/notify"
24+
25+
"xorm.io/builder"
2326
)
2427

2528
// ErrForkAlreadyExist represents a "ForkAlreadyExist" kind of error.
@@ -247,3 +250,24 @@ func ConvertForkToNormalRepository(ctx context.Context, repo *repo_model.Reposit
247250

248251
return err
249252
}
253+
254+
type findForksOptions struct {
255+
db.ListOptions
256+
RepoID int64
257+
Doer *user_model.User
258+
}
259+
260+
func (opts findForksOptions) ToConds() builder.Cond {
261+
return builder.Eq{"fork_id": opts.RepoID}.And(
262+
repo_model.AccessibleRepositoryCondition(opts.Doer, unit.TypeInvalid),
263+
)
264+
}
265+
266+
// FindForks returns all the forks of the repository
267+
func FindForks(ctx context.Context, repo *repo_model.Repository, doer *user_model.User, listOptions db.ListOptions) ([]*repo_model.Repository, int64, error) {
268+
return db.FindAndCount[repo_model.Repository](ctx, findForksOptions{
269+
ListOptions: listOptions,
270+
RepoID: repo.ID,
271+
Doer: doer,
272+
})
273+
}

‎templates/repo/forks.tmpl

+5-3
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@
55
<h2 class="ui dividing header">
66
{{ctx.Locale.Tr "repo.forks"}}
77
</h2>
8+
<div class="flex-list">
89
{{range .Forks}}
9-
<div class="tw-flex tw-items-center tw-py-2">
10-
<span class="tw-mr-1">{{ctx.AvatarUtils.Avatar .Owner}}</span>
11-
<a href="{{.Owner.HomeLink}}">{{.Owner.Name}}</a> / <a href="{{.Link}}">{{.Name}}</a>
10+
<div class="flex-item tw-border-0 repo-fork-item">
11+
<span>{{ctx.AvatarUtils.Avatar .Owner}}</span>
12+
<span><a href="{{.Owner.HomeLink}}">{{.Owner.Name}}</a> / <a href="{{.Link}}">{{.Name}}</a></span>
1213
</div>
1314
{{end}}
15+
</div>
1416
</div>
1517

1618
{{template "base/paginate" .}}

‎tests/integration/api_fork_test.go

+80
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,92 @@ import (
77
"net/http"
88
"testing"
99

10+
"code.gitea.io/gitea/models"
11+
auth_model "code.gitea.io/gitea/models/auth"
12+
"code.gitea.io/gitea/models/db"
13+
org_model "code.gitea.io/gitea/models/organization"
14+
"code.gitea.io/gitea/models/unittest"
15+
user_model "code.gitea.io/gitea/models/user"
1016
api "code.gitea.io/gitea/modules/structs"
1117
"code.gitea.io/gitea/tests"
18+
19+
"github.com/stretchr/testify/assert"
1220
)
1321

1422
func TestCreateForkNoLogin(t *testing.T) {
1523
defer tests.PrepareTestEnv(t)()
1624
req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/forks", &api.CreateForkOption{})
1725
MakeRequest(t, req, http.StatusUnauthorized)
1826
}
27+
28+
func TestAPIForkListLimitedAndPrivateRepos(t *testing.T) {
29+
defer tests.PrepareTestEnv(t)()
30+
31+
user1Sess := loginUser(t, "user1")
32+
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user1"})
33+
34+
// fork into a limited org
35+
limitedOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 22})
36+
assert.EqualValues(t, api.VisibleTypeLimited, limitedOrg.Visibility)
37+
38+
ownerTeam1, err := org_model.OrgFromUser(limitedOrg).GetOwnerTeam(db.DefaultContext)
39+
assert.NoError(t, err)
40+
assert.NoError(t, models.AddTeamMember(db.DefaultContext, ownerTeam1, user1))
41+
user1Token := getTokenForLoggedInUser(t, user1Sess, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteOrganization)
42+
req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/forks", &api.CreateForkOption{
43+
Organization: &limitedOrg.Name,
44+
}).AddTokenAuth(user1Token)
45+
MakeRequest(t, req, http.StatusAccepted)
46+
47+
// fork into a private org
48+
user4Sess := loginUser(t, "user4")
49+
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user4"})
50+
privateOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 23})
51+
assert.EqualValues(t, api.VisibleTypePrivate, privateOrg.Visibility)
52+
53+
ownerTeam2, err := org_model.OrgFromUser(privateOrg).GetOwnerTeam(db.DefaultContext)
54+
assert.NoError(t, err)
55+
assert.NoError(t, models.AddTeamMember(db.DefaultContext, ownerTeam2, user4))
56+
user4Token := getTokenForLoggedInUser(t, user4Sess, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteOrganization)
57+
req = NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/forks", &api.CreateForkOption{
58+
Organization: &privateOrg.Name,
59+
}).AddTokenAuth(user4Token)
60+
MakeRequest(t, req, http.StatusAccepted)
61+
62+
t.Run("Anonymous", func(t *testing.T) {
63+
defer tests.PrintCurrentTest(t)()
64+
65+
req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks")
66+
resp := MakeRequest(t, req, http.StatusOK)
67+
68+
var forks []*api.Repository
69+
DecodeJSON(t, resp, &forks)
70+
71+
assert.Empty(t, forks)
72+
assert.EqualValues(t, "0", resp.Header().Get("X-Total-Count"))
73+
})
74+
75+
t.Run("Logged in", func(t *testing.T) {
76+
defer tests.PrintCurrentTest(t)()
77+
78+
req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks").AddTokenAuth(user1Token)
79+
resp := MakeRequest(t, req, http.StatusOK)
80+
81+
var forks []*api.Repository
82+
DecodeJSON(t, resp, &forks)
83+
84+
assert.Len(t, forks, 1)
85+
assert.EqualValues(t, "1", resp.Header().Get("X-Total-Count"))
86+
87+
assert.NoError(t, models.AddTeamMember(db.DefaultContext, ownerTeam2, user1))
88+
89+
req = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/forks").AddTokenAuth(user1Token)
90+
resp = MakeRequest(t, req, http.StatusOK)
91+
92+
forks = []*api.Repository{}
93+
DecodeJSON(t, resp, &forks)
94+
95+
assert.Len(t, forks, 2)
96+
assert.EqualValues(t, "2", resp.Header().Get("X-Total-Count"))
97+
})
98+
}

‎tests/integration/repo_fork_test.go

+52
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,12 @@ import (
99
"net/http/httptest"
1010
"testing"
1111

12+
"code.gitea.io/gitea/models"
13+
"code.gitea.io/gitea/models/db"
14+
org_model "code.gitea.io/gitea/models/organization"
1215
"code.gitea.io/gitea/models/unittest"
1316
user_model "code.gitea.io/gitea/models/user"
17+
"code.gitea.io/gitea/modules/structs"
1418
"code.gitea.io/gitea/tests"
1519

1620
"github.com/stretchr/testify/assert"
@@ -74,3 +78,51 @@ func TestRepoForkToOrg(t *testing.T) {
7478
_, exists := htmlDoc.doc.Find(`a.ui.button[href*="/fork"]`).Attr("href")
7579
assert.False(t, exists, "Forking should not be allowed anymore")
7680
}
81+
82+
func TestForkListLimitedAndPrivateRepos(t *testing.T) {
83+
defer tests.PrepareTestEnv(t)()
84+
forkItemSelector := ".repo-fork-item"
85+
86+
user1Sess := loginUser(t, "user1")
87+
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user1"})
88+
89+
// fork to a limited org
90+
limitedOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 22})
91+
assert.EqualValues(t, structs.VisibleTypeLimited, limitedOrg.Visibility)
92+
ownerTeam1, err := org_model.OrgFromUser(limitedOrg).GetOwnerTeam(db.DefaultContext)
93+
assert.NoError(t, err)
94+
assert.NoError(t, models.AddTeamMember(db.DefaultContext, ownerTeam1, user1))
95+
testRepoFork(t, user1Sess, "user2", "repo1", limitedOrg.Name, "repo1", "")
96+
97+
// fork to a private org
98+
user4Sess := loginUser(t, "user4")
99+
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user4"})
100+
privateOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 23})
101+
assert.EqualValues(t, structs.VisibleTypePrivate, privateOrg.Visibility)
102+
ownerTeam2, err := org_model.OrgFromUser(privateOrg).GetOwnerTeam(db.DefaultContext)
103+
assert.NoError(t, err)
104+
assert.NoError(t, models.AddTeamMember(db.DefaultContext, ownerTeam2, user4))
105+
testRepoFork(t, user4Sess, "user2", "repo1", privateOrg.Name, "repo1", "")
106+
107+
t.Run("Anonymous", func(t *testing.T) {
108+
defer tests.PrintCurrentTest(t)()
109+
req := NewRequest(t, "GET", "/user2/repo1/forks")
110+
resp := MakeRequest(t, req, http.StatusOK)
111+
htmlDoc := NewHTMLParser(t, resp.Body)
112+
assert.EqualValues(t, 0, htmlDoc.Find(forkItemSelector).Length())
113+
})
114+
115+
t.Run("Logged in", func(t *testing.T) {
116+
defer tests.PrintCurrentTest(t)()
117+
118+
req := NewRequest(t, "GET", "/user2/repo1/forks")
119+
resp := user1Sess.MakeRequest(t, req, http.StatusOK)
120+
htmlDoc := NewHTMLParser(t, resp.Body)
121+
assert.EqualValues(t, 1, htmlDoc.Find(forkItemSelector).Length())
122+
123+
assert.NoError(t, models.AddTeamMember(db.DefaultContext, ownerTeam2, user1))
124+
resp = user1Sess.MakeRequest(t, req, http.StatusOK)
125+
htmlDoc = NewHTMLParser(t, resp.Body)
126+
assert.EqualValues(t, 2, htmlDoc.Find(forkItemSelector).Length())
127+
})
128+
}

0 commit comments

Comments
 (0)