From a2958f5a26a0acf0ee5b0139a713df8f1dd5debe Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 6 Mar 2025 23:26:08 +0000 Subject: [PATCH] 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 Co-authored-by: Gusted Co-committed-by: Gusted --- .../fixtures/PrivateIssueProjects/project.yml | 4 +- .../PrivateIssueProjects/project_issue.yml | 12 ++++++ models/issues/issue_project.go | 3 +- models/issues/issue_project_test.go | 39 ++++++++++++++----- models/issues/issue_search.go | 22 ++++++++--- tests/integration/private_project_test.go | 23 +++++++---- 6 files changed, 76 insertions(+), 27 deletions(-) diff --git a/models/fixtures/PrivateIssueProjects/project.yml b/models/fixtures/PrivateIssueProjects/project.yml index cf63e4e413..8950b33606 100644 --- a/models/fixtures/PrivateIssueProjects/project.yml +++ b/models/fixtures/PrivateIssueProjects/project.yml @@ -1,6 +1,6 @@ - id: 1001 - title: Org project that contains private issues + title: Org project that contains private and public issues owner_id: 3 repo_id: 0 is_closed: false @@ -12,7 +12,7 @@ - id: 1002 - title: User project that contains private issues + title: User project that contains private and public issues owner_id: 2 repo_id: 0 is_closed: false diff --git a/models/fixtures/PrivateIssueProjects/project_issue.yml b/models/fixtures/PrivateIssueProjects/project_issue.yml index 222b2e5f71..0245fb47f3 100644 --- a/models/fixtures/PrivateIssueProjects/project_issue.yml +++ b/models/fixtures/PrivateIssueProjects/project_issue.yml @@ -9,3 +9,15 @@ issue_id: 7 project_id: 1002 project_board_id: 1002 + +- + id: 1003 + issue_id: 16 + project_id: 1001 + project_board_id: 1001 + +- + id: 1004 + issue_id: 1 + project_id: 1002 + project_board_id: 1002 diff --git a/models/issues/issue_project.go b/models/issues/issue_project.go index f606b713cf..697ef7fad6 100644 --- a/models/issues/issue_project.go +++ b/models/issues/issue_project.go @@ -56,12 +56,11 @@ func LoadIssuesFromColumn(ctx context.Context, b *project_model.Column, doer *us ProjectID: b.ProjectID, SortType: "project-column-sorting", IsClosed: isClosed, + AllPublic: true, } if doer != nil { issueOpts.User = doer issueOpts.Org = org - } else { - issueOpts.AllPublic = true } issueList, err := Issues(ctx, issueOpts) diff --git a/models/issues/issue_project_test.go b/models/issues/issue_project_test.go index 6ebc803722..e28b52128e 100644 --- a/models/issues/issue_project_test.go +++ b/models/issues/issue_project_test.go @@ -33,12 +33,13 @@ func TestPrivateIssueProjects(t *testing.T) { defer tests.PrintCurrentTest(t)() issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, user2, org, optional.None[bool]()) require.NoError(t, err) - assert.Len(t, issueList, 1) - assert.EqualValues(t, 6, issueList[0].ID) + assert.Len(t, issueList, 2) + assert.EqualValues(t, 16, issueList[0].ID) + assert.EqualValues(t, 6, issueList[1].ID) issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.None[bool]()) require.NoError(t, err) - assert.EqualValues(t, 1, issuesNum) + assert.EqualValues(t, 2, issuesNum) issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.Some(true)) require.NoError(t, err) @@ -46,18 +47,27 @@ func TestPrivateIssueProjects(t *testing.T) { issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.Some(false)) require.NoError(t, err) - assert.EqualValues(t, 1, issuesNum) + assert.EqualValues(t, 2, issuesNum) }) t.Run("Anonymous user", func(t *testing.T) { defer tests.PrintCurrentTest(t)() issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, nil, org, optional.None[bool]()) require.NoError(t, err) - assert.Empty(t, issueList) + assert.Len(t, issueList, 1) + assert.EqualValues(t, 16, issueList[0].ID) issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.None[bool]()) require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.Some(true)) + require.NoError(t, err) assert.EqualValues(t, 0, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.Some(false)) + require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) }) }) @@ -69,12 +79,13 @@ func TestPrivateIssueProjects(t *testing.T) { defer tests.PrintCurrentTest(t)() issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, user2, nil, optional.None[bool]()) require.NoError(t, err) - assert.Len(t, issueList, 1) + assert.Len(t, issueList, 2) assert.EqualValues(t, 7, issueList[0].ID) + assert.EqualValues(t, 1, issueList[1].ID) issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.None[bool]()) require.NoError(t, err) - assert.EqualValues(t, 1, issuesNum) + assert.EqualValues(t, 2, issuesNum) issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.Some(true)) require.NoError(t, err) @@ -82,19 +93,27 @@ func TestPrivateIssueProjects(t *testing.T) { issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.Some(false)) require.NoError(t, err) - assert.EqualValues(t, 1, issuesNum) + assert.EqualValues(t, 2, issuesNum) }) t.Run("Anonymous user", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, nil, nil, optional.None[bool]()) require.NoError(t, err) - assert.Empty(t, issueList) + assert.Len(t, issueList, 1) + assert.EqualValues(t, 1, issueList[0].ID) issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.None[bool]()) require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.Some(true)) + require.NoError(t, err) assert.EqualValues(t, 0, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.Some(false)) + require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) }) }) } diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index e9f116bfc6..56c219af39 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -49,9 +49,13 @@ type IssuesOptions struct { //nolint // prioritize issues from this repo PriorityRepoID int64 IsArchived optional.Option[bool] - Org *organization.Organization // issues permission scope - Team *organization.Team // issues permission scope - User *user_model.User // issues permission scope + + // If combined with AllPublic, then private as well as public issues + // that matches the criteria will be returned, if AllPublic is false + // only the private issues will be returned. + Org *organization.Organization // issues permission scope + Team *organization.Team // issues permission scope + User *user_model.User // issues permission scope } // applySorts sort an issues-related session based on the provided @@ -196,7 +200,8 @@ func applyRepoConditions(sess *xorm.Session, opts *IssuesOptions) { } else if len(opts.RepoIDs) > 1 { opts.RepoCond = builder.In("issue.repo_id", opts.RepoIDs) } - if opts.AllPublic { + // If permission scoping is set, then we set this condition at a later stage. + if opts.AllPublic && opts.User == nil { if opts.RepoCond == nil { opts.RepoCond = builder.NewCond() } @@ -268,7 +273,14 @@ func applyConditions(sess *xorm.Session, opts *IssuesOptions) { applyLabelsCondition(sess, opts) if opts.User != nil { - sess.And(issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.Value())) + cond := issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.Value()) + // If AllPublic was set, then also consider all issues in public + // repositories in addition to the private repositories the user has access + // to. + if opts.AllPublic { + cond = cond.Or(builder.In("issue.repo_id", builder.Select("id").From("repository").Where(builder.Eq{"is_private": false}))) + } + sess.And(cond) } } diff --git a/tests/integration/private_project_test.go b/tests/integration/private_project_test.go index 1a4adb4366..663c1c1e75 100644 --- a/tests/integration/private_project_test.go +++ b/tests/integration/private_project_test.go @@ -24,7 +24,7 @@ func TestPrivateIssueProject(t *testing.T) { user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) sess := loginUser(t, user2.Name) - test := func(t *testing.T, sess *TestSession, username string, projectID int64, hasAccess bool) { + test := func(t *testing.T, sess *TestSession, username string, projectID int64, hasAccess bool, publicIssueHref ...string) { t.Helper() defer tests.PrintCurrentTest(t, 1)() @@ -35,9 +35,9 @@ func TestPrivateIssueProject(t *testing.T) { htmlDoc := NewHTMLParser(t, resp.Body) openCloseStats := htmlDoc.Find(".milestone-toolbar .group").First().Text() if hasAccess { - assert.Contains(t, openCloseStats, "1\u00a0Open") + assert.Contains(t, openCloseStats, "2\u00a0Open") } else { - assert.Contains(t, openCloseStats, "0\u00a0Open") + assert.Contains(t, openCloseStats, "1\u00a0Open") } assert.Contains(t, openCloseStats, "0\u00a0Closed") @@ -46,14 +46,21 @@ func TestPrivateIssueProject(t *testing.T) { resp = sess.MakeRequest(t, req, http.StatusOK) htmlDoc = NewHTMLParser(t, resp.Body) - htmlDoc.AssertElement(t, ".project-column .issue-card", hasAccess) + issueCardsLen := htmlDoc.Find(".project-column .issue-card").Length() + if hasAccess { + assert.EqualValues(t, 2, issueCardsLen) + } else { + assert.EqualValues(t, 1, issueCardsLen) + // Ensure that the public issue is shown. + assert.EqualValues(t, publicIssueHref[0], htmlDoc.Find(".project-column .issue-card .issue-card-title").AttrOr("href", "")) + } // And that the issue count is correct. issueCount := strings.TrimSpace(htmlDoc.Find(".project-column-issue-count").Text()) if hasAccess { - assert.EqualValues(t, "1", issueCount) + assert.EqualValues(t, "2", issueCount) } else { - assert.EqualValues(t, "0", issueCount) + assert.EqualValues(t, "1", issueCount) } } @@ -66,7 +73,7 @@ func TestPrivateIssueProject(t *testing.T) { }) t.Run("Anonymous user", func(t *testing.T) { - test(t, emptyTestSession(t), org.Name, orgProject.ID, false) + test(t, emptyTestSession(t), org.Name, orgProject.ID, false, "/org3/repo21/issues/1") }) }) @@ -78,7 +85,7 @@ func TestPrivateIssueProject(t *testing.T) { }) t.Run("Anonymous user", func(t *testing.T) { - test(t, emptyTestSession(t), user2.Name, userProject.ID, false) + test(t, emptyTestSession(t), user2.Name, userProject.ID, false, "/user2/repo1/issues/1") }) }) }