diff --git a/pkg/models/export.go b/pkg/models/export.go index 504a02b3c..301b16443 100644 --- a/pkg/models/export.go +++ b/pkg/models/export.go @@ -135,9 +135,8 @@ func ExportUserData(s *xorm.Session, u *user.User) (previousExportFileID int64, return current.ExportFileID, nil } -// deleteExportFile removes an export that is no longer referenced. Errors are only logged since -// the new export already succeeded. -func deleteExportFile(fileID int64) { +// Only logs errors: the caller's change is already committed. +func deleteFileInNewSession(fileID int64) { if fileID == 0 { return } @@ -145,17 +144,27 @@ func deleteExportFile(fileID int64) { s := db.NewSession() defer s.Close() - err := (&files.File{ID: fileID}).Delete(s) - if err != nil && !files.IsErrFileDoesNotExist(err) { + if err := deleteFileIfExists(s, fileID); err != nil { _ = s.Rollback() - log.Errorf("Could not remove previous export file %d: %s", fileID, err) + log.Errorf("Could not remove file %d: %s", fileID, err) return } if err := s.Commit(); err != nil { - log.Errorf("Could not remove previous export file %d: %s", fileID, err) + log.Errorf("Could not remove file %d: %s", fileID, err) } } +func deleteFileIfExists(s *xorm.Session, fileID int64) error { + if fileID == 0 { + return nil + } + err := (&files.File{ID: fileID}).Delete(s) + if err != nil && !files.IsErrFileDoesNotExist(err) { + return err + } + return nil +} + func getRawTasksForExport(s *xorm.Session, projectIDs []int64, a web.Auth) (tasks []*Task, err error) { if len(projectIDs) == 0 { return []*Task{}, nil @@ -544,7 +553,7 @@ func cleanupOldUserExports() { expiredIDs := make([]int64, 0, len(fs)) for _, f := range fs { expiredIDs = append(expiredIDs, f.ID) - err = f.Delete(s) + err = deleteFileIfExists(s, f.ID) if err != nil { log.Errorf(logPrefix+"Could not remove user export file %d: %s", f.ID, err) return diff --git a/pkg/models/listeners.go b/pkg/models/listeners.go index 62b9df8c7..b56decf5f 100644 --- a/pkg/models/listeners.go +++ b/pkg/models/listeners.go @@ -1753,7 +1753,7 @@ func (s *HandleUserDataExport) Handle(msg *message.Message) (err error) { return err } - deleteExportFile(previousExportFileID) + deleteFileInNewSession(previousExportFileID) return nil } diff --git a/pkg/models/user_delete.go b/pkg/models/user_delete.go index 7afceb479..ede6ade68 100644 --- a/pkg/models/user_delete.go +++ b/pkg/models/user_delete.go @@ -20,6 +20,7 @@ import ( "context" "time" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/cron" "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/events" @@ -162,6 +163,11 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) { } } + err = deleteUserFiles(s, u.ID) + if err != nil { + return err + } + // Delete all related entities relatedEntities := []struct { column string @@ -203,6 +209,30 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) { return err } +func deleteUserFiles(s *xorm.Session, userID int64) error { + u := &user.User{} + has, err := s.Where("id = ?", userID).Cols("export_file_id", "avatar_file_id").Get(u) + if err != nil || !has { + return err + } + + fileIDs := []int64{u.ExportFileID} + // The default avatar file is shared by every user. + if u.AvatarFileID != config.DefaultSettingsAvatarFileID.GetInt64() { + fileIDs = append(fileIDs, u.AvatarFileID) + } + + // Blob removal can't roll back, so it waits for the commit. + for _, id := range fileIDs { + events.OnCommit(s, func() error { + deleteFileInNewSession(id) + return nil + }) + } + + return nil +} + func ensureProjectAdminUser(s *xorm.Session, l *Project) (hadUsers bool, err error) { projectUsers := []*ProjectUser{} err = s.Where("project_id = ?", l.ID).Find(&projectUsers) diff --git a/pkg/models/user_delete_test.go b/pkg/models/user_delete_test.go index dc7761069..5d3a89d01 100644 --- a/pkg/models/user_delete_test.go +++ b/pkg/models/user_delete_test.go @@ -17,16 +17,20 @@ package models import ( + "bytes" "context" "testing" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/events" + "code.vikunja.io/api/pkg/files" "code.vikunja.io/api/pkg/notifications" "code.vikunja.io/api/pkg/user" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/xorm" ) func TestDeleteUser(t *testing.T) { @@ -249,3 +253,67 @@ func TestDeleteUser_DoesNotPromoteSharesOnProjectsNotOwned(t *testing.T) { db.AssertExists(t, "team_projects", map[string]interface{}{"project_id": project.ID, "team_id": 9, "permission": PermissionRead}, false) }) } + +// GHSA-7gmh-4fw6-8xc5 +func TestDeleteUser_RemovesExportAndAvatarFiles(t *testing.T) { + setup := func(t *testing.T) (s *xorm.Session, avatar *files.File) { + db.LoadAndAssertFixtures(t) + files.InitTestFileFixtures(t) + notifications.Fake() + t.Cleanup(notifications.Unfake) + + s = db.NewSession() + t.Cleanup(func() { + events.CleanupPending(s) + _ = s.Close() + }) + avatar, err := files.CreateWithSession(s, bytes.NewReader([]byte("avatar")), "avatar.png", 6, &user.User{ID: 4}) + require.NoError(t, err) + _, err = s.ID(4).Cols("export_file_id", "avatar_file_id").Update(&user.User{ExportFileID: 1, AvatarFileID: avatar.ID}) + require.NoError(t, err) + return s, avatar + } + + t.Run("removes both", func(t *testing.T) { + s, avatar := setup(t) + + require.NoError(t, DeleteUser(s, &user.User{ID: 4})) + assertBlobExists(t, 1) + require.NoError(t, s.Commit()) + events.DispatchPending(context.Background(), s) + + db.AssertMissing(t, "files", map[string]interface{}{"id": 1}) + db.AssertMissing(t, "files", map[string]interface{}{"id": avatar.ID}) + assertBlobMissing(t, 1) + assertBlobMissing(t, avatar.ID) + }) + t.Run("keeps the shared default avatar", func(t *testing.T) { + s, avatar := setup(t) + config.DefaultSettingsAvatarFileID.Set(avatar.ID) + t.Cleanup(func() { config.DefaultSettingsAvatarFileID.Set(0) }) + + require.NoError(t, DeleteUser(s, &user.User{ID: 4})) + require.NoError(t, s.Commit()) + events.DispatchPending(context.Background(), s) + + db.AssertExists(t, "files", map[string]interface{}{"id": avatar.ID}, false) + _, err := files.FileStat(avatar) + require.NoError(t, err) + }) + t.Run("keeps files when the deletion rolls back", func(t *testing.T) { + s, _ := setup(t) + + require.NoError(t, DeleteUser(s, &user.User{ID: 4})) + require.NoError(t, s.Rollback()) + events.CleanupPending(s) + + db.AssertExists(t, "files", map[string]interface{}{"id": 1}, false) + assertBlobExists(t, 1) + }) +} + +func assertBlobExists(t *testing.T, fileID int64) { + t.Helper() + _, err := files.FileStat(&files.File{ID: fileID}) + require.NoError(t, err) +}