fix(user): remove export and avatar files on account deletion

Refs GHSA-7gmh-4fw6-8xc5

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

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
 }
 

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)

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)
+}