From d1d78c1b144054ed018a77e0601fc1c9c833456f Mon Sep 17 00:00:00 2001 From: 0ko <0ko@noreply.codeberg.org> Date: Wed, 5 Feb 2025 17:34:45 +0000 Subject: [PATCH] fix(commenter roles): don't give system users roles (#6766) Currently on every pull request Ghost would have a misleading "First-time contributor" role. Also, if the issue author is a Ghost, all other ghosts who commented will be labeled as authors even if they are different ghosts. I've added a missing check to abort all other permission and contribution checks early if the user is a ghost. Also applies to other system users, as suggested by @earl-warren. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6766 Reviewed-by: Gusted Co-authored-by: 0ko <0ko@noreply.codeberg.org> Co-committed-by: 0ko <0ko@noreply.codeberg.org> --- models/user/user_system.go | 5 ++ routers/web/repo/issue.go | 13 +++- .../integration/comment_roles_system_test.go | 56 ++++++++++++++++ ...t_labels_test.go => comment_roles_test.go} | 4 +- .../fixtures/TestSystemCommentRoles/issue.yml | 67 +++++++++++++++++++ 5 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 tests/integration/comment_roles_system_test.go rename tests/integration/{issues_comment_labels_test.go => comment_roles_test.go} (98%) create mode 100644 tests/integration/fixtures/TestSystemCommentRoles/issue.yml diff --git a/models/user/user_system.go b/models/user/user_system.go index ba9a2131b2..b13b355d37 100644 --- a/models/user/user_system.go +++ b/models/user/user_system.go @@ -1,4 +1,5 @@ // Copyright 2022 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package user @@ -95,3 +96,7 @@ func APActorUserAPActorID() string { path, _ := url.JoinPath(setting.AppURL, "/api/v1/activitypub/actor") return path } + +func (u *User) IsAPActor() bool { + return u != nil && u.ID == APActorUserID +} diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 61711095b9..5f9b952119 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1,5 +1,6 @@ // Copyright 2014 The Gogs Authors. All rights reserved. // Copyright 2018 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package repo @@ -1308,18 +1309,24 @@ func NewIssuePost(ctx *context.Context) { func roleDescriptor(ctx stdCtx.Context, repo *repo_model.Repository, poster *user_model.User, issue *issues_model.Issue, hasOriginalAuthor bool) (issues_model.RoleDescriptor, error) { roleDescriptor := issues_model.RoleDescriptor{} + // Migrated comment with no associated local user if hasOriginalAuthor { return roleDescriptor, nil } + // Special user that can't have associated contributions and permissions in the repo. + if poster.IsGhost() || poster.IsActions() || poster.IsAPActor() { + return roleDescriptor, nil + } + + // If the poster is the actual poster of the issue, enable Poster role. + roleDescriptor.IsPoster = issue.IsPoster(poster.ID) + perm, err := access_model.GetUserRepoPermission(ctx, repo, poster) if err != nil { return roleDescriptor, err } - // If the poster is the actual poster of the issue, enable Poster role. - roleDescriptor.IsPoster = issue.IsPoster(poster.ID) - // Check if the poster is owner of the repo. if perm.IsOwner() { // If the poster isn't an admin, enable the owner role. diff --git a/tests/integration/comment_roles_system_test.go b/tests/integration/comment_roles_system_test.go new file mode 100644 index 0000000000..4c5d5ed8da --- /dev/null +++ b/tests/integration/comment_roles_system_test.go @@ -0,0 +1,56 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "net/http" + "testing" + + issues_model "code.gitea.io/gitea/models/issues" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +// TestSystemCommentRoles verifies that system users don't have role labels. +// As it is not possible to do actions as system users, the tests are done using fixtures. + +func TestSystemCommentRoles(t *testing.T) { + defer tests.AddFixtures("tests/integration/fixtures/TestSystemCommentRoles/")() + defer tests.PrepareTestEnv(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + testCases := []struct { + name string + username string + index int64 + roleCount int64 + }{ + {"user2", "user2", 1000, 1}, // As a verification, also check a normal user with one role. + {"Ghost", "Ghost", 1001, 0}, // System users should not have any roles, so 0. + {"Actions", "forgejo-actions", 1002, 0}, + {"APActor", "Ghost", 1003, 0}, // actor is displayed as Ghost, could be a bug. + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ + RepoID: repo.ID, + Index: tc.index, + }) + + req := NewRequestf(t, "GET", "%s/issues/%d", repo.Link(), issue.Index) + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + assert.EqualValues(t, tc.username, htmlDoc.Find("a.author").Text()) + assert.EqualValues(t, tc.roleCount, htmlDoc.Find(".role-label").Length()) + }) + } +} diff --git a/tests/integration/issues_comment_labels_test.go b/tests/integration/comment_roles_test.go similarity index 98% rename from tests/integration/issues_comment_labels_test.go rename to tests/integration/comment_roles_test.go index 5299d8a057..00bec95722 100644 --- a/tests/integration/issues_comment_labels_test.go +++ b/tests/integration/comment_roles_test.go @@ -14,8 +14,8 @@ import ( "github.com/stretchr/testify/assert" ) -// TestIssuesCommentLabels is a test for user (role) labels in comment headers in PRs and issues. -func TestIssuesCommentLabels(t *testing.T) { +// TestCommentRoles is a test for role labels of normal users in comment headers in PRs and issues. +func TestCommentRoles(t *testing.T) { user := "user2" repo := "repo1" diff --git a/tests/integration/fixtures/TestSystemCommentRoles/issue.yml b/tests/integration/fixtures/TestSystemCommentRoles/issue.yml new file mode 100644 index 0000000000..07e0c01edf --- /dev/null +++ b/tests/integration/fixtures/TestSystemCommentRoles/issue.yml @@ -0,0 +1,67 @@ +- + id: 1000 + repo_id: 1 + index: 1000 + poster_id: 2 + original_author_id: 0 + name: issue1 + content: authored by a normal user 2 + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: false + num_comments: 1 + created_unix: 946684800 + updated_unix: 978307200 + is_locked: false + +- + id: 1001 + repo_id: 1 + index: 1001 + poster_id: -1 + original_author_id: 0 + name: issue1 + content: authored by the system user -1 / Ghost + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: false + num_comments: 1 + created_unix: 946684800 + updated_unix: 978307200 + is_locked: false + +- + id: 1002 + repo_id: 1 + index: 1002 + poster_id: -2 + original_author_id: 0 + name: issue2 + content: authored by the system user -2 / Action + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: false + num_comments: 1 + created_unix: 946684800 + updated_unix: 978307200 + is_locked: false + +- + id: 1003 + repo_id: 1 + index: 1003 + poster_id: -3 + original_author_id: 0 + name: issue3 + content: authored by the system user -3 / APActor + milestone_id: 0 + priority: 0 + is_closed: false + is_pull: false + num_comments: 1 + created_unix: 946684800 + updated_unix: 978307200 + is_locked: false