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
This commit is contained in:
parent
80bb9aadc1
commit
5236e0c306
|
|
@ -76,6 +76,18 @@ func ClearDispatchedEvents() {
|
||||||
dispatchedTestEvents = nil
|
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.
|
// CountDispatchedEvents counts how many events of a specific type have been dispatched.
|
||||||
func CountDispatchedEvents(eventName string) int {
|
func CountDispatchedEvents(eventName string) int {
|
||||||
count := 0
|
count := 0
|
||||||
|
|
|
||||||
|
|
@ -21,7 +21,6 @@ import (
|
||||||
|
|
||||||
"code.vikunja.io/api/pkg/db"
|
"code.vikunja.io/api/pkg/db"
|
||||||
"code.vikunja.io/api/pkg/events"
|
"code.vikunja.io/api/pkg/events"
|
||||||
"code.vikunja.io/api/pkg/user"
|
|
||||||
"code.vikunja.io/api/pkg/web"
|
"code.vikunja.io/api/pkg/web"
|
||||||
"xorm.io/xorm"
|
"xorm.io/xorm"
|
||||||
)
|
)
|
||||||
|
|
@ -252,10 +251,9 @@ func (b *TaskBucket) Update(s *xorm.Session, a web.Auth) (err error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
if b.Task != nil {
|
if b.Task != nil {
|
||||||
doer, _ := user.GetFromAuth(a)
|
|
||||||
events.DispatchOnCommit(s, &TaskUpdatedEvent{
|
events.DispatchOnCommit(s, &TaskUpdatedEvent{
|
||||||
Task: b.Task,
|
Task: b.Task,
|
||||||
Doer: doer,
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
return nil
|
return nil
|
||||||
|
|
|
||||||
|
|
@ -1072,7 +1072,7 @@ func CreateProject(s *xorm.Session, project *Project, auth web.Auth, createBackl
|
||||||
|
|
||||||
events.DispatchOnCommit(s, &ProjectCreatedEvent{
|
events.DispatchOnCommit(s, &ProjectCreatedEvent{
|
||||||
Project: project,
|
Project: project,
|
||||||
Doer: doer,
|
Doer: doerFromAuth(s, auth),
|
||||||
})
|
})
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
@ -1219,7 +1219,7 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje
|
||||||
|
|
||||||
events.DispatchOnCommit(s, &ProjectUpdatedEvent{
|
events.DispatchOnCommit(s, &ProjectUpdatedEvent{
|
||||||
Project: project,
|
Project: project,
|
||||||
Doer: doerFromAuth(auth),
|
Doer: doerFromAuth(s, auth),
|
||||||
})
|
})
|
||||||
|
|
||||||
l, err := GetProjectSimpleByID(s, project.ID)
|
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{
|
events.DispatchOnCommit(s, &ProjectDeletedEvent{
|
||||||
Project: fullProject,
|
Project: fullProject,
|
||||||
Doer: doerFromAuth(a),
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
|
|
||||||
childProjects := []*Project{}
|
childProjects := []*Project{}
|
||||||
|
|
|
||||||
|
|
@ -112,7 +112,7 @@ func (tl *TeamProject) Create(s *xorm.Session, a web.Auth) (err error) {
|
||||||
events.DispatchOnCommit(s, &ProjectSharedWithTeamEvent{
|
events.DispatchOnCommit(s, &ProjectSharedWithTeamEvent{
|
||||||
Project: l,
|
Project: l,
|
||||||
Team: team,
|
Team: team,
|
||||||
Doer: doerFromAuth(a),
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
|
|
||||||
err = updateProjectLastUpdated(s, l)
|
err = updateProjectLastUpdated(s, l)
|
||||||
|
|
|
||||||
|
|
@ -118,7 +118,7 @@ func (lu *ProjectUser) Create(s *xorm.Session, a web.Auth) (err error) {
|
||||||
events.DispatchOnCommit(s, &ProjectSharedWithUserEvent{
|
events.DispatchOnCommit(s, &ProjectSharedWithUserEvent{
|
||||||
Project: l,
|
Project: l,
|
||||||
User: u,
|
User: u,
|
||||||
Doer: doerFromAuth(a),
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
|
|
||||||
err = updateProjectLastUpdated(s, l)
|
err = updateProjectLastUpdated(s, l)
|
||||||
|
|
|
||||||
|
|
@ -181,7 +181,7 @@ func (la *TaskAssginee) Delete(s *xorm.Session, a web.Auth) (err error) {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
doer, _ := user.GetFromAuth(a)
|
doer := doerFromAuth(s, a)
|
||||||
task, err := GetTaskByIDSimple(s, la.TaskID)
|
task, err := GetTaskByIDSimple(s, la.TaskID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
|
|
@ -270,7 +270,7 @@ func (t *Task) addNewAssigneeByID(s *xorm.Session, newAssigneeID int64, project
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
doer, _ := user.GetFromAuth(auth)
|
doer := doerFromAuth(s, auth)
|
||||||
task, err := GetTaskSimple(s, &Task{ID: t.ID})
|
task, err := GetTaskSimple(s, &Task{ID: t.ID})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
|
|
|
||||||
|
|
@ -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 <https://www.gnu.org/licenses/>.
|
||||||
|
|
||||||
|
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)
|
||||||
|
}
|
||||||
|
|
@ -1462,10 +1462,9 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e
|
||||||
}
|
}
|
||||||
t.Updated = nt.Updated
|
t.Updated = nt.Updated
|
||||||
|
|
||||||
doer, _ := user.GetFromAuth(a)
|
|
||||||
events.DispatchOnCommit(s, &TaskUpdatedEvent{
|
events.DispatchOnCommit(s, &TaskUpdatedEvent{
|
||||||
Task: t,
|
Task: t,
|
||||||
Doer: doer,
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
|
|
||||||
return updateProjectLastUpdated(s, &Project{ID: t.ProjectID})
|
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
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
doer, _ := user.GetFromAuth(a)
|
|
||||||
events.DispatchOnCommit(s, &TaskDeletedEvent{
|
events.DispatchOnCommit(s, &TaskDeletedEvent{
|
||||||
Task: fullTask,
|
Task: fullTask,
|
||||||
Doer: doer,
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
|
|
||||||
err = updateProjectLastUpdated(s, &Project{ID: t.ProjectID})
|
err = updateProjectLastUpdated(s, &Project{ID: t.ProjectID})
|
||||||
|
|
@ -2032,10 +2030,9 @@ func triggerTaskUpdatedEventForTaskID(s *xorm.Session, auth web.Auth, taskID int
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
doer, _ := user.GetFromAuth(auth)
|
|
||||||
events.DispatchOnCommit(s, &TaskUpdatedEvent{
|
events.DispatchOnCommit(s, &TaskUpdatedEvent{
|
||||||
Task: &t,
|
Task: &t,
|
||||||
Doer: doer,
|
Doer: doerFromAuth(s, auth),
|
||||||
})
|
})
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -69,11 +69,10 @@ func (tm *TeamMember) Create(s *xorm.Session, a web.Auth) (err error) {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
||||||
doer, _ := user2.GetFromAuth(a)
|
|
||||||
events.DispatchOnCommit(s, &TeamMemberAddedEvent{
|
events.DispatchOnCommit(s, &TeamMemberAddedEvent{
|
||||||
Team: team,
|
Team: team,
|
||||||
Member: member,
|
Member: member,
|
||||||
Doer: doer,
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -362,7 +362,7 @@ func (t *Team) Delete(s *xorm.Session, a web.Auth) (err error) {
|
||||||
|
|
||||||
events.DispatchOnCommit(s, &TeamDeletedEvent{
|
events.DispatchOnCommit(s, &TeamDeletedEvent{
|
||||||
Team: t,
|
Team: t,
|
||||||
Doer: doerFromAuth(a),
|
Doer: doerFromAuth(s, a),
|
||||||
})
|
})
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -22,14 +22,24 @@ import (
|
||||||
"xorm.io/xorm"
|
"xorm.io/xorm"
|
||||||
)
|
)
|
||||||
|
|
||||||
// doerFromAuth converts the authenticated principal into a user for event
|
// doerFromAuth resolves the authenticated principal into a full user for event payloads. The JWT
|
||||||
// payloads without re-fetching it. A re-fetch would fail its status check in
|
// only carries id + username, so without a re-fetch notifications and emails render the
|
||||||
// flows acting on behalf of disabled accounts (e.g. user deletion), and the
|
// auto-generated username instead of the display name (#2720). Status errors (disabled/locked) are
|
||||||
// event only needs the principal as it authenticated.
|
// swallowed because their user is still populated and some flows act on behalf of such accounts
|
||||||
func doerFromAuth(a web.Auth) *user.User {
|
// (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 {
|
if a == nil {
|
||||||
return 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 {
|
if u, is := a.(*user.User); is {
|
||||||
return u
|
return u
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue