Edit attachment permission (#36227) (#36723)

Automatic Merge

server/channels/api4/data_retention_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/mattermost/mattermost/server/v8/channels/utils"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -1057,28 +1058,10 @@ func TestAddChannelsToPolicy(t *testing.T) {
 	validChannelIDs := []string{model.NewId(), model.NewId()}
 	invalidChannelIDs := []string{"invalid_channel_id"}
 
-	// Custom function to compare slices regardless of order
-	unorderedSlicesEqual := func(a, b []string) bool {
-		if len(a) != len(b) {
-			return false
-		}
-		counts := make(map[string]int)
-		for _, item := range a {
-			counts[item]++
-		}
-		for _, item := range b {
-			counts[item]--
-			if counts[item] < 0 {
-				return false
-			}
-		}
-		return true
-	}
-
 	// Custom matcher for unordered slice comparison
 	unorderedSliceMatcher := func(expected []string) func(actual []string) bool {
 		return func(actual []string) bool {
-			return unorderedSlicesEqual(expected, actual)
+			return utils.SliceEqualUnordered(expected, actual)
 		}
 	}
 

server/channels/api4/post.go

@@ -1065,6 +1065,12 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Check edit_file_attachment permission if file IDs are being changed (files added or removed)
+	checkEditFileAttachmentPermission(c, post.FileIds, originalPost)
+	if c.Err != nil {
+		return
+	}
+
 	if c.AppContext.Session().UserId != originalPost.UserId {
 		// We don't need to check the member here, since we already checked it above
 		if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditOthersPosts); !ok {
@@ -1139,6 +1145,11 @@ func patchPost(c *Context, w http.ResponseWriter, r *http.Request) {
 		if c.Err != nil {
 			return
 		}
+
+		checkEditFileAttachmentPermission(c, *post.FileIds, originalPost)
+		if c.Err != nil {
+			return
+		}
 	}
 
 	patchedPost, isMemberForPReviews, err := c.App.PatchPost(c.AppContext, c.Params.PostId, c.App.PostPatchWithProxyRemovedFromImageURLs(&post), nil)

server/channels/api4/post_test.go

@@ -1900,6 +1900,124 @@ func TestUpdatePost(t *testing.T) {
 		require.Equal(t, int64(0), postFileInfos[0].DeleteAt)
 	})
 
+	t.Run("should prevent adding files when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: channel.Id,
+			Message:   "Post without files",
+		}, channel, model.CreatePostFlags{SetOnline: true})
+		require.Nil(t, appErr)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		updatePost := &model.Post{
+			Id:        postWithoutFiles.Id,
+			ChannelId: channel.Id,
+			Message:   "Updated post with file",
+			FileIds:   model.StringArray{fileId},
+		}
+		_, resp, err := client.UpdatePost(context.Background(), postWithoutFiles.Id, updatePost)
+		require.Error(t, err)
+		require.Equal(t, http.StatusForbidden, resp.StatusCode)
+		require.Equal(t, "You do not have the appropriate permissions.", err.Error())
+		CheckForbiddenStatus(t, resp)
+	})
+
+	t.Run("should prevent removing files when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postWithFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: channel.Id,
+			Message:   "Post with files",
+			FileIds:   model.StringArray{fileId},
+		}, channel, model.CreatePostFlags{SetOnline: true})
+		require.Nil(t, appErr)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		updatePost := &model.Post{
+			Id:        postWithFiles.Id,
+			ChannelId: channel.Id,
+			Message:   "Updated post without file",
+			FileIds:   model.StringArray{},
+		}
+		_, resp, err := client.UpdatePost(context.Background(), postWithFiles.Id, updatePost)
+		require.Error(t, err)
+		require.Equal(t, http.StatusForbidden, resp.StatusCode)
+		require.Equal(t, "You do not have the appropriate permissions.", err.Error())
+		CheckForbiddenStatus(t, resp)
+	})
+
+	t.Run("should allow updating post with unchanged files when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postWithFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: channel.Id,
+			Message:   "Post with files",
+			FileIds:   model.StringArray{fileId},
+		}, channel, model.CreatePostFlags{SetOnline: true})
+		require.Nil(t, appErr)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		updatePost := &model.Post{
+			Id:        postWithFiles.Id,
+			ChannelId: channel.Id,
+			Message:   "Updated message only",
+			FileIds:   model.StringArray{fileId},
+		}
+		updatedPost, resp, err := client.UpdatePost(context.Background(), postWithFiles.Id, updatePost)
+		require.NoError(t, err)
+		CheckOKStatus(t, resp)
+		require.NotNil(t, updatedPost)
+		assert.Equal(t, "Updated message only", updatedPost.Message)
+	})
+
+	t.Run("should allow changing files when edit_file_attachment permission is present", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postWithoutFiles, _, appErr := th.App.CreatePost(th.Context, &model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: channel.Id,
+			Message:   "Post without files",
+		}, channel, model.CreatePostFlags{SetOnline: true})
+		require.Nil(t, appErr)
+
+		updatePost := &model.Post{
+			Id:        postWithoutFiles.Id,
+			ChannelId: channel.Id,
+			Message:   "Updated post with file",
+			FileIds:   model.StringArray{fileId},
+		}
+		updatedPost, resp, err := client.UpdatePost(context.Background(), postWithoutFiles.Id, updatePost)
+		require.NoError(t, err)
+		CheckOKStatus(t, resp)
+		require.NotNil(t, updatedPost)
+	})
+
 	t.Run("should be able to add and remove files simultaneously", func(t *testing.T) {
 		th.LoginBasic()
 		// create new file
@@ -2159,6 +2277,129 @@ func TestPatchPost(t *testing.T) {
 		CheckForbiddenStatus(t, resp)
 	})
 
+	t.Run("should prevent patching file ids when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postToEdit, _, err := client.CreatePost(context.Background(), &model.Post{
+			ChannelId: channel.Id,
+			Message:   "original message",
+		})
+		require.NoError(t, err)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		patch := &model.PostPatch{
+			FileIds: &model.StringArray{fileId},
+		}
+		_, resp, err := client.PatchPost(context.Background(), postToEdit.Id, patch)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, resp)
+	})
+
+	t.Run("should prevent removing files via patch when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postToEdit, _, err := client.CreatePost(context.Background(), &model.Post{
+			ChannelId: channel.Id,
+			Message:   "post with file",
+			FileIds:   model.StringArray{fileId},
+		})
+		require.NoError(t, err)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		emptyFileIds := model.StringArray{}
+		patch := &model.PostPatch{
+			FileIds: &emptyFileIds,
+		}
+		_, resp, err := client.PatchPost(context.Background(), postToEdit.Id, patch)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, resp)
+	})
+
+	t.Run("should allow patching message without file change when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postToEdit, _, err := client.CreatePost(context.Background(), &model.Post{
+			ChannelId: channel.Id,
+			Message:   "original message",
+			FileIds:   model.StringArray{fileId},
+		})
+		require.NoError(t, err)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		patch := &model.PostPatch{
+			Message: model.NewPointer("updated message only"),
+		}
+		patchedPost, _, err := client.PatchPost(context.Background(), postToEdit.Id, patch)
+		require.NoError(t, err)
+		assert.Equal(t, "updated message only", patchedPost.Message)
+	})
+
+	t.Run("should allow patching with same file ids when edit_file_attachment permission is revoked", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postToEdit, _, err := client.CreatePost(context.Background(), &model.Post{
+			ChannelId: channel.Id,
+			Message:   "original message",
+			FileIds:   model.StringArray{fileId},
+		})
+		require.NoError(t, err)
+
+		th.RemovePermissionFromRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+		defer th.AddPermissionToRole(model.PermissionEditFileAttachment.Id, model.ChannelUserRoleId)
+
+		sameFileIds := model.StringArray{fileId}
+		patch := &model.PostPatch{
+			Message: model.NewPointer("updated message"),
+			FileIds: &sameFileIds,
+		}
+		patchedPost, _, err := client.PatchPost(context.Background(), postToEdit.Id, patch)
+		require.NoError(t, err)
+		assert.Equal(t, "updated message", patchedPost.Message)
+	})
+
+	t.Run("should allow patching files when edit_file_attachment permission is present", func(t *testing.T) {
+		th.LoginBasic()
+
+		fileResp, _, err := client.UploadFile(context.Background(), data, channel.Id, "test.png")
+		require.NoError(t, err)
+		fileId := fileResp.FileInfos[0].Id
+
+		postToEdit, _, err := client.CreatePost(context.Background(), &model.Post{
+			ChannelId: channel.Id,
+			Message:   "original message",
+		})
+		require.NoError(t, err)
+
+		patch := &model.PostPatch{
+			FileIds: &model.StringArray{fileId},
+		}
+		patchedPost, _, err := client.PatchPost(context.Background(), postToEdit.Id, patch)
+		require.NoError(t, err)
+		require.NotNil(t, patchedPost)
+	})
+
 	t.Run("time limit expired", func(t *testing.T) {
 		th.App.UpdateConfig(func(cfg *model.Config) {
 			*cfg.ServiceSettings.PostEditTimeLimit = 1

server/channels/api4/post_utils.go

@@ -6,6 +6,7 @@ package api4
 import (
 	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/mattermost/mattermost/server/v8/channels/app"
+	"github.com/mattermost/mattermost/server/v8/channels/utils"
 )
 
 func userCreatePostPermissionCheckWithContext(c *Context, channelId string) {
@@ -69,3 +70,14 @@ func checkUploadFilePermissionForNewFiles(c *Context, newFileIds []string, origi
 		}
 	}
 }
+
+// checkEditFileAttachmentPermission checks edit_file_attachment permission
+// when file IDs are being changed (files added or removed) during post edit.
+func checkEditFileAttachmentPermission(c *Context, newFileIds []string, originalPost *model.Post) {
+	if utils.SliceEqualUnordered(newFileIds, originalPost.FileIds) {
+		return
+	}
+	if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), originalPost.ChannelId, model.PermissionEditFileAttachment); !ok {
+		c.SetPermissionError(model.PermissionEditFileAttachment)
+	}
+}

server/channels/api4/post_utils_test.go

@@ -0,0 +1,94 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package api4
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost/server/v8/channels/utils"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSameFileIDs(t *testing.T) {
+	tests := []struct {
+		name     string
+		a        []string
+		b        []string
+		expected bool
+	}{
+		{
+			name:     "both empty",
+			a:        []string{},
+			b:        []string{},
+			expected: true,
+		},
+		{
+			name:     "both nil",
+			a:        nil,
+			b:        nil,
+			expected: true,
+		},
+		{
+			name:     "same files same order",
+			a:        []string{"file1", "file2", "file3"},
+			b:        []string{"file1", "file2", "file3"},
+			expected: true,
+		},
+		{
+			name:     "same files different order",
+			a:        []string{"file3", "file1", "file2"},
+			b:        []string{"file1", "file2", "file3"},
+			expected: true,
+		},
+		{
+			name:     "one file added",
+			a:        []string{"file1", "file2", "file3"},
+			b:        []string{"file1", "file2"},
+			expected: false,
+		},
+		{
+			name:     "one file removed",
+			a:        []string{"file1"},
+			b:        []string{"file1", "file2"},
+			expected: false,
+		},
+		{
+			name:     "different files same length",
+			a:        []string{"file1", "file2"},
+			b:        []string{"file1", "file3"},
+			expected: false,
+		},
+		{
+			name:     "duplicate IDs in a",
+			a:        []string{"file1", "file1"},
+			b:        []string{"file1", "file2"},
+			expected: false,
+		},
+		{
+			name:     "duplicate IDs same in both",
+			a:        []string{"file1", "file1"},
+			b:        []string{"file1", "file1"},
+			expected: true,
+		},
+		{
+			name:     "empty vs non-empty",
+			a:        []string{},
+			b:        []string{"file1"},
+			expected: false,
+		},
+		{
+			name:     "nil vs non-empty",
+			a:        nil,
+			b:        []string{"file1"},
+			expected: false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			result := utils.SliceEqualUnordered(tc.a, tc.b)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}

server/channels/app/app_test.go

@@ -129,6 +129,7 @@ func TestDoAdvancedPermissionsMigration(t *testing.T) {
 			model.PermissionManagePrivateChannelMembers.Id,
 			model.PermissionDeletePost.Id,
 			model.PermissionEditPost.Id,
+			model.PermissionEditFileAttachment.Id,
 			model.PermissionAddBookmarkPublicChannel.Id,
 			model.PermissionEditBookmarkPublicChannel.Id,
 			model.PermissionDeleteBookmarkPublicChannel.Id,

server/channels/app/permissions_migrations.go

@@ -1206,6 +1206,15 @@ func (a *App) getRestrictAcessToChannelConversionToPublic() (permissionsMap, err
 	}, nil
 }
 
+func (a *App) getAddEditFileAttachmentPermissionMigration() (permissionsMap, error) {
+	return permissionsMap{
+		permissionTransformation{
+			On:  permissionExists(model.PermissionEditPost.Id),
+			Add: []string{model.PermissionEditFileAttachment.Id},
+		},
+	}, nil
+}
+
 // DoPermissionsMigrations execute all the permissions migrations need by the current version.
 func (a *App) DoPermissionsMigrations() error {
 	return a.Srv().doPermissionsMigrations()
@@ -1260,6 +1269,7 @@ func (s *Server) doPermissionsMigrations() error {
 		{Key: model.MigrationRemoveGetAnalyticsPermission, Migration: a.removeGetAnalyticsPermissionMigration},
 		{Key: model.MigrationAddSysconsoleMobileSecurityPermission, Migration: a.addSysConsoleMobileSecurityPermission},
 		{Key: model.MigrationKeyAddChannelBannerPermissions, Migration: a.getAddChannelBannerPermissionMigration},
+		{Key: model.MigrationKeyAddEditFileAttachmentPermission, Migration: a.getAddEditFileAttachmentPermissionMigration},
 	}
 
 	roles, err := s.Store().Role().GetAll()

server/channels/testlib/store.go

@@ -87,6 +87,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store {
 	systemStore.On("GetByName", "elasticsearch_fix_channel_index_migration").Return(&model.System{Name: "elasticsearch_fix_channel_index_migration", Value: "true"}, nil)
 	systemStore.On("GetByName", model.MigrationAddSysconsoleMobileSecurityPermission).Return(&model.System{Name: model.MigrationAddSysconsoleMobileSecurityPermission, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MigrationKeyAddChannelBannerPermissions).Return(&model.System{Name: model.MigrationKeyAddChannelBannerPermissions, Value: "true"}, nil)
+	systemStore.On("GetByName", model.MigrationKeyAddEditFileAttachmentPermission).Return(&model.System{Name: model.MigrationKeyAddEditFileAttachmentPermission, Value: "true"}, nil)
 
 	systemStore.On("InsertIfExists", mock.AnythingOfType("*model.System")).Return(&model.System{}, nil).Once()
 	systemStore.On("Save", mock.AnythingOfType("*model.System")).Return(nil)

server/channels/utils/utils.go

@@ -264,3 +264,22 @@ func RoundOffToZeroesResolution(n float64, minResolution int) int64 {
 	significantDigits := int64(n) / tens
 	return significantDigits * tens
 }
+
+// SliceEqualUnordered returns true if both slices contain the same set of elements,
+// regardless of order.
+func SliceEqualUnordered[K comparable](a, b []K) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	set := make(map[K]int, len(a))
+	for _, id := range a {
+		set[id]++
+	}
+	for _, id := range b {
+		set[id]--
+		if set[id] < 0 {
+			return false
+		}
+	}
+	return true
+}