diff --git a/server/channels/app/channel_bookmark.go b/server/channels/app/channel_bookmark.go index 0254a1ac3d..8636e75a12 100644 --- a/server/channels/app/channel_bookmark.go +++ b/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) } } diff --git a/server/channels/app/channel_bookmark_test.go b/server/channels/app/channel_bookmark_test.go index d6730cfbe4..af8ff3867b 100644 --- a/server/channels/app/channel_bookmark_test.go +++ b/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", diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 97dc64c2f3..df4b7a47ae 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/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 } diff --git a/server/channels/store/sqlstore/channel_bookmark_store.go b/server/channels/store/sqlstore/channel_bookmark_store.go index 3196e4d532..770060c06d 100644 --- a/server/channels/store/sqlstore/channel_bookmark_store.go +++ b/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") } diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 6438d77344..1c9b66bbdd 100644 --- a/server/channels/store/store.go +++ b/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 diff --git a/server/channels/store/storetest/channel_bookmark.go b/server/channels/store/storetest/channel_bookmark.go index 2638e83f7c..eacbcf5db6 100644 --- a/server/channels/store/storetest/channel_bookmark.go +++ b/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", diff --git a/server/channels/store/storetest/mocks/ChannelBookmarkStore.go b/server/channels/store/storetest/mocks/ChannelBookmarkStore.go index 11e0983b3c..eb5788765b 100644 --- a/server/channels/store/storetest/mocks/ChannelBookmarkStore.go +++ b/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 -func (_m *ChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string) error { - ret := _m.Called(fileID) +// ErrorIfBookmarkFileInfoAlreadyAttached provides a mock function with given fields: fileID, channelID +func (_m *ChannelBookmarkStore) ErrorIfBookmarkFileInfoAlreadyAttached(fileID string, channelID string) error { + 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 { - r0 = rf(fileID) + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(fileID, channelID) } else { r0 = ret.Error(0) } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 27e5f48745..ddf1822946 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/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 {