fix: co-author detection (#38392) (#38397)

Backport #38392

Fix #38384

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/repo/user_repo.go

@@ -22,6 +22,9 @@ type StarredReposOptions struct {
 	StarrerID      int64
 	RepoOwnerID    int64
 	IncludePrivate bool
+	// Actor is the user the private repositories are gated on: a private repo is only
+	// returned when Actor still has access to it, even if it was starred while access was granted.
+	Actor *user_model.User
 }
 
 func (opts *StarredReposOptions) ApplyPublicOnly(publicOnly bool) {
@@ -39,7 +42,10 @@ func (opts *StarredReposOptions) ToConds() builder.Cond {
 			"repository.owner_id": opts.RepoOwnerID,
 		})
 	}
-	if !opts.IncludePrivate {
+	if opts.IncludePrivate {
+		// only include private repos the actor can still access, so metadata does not leak after access revocation
+		cond = cond.And(AccessibleRepositoryCondition(opts.Actor, unit.TypeInvalid))
+	} else {
 		cond = cond.And(builder.Eq{
 			"repository.is_private": false,
 		})
@@ -66,6 +72,9 @@ type WatchedReposOptions struct {
 	WatcherID      int64
 	RepoOwnerID    int64
 	IncludePrivate bool
+	// Actor is the user the private repositories are gated on: a private repo is only
+	// returned when Actor still has access to it, even if it was watched while access was granted.
+	Actor *user_model.User
 }
 
 func (opts *WatchedReposOptions) ApplyPublicOnly(publicOnly bool) {
@@ -83,7 +92,10 @@ func (opts *WatchedReposOptions) ToConds() builder.Cond {
 			"repository.owner_id": opts.RepoOwnerID,
 		})
 	}
-	if !opts.IncludePrivate {
+	if opts.IncludePrivate {
+		// only include private repos the actor can still access, so metadata does not leak after access revocation
+		cond = cond.And(AccessibleRepositoryCondition(opts.Actor, unit.TypeInvalid))
+	} else {
 		cond = cond.And(builder.Eq{
 			"repository.is_private": false,
 		})

modules/git/commit_message.go

@@ -10,16 +10,22 @@ import (
 	"sync"
 
 	"gitea.dev/modules/charset"
-	"gitea.dev/modules/container"
 	"gitea.dev/modules/util"
 )
 
 // CoAuthoredByTrailer is the canonical token for the `Co-authored-by:` git trailer.
 const CoAuthoredByTrailer = "Co-authored-by"
 
+const (
+	commitIdentityRoleAuthor    = 1
+	commitIdentityRoleCommitter = 2
+	commitIdentityRoleCoAuthor  = 3
+)
+
 type CommitIdentity struct {
 	Name  string
 	Email string
+	role  int
 }
 
 // CommitMessageTrailerValues keys are all in lower-case
@@ -33,7 +39,9 @@ type CommitMessage struct {
 
 	trailerValues CommitMessageTrailerValues
 
-	allParticipants []*CommitIdentity
+	allParticipants      []*CommitIdentity
+	committerCoAuthorIdx int
+	committerCoAuthor    *CommitIdentity
 }
 
 func (c *CommitMessage) MessageUTF8() string {
@@ -105,43 +113,57 @@ func (c *Commit) AllParticipantIdentities() []*CommitIdentity {
 		return c.allParticipants
 	}
 
-	exclude := container.Set[string]{}
-	c.allParticipants = append(c.allParticipants, &CommitIdentity{Name: c.Author.Name, Email: c.Author.Email})
-	exclude.Add(strings.ToLower(c.Author.Email))
-
-	addParticipant := func(name, email string) {
+	exclude := map[string]int{}
+	addParticipant := func(name, email string, role int) (existingRole int) {
 		if name == "" && email == "" {
-			return
+			return 0
 		}
 		emailLower := strings.ToLower(email)
-		if emailLower != "" && exclude.Contains(emailLower) {
-			return
+		if existingRole = exclude[emailLower]; emailLower != "" && existingRole != 0 {
+			return existingRole
 		}
-		c.allParticipants = append(c.allParticipants, &CommitIdentity{Name: name, Email: email})
-		exclude.Add(emailLower)
+		c.allParticipants = append(c.allParticipants, &CommitIdentity{Name: name, Email: email, role: role})
+		exclude[emailLower] = role
+		return 0
 	}
-	addParticipant(c.Committer.Name, c.Committer.Email)
+
+	c.committerCoAuthorIdx = -1
+	addParticipant(c.Author.Name, c.Author.Email, commitIdentityRoleAuthor)
+	addParticipant(c.Committer.Name, c.Committer.Email, commitIdentityRoleCommitter)
 	for _, coAuthorValue := range c.MessageTrailer()["co-authored-by"] {
 		addr, err := mail.ParseAddress(coAuthorValue)
+		coAuthorName, coAuthorEmail := coAuthorValue, ""
 		if err == nil {
-			addParticipant(addr.Name, addr.Address)
-		} else {
-			addParticipant(coAuthorValue, "")
+			coAuthorName, coAuthorEmail = addr.Name, addr.Address
+		}
+		existingRole := addParticipant(coAuthorName, coAuthorEmail, commitIdentityRoleCoAuthor)
+		if existingRole == commitIdentityRoleCommitter && c.committerCoAuthorIdx == -1 {
+			c.committerCoAuthorIdx = len(c.allParticipants)
+			c.committerCoAuthor = &CommitIdentity{coAuthorName, coAuthorEmail, commitIdentityRoleCoAuthor}
 		}
 	}
 	return c.allParticipants
 }
 
-// CoAuthorIdentities returns the commit's co-authors: the participants without the
-// author and the committer, which are already displayed separately. This avoids
-// surfacing the committer as a "co-author" (see issue #38384).
-func (c *Commit) CoAuthorIdentities() []*CommitIdentity {
-	var ret []*CommitIdentity
-	for _, p := range c.AllParticipantIdentities()[1:] { // index 0 is always the author
-		if c.Committer.Email != "" && strings.EqualFold(p.Email, c.Committer.Email) {
-			continue
-		}
-		ret = append(ret, p)
+// CoAuthorIdentities returns co-author identities defined by "Co-authored-by:" in the git message trailer
+// Only the commit's author is excluded. If committer is declared as co-author, it will be included in the result.
+// * Author & Co-author: they changed the code (attribution)
+// * Committer: they submitted the commit but didn't change the code (e.g.: maintainer signed a commit)
+// So, a committer can also be a co-author if they changed the code.
+func (c *Commit) CoAuthorIdentities() (coAuthors []*CommitIdentity) {
+	all := c.AllParticipantIdentities()
+	if len(all) <= 1 {
+		return nil // no co-author list
 	}
-	return ret
+	if all[1].role != commitIdentityRoleCommitter {
+		return all[1:] // no committer, so all after author are co-authors
+	}
+	if c.committerCoAuthorIdx == -1 {
+		return all[2:] // the committer is not in the co-author list, so just return the co-author list
+	}
+	// the committer is in the co-author list but de-duplicated, so include them as co-author again
+	coAuthors = append(coAuthors, all[2:c.committerCoAuthorIdx]...)
+	coAuthors = append(coAuthors, c.committerCoAuthor)
+	coAuthors = append(coAuthors, all[c.committerCoAuthorIdx:]...)
+	return coAuthors
 }

modules/git/commit_message_test.go

@@ -47,75 +47,83 @@ func TestCommitMessageTrailer(t *testing.T) {
 	}
 }
 
-func TestCommitMessageAllParticipantIdentities(t *testing.T) {
+func TestCommitMessageParticipants(t *testing.T) {
 	sig := func(n, e string) *Signature { return &Signature{Name: n, Email: e} }
-	idt := func(n, e string) *CommitIdentity { return &CommitIdentity{Name: n, Email: e} }
-	cases := []struct {
-		commit      *Commit
-		participant []*CommitIdentity
-	}{
-		{
-			&Commit{
-				Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
-				CommitMessage: CommitMessage{MessageRaw: "CO-Authored-BY: x@m.com"},
-			},
-			[]*CommitIdentity{idt("a", "a@m.com"), idt("c", "c@m.com"), idt("", "x@m.com")},
-		},
-		{
-			&Commit{
-				Author: sig("a", "a@m.com"), Committer: sig("a", "A@M.com"),
-				CommitMessage: CommitMessage{MessageRaw: "CO-Authored-BY: a@m.com"},
-			},
-			[]*CommitIdentity{idt("a", "a@m.com")},
-		},
-		{
-			&Commit{
-				Author: sig("a", "a@m.com"), Committer: sig("", ""),
-				CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: Full Name <X@M.com>"},
-			},
-			[]*CommitIdentity{idt("a", "a@m.com"), idt("Full Name", "X@M.com")},
-		},
-	}
-	for _, c := range cases {
-		assert.Equal(t, c.participant, c.commit.AllParticipantIdentities())
-	}
-}
-
-func TestCommitMessageCoAuthorIdentities(t *testing.T) {
-	sig := func(n, e string) *Signature { return &Signature{Name: n, Email: e} }
-	idt := func(n, e string) *CommitIdentity { return &CommitIdentity{Name: n, Email: e} }
-	cases := []struct {
-		commit    *Commit
-		coAuthors []*CommitIdentity
-	}{
-		{
-			// a genuine co-author (neither author nor committer) is reported
-			&Commit{
-				Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
-				CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: x <x@m.com>"},
-			},
-			[]*CommitIdentity{idt("x", "x@m.com")},
-		},
-		{
-			// the committer is shown separately as "committed by", so it must
-			// not appear as a co-author even though it is a participant
-			&Commit{
-				Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
-				CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: c <c@m.com>"},
-			},
-			nil,
-		},
-		{
-			// regression for #38384: a Co-authored-by trailer naming the author
-			// must not cause the committer to be surfaced as the co-author
-			&Commit{
-				Author: sig("silverwind", "me@silverwind.io"), Committer: sig("bircni", "bircni@icloud.com"),
-				CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: silverwind <me@silverwind.io>"},
-			},
-			nil,
-		},
-	}
-	for _, c := range cases {
-		assert.Equal(t, c.coAuthors, c.commit.CoAuthorIdentities())
+	idt := func(n, e string, r int) *CommitIdentity { return &CommitIdentity{n, e, r} }
+	roleAuthor, roleCommitter, roleCoAuthor := commitIdentityRoleAuthor, commitIdentityRoleCommitter, commitIdentityRoleCoAuthor
+	type testCase struct {
+		name       string
+		commit     *Commit
+		identities []*CommitIdentity
 	}
+	t.Run("AllParticipants", func(t *testing.T) {
+		cases := []testCase{
+			{
+				"DifferentUsers",
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
+					CommitMessage: CommitMessage{MessageRaw: "CO-Authored-BY: x@m.com"},
+				},
+				[]*CommitIdentity{idt("a", "a@m.com", roleAuthor), idt("c", "c@m.com", roleCommitter), idt("", "x@m.com", roleCoAuthor)},
+			},
+			{
+				"SameUser",
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("a", "A@M.com"),
+					CommitMessage: CommitMessage{MessageRaw: "CO-Authored-BY: a@m.com"},
+				},
+				[]*CommitIdentity{idt("a", "a@m.com", roleAuthor)},
+			},
+			{
+				"NoCommitter",
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("", ""),
+					CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: Full Name <X@M.com>"},
+				},
+				[]*CommitIdentity{idt("a", "a@m.com", roleAuthor), idt("Full Name", "X@M.com", roleCoAuthor)},
+			},
+		}
+		for _, c := range cases {
+			assert.Equal(t, c.identities, c.commit.AllParticipantIdentities(), "case: %s", c.name)
+		}
+	})
+	t.Run("CoAuthors", func(t *testing.T) {
+		cases := []testCase{
+			{
+				"GenuineCoAuthor",
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
+					CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: x <x@m.com>"},
+				},
+				[]*CommitIdentity{idt("x", "x@m.com", roleCoAuthor)},
+			},
+			{
+				"CoAuthorIsCommitter",
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
+					CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: c <c@m.com>"},
+				},
+				[]*CommitIdentity{idt("c", "c@m.com", roleCoAuthor)},
+			},
+			{
+				"CoAuthorIsAuthor",
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
+					CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: a <a@m.com>"},
+				},
+				[]*CommitIdentity{},
+			},
+			{
+				"CoAuthorCommitterNameWithIndex", // restore the committer co-author to the co-author list by the index with correct name
+				&Commit{
+					Author: sig("a", "a@m.com"), Committer: sig("c", "c@m.com"),
+					CommitMessage: CommitMessage{MessageRaw: "Co-authored-by: x <x@m.com>\nCo-authored-by: c-other <c@m.com>\nCo-authored-by: y <y@m.com>"},
+				},
+				[]*CommitIdentity{idt("x", "x@m.com", roleCoAuthor), idt("c-other", "c@m.com", roleCoAuthor), idt("y", "y@m.com", roleCoAuthor)},
+			},
+		}
+		for _, c := range cases {
+			assert.Equal(t, c.identities, c.commit.CoAuthorIdentities(), "case: %s", c.name)
+		}
+	})
 }

routers/api/v1/user/star.go

@@ -24,6 +24,7 @@ func getStarredRepos(ctx *context.APIContext, user *user_model.User, private boo
 		ListOptions:    utils.GetListOptions(ctx),
 		StarrerID:      user.ID,
 		IncludePrivate: private,
+		Actor:          user,
 	}
 	opts.ApplyPublicOnly(ctx.PublicOnly)
 

routers/api/v1/user/watch.go

@@ -22,6 +22,7 @@ func getWatchedRepos(ctx *context.APIContext, user *user_model.User, private boo
 		ListOptions:    utils.GetListOptions(ctx),
 		WatcherID:      user.ID,
 		IncludePrivate: private,
+		Actor:          user,
 	}
 	opts.ApplyPublicOnly(ctx.PublicOnly)
 

tests/integration/api_user_star_test.go

@@ -9,11 +9,14 @@ import (
 	"testing"
 
 	auth_model "gitea.dev/models/auth"
+	"gitea.dev/models/perm"
+	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/setting"
 	api "gitea.dev/modules/structs"
 	"gitea.dev/modules/test"
+	repo_service "gitea.dev/services/repository"
 	"gitea.dev/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -93,6 +96,52 @@ func TestAPIStar(t *testing.T) {
 	})
 }
 
+// TestAPIStarredReposAccessRevoked ensures a private repository disappears from a user's
+// starred list once their access to it has been revoked, so no metadata leaks afterwards.
+func TestAPIStarredReposAccessRevoked(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	// user2/repo2 is a private repository owned by user2
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
+	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	collaborator := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+
+	// grant the collaborator read access and star the private repo as them
+	require.NoError(t, repo_service.AddOrUpdateCollaborator(t.Context(), repo, collaborator, perm.AccessModeRead))
+	require.NoError(t, repo_model.StarRepo(t.Context(), collaborator, repo, true))
+
+	token := getUserToken(t, collaborator.Name, auth_model.AccessTokenScopeReadUser, auth_model.AccessTokenScopeReadRepository)
+
+	// while access is granted, the private repo is visible in the starred list
+	req := NewRequest(t, "GET", "/api/v1/user/starred").AddTokenAuth(token)
+	resp := MakeRequest(t, req, http.StatusOK)
+	repos := DecodeJSON(t, resp, []api.Repository{})
+	require.Len(t, repos, 1)
+	assert.Equal(t, repo.FullName(), repos[0].FullName)
+
+	// revoke access
+	require.NoError(t, repo_service.DeleteCollaboration(t.Context(), repo, collaborator))
+
+	// the star record still exists, but the repo (and its metadata) must no longer be returned
+	assert.True(t, repo_model.IsStaring(t.Context(), collaborator.ID, repo.ID))
+	req = NewRequest(t, "GET", "/api/v1/user/starred").AddTokenAuth(token)
+	resp = MakeRequest(t, req, http.StatusOK)
+	repos = DecodeJSON(t, resp, []api.Repository{})
+	assert.Empty(t, repos)
+
+	// sanity: the owner still sees their own private repo
+	ownerToken := getUserToken(t, owner.Name, auth_model.AccessTokenScopeReadUser, auth_model.AccessTokenScopeReadRepository)
+	require.NoError(t, repo_model.StarRepo(t.Context(), owner, repo, true))
+	req = NewRequest(t, "GET", "/api/v1/user/starred").AddTokenAuth(ownerToken)
+	resp = MakeRequest(t, req, http.StatusOK)
+	repos = DecodeJSON(t, resp, []api.Repository{})
+	fullNames := make([]string, len(repos))
+	for i, r := range repos {
+		fullNames[i] = r.FullName
+	}
+	assert.Contains(t, fullNames, repo.FullName())
+}
+
 func TestAPIStarDisabled(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()