Merge pull request '[v9.0/forgejo] fix: use better code to group UID and stopwatches' (#6004) from bp-v9.0/forgejo-e4eb82b into v9.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6004
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

models/issues/TestGetUIDsAndStopwatch/stopwatch.yml

@@ -0,0 +1,11 @@
+-
+  id: 3
+  user_id: 1
+  issue_id: 2
+  created_unix: 1500988004
+
+-
+  id: 4
+  user_id: 3
+  issue_id: 0
+  created_unix: 1500988003

models/issues/stopwatch.go

@@ -60,34 +60,19 @@ func getStopwatch(ctx context.Context, userID, issueID int64) (sw *Stopwatch, ex
 	return sw, exists, err
 }
 
-// UserIDCount is a simple coalition of UserID and Count
-type UserStopwatch struct {
-	UserID      int64
-	StopWatches []*Stopwatch
-}
-
 // GetUIDsAndNotificationCounts between the two provided times
-func GetUIDsAndStopwatch(ctx context.Context) ([]*UserStopwatch, error) {
+func GetUIDsAndStopwatch(ctx context.Context) (map[int64][]*Stopwatch, error) {
 	sws := []*Stopwatch{}
 	if err := db.GetEngine(ctx).Where("issue_id != 0").Find(&sws); err != nil {
 		return nil, err
 	}
+	res := map[int64][]*Stopwatch{}
 	if len(sws) == 0 {
-		return []*UserStopwatch{}, nil
+		return res, nil
 	}
 
-	lastUserID := int64(-1)
-	res := []*UserStopwatch{}
 	for _, sw := range sws {
-		if lastUserID == sw.UserID {
-			lastUserStopwatch := res[len(res)-1]
-			lastUserStopwatch.StopWatches = append(lastUserStopwatch.StopWatches, sw)
-		} else {
-			res = append(res, &UserStopwatch{
-				UserID:      sw.UserID,
-				StopWatches: []*Stopwatch{sw},
-			})
-		}
+		res[sw.UserID] = append(res[sw.UserID], sw)
 	}
 	return res, nil
 }

models/issues/stopwatch_test.go

@@ -4,12 +4,14 @@
 package issues_test
 
 import (
+	"path/filepath"
 	"testing"
 
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
@@ -77,3 +79,41 @@ func TestCreateOrStopIssueStopwatch(t *testing.T) {
 	unittest.AssertNotExistsBean(t, &issues_model.Stopwatch{UserID: 2, IssueID: 2})
 	unittest.AssertExistsAndLoadBean(t, &issues_model.TrackedTime{UserID: 2, IssueID: 2})
 }
+
+func TestGetUIDsAndStopwatch(t *testing.T) {
+	defer unittest.OverrideFixtures(
+		unittest.FixturesOptions{
+			Dir:  filepath.Join(setting.AppWorkPath, "models/fixtures/"),
+			Base: setting.AppWorkPath,
+			Dirs: []string{"models/issues/TestGetUIDsAndStopwatch/"},
+		},
+	)()
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	uidStopwatches, err := issues_model.GetUIDsAndStopwatch(db.DefaultContext)
+	require.NoError(t, err)
+	assert.EqualValues(t, map[int64][]*issues_model.Stopwatch{
+		1: {
+			{
+				ID:          1,
+				UserID:      1,
+				IssueID:     1,
+				CreatedUnix: timeutil.TimeStamp(1500988001),
+			},
+			{
+				ID:          3,
+				UserID:      1,
+				IssueID:     2,
+				CreatedUnix: timeutil.TimeStamp(1500988004),
+			},
+		},
+		2: {
+			{
+				ID:          2,
+				UserID:      2,
+				IssueID:     2,
+				CreatedUnix: timeutil.TimeStamp(1500988002),
+			},
+		},
+	}, uidStopwatches)
+}

modules/eventsource/manager_run.go

@@ -90,8 +90,8 @@ loop:
 					return
 				}
 
-				for _, userStopwatches := range usersStopwatches {
-					apiSWs, err := convert.ToStopWatches(ctx, userStopwatches.StopWatches)
+				for uid, stopwatches := range usersStopwatches {
+					apiSWs, err := convert.ToStopWatches(ctx, stopwatches)
 					if err != nil {
 						if !issues_model.IsErrIssueNotExist(err) {
 							log.Error("Unable to APIFormat stopwatches: %v", err)
@@ -103,7 +103,7 @@ loop:
 						log.Error("Unable to marshal stopwatches: %v", err)
 						continue
 					}
-					m.SendMessage(userStopwatches.UserID, &Event{
+					m.SendMessage(uid, &Event{
 						Name: "stopwatches",
 						Data: string(dataBs),
 					})

services/issue/pull.go

@@ -10,6 +10,8 @@ import (
 
 	issues_model "code.gitea.io/gitea/models/issues"
 	org_model "code.gitea.io/gitea/models/organization"
+	access_model "code.gitea.io/gitea/models/perm/access"
+	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
@@ -117,7 +119,11 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue,
 	}
 
 	for _, u := range uniqUsers {
-		if u.ID != issue.Poster.ID {
+		permission, err := access_model.GetUserRepoPermission(ctx, issue.Repo, u)
+		if err != nil {
+			return nil, fmt.Errorf("GetUserRepoPermission: %w", err)
+		}
+		if u.ID != issue.Poster.ID && permission.CanRead(unit.TypePullRequests) {
 			comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster)
 			if err != nil {
 				log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)

tests/integration/codeowner_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 	"time"
 
+	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	unit_model "code.gitea.io/gitea/models/unit"
@@ -159,5 +160,42 @@ func TestCodeOwner(t *testing.T) {
 			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "branch"})
 			unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 4})
 		})
+
+		t.Run("Codeowner user with no permission", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			// Make repository private, only user2 (owner of repository) has now access to this repository.
+			repo.IsPrivate = true
+			_, err := db.GetEngine(db.DefaultContext).Cols("is_private").Update(repo)
+			require.NoError(t, err)
+
+			err = os.WriteFile(path.Join(dstPath, "README.md"), []byte("## very senstive info"), 0o666)
+			require.NoError(t, err)
+
+			err = git.AddChanges(dstPath, true)
+			require.NoError(t, err)
+
+			err = git.CommitChanges(dstPath, git.CommitChangesOptions{
+				Committer: &git.Signature{
+					Email: "user2@example.com",
+					Name:  "user2",
+					When:  time.Now(),
+				},
+				Author: &git.Signature{
+					Email: "user2@example.com",
+					Name:  "user2",
+					When:  time.Now(),
+				},
+				Message: "Add secrets to the README.",
+			})
+			require.NoError(t, err)
+
+			err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/main", "-o", "topic=codeowner-private").Run(&git.RunOpts{Dir: dstPath})
+			require.NoError(t, err)
+
+			// In CODEOWNERS file the codeowner for README.md is user5, but does not have access to this private repository.
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "user2/codeowner-private"})
+			unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
+		})
 	})
 }