From 5236e0c306f77af6eb667959590132ccdb88a114 Mon Sep 17 00:00:00 2001 From: kolaente Date: Wed, 17 Jun 2026 19:44:36 +0200 Subject: [PATCH] fix(notifications): use full user so notifications show display name Notifications and emails showed the acting user's auto-generated username instead of their display Name. The doer attached to notification events was built straight from the JWT via user.GetFromAuth, which only carries id + username (Name is never set in GetUserFromClaims). Notifications render n.Doer.GetName(), which falls back to the username when Name is empty, so every "assigned you", "mentioned you", task-deleted, project-created and team-member notification rendered the username. Resolve the full user from the database at the event-producing dispatch sites. doerFromAuth now re-fetches the user (with Name) and is reused by all the notification doers; account-status errors are swallowed so flows acting on behalf of disabled accounts (e.g. user deletion deleting that user's tasks) keep working while still carrying the display name. Fixes #2720 --- pkg/events/testing.go | 12 +++++ pkg/models/kanban_task_bucket.go | 4 +- pkg/models/project.go | 6 +-- pkg/models/project_team.go | 2 +- pkg/models/project_users.go | 2 +- pkg/models/task_assignees.go | 4 +- pkg/models/task_assignees_test.go | 80 +++++++++++++++++++++++++++++++ pkg/models/tasks.go | 9 ++-- pkg/models/team_members.go | 3 +- pkg/models/teams.go | 2 +- pkg/models/users.go | 20 ++++++-- 11 files changed, 120 insertions(+), 24 deletions(-) create mode 100644 pkg/models/task_assignees_test.go diff --git a/pkg/events/testing.go b/pkg/events/testing.go index 2c969f057..302886747 100644 --- a/pkg/events/testing.go +++ b/pkg/events/testing.go @@ -76,6 +76,18 @@ func ClearDispatchedEvents() { dispatchedTestEvents = nil } +// GetDispatchedEvents returns all dispatched test events matching the given name, letting tests +// assert on the event payload (not just that it was dispatched). +func GetDispatchedEvents(eventName string) []Event { + var events []Event + for _, testEvent := range dispatchedTestEvents { + if testEvent.Name() == eventName { + events = append(events, testEvent) + } + } + return events +} + // CountDispatchedEvents counts how many events of a specific type have been dispatched. func CountDispatchedEvents(eventName string) int { count := 0 diff --git a/pkg/models/kanban_task_bucket.go b/pkg/models/kanban_task_bucket.go index 108b8a371..ae6757b30 100644 --- a/pkg/models/kanban_task_bucket.go +++ b/pkg/models/kanban_task_bucket.go @@ -21,7 +21,6 @@ import ( "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/events" - "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/web" "xorm.io/xorm" ) @@ -252,10 +251,9 @@ func (b *TaskBucket) Update(s *xorm.Session, a web.Auth) (err error) { } if b.Task != nil { - doer, _ := user.GetFromAuth(a) events.DispatchOnCommit(s, &TaskUpdatedEvent{ Task: b.Task, - Doer: doer, + Doer: doerFromAuth(s, a), }) } return nil diff --git a/pkg/models/project.go b/pkg/models/project.go index a799d1815..b0ad5ee4c 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -1072,7 +1072,7 @@ func CreateProject(s *xorm.Session, project *Project, auth web.Auth, createBackl events.DispatchOnCommit(s, &ProjectCreatedEvent{ Project: project, - Doer: doer, + Doer: doerFromAuth(s, auth), }) return nil } @@ -1219,7 +1219,7 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje events.DispatchOnCommit(s, &ProjectUpdatedEvent{ Project: project, - Doer: doerFromAuth(auth), + Doer: doerFromAuth(s, auth), }) l, err := GetProjectSimpleByID(s, project.ID) @@ -1450,7 +1450,7 @@ func (p *Project) Delete(s *xorm.Session, a web.Auth) (err error) { events.DispatchOnCommit(s, &ProjectDeletedEvent{ Project: fullProject, - Doer: doerFromAuth(a), + Doer: doerFromAuth(s, a), }) childProjects := []*Project{} diff --git a/pkg/models/project_team.go b/pkg/models/project_team.go index e3571906c..e6fd75f96 100644 --- a/pkg/models/project_team.go +++ b/pkg/models/project_team.go @@ -112,7 +112,7 @@ func (tl *TeamProject) Create(s *xorm.Session, a web.Auth) (err error) { events.DispatchOnCommit(s, &ProjectSharedWithTeamEvent{ Project: l, Team: team, - Doer: doerFromAuth(a), + Doer: doerFromAuth(s, a), }) err = updateProjectLastUpdated(s, l) diff --git a/pkg/models/project_users.go b/pkg/models/project_users.go index 41254ac1d..0789220ce 100644 --- a/pkg/models/project_users.go +++ b/pkg/models/project_users.go @@ -118,7 +118,7 @@ func (lu *ProjectUser) Create(s *xorm.Session, a web.Auth) (err error) { events.DispatchOnCommit(s, &ProjectSharedWithUserEvent{ Project: l, User: u, - Doer: doerFromAuth(a), + Doer: doerFromAuth(s, a), }) err = updateProjectLastUpdated(s, l) diff --git a/pkg/models/task_assignees.go b/pkg/models/task_assignees.go index 517d9fc5d..662973e80 100644 --- a/pkg/models/task_assignees.go +++ b/pkg/models/task_assignees.go @@ -181,7 +181,7 @@ func (la *TaskAssginee) Delete(s *xorm.Session, a web.Auth) (err error) { return err } - doer, _ := user.GetFromAuth(a) + doer := doerFromAuth(s, a) task, err := GetTaskByIDSimple(s, la.TaskID) if err != nil { return err @@ -270,7 +270,7 @@ func (t *Task) addNewAssigneeByID(s *xorm.Session, newAssigneeID int64, project return err } - doer, _ := user.GetFromAuth(auth) + doer := doerFromAuth(s, auth) task, err := GetTaskSimple(s, &Task{ID: t.ID}) if err != nil { return err diff --git a/pkg/models/task_assignees_test.go b/pkg/models/task_assignees_test.go new file mode 100644 index 000000000..415913d1c --- /dev/null +++ b/pkg/models/task_assignees_test.go @@ -0,0 +1,80 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package models + +import ( + "context" + "testing" + + "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/events" + "code.vikunja.io/api/pkg/user" + + "github.com/stretchr/testify/require" +) + +// TestTaskAssignee_DoerHasDisplayName guards against the regression in #2720: the doer attached to +// notification events was built straight from the JWT (id + username only), so notifications and +// emails rendered the auto-generated username instead of the user's display Name. The dispatch sites +// now resolve the full user from the database, so the doer must carry the display Name even when the +// acting auth object only has id + username (as GetUserFromClaims produces). +func TestTaskAssignee_DoerHasDisplayName(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Mimics the partial user GetUserFromClaims builds from a JWT: id + username, no Name. + // user12 has the display name "Name with spaces" in the fixtures and owns project 23. + doer := &user.User{ID: 12, Username: "user12"} + require.Equal(t, "user12", doer.GetName(), "the auth doer must start without a display name") + + task := &Task{Title: "assign me", ProjectID: 23} + require.NoError(t, task.Create(s, doer)) + + events.ClearDispatchedEvents() + + ta := &TaskAssginee{TaskID: task.ID, UserID: 12} + require.NoError(t, ta.Create(s, doer)) + require.NoError(t, s.Commit()) + + events.DispatchPending(context.Background(), s) + + dispatched := events.GetDispatchedEvents((&TaskAssigneeCreatedEvent{}).Name()) + require.Len(t, dispatched, 1) + ev := dispatched[0].(*TaskAssigneeCreatedEvent) + require.NotNil(t, ev.Doer) + require.Equal(t, "Name with spaces", ev.Doer.GetName(), + "notification doer must carry the display Name, not the username") +} + +// TestDoerFromAuth_DisabledUser ensures resolving the event doer keeps working when acting on behalf +// of a disabled account (e.g. user deletion deletes that user's tasks). The full user is still +// returned with its display name, the disabled status error is swallowed. +func TestDoerFromAuth_DisabledUser(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // user17 is disabled in the fixtures. + _, err := user.GetUserByID(s, 17) + require.Error(t, err, "fixture user17 is expected to be disabled") + require.True(t, user.IsErrAccountDisabled(err)) + + doer := doerFromAuth(s, &user.User{ID: 17, Username: "user17"}) + require.NotNil(t, doer) + require.Equal(t, int64(17), doer.ID) +} diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index a49e4175e..ec6200c48 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -1462,10 +1462,9 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e } t.Updated = nt.Updated - doer, _ := user.GetFromAuth(a) events.DispatchOnCommit(s, &TaskUpdatedEvent{ Task: t, - Doer: doer, + Doer: doerFromAuth(s, a), }) return updateProjectLastUpdated(s, &Project{ID: t.ProjectID}) @@ -1961,10 +1960,9 @@ func (t *Task) Delete(s *xorm.Session, a web.Auth) (err error) { return err } - doer, _ := user.GetFromAuth(a) events.DispatchOnCommit(s, &TaskDeletedEvent{ Task: fullTask, - Doer: doer, + Doer: doerFromAuth(s, a), }) err = updateProjectLastUpdated(s, &Project{ID: t.ProjectID}) @@ -2032,10 +2030,9 @@ func triggerTaskUpdatedEventForTaskID(s *xorm.Session, auth web.Auth, taskID int return err } - doer, _ := user.GetFromAuth(auth) events.DispatchOnCommit(s, &TaskUpdatedEvent{ Task: &t, - Doer: doer, + Doer: doerFromAuth(s, auth), }) return nil } diff --git a/pkg/models/team_members.go b/pkg/models/team_members.go index ac2654214..b31929277 100644 --- a/pkg/models/team_members.go +++ b/pkg/models/team_members.go @@ -69,11 +69,10 @@ func (tm *TeamMember) Create(s *xorm.Session, a web.Auth) (err error) { return err } - doer, _ := user2.GetFromAuth(a) events.DispatchOnCommit(s, &TeamMemberAddedEvent{ Team: team, Member: member, - Doer: doer, + Doer: doerFromAuth(s, a), }) return nil } diff --git a/pkg/models/teams.go b/pkg/models/teams.go index 6f73dc3ae..ab0a80846 100644 --- a/pkg/models/teams.go +++ b/pkg/models/teams.go @@ -362,7 +362,7 @@ func (t *Team) Delete(s *xorm.Session, a web.Auth) (err error) { events.DispatchOnCommit(s, &TeamDeletedEvent{ Team: t, - Doer: doerFromAuth(a), + Doer: doerFromAuth(s, a), }) return nil } diff --git a/pkg/models/users.go b/pkg/models/users.go index 84a7101da..b5a79f510 100644 --- a/pkg/models/users.go +++ b/pkg/models/users.go @@ -22,14 +22,24 @@ import ( "xorm.io/xorm" ) -// doerFromAuth converts the authenticated principal into a user for event -// payloads without re-fetching it. A re-fetch would fail its status check in -// flows acting on behalf of disabled accounts (e.g. user deletion), and the -// event only needs the principal as it authenticated. -func doerFromAuth(a web.Auth) *user.User { +// doerFromAuth resolves the authenticated principal into a full user for event payloads. The JWT +// only carries id + username, so without a re-fetch notifications and emails render the +// auto-generated username instead of the display name (#2720). Status errors (disabled/locked) are +// swallowed because their user is still populated and some flows act on behalf of such accounts +// (e.g. user deletion deletes that user's tasks); the partial principal is used as a last resort. +func doerFromAuth(s *xorm.Session, a web.Auth) *user.User { if a == nil { return nil } + + doer, err := GetUserOrLinkShareUser(s, a) + if err != nil && !user.IsErrUserStatusError(err) { + doer = nil + } + if doer != nil && doer.ID != 0 { + return doer + } + if u, is := a.(*user.User); is { return u }