MM-61375: Update file handling for bookmarks (#30034)

(cherry picked from commit 68c11e9ecb)

server/channels/app/channel_bookmark.go

@@ -55,7 +55,7 @@ func (a *App) UpdateChannelBookmark(c request.CTX, updateBookmark *model.Channel
 		isAnotherFile := updateBookmark.FileInfo != nil && updateBookmark.FileId != "" && updateBookmark.FileId != updateBookmark.FileInfo.Id
 
 		if isAnotherFile {
-			if fileAlreadyAttachedErr := a.Srv().Store().ChannelBookmark().ErrorIfBookmarkFileInfoAlreadyAttached(updateBookmark.FileId); fileAlreadyAttachedErr != nil {
+			if fileAlreadyAttachedErr := a.Srv().Store().ChannelBookmark().ErrorIfBookmarkFileInfoAlreadyAttached(updateBookmark.FileId, updateBookmark.ChannelId); fileAlreadyAttachedErr != nil {
 				return nil, model.NewAppError("UpdateChannelBookmark", "app.channel.bookmark.update.app_error", nil, "", http.StatusInternalServerError).Wrap(fileAlreadyAttachedErr)
 			}
 		}

server/channels/app/channel_bookmark_test.go

@@ -89,6 +89,7 @@ func TestUpdateBookmark(t *testing.T) {
 	var testUpdateAnotherFile = func(th *TestHelper, t *testing.T) {
 		file := &model.FileInfo{
 			Id:              model.NewId(),
+			ChannelId:       th.BasicChannel.Id,
 			CreatorId:       model.BookmarkFileOwner,
 			Path:            "somepath",
 			ThumbnailPath:   "thumbpath",
@@ -116,6 +117,7 @@ func TestUpdateBookmark(t *testing.T) {
 
 		file2 := &model.FileInfo{
 			Id:              model.NewId(),
+			ChannelId:       th.BasicChannel.Id,
 			CreatorId:       model.BookmarkFileOwner,
 			Path:            "somepath",
 			ThumbnailPath:   "thumbpath",
@@ -144,6 +146,106 @@ func TestUpdateBookmark(t *testing.T) {
 		require.Nil(t, bookmarkResp)
 	}
 
+	var testUpdateInvalidFiles = func(th *TestHelper, t *testing.T, creatingUserId string, updatingUserId string) {
+		file := &model.FileInfo{
+			Id:              model.NewId(),
+			ChannelId:       th.BasicChannel.Id,
+			CreatorId:       model.BookmarkFileOwner,
+			Path:            "somepath",
+			ThumbnailPath:   "thumbpath",
+			PreviewPath:     "prevPath",
+			Name:            "test file",
+			Extension:       "png",
+			MimeType:        "images/png",
+			Size:            873182,
+			Width:           3076,
+			Height:          2200,
+			HasPreviewImage: true,
+		}
+
+		_, err := th.App.Srv().Store().FileInfo().Save(th.Context, file)
+		assert.NoError(t, err)
+		defer func() {
+			err = th.App.Srv().Store().FileInfo().PermanentDelete(th.Context, file.Id)
+			assert.NoError(t, err)
+		}()
+
+		th.Context.Session().UserId = creatingUserId
+
+		bookmark := createBookmark("File to be updated", model.ChannelBookmarkFile, th.BasicChannel.Id, file.Id)
+		bookmarkToEdit, appErr := th.App.CreateChannelBookmark(th.Context, bookmark, "")
+		require.Nil(t, appErr)
+		require.NotNil(t, bookmarkToEdit)
+
+		otherChannel := th.CreateChannel(th.Context, th.BasicTeam)
+
+		createAt := time.Now().Add(-1 * time.Minute)
+		deleteAt := createAt.Add(1 * time.Second)
+
+		deletedFile := &model.FileInfo{
+			Id:              model.NewId(),
+			ChannelId:       th.BasicChannel.Id,
+			CreatorId:       model.BookmarkFileOwner,
+			Path:            "somepath",
+			ThumbnailPath:   "thumbpath",
+			PreviewPath:     "prevPath",
+			Name:            "test file",
+			Extension:       "png",
+			MimeType:        "images/png",
+			Size:            873182,
+			Width:           3076,
+			Height:          2200,
+			HasPreviewImage: true,
+			CreateAt:        createAt.UnixMilli(),
+			UpdateAt:        createAt.UnixMilli(),
+			DeleteAt:        deleteAt.UnixMilli(),
+		}
+
+		_, err = th.App.Srv().Store().FileInfo().Save(th.Context, deletedFile)
+		assert.NoError(t, err)
+		defer func() {
+			err = th.App.Srv().Store().FileInfo().PermanentDelete(th.Context, deletedFile.Id)
+			assert.NoError(t, err)
+		}()
+
+		th.Context.Session().UserId = updatingUserId
+
+		updateBookmarkPending := bookmarkToEdit.Clone()
+		updateBookmarkPending.FileId = deletedFile.Id
+		bookmarkEdited, appErr := th.App.UpdateChannelBookmark(th.Context, updateBookmarkPending, "")
+		assert.NotNil(t, appErr)
+		require.Nil(t, bookmarkEdited)
+
+		anotherChannelFile := &model.FileInfo{
+			Id:              model.NewId(),
+			ChannelId:       otherChannel.Id,
+			CreatorId:       model.BookmarkFileOwner,
+			Path:            "somepath",
+			ThumbnailPath:   "thumbpath",
+			PreviewPath:     "prevPath",
+			Name:            "test file",
+			Extension:       "png",
+			MimeType:        "images/png",
+			Size:            873182,
+			Width:           3076,
+			Height:          2200,
+			HasPreviewImage: true,
+		}
+
+		_, err = th.App.Srv().Store().FileInfo().Save(th.Context, anotherChannelFile)
+		assert.NoError(t, err)
+		defer func() {
+			err = th.App.Srv().Store().FileInfo().PermanentDelete(th.Context, anotherChannelFile.Id)
+			require.NoError(t, err)
+		}()
+
+		updateBookmarkPending = bookmarkToEdit.Clone()
+		updateBookmarkPending.FileId = anotherChannelFile.Id
+		bookmarkEdited, appErr = th.App.UpdateChannelBookmark(th.Context, updateBookmarkPending, "")
+		assert.NotNil(t, appErr)
+		require.Nil(t, bookmarkEdited)
+	}
+
 	t.Run("same user update a channel bookmark", func(t *testing.T) {
 		bookmark1 := &model.ChannelBookmark{
 			ChannelId:   th.BasicChannel.Id,
@@ -166,6 +268,8 @@ func TestUpdateBookmark(t *testing.T) {
 		assert.Greater(t, response.Updated.UpdateAt, response.Updated.CreateAt)
 
 		testUpdateAnotherFile(th, t)
+
+		testUpdateInvalidFiles(th, t, th.BasicUser.Id, th.BasicUser.Id)
 	})
 
 	t.Run("another user update a channel bookmark", func(t *testing.T) {
@@ -181,6 +285,8 @@ func TestUpdateBookmark(t *testing.T) {
 		assert.Equal(t, "New name", response.Deleted.DisplayName)
 
 		testUpdateAnotherFile(th, t)
+
+		testUpdateInvalidFiles(th, t, th.BasicUser.Id, th.BasicUser.Id)
 	})
 
 	t.Run("update an already deleted channel bookmark", func(t *testing.T) {
@@ -265,6 +371,7 @@ func TestGetChannelBookmarks(t *testing.T) {
 
 	file := &model.FileInfo{
 		Id:              model.NewId(),
+		ChannelId:       th.BasicChannel.Id,
 		CreatorId:       model.BookmarkFileOwner,
 		Path:            "somepath",
 		ThumbnailPath:   "thumbpath",
@@ -346,6 +453,7 @@ func TestUpdateChannelBookmarkSortOrder(t *testing.T) {
 
 	file := &model.FileInfo{
 		Id:              model.NewId(),
+		ChannelId:       th.BasicChannel.Id,
 		CreatorId:       model.BookmarkFileOwner,
 		Path:            "somepath",
 		ThumbnailPath:   "thumbpath",

server/channels/store/retrylayer/retrylayer.go

@@ -3132,11 +3132,11 @@ func (s *RetryLayerChannelBookmarkStore) Delete(bookmarkID string, deleteFile bo
 
 }
 
-func (s *RetryLayerChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string) error {
+func (s *RetryLayerChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string, channelID string) error {
 
 	tries := 0
 	for {
-		err := s.ChannelBookmarkStore.ErrorIfBookmarkFileInfoAlreadyAttached(fileID)
+		err := s.ChannelBookmarkStore.ErrorIfBookmarkFileInfoAlreadyAttached(fileID, channelID)
 		if err == nil {
 			return nil
 		}

server/channels/store/sqlstore/channel_bookmark_store.go

@@ -51,7 +51,7 @@ func bookmarkWithFileInfoSliceColumns() []string {
 	}
 }
 
-func (s *SqlChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileId string) error {
+func (s *SqlChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileId string, channelId string) error {
 	existingQuery := s.getSubQueryBuilder().
 		Select("FileInfoId").
 		From("ChannelBookmarks").
@@ -66,11 +66,13 @@ func (s *SqlChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileId
 		Where(sq.Or{
 			sq.Expr("Id IN (?)", existingQuery),
 			sq.And{
+				sq.Eq{"Id": fileId},
 				sq.Or{
 					sq.NotEq{"PostId": ""},
 					sq.NotEq{"CreatorId": model.BookmarkFileOwner},
+					sq.NotEq{"ChannelId": channelId},
+					sq.NotEq{"DeleteAt": 0},
 				},
-				sq.Eq{"Id": fileId},
 			},
 		})
 
@@ -139,7 +141,7 @@ func (s *SqlChannelBookmarkStore) Save(bookmark *model.ChannelBookmark, increase
 	}
 
 	if bookmark.FileId != "" {
-		err = s.ErrorIfBookmarkFileInfoAlreadyAttached(bookmark.FileId)
+		err = s.ErrorIfBookmarkFileInfoAlreadyAttached(bookmark.FileId, bookmark.ChannelId)
 		if err != nil {
 			return nil, errors.Wrap(err, "unable_to_save_channel_bookmark")
 		}

server/channels/store/store.go

@@ -1053,7 +1053,7 @@ type PostPersistentNotificationStore interface {
 	DeleteByTeam(teamIds []string) error
 }
 type ChannelBookmarkStore interface {
-	ErrorIfBookmarkFileInfoAlreadyAttached(fileID string) error
+	ErrorIfBookmarkFileInfoAlreadyAttached(fileID string, channelID string) error
 	Get(Id string, includeDeleted bool) (b *model.ChannelBookmarkWithFileInfo, err error)
 	Save(bookmark *model.ChannelBookmark, increaseSortOrder bool) (b *model.ChannelBookmarkWithFileInfo, err error)
 	Update(bookmark *model.ChannelBookmark) error

server/channels/store/storetest/channel_bookmark.go

@@ -34,8 +34,12 @@ func TestChannelBookmarkStore(t *testing.T, rctx request.CTX, ss store.Store, s
 
 func testSaveChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 	channelID := model.NewId()
+	otherChannelID := model.NewId()
 	userID := model.NewId()
 
+	createAt := time.Now().Add(-1 * time.Minute)
+	deleteAt := createAt.Add(1 * time.Second)
+
 	bookmark1 := &model.ChannelBookmark{
 		ChannelId:   channelID,
 		OwnerId:     userID,
@@ -47,6 +51,7 @@ func testSaveChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 
 	file := &model.FileInfo{
 		Id:              model.NewId(),
+		ChannelId:       channelID,
 		CreatorId:       model.BookmarkFileOwner,
 		Path:            "somepath",
 		ThumbnailPath:   "thumbpath",
@@ -80,6 +85,7 @@ func testSaveChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 
 	file2 := &model.FileInfo{
 		Id:              model.NewId(),
+		ChannelId:       channelID,
 		CreatorId:       userID,
 		Path:            "somepath",
 		ThumbnailPath:   "thumbpath",
@@ -102,6 +108,60 @@ func testSaveChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 		Emoji:       ":smile:",
 	}
 
+	deletedFile := &model.FileInfo{
+		Id:              model.NewId(),
+		ChannelId:       channelID,
+		CreatorId:       model.BookmarkFileOwner,
+		Path:            "somepath",
+		ThumbnailPath:   "thumbpath",
+		PreviewPath:     "prevPath",
+		Name:            "test file",
+		Extension:       "png",
+		MimeType:        "images/png",
+		Size:            873182,
+		Width:           3076,
+		Height:          2200,
+		HasPreviewImage: true,
+		CreateAt:        createAt.UnixMilli(),
+		UpdateAt:        createAt.UnixMilli(),
+		DeleteAt:        deleteAt.UnixMilli(),
+	}
+
+	bookmarkFileDeleted := &model.ChannelBookmark{
+		ChannelId:   channelID,
+		OwnerId:     userID,
+		DisplayName: "file deleted",
+		FileId:      deletedFile.Id,
+		Type:        model.ChannelBookmarkFile,
+		Emoji:       ":smile:",
+	}
+
+	// another channel
+	anotherChannelFile := &model.FileInfo{
+		Id:              model.NewId(),
+		ChannelId:       otherChannelID,
+		CreatorId:       model.BookmarkFileOwner,
+		Path:            "somepath",
+		ThumbnailPath:   "thumbpath",
+		PreviewPath:     "prevPath",
+		Name:            "test file",
+		Extension:       "png",
+		MimeType:        "images/png",
+		Size:            873182,
+		Width:           3076,
+		Height:          2200,
+		HasPreviewImage: true,
+	}
+
+	bookmarkFileAnotherChannel := &model.ChannelBookmark{
+		ChannelId:   channelID,
+		OwnerId:     userID,
+		DisplayName: "file another channel",
+		FileId:      anotherChannelFile.Id,
+		Type:        model.ChannelBookmarkFile,
+		Emoji:       ":smile:",
+	}
+
 	_, err := ss.FileInfo().Save(rctx, file)
 	require.NoError(t, err)
 	defer ss.FileInfo().PermanentDelete(rctx, file.Id)
@@ -113,6 +173,14 @@ func testSaveChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 	err = ss.FileInfo().AttachToPost(rctx, file2.Id, model.NewId(), channelID, userID)
 	require.NoError(t, err)
 
+	_, err = ss.FileInfo().Save(rctx, deletedFile)
+	require.NoError(t, err)
+	defer ss.FileInfo().PermanentDelete(rctx, deletedFile.Id)
+
+	_, err = ss.FileInfo().Save(rctx, anotherChannelFile)
+	require.NoError(t, err)
+	defer ss.FileInfo().PermanentDelete(rctx, anotherChannelFile.Id)
+
 	t.Run("save bookmarks", func(t *testing.T) {
 		bookmarkResp, err := ss.ChannelBookmark().Save(bookmark1.Clone(), true)
 		assert.NoError(t, err)
@@ -137,6 +205,12 @@ func testSaveChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 
 		_, err = ss.ChannelBookmark().Save(bookmark4.Clone(), true)
 		assert.Error(t, err) // Error as the file is attached to a post
+
+		_, err = ss.ChannelBookmark().Save(bookmarkFileDeleted.Clone(), true)
+		assert.Error(t, err) // Error as the file is deleted
+
+		_, err = ss.ChannelBookmark().Save(bookmarkFileAnotherChannel.Clone(), true)
+		assert.Error(t, err) // Error as the file is from another channel
 	})
 }
 
@@ -204,6 +278,7 @@ func testUpdateSortOrderChannelBookmark(t *testing.T, rctx request.CTX, ss store
 
 	file := &model.FileInfo{
 		Id:              model.NewId(),
+		ChannelId:       channelID,
 		CreatorId:       model.BookmarkFileOwner,
 		Path:            "somepath",
 		ThumbnailPath:   "thumbpath",
@@ -391,6 +466,7 @@ func testDeleteChannelBookmark(t *testing.T, rctx request.CTX, ss store.Store) {
 
 	file := &model.FileInfo{
 		Id:              model.NewId(),
+		ChannelId:       channelID,
 		CreatorId:       model.BookmarkFileOwner,
 		Path:            "somepath",
 		ThumbnailPath:   "thumbpath",

server/channels/store/storetest/mocks/ChannelBookmarkStore.go

@@ -32,17 +32,17 @@ func (_m *ChannelBookmarkStore) Delete(bookmarkID string, deleteFile bool) error
 	return r0
 }
 
-// ErrorIfBookmarkFileInfoAlreadyAttached provides a mock function with given fields: fileID
+// ErrorIfBookmarkFileInfoAlreadyAttached provides a mock function with given fields: fileID, channelID
-func (_m *ChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string) error {
+func (_m *ChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string, channelID string) error {
-	ret := _m.Called(fileID)
+	ret := _m.Called(fileID, channelID)
 
 	if len(ret) == 0 {
 		panic("no return value specified for ErrorIfBookmarkFileInfoAlreadyAttached")
 	}
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(string) error); ok {
+	if rf, ok := ret.Get(0).(func(string, string) error); ok {
-		r0 = rf(fileID)
+		r0 = rf(fileID, channelID)
 	} else {
 		r0 = ret.Error(0)
 	}

server/channels/store/timerlayer/timerlayer.go

@@ -2598,10 +2598,10 @@ func (s *TimerLayerChannelBookmarkStore) Delete(bookmarkID string, deleteFile bo
 	return err
 }
 
-func (s *TimerLayerChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string) error {
+func (s *TimerLayerChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string, channelID string) error {
 	start := time.Now()
 
-	err := s.ChannelBookmarkStore.ErrorIfBookmarkFileInfoAlreadyAttached(fileID)
+	err := s.ChannelBookmarkStore.ErrorIfBookmarkFileInfoAlreadyAttached(fileID, channelID)
 
 	elapsed := float64(time.Since(start)) / float64(time.Second)
 	if s.Root.Metrics != nil {