From c12e6d9e9c24dafbff63ee8c7c281b05869745f4 Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Tue, 17 Sep 2024 15:19:03 -0600 Subject: [PATCH] MM-60240 Sanitize Channels based on user permissions (#28086) * initial commit * add unit tests * update tests, self review * update tests to verify something returned. * update app-layer * update unit tests * fix tests * add check for PermissionSysconsoleReadComplianceDataRetentionPolicy * revert package-lock * review suggestions --------- Co-authored-by: Mattermost Build --- server/channels/api4/channel.go | 26 ++++++++++- server/channels/api4/channel_test.go | 68 ++++++++++++++++++++++++++++ server/public/model/channel.go | 9 ++++ server/public/model/channel_test.go | 46 +++++++++++++++++++ 4 files changed, 147 insertions(+), 2 deletions(-) diff --git a/server/channels/api4/channel.go b/server/channels/api4/channel.go index 7119099cb6..c024b8242a 100644 --- a/server/channels/api4/channel.go +++ b/server/channels/api4/channel.go @@ -793,8 +793,9 @@ func getPinnedPosts(c *Context, w http.ResponseWriter, r *http.Request) { func getAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { permissions := []*model.Permission{ - model.PermissionSysconsoleReadUserManagementGroups, + model.PermissionSysconsoleWriteUserManagementGroups, model.PermissionSysconsoleReadUserManagementChannels, + model.PermissionSysconsoleReadComplianceDataRetentionPolicy, } if !c.App.SessionHasPermissionToAny(*c.AppContext.Session(), permissions) { c.SetPermissionError(permissions...) @@ -822,6 +823,8 @@ func getAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { return } + channels = sanitizeAllChannelsResponse(c, channels) + if c.Params.IncludeTotalCount { totalCount, err := c.App.GetAllChannelsCount(c.AppContext, opts) if err != nil { @@ -843,6 +846,18 @@ func getAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { } } +func sanitizeAllChannelsResponse(c *Context, channels model.ChannelListWithTeamData) model.ChannelListWithTeamData { + if !c.App.SessionHasPermissionToAny(*c.AppContext.Session(), []*model.Permission{ + model.PermissionSysconsoleReadComplianceDataRetentionPolicy, + model.PermissionSysconsoleReadUserManagementChannels, + }) { + for _, channel := range channels { + channel.Channel = channel.Channel.Sanitize() + } + } + return channels +} + func getPublicChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Request) { c.RequireTeamId() if c.Err != nil { @@ -1256,7 +1271,12 @@ func searchAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { return } - if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementChannels) { + if !c.App.SessionHasPermissionToAny(*c.AppContext.Session(), + []*model.Permission{ + model.PermissionSysconsoleWriteUserManagementGroups, + model.PermissionSysconsoleReadUserManagementChannels, + model.PermissionSysconsoleReadComplianceDataRetentionPolicy, + }) { c.SetPermissionError(model.PermissionSysconsoleReadUserManagementChannels) return } @@ -1288,6 +1308,8 @@ func searchAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { return } + channels = sanitizeAllChannelsResponse(c, channels) + // Don't fill in channels props, since unused by client and potentially expensive. if props.Page != nil && props.PerPage != nil { data := model.ChannelsWithCount{Channels: channels, TotalCount: totalCount} diff --git a/server/channels/api4/channel_test.go b/server/channels/api4/channel_test.go index f9394b9871..2bdb82e771 100644 --- a/server/channels/api4/channel_test.go +++ b/server/channels/api4/channel_test.go @@ -1404,6 +1404,40 @@ func TestGetAllChannels(t *testing.T) { } require.True(t, found) }) + + t.Run("verify correct sanitization", func(t *testing.T) { + channels, resp, err := th.SystemAdminClient.GetAllChannels(context.Background(), 0, 10000, "") + require.NoError(t, err) + CheckOKStatus(t, resp) + require.True(t, len(channels) > 0) + for _, channel := range channels { + if channel.DisplayName != "Off-Topic" && channel.DisplayName != "Town Square" { + require.NotEqual(t, "", channel.CreatorId) + require.NotEqual(t, "", channel.Name) + } + } + + channels, resp, err = th.SystemManagerClient.GetAllChannels(context.Background(), 0, 10000, "") + require.NoError(t, err) + CheckOKStatus(t, resp) + require.True(t, len(channels) > 0) + for _, channel := range channels { + if channel.DisplayName != "Off-Topic" && channel.DisplayName != "Town Square" { + require.NotEqual(t, "", channel.CreatorId) + require.NotEqual(t, "", channel.Name) + } + } + + th.RemovePermissionFromRole(model.PermissionSysconsoleReadUserManagementChannels.Id, model.SystemManagerRoleId) + channels, resp, err = th.SystemManagerClient.GetAllChannels(context.Background(), 0, 10000, "") + require.NoError(t, err) + CheckOKStatus(t, resp) + require.True(t, len(channels) > 0) + for _, channel := range channels { + require.Equal(t, "", channel.CreatorId) + require.Equal(t, "", channel.Name) + } + }) } func TestGetAllChannelsWithCount(t *testing.T) { @@ -1887,6 +1921,40 @@ func TestSearchAllChannels(t *testing.T) { } require.True(t, found) }) + + t.Run("verify correct sanitization", func(t *testing.T) { + channels, resp, err := th.SystemAdminClient.SearchAllChannels(context.Background(), &model.ChannelSearch{Term: ""}) + require.NoError(t, err) + CheckOKStatus(t, resp) + require.True(t, len(channels) > 0) + for _, channel := range channels { + if channel.DisplayName != "Off-Topic" && channel.DisplayName != "Town Square" { + require.NotEqual(t, "", channel.CreatorId) + require.NotEqual(t, "", channel.Name) + } + } + + channels, resp, err = th.SystemManagerClient.SearchAllChannels(context.Background(), &model.ChannelSearch{Term: ""}) + require.NoError(t, err) + CheckOKStatus(t, resp) + require.True(t, len(channels) > 0) + for _, channel := range channels { + if channel.DisplayName != "Off-Topic" && channel.DisplayName != "Town Square" { + require.NotEqual(t, "", channel.CreatorId) + require.NotEqual(t, "", channel.Name) + } + } + + th.RemovePermissionFromRole(model.PermissionSysconsoleReadUserManagementChannels.Id, model.SystemManagerRoleId) + channels, resp, err = th.SystemManagerClient.SearchAllChannels(context.Background(), &model.ChannelSearch{Term: ""}) + require.NoError(t, err) + require.True(t, len(channels) > 0) + CheckOKStatus(t, resp) + for _, channel := range channels { + require.Equal(t, "", channel.CreatorId) + require.Equal(t, "", channel.Name) + } + }) } func TestSearchAllChannelsPaged(t *testing.T) { diff --git a/server/public/model/channel.go b/server/public/model/channel.go index 547ca30618..193baa182c 100644 --- a/server/public/model/channel.go +++ b/server/public/model/channel.go @@ -356,6 +356,15 @@ func (o *Channel) GetOtherUserIdForDM(userId string) string { return otherUserId } +func (o *Channel) Sanitize() Channel { + return Channel{ + Id: o.Id, + TeamId: o.TeamId, + Type: o.Type, + DisplayName: o.DisplayName, + } +} + func (t ChannelType) MarshalJSON() ([]byte, error) { return json.Marshal(string(t)) } diff --git a/server/public/model/channel_test.go b/server/public/model/channel_test.go index 038720a6d2..937241638f 100644 --- a/server/public/model/channel_test.go +++ b/server/public/model/channel_test.go @@ -114,3 +114,49 @@ func TestGetGroupNameFromUserIds(t *testing.T) { require.LessOrEqual(t, len(name), ChannelNameMaxLength) } + +func TestSanitize(t *testing.T) { + schemaId := NewId() + o := Channel{ + Id: NewId(), + CreateAt: 1, + UpdateAt: 1, + DeleteAt: 1, + Name: NewId(), + DisplayName: NewId(), + Header: NewId(), + Purpose: NewId(), + LastPostAt: 1, + TotalMsgCount: 1, + ExtraUpdateAt: 1, + CreatorId: NewId(), + SchemeId: &schemaId, + Props: make(map[string]any), + GroupConstrained: NewPointer(true), + Shared: NewPointer(true), + TotalMsgCountRoot: 1, + PolicyID: &schemaId, + LastRootPostAt: 1, + } + s := o.Sanitize() + + require.NotEqual(t, "", s.Id) + require.Equal(t, int64(0), s.CreateAt) + require.Equal(t, int64(0), s.UpdateAt) + require.Equal(t, int64(0), s.DeleteAt) + require.Equal(t, "", s.Name) + require.NotEqual(t, "", s.DisplayName) + require.Equal(t, "", s.Header) + require.Equal(t, "", s.Purpose) + require.Equal(t, int64(0), s.LastPostAt) + require.Equal(t, int64(0), s.TotalMsgCount) + require.Equal(t, int64(0), s.ExtraUpdateAt) + require.Equal(t, "", s.CreatorId) + require.Nil(t, s.SchemeId) + require.Nil(t, s.Props) + require.Nil(t, s.GroupConstrained) + require.Nil(t, s.Shared) + require.Equal(t, int64(0), s.TotalMsgCountRoot) + require.Nil(t, s.PolicyID) + require.Equal(t, int64(0), s.LastRootPostAt) +}