diff --git a/pkg/files/files.go b/pkg/files/files.go index 97e96f578..fbb268bf4 100644 --- a/pkg/files/files.go +++ b/pkg/files/files.go @@ -28,6 +28,7 @@ import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/web" @@ -206,7 +207,23 @@ func CreateWithMimeAndSession(s *xorm.Session, f io.ReadSeeker, realname string, // Delete removes a file from the DB and the file system. // The caller is responsible for committing or rolling back the session. -func (f *File) Delete(s *xorm.Session) (err error) { +func (f *File) Delete(s *xorm.Session) error { + if err := f.deleteRow(s); err != nil { + return err + } + return f.removeBlob() +} + +// The blob goes in events.DispatchPending(s), so a rolled back session keeps it. +func (f *File) DeleteOnCommit(s *xorm.Session) error { + if err := f.deleteRow(s); err != nil { + return err + } + events.OnCommit(s, f.removeBlob) + return nil +} + +func (f *File) deleteRow(s *xorm.Session) error { deleted, err := s.Where("id = ?", f.ID).Delete(&File{}) if err != nil { return err @@ -214,8 +231,11 @@ func (f *File) Delete(s *xorm.Session) (err error) { if deleted == 0 { return ErrFileDoesNotExist{FileID: f.ID} } + return nil +} - err = storage.Remove(f.fileID()) +func (f *File) removeBlob() error { + err := storage.Remove(f.fileID()) // After the file, so SavePreview catches a preview stored in between. f.removePreviews() if err != nil { diff --git a/pkg/files/files_test.go b/pkg/files/files_test.go index fe27c5e42..e2a96b36a 100644 --- a/pkg/files/files_test.go +++ b/pkg/files/files_test.go @@ -22,11 +22,13 @@ import ( "image" "image/png" "io" + "io/fs" "net/http" "testing" "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -169,6 +171,51 @@ func TestFile_Delete(t *testing.T) { }) } +func TestFile_DeleteOnCommit(t *testing.T) { + t.Run("removes the blob and previews once dispatched", func(t *testing.T) { + s := db.NewSession() + defer s.Close() + initFixtures(t) + f := &File{ID: 1} + require.NoError(t, f.SavePreview(PreviewMedium, []byte("preview"))) + + require.NoError(t, f.DeleteOnCommit(s)) + require.NoError(t, s.Commit()) + db.AssertMissing(t, "files", map[string]interface{}{"id": 1}) + _, err := FileStat(f) + require.NoError(t, err) + + events.DispatchPending(t.Context(), s) + _, err = FileStat(f) + require.ErrorIs(t, err, fs.ErrNotExist) + _, _, err = f.OpenPreview(PreviewMedium) + require.ErrorIs(t, err, fs.ErrNotExist) + }) + t.Run("keeps row and blob on rollback", func(t *testing.T) { + s := db.NewSession() + defer s.Close() + initFixtures(t) + f := &File{ID: 1} + + require.NoError(t, f.DeleteOnCommit(s)) + require.NoError(t, s.Rollback()) + events.CleanupPending(s) + events.DispatchPending(t.Context(), s) + + db.AssertExists(t, "files", map[string]interface{}{"id": 1}, false) + _, err := FileStat(f) + require.NoError(t, err) + }) + t.Run("Nonexisting", func(t *testing.T) { + s := db.NewSession() + defer s.Close() + initFixtures(t) + + err := (&File{ID: 9999}).DeleteOnCommit(s) + assert.True(t, IsErrFileDoesNotExist(err)) + }) +} + func TestFile_LoadFileByID(t *testing.T) { t.Run("Normal", func(t *testing.T) { initFixtures(t) diff --git a/pkg/modules/avatar/avatar.go b/pkg/modules/avatar/avatar.go index 756f3e16b..ccc8fa3dc 100644 --- a/pkg/modules/avatar/avatar.go +++ b/pkg/modules/avatar/avatar.go @@ -25,6 +25,7 @@ import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/modules/avatar/botmarble" "code.vikunja.io/api/pkg/modules/avatar/empty" @@ -197,6 +198,7 @@ func SyncFromProvider(ctx context.Context, userID int64, provider string, raw [] s := db.NewSession() defer s.Close() + defer events.CleanupPending(s) u, err := user.GetUserByID(s, userID) if err != nil { @@ -212,6 +214,7 @@ func SyncFromProvider(ctx context.Context, userID int64, provider string, raw [] if err := s.Commit(); err != nil { return fmt.Errorf("error committing avatar: %w", err) } + events.DispatchPending(ctx, s) FlushAllCaches(u) diff --git a/pkg/modules/avatar/avatar_test.go b/pkg/modules/avatar/avatar_test.go index d5ebeb075..c1076f670 100644 --- a/pkg/modules/avatar/avatar_test.go +++ b/pkg/modules/avatar/avatar_test.go @@ -20,6 +20,7 @@ import ( "bytes" "image" "image/png" + "io/fs" "testing" "time" @@ -100,6 +101,27 @@ func TestSyncFromProvider(t *testing.T) { assert.Equal(t, 4, cfg.Height) }) + t.Run("removes the replaced avatar file and blob", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + files.InitTestFileFixtures(t) + + setup := db.NewSession() + _, err := setup.ID(1).Cols("avatar_file_id", "avatar_provider").Update(&user.User{AvatarFileID: 1, AvatarProvider: "upload"}) + require.NoError(t, err) + require.NoError(t, setup.Commit()) + setup.Close() + + _, err = files.FileStat(&files.File{ID: 1}) + require.NoError(t, err) + + require.NoError(t, SyncFromProvider(t.Context(), 1, "ldap", raw.Bytes())) + + db.AssertMissing(t, "files", map[string]interface{}{"id": 1}) + _, err = files.FileStat(&files.File{ID: 1}) + require.Error(t, err) + assert.ErrorIs(t, err, fs.ErrNotExist) + }) + t.Run("busy decoder leaves the avatar unchanged", func(t *testing.T) { db.LoadAndAssertFixtures(t) t.Cleanup(imageutils.ExhaustBudgetForTests(10 * time.Millisecond)) diff --git a/pkg/modules/avatar/upload/upload.go b/pkg/modules/avatar/upload/upload.go index 0d7c7b87f..e11f5576f 100644 --- a/pkg/modules/avatar/upload/upload.go +++ b/pkg/modules/avatar/upload/upload.go @@ -158,6 +158,7 @@ func centeredSquare(img image.Image) image.Image { return sub.SubImage(image.Rectangle{Min: topLeft, Max: topLeft.Add(image.Pt(side, side))}) } +// SaveAvatar removes the replaced avatar blob in events.DispatchPending(s). func SaveAvatar(s *xorm.Session, u *user.User, provider string, a *EncodedAvatar) (err error) { oldAvatarFileID := u.AvatarFileID oldProvider := u.AvatarProvider @@ -189,7 +190,7 @@ func SaveAvatar(s *xorm.Session, u *user.User, provider string, a *EncodedAvatar // Log only: the new avatar is already persisted. if oldAvatarFileID != 0 { old := &files.File{ID: oldAvatarFileID} - if delErr := old.Delete(s); delErr != nil && !files.IsErrFileDoesNotExist(delErr) { + if delErr := old.DeleteOnCommit(s); delErr != nil && !files.IsErrFileDoesNotExist(delErr) { log.Errorf("Could not delete old avatar file %d of user %d: %s", oldAvatarFileID, u.ID, delErr) } } diff --git a/pkg/modules/avatar/upload/upload_test.go b/pkg/modules/avatar/upload/upload_test.go index 87c6bfde6..1894c799b 100644 --- a/pkg/modules/avatar/upload/upload_test.go +++ b/pkg/modules/avatar/upload/upload_test.go @@ -30,6 +30,7 @@ import ( "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/log" "code.vikunja.io/api/pkg/modules/imageutils" @@ -221,10 +222,28 @@ func TestSaveAvatar(t *testing.T) { db.AssertExists(t, "users", map[string]interface{}{"id": 1, "avatar_file_id": u.AvatarFileID}, false) db.AssertMissing(t, "files", map[string]interface{}{"id": 1}) _, err := files.FileStat(&files.File{ID: 1}) + require.NoError(t, err, "old avatar blob must survive until the commit is dispatched") + + events.DispatchPending(t.Context(), s) + _, err = files.FileStat(&files.File{ID: 1}) require.Error(t, err, "old avatar blob must be gone") assert.True(t, os.IsNotExist(err)) }) + t.Run("keeps the old avatar when the session rolls back", func(t *testing.T) { + s, u := newUserWithAvatarFile(t) + + require.NoError(t, SaveAvatar(s, u, "upload", prepare(t, encodePNG(t, gradient(2, 2))))) + require.NoError(t, s.Rollback()) + events.CleanupPending(s) + events.DispatchPending(t.Context(), s) + + db.AssertExists(t, "users", map[string]interface{}{"id": 1, "avatar_file_id": 1}, false) + db.AssertExists(t, "files", map[string]interface{}{"id": 1}, false) + _, err := files.FileStat(&files.File{ID: 1}) + require.NoError(t, err, "old avatar blob must still exist") + }) + t.Run("restores the user when the update fails", func(t *testing.T) { s, u := newUserWithAvatarFile(t) oldFileID, oldProvider := u.AvatarFileID, u.AvatarProvider diff --git a/pkg/routes/api/v1/avatar.go b/pkg/routes/api/v1/avatar.go index 89b0c36d6..907af0810 100644 --- a/pkg/routes/api/v1/avatar.go +++ b/pkg/routes/api/v1/avatar.go @@ -18,6 +18,7 @@ package v1 import ( "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/avatar" @@ -104,6 +105,7 @@ func UploadAvatar(c *echo.Context) (err error) { s := db.NewSession() defer s.Close() + defer events.CleanupPending(s) u, err := user.GetUserByID(s, uc.ID) if err != nil { @@ -120,6 +122,7 @@ func UploadAvatar(c *echo.Context) (err error) { _ = s.Rollback() return err } + events.DispatchPending(c.Request().Context(), s) return c.JSON(http.StatusOK, models.Message{Message: "Avatar was uploaded successfully."}) } diff --git a/pkg/routes/api/v2/avatar_upload.go b/pkg/routes/api/v2/avatar_upload.go index 2ea4f377c..36a01d591 100644 --- a/pkg/routes/api/v2/avatar_upload.go +++ b/pkg/routes/api/v2/avatar_upload.go @@ -22,6 +22,7 @@ import ( "net/http" "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/avatar" "code.vikunja.io/api/pkg/user" @@ -81,6 +82,7 @@ func avatarUpload(ctx context.Context, in *avatarUploadInput) (*avatarUploadBody s := db.NewSession() defer s.Close() + defer events.CleanupPending(s) // Re-fetch the full user: the auth user from the JWT claims is partial. u, err := user.GetUserByID(s, authUser.ID) @@ -98,6 +100,7 @@ func avatarUpload(ctx context.Context, in *avatarUploadInput) (*avatarUploadBody _ = s.Rollback() return nil, translateDomainError(err) } + events.DispatchPending(ctx, s) return &avatarUploadBody{Body: &models.Message{Message: "Avatar was uploaded successfully."}}, nil } diff --git a/pkg/webtests/huma_avatar_upload_test.go b/pkg/webtests/huma_avatar_upload_test.go index 93b00fc54..4f33ccffd 100644 --- a/pkg/webtests/huma_avatar_upload_test.go +++ b/pkg/webtests/huma_avatar_upload_test.go @@ -25,6 +25,7 @@ import ( "image" "image/color" "image/png" + "io/fs" "mime/multipart" "net/http" "net/http/httptest" @@ -33,6 +34,7 @@ import ( "time" "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/files" "code.vikunja.io/api/pkg/modules/imageutils" "code.vikunja.io/api/pkg/user" @@ -215,6 +217,43 @@ func pngWithClaimedDimensionsPatch(t *testing.T, width, height int) []byte { return data } +func TestAvatarUploadRemovesReplacedFile(t *testing.T) { + for _, path := range []string{avatarUploadPath, "/api/v1/user/settings/avatar/upload"} { + t.Run(path, func(t *testing.T) { + e, err := setupTestEnv() + require.NoError(t, err) + token := humaTokenFor(t, &testuser1) + + upload := func() int64 { + body, contentType := multipartAvatarBody(t, "avatar", "avatar.png", pngBytes(t)) + req := httptest.NewRequest(http.MethodPut, path, body) + req.Header.Set("Content-Type", contentType) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + + s := db.NewSession() + defer s.Close() + u, err := user.GetUserByID(s, testuser1.ID) + require.NoError(t, err) + return u.AvatarFileID + } + + oldID := upload() + require.NotZero(t, oldID) + _, err = files.FileStat(&files.File{ID: oldID}) + require.NoError(t, err) + + newID := upload() + require.NotEqual(t, oldID, newID) + db.AssertMissing(t, "files", map[string]interface{}{"id": oldID}) + _, err = files.FileStat(&files.File{ID: oldID}) + require.ErrorIs(t, err, fs.ErrNotExist) + }) + } +} + // Guards image allocation bounds (GHSA-4vh2-39rq-rq8j). func TestAvatarUploadImageBounds(t *testing.T) { t.Run("8000 by 8000 avatar upload is rejected", func(t *testing.T) { @@ -305,7 +344,11 @@ func TestAvatarDecodeIsBounded(t *testing.T) { e.ServeHTTP(rec, req) }() - <-ctx.waiting + select { + case <-ctx.waiting: + case <-time.After(5 * time.Second): + require.FailNow(t, "the upload never queued for decode capacity") + } s := db.NewSession() _, err = s.ID(testuser2.ID).Cols("name").Update(&user.User{Name: "written while an upload is queued"}) if err == nil { @@ -319,7 +362,11 @@ func TestAvatarDecodeIsBounded(t *testing.T) { }, false) release() - <-finished + select { + case <-finished: + case <-time.After(10 * time.Second): + require.FailNow(t, "the upload did not finish after the budget was released") + } assert.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) })