[v10.0/forgejo] fix: consider public issues for project boards (#7144)

**Backport:** https://codeberg.org/forgejo/forgejo/pulls/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

Co-authored-by: Gusted <postmaster@gusted.xyz>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7144
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>

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

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

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)

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.Len(t, issueList, 2)
-			assert.EqualValues(t, 6, issueList[0].ID)
+			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)
 		})
 	})
 }

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
+	// If combined with AllPublic, then private as well as public issues
-	User           *user_model.User           // issues permission scope
+	// 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)
 	}
 }
 

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")
 		})
 	})
 }