mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-03-12 00:12:37 +01:00
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 <gusted@noreply.codeberg.org> Co-authored-by: 0ko <0ko@noreply.codeberg.org> Co-committed-by: 0ko <0ko@noreply.codeberg.org>
This commit is contained in:
parent
862f4ad60c
commit
d1d78c1b14
5 changed files with 140 additions and 5 deletions
|
@ -1,4 +1,5 @@
|
||||||
// Copyright 2022 The Gitea Authors. All rights reserved.
|
// Copyright 2022 The Gitea Authors. All rights reserved.
|
||||||
|
// Copyright 2024 The Forgejo Authors. All rights reserved.
|
||||||
// SPDX-License-Identifier: MIT
|
// SPDX-License-Identifier: MIT
|
||||||
|
|
||||||
package user
|
package user
|
||||||
|
@ -95,3 +96,7 @@ func APActorUserAPActorID() string {
|
||||||
path, _ := url.JoinPath(setting.AppURL, "/api/v1/activitypub/actor")
|
path, _ := url.JoinPath(setting.AppURL, "/api/v1/activitypub/actor")
|
||||||
return path
|
return path
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (u *User) IsAPActor() bool {
|
||||||
|
return u != nil && u.ID == APActorUserID
|
||||||
|
}
|
||||||
|
|
|
@ -1,5 +1,6 @@
|
||||||
// Copyright 2014 The Gogs Authors. All rights reserved.
|
// Copyright 2014 The Gogs Authors. All rights reserved.
|
||||||
// Copyright 2018 The Gitea Authors. All rights reserved.
|
// Copyright 2018 The Gitea Authors. All rights reserved.
|
||||||
|
// Copyright 2024 The Forgejo Authors. All rights reserved.
|
||||||
// SPDX-License-Identifier: MIT
|
// SPDX-License-Identifier: MIT
|
||||||
|
|
||||||
package repo
|
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) {
|
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{}
|
roleDescriptor := issues_model.RoleDescriptor{}
|
||||||
|
|
||||||
|
// Migrated comment with no associated local user
|
||||||
if hasOriginalAuthor {
|
if hasOriginalAuthor {
|
||||||
return roleDescriptor, nil
|
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)
|
perm, err := access_model.GetUserRepoPermission(ctx, repo, poster)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return roleDescriptor, err
|
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.
|
// Check if the poster is owner of the repo.
|
||||||
if perm.IsOwner() {
|
if perm.IsOwner() {
|
||||||
// If the poster isn't an admin, enable the owner role.
|
// If the poster isn't an admin, enable the owner role.
|
||||||
|
|
56
tests/integration/comment_roles_system_test.go
Normal file
56
tests/integration/comment_roles_system_test.go
Normal file
|
@ -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())
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
|
@ -14,8 +14,8 @@ import (
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
)
|
)
|
||||||
|
|
||||||
// TestIssuesCommentLabels is a test for user (role) labels in comment headers in PRs and issues.
|
// TestCommentRoles is a test for role labels of normal users in comment headers in PRs and issues.
|
||||||
func TestIssuesCommentLabels(t *testing.T) {
|
func TestCommentRoles(t *testing.T) {
|
||||||
user := "user2"
|
user := "user2"
|
||||||
repo := "repo1"
|
repo := "repo1"
|
||||||
|
|
67
tests/integration/fixtures/TestSystemCommentRoles/issue.yml
Normal file
67
tests/integration/fixtures/TestSystemCommentRoles/issue.yml
Normal file
|
@ -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
|
Loading…
Add table
Reference in a new issue