loading up the forgejo repo on tangled to test page performance
fix: consider public issues for project boards (#7143)
- The security patch of forgejo/forgejo#6843 fixed the issue where project boards loaded all issues without considering if the doer actually had permission to view that issue. Within that patch the call to `Issues` was modified to include this permission checking.
- The query being generated was not entirely correct. Issues in public repositories weren't considered correctly (partly the fault of not setting `AllPublic` unconditionally) in the cause an authenticated user loaded the project.
- This is now fixed by setting `AllPublic` unconditionally and subsequently fixing the `Issue` function to ensure that the combination of setting `AllPublic` and `User` generates the correct query, by combining the permission check and issues in public repositories as one `AND` query.
- Added unit testing.
- Added integration testing.
- Resolves Codeberg/Community#1809
- Regression of https://codeberg.org/forgejo/forgejo/pulls/6843
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7143
Reviewed-by: Otto <otto@codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>
+76
-27
+2
-2
models/fixtures/PrivateIssueProjects/project.yml
+2
-2
models/fixtures/PrivateIssueProjects/project.yml
···
1
1
-
2
2
id: 1001
3
-
title: Org project that contains private issues
3
+
title: Org project that contains private and public issues
4
4
owner_id: 3
5
5
repo_id: 0
6
6
is_closed: false
···
12
12
13
13
-
14
14
id: 1002
15
-
title: User project that contains private issues
15
+
title: User project that contains private and public issues
16
16
owner_id: 2
17
17
repo_id: 0
18
18
is_closed: false
+12
models/fixtures/PrivateIssueProjects/project_issue.yml
+12
models/fixtures/PrivateIssueProjects/project_issue.yml
+1
-2
models/issues/issue_project.go
+1
-2
models/issues/issue_project.go
···
56
56
ProjectID: b.ProjectID,
57
57
SortType: "project-column-sorting",
58
58
IsClosed: isClosed,
59
+
AllPublic: true,
59
60
}
60
61
if doer != nil {
61
62
issueOpts.User = doer
62
63
issueOpts.Org = org
63
-
} else {
64
-
issueOpts.AllPublic = true
65
64
}
66
65
67
66
issueList, err := Issues(ctx, issueOpts)
+29
-10
models/issues/issue_project_test.go
+29
-10
models/issues/issue_project_test.go
···
33
33
defer tests.PrintCurrentTest(t)()
34
34
issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, user2, org, optional.None[bool]())
35
35
require.NoError(t, err)
36
-
assert.Len(t, issueList, 1)
37
-
assert.EqualValues(t, 6, issueList[0].ID)
36
+
assert.Len(t, issueList, 2)
37
+
assert.EqualValues(t, 16, issueList[0].ID)
38
+
assert.EqualValues(t, 6, issueList[1].ID)
38
39
39
40
issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.None[bool]())
40
41
require.NoError(t, err)
41
-
assert.EqualValues(t, 1, issuesNum)
42
+
assert.EqualValues(t, 2, issuesNum)
42
43
43
44
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.Some(true))
44
45
require.NoError(t, err)
···
46
47
47
48
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.Some(false))
48
49
require.NoError(t, err)
49
-
assert.EqualValues(t, 1, issuesNum)
50
+
assert.EqualValues(t, 2, issuesNum)
50
51
})
51
52
52
53
t.Run("Anonymous user", func(t *testing.T) {
53
54
defer tests.PrintCurrentTest(t)()
54
55
issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, nil, org, optional.None[bool]())
55
56
require.NoError(t, err)
56
-
assert.Empty(t, issueList)
57
+
assert.Len(t, issueList, 1)
58
+
assert.EqualValues(t, 16, issueList[0].ID)
57
59
58
60
issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.None[bool]())
59
61
require.NoError(t, err)
62
+
assert.EqualValues(t, 1, issuesNum)
63
+
64
+
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.Some(true))
65
+
require.NoError(t, err)
60
66
assert.EqualValues(t, 0, issuesNum)
67
+
68
+
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.Some(false))
69
+
require.NoError(t, err)
70
+
assert.EqualValues(t, 1, issuesNum)
61
71
})
62
72
})
63
73
···
69
79
defer tests.PrintCurrentTest(t)()
70
80
issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, user2, nil, optional.None[bool]())
71
81
require.NoError(t, err)
72
-
assert.Len(t, issueList, 1)
82
+
assert.Len(t, issueList, 2)
73
83
assert.EqualValues(t, 7, issueList[0].ID)
84
+
assert.EqualValues(t, 1, issueList[1].ID)
74
85
75
86
issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.None[bool]())
76
87
require.NoError(t, err)
77
-
assert.EqualValues(t, 1, issuesNum)
88
+
assert.EqualValues(t, 2, issuesNum)
78
89
79
90
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.Some(true))
80
91
require.NoError(t, err)
···
82
93
83
94
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.Some(false))
84
95
require.NoError(t, err)
85
-
assert.EqualValues(t, 1, issuesNum)
96
+
assert.EqualValues(t, 2, issuesNum)
86
97
})
87
98
88
99
t.Run("Anonymous user", func(t *testing.T) {
89
100
defer tests.PrintCurrentTest(t)()
90
-
91
101
issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, nil, nil, optional.None[bool]())
92
102
require.NoError(t, err)
93
-
assert.Empty(t, issueList)
103
+
assert.Len(t, issueList, 1)
104
+
assert.EqualValues(t, 1, issueList[0].ID)
94
105
95
106
issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.None[bool]())
107
+
require.NoError(t, err)
108
+
assert.EqualValues(t, 1, issuesNum)
109
+
110
+
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.Some(true))
96
111
require.NoError(t, err)
97
112
assert.EqualValues(t, 0, issuesNum)
113
+
114
+
issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.Some(false))
115
+
require.NoError(t, err)
116
+
assert.EqualValues(t, 1, issuesNum)
98
117
})
99
118
})
100
119
}
+17
-5
models/issues/issue_search.go
+17
-5
models/issues/issue_search.go
···
49
49
// prioritize issues from this repo
50
50
PriorityRepoID int64
51
51
IsArchived optional.Option[bool]
52
-
Org *organization.Organization // issues permission scope
53
-
Team *organization.Team // issues permission scope
54
-
User *user_model.User // issues permission scope
52
+
53
+
// If combined with AllPublic, then private as well as public issues
54
+
// that matches the criteria will be returned, if AllPublic is false
55
+
// only the private issues will be returned.
56
+
Org *organization.Organization // issues permission scope
57
+
Team *organization.Team // issues permission scope
58
+
User *user_model.User // issues permission scope
55
59
}
56
60
57
61
// applySorts sort an issues-related session based on the provided
···
196
200
} else if len(opts.RepoIDs) > 1 {
197
201
opts.RepoCond = builder.In("issue.repo_id", opts.RepoIDs)
198
202
}
199
-
if opts.AllPublic {
203
+
// If permission scoping is set, then we set this condition at a later stage.
204
+
if opts.AllPublic && opts.User == nil {
200
205
if opts.RepoCond == nil {
201
206
opts.RepoCond = builder.NewCond()
202
207
}
···
268
273
applyLabelsCondition(sess, opts)
269
274
270
275
if opts.User != nil {
271
-
sess.And(issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.Value()))
276
+
cond := issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.Value())
277
+
// If AllPublic was set, then also consider all issues in public
278
+
// repositories in addition to the private repositories the user has access
279
+
// to.
280
+
if opts.AllPublic {
281
+
cond = cond.Or(builder.In("issue.repo_id", builder.Select("id").From("repository").Where(builder.Eq{"is_private": false})))
282
+
}
283
+
sess.And(cond)
272
284
}
273
285
}
274
286
+15
-8
tests/integration/private_project_test.go
+15
-8
tests/integration/private_project_test.go
···
24
24
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
25
25
sess := loginUser(t, user2.Name)
26
26
27
-
test := func(t *testing.T, sess *TestSession, username string, projectID int64, hasAccess bool) {
27
+
test := func(t *testing.T, sess *TestSession, username string, projectID int64, hasAccess bool, publicIssueHref ...string) {
28
28
t.Helper()
29
29
defer tests.PrintCurrentTest(t, 1)()
30
30
···
35
35
htmlDoc := NewHTMLParser(t, resp.Body)
36
36
openCloseStats := htmlDoc.Find(".milestone-toolbar .group").First().Text()
37
37
if hasAccess {
38
-
assert.Contains(t, openCloseStats, "1\u00a0Open")
38
+
assert.Contains(t, openCloseStats, "2\u00a0Open")
39
39
} else {
40
-
assert.Contains(t, openCloseStats, "0\u00a0Open")
40
+
assert.Contains(t, openCloseStats, "1\u00a0Open")
41
41
}
42
42
assert.Contains(t, openCloseStats, "0\u00a0Closed")
43
43
···
46
46
resp = sess.MakeRequest(t, req, http.StatusOK)
47
47
48
48
htmlDoc = NewHTMLParser(t, resp.Body)
49
-
htmlDoc.AssertElement(t, ".project-column .issue-card", hasAccess)
49
+
issueCardsLen := htmlDoc.Find(".project-column .issue-card").Length()
50
+
if hasAccess {
51
+
assert.EqualValues(t, 2, issueCardsLen)
52
+
} else {
53
+
assert.EqualValues(t, 1, issueCardsLen)
54
+
// Ensure that the public issue is shown.
55
+
assert.EqualValues(t, publicIssueHref[0], htmlDoc.Find(".project-column .issue-card .issue-card-title").AttrOr("href", ""))
56
+
}
50
57
51
58
// And that the issue count is correct.
52
59
issueCount := strings.TrimSpace(htmlDoc.Find(".project-column-issue-count").Text())
53
60
if hasAccess {
61
+
assert.EqualValues(t, "2", issueCount)
62
+
} else {
54
63
assert.EqualValues(t, "1", issueCount)
55
-
} else {
56
-
assert.EqualValues(t, "0", issueCount)
57
64
}
58
65
}
59
66
···
66
73
})
67
74
68
75
t.Run("Anonymous user", func(t *testing.T) {
69
-
test(t, emptyTestSession(t), org.Name, orgProject.ID, false)
76
+
test(t, emptyTestSession(t), org.Name, orgProject.ID, false, "/org3/repo21/issues/1")
70
77
})
71
78
})
72
79
···
78
85
})
79
86
80
87
t.Run("Anonymous user", func(t *testing.T) {
81
-
test(t, emptyTestSession(t), user2.Name, userProject.ID, false)
88
+
test(t, emptyTestSession(t), user2.Name, userProject.ID, false, "/user2/repo1/issues/1")
82
89
})
83
90
})
84
91
}