From 39f7f4fddfcf04b46588c0c26066252e88390258 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Mon, 16 Mar 2026 11:48:25 -0400 Subject: [PATCH 1/5] [MM-67135] show premade themes when custom themes are disabled (#35540) --- .../user_settings_theme.test.tsx | 21 +++++++++++++++++++ .../user_settings_theme.tsx | 2 ++ 2 files changed, 23 insertions(+) diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.test.tsx b/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.test.tsx index d882a9f2c87..be739dfe697 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.test.tsx +++ b/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.test.tsx @@ -64,6 +64,27 @@ describe('components/user_settings/display/user_settings_theme/user_settings_the expect(requiredProps.actions.saveTheme).toHaveBeenCalled(); }); + it('should show premade themes when custom themes are disabled', () => { + const props = { + ...requiredProps, + selected: true, + allowCustomThemes: false, + }; + + renderWithContext( + , + ); + + // Premade theme chooser should still be rendered + expect(screen.getByText('Save')).toBeInTheDocument(); + expect(screen.queryByLabelText('Premade Themes')).not.toBeInTheDocument(); + expect(screen.queryByLabelText('Custom Theme')).not.toBeInTheDocument(); + + // The premade themes should be visible (theme thumbnails are rendered) + const premadeThemes = document.querySelectorAll('.premade-themes'); + expect(premadeThemes.length).toBeGreaterThan(0); + }); + it('should deleteTeamSpecificThemes if applyToAllTeams is enabled', async () => { const props = { ...requiredProps, diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.tsx b/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.tsx index 02927d1dad4..0a076ba9912 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.tsx +++ b/webapp/channels/src/components/user_settings/display/user_settings_theme/user_settings_theme.tsx @@ -238,6 +238,8 @@ export default class ThemeSetting extends React.PureComponent { , ); + } else { + inputs.push(premade); } let allTeamsCheckbox = null; From 090408f09f53ffc9afc6c65c7c7c1fd3a8cd22f3 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Mon, 16 Mar 2026 13:17:43 -0400 Subject: [PATCH 2/5] [MM-67809] Check create post permission when editing posts (#35558) --- server/channels/api4/post.go | 12 +++++++++ server/channels/api4/post_test.go | 44 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/server/channels/api4/post.go b/server/channels/api4/post.go index 952c1d980c8..0f6b5db44d1 100644 --- a/server/channels/api4/post.go +++ b/server/channels/api4/post.go @@ -1046,6 +1046,12 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) { return } + // Users who can't create posts in a channel shouldn't be able to edit them either. + userCreatePostPermissionCheckWithContext(c, originalPost.ChannelId) + if c.Err != nil { + return + } + auditRec.AddEventPriorState(originalPost) auditRec.AddEventObjectType("post") @@ -1183,6 +1189,12 @@ func postPatchChecks(c *Context, auditRec *model.AuditRecord, message *string) b return false } + // Users who can't create posts in a channel shouldn't be able to edit them either. + userCreatePostPermissionCheckWithContext(c, originalPost.ChannelId) + if c.Err != nil { + return false + } + if *c.App.Config().ServiceSettings.PostEditTimeLimit != -1 && model.GetMillis() > originalPost.CreateAt+int64(*c.App.Config().ServiceSettings.PostEditTimeLimit*1000) && message != nil { c.Err = model.NewAppError("patchPost", "api.post.update_post.permissions_time_limit.app_error", map[string]any{"timeLimit": *c.App.Config().ServiceSettings.PostEditTimeLimit}, "", http.StatusBadRequest) return isMember diff --git a/server/channels/api4/post_test.go b/server/channels/api4/post_test.go index efff02abdcc..9697fde175d 100644 --- a/server/channels/api4/post_test.go +++ b/server/channels/api4/post_test.go @@ -1701,6 +1701,29 @@ func TestUpdatePost(t *testing.T) { assert.Contains(t, updatedPost.FileIds, fileId) }) + t.Run("should prevent editing when create_post permission is revoked", func(t *testing.T) { + th.LoginBasic(t) + + postToEdit, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: th.BasicUser.Id, + ChannelId: channel.Id, + Message: "original message", + }, channel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + th.RemovePermissionFromRole(t, model.PermissionCreatePost.Id, model.ChannelUserRoleId) + defer th.AddPermissionToRole(t, model.PermissionCreatePost.Id, model.ChannelUserRoleId) + + updatePost := &model.Post{ + Id: postToEdit.Id, + ChannelId: channel.Id, + Message: "edited message", + } + _, resp, err := client.UpdatePost(context.Background(), postToEdit.Id, updatePost) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + t.Run("logged out", func(t *testing.T) { _, err := client.Logout(context.Background()) require.NoError(t, err) @@ -2085,6 +2108,27 @@ func TestPatchPost(t *testing.T) { require.NoError(t, err) }) + t.Run("should prevent patching when create_post permission is revoked", func(t *testing.T) { + th.LoginBasic(t) + + postToEdit, _, err := client.CreatePost(context.Background(), &model.Post{ + ChannelId: channel.Id, + Message: "original message", + }) + require.NoError(t, err) + + defaultPerms := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultPerms) + th.RemovePermissionFromRole(t, model.PermissionCreatePost.Id, model.ChannelUserRoleId) + + patch := &model.PostPatch{ + Message: model.NewPointer("edited message"), + } + _, resp, err := client.PatchPost(context.Background(), postToEdit.Id, patch) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + t.Run("time limit expired", func(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.PostEditTimeLimit = 1 From 7425c6817bf244f976c729f8a73cecac8039a1e1 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Mon, 16 Mar 2026 14:36:55 -0400 Subject: [PATCH 3/5] [MM-67741] Scope role_updated WS events to affected team/channel (#35497) With this change, we now scope role_updated websocket events to users that need to receive them. Built-in and unowned role broadcast globally, team-scheme roles emit one event per team using the role, channel-scheme roles emit one event per channel using the role. To efficiently find a role's owning scheme, a schemeid column is added to the roles table. The ID is set when the scheme creates its related roles. --- server/channels/app/role.go | 88 ++++++- server/channels/app/role_test.go | 225 ++++++++++++++++++ server/channels/db/migrations/migrations.list | 6 + .../000156_add_schemeid_to_roles.down.sql | 1 + .../000156_add_schemeid_to_roles.up.sql | 1 + .../000157_backfill_roles_schemeid.down.sql | 1 + .../000157_backfill_roles_schemeid.up.sql | 23 ++ .../000158_add_roles_schemeid_index.down.sql | 2 + .../000158_add_roles_schemeid_index.up.sql | 2 + server/channels/store/sqlstore/role_store.go | 26 +- .../channels/store/sqlstore/scheme_store.go | 36 +-- server/channels/store/storetest/role_store.go | 56 +++-- .../channels/store/storetest/scheme_store.go | 22 ++ server/i18n/en.json | 8 + server/public/model/role.go | 6 + 15 files changed, 457 insertions(+), 46 deletions(-) create mode 100644 server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.down.sql create mode 100644 server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.up.sql create mode 100644 server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.down.sql create mode 100644 server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.up.sql create mode 100644 server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.down.sql create mode 100644 server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.up.sql diff --git a/server/channels/app/role.go b/server/channels/app/role.go index dfcc8a1dab1..44de9f594ed 100644 --- a/server/channels/app/role.go +++ b/server/channels/app/role.go @@ -7,12 +7,14 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "reflect" "slices" "strings" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/v8/channels/store" @@ -276,13 +278,93 @@ func (a *App) CheckRolesExist(roleNames []string) *model.AppError { } func (a *App) sendUpdatedRoleEvent(role *model.Role) *model.AppError { - message := model.NewWebSocketEvent(model.WebsocketEventRoleUpdated, "", "", "", nil, "") roleJSON, jsonErr := json.Marshal(role) if jsonErr != nil { return model.NewAppError("sendUpdatedRoleEvent", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(jsonErr) } - message.Add("role", string(roleJSON)) - a.Publish(message) + + publishEvent := func(teamID, channelID string) { + message := model.NewWebSocketEvent(model.WebsocketEventRoleUpdated, teamID, channelID, "", nil, "") + message.Add("role", string(roleJSON)) + a.Publish(message) + } + + // Built-in system roles apply to all users; broadcast globally without a DB lookup. + if role.BuiltIn { + publishEvent("", "") + return nil + } + + // Scheme-managed roles: use SchemeId to look up the owning scheme. + if role.SchemeId == nil { + // No owning scheme — treat as global (e.g. custom non-scheme role). + publishEvent("", "") + return nil + } + scheme, err := a.Srv().Store().Scheme().Get(*role.SchemeId) + if err != nil { + a.Log().Error("Failed to look up scheme for role event; skipping broadcast", + mlog.String("role_id", role.Id), + mlog.String("scheme_id", *role.SchemeId), + mlog.Err(err)) + return nil + } + + const pageSize = 1000 + const maxBroadcasts = 100000 + switch scheme.Scope { + case model.SchemeScopeTeam: + totalBroadcasts := 0 + offset := 0 + for { + teams, storeErr := a.Srv().Store().Team().GetTeamsByScheme(scheme.Id, offset, pageSize) + if storeErr != nil { + return model.NewAppError("sendUpdatedRoleEvent", "app.role.send_updated_role_event.app_error", nil, "", http.StatusInternalServerError).Wrap(storeErr) + } + for _, team := range teams { + publishEvent(team.Id, "") + } + totalBroadcasts += len(teams) + if len(teams) < pageSize { + break + } + if totalBroadcasts >= maxBroadcasts { + a.Log().Error("sendUpdatedRoleEvent: hit broadcast limit for team scheme", + mlog.String("scheme_id", scheme.Id), + mlog.Int("totalBroadcasts", totalBroadcasts)) + break + } + offset += pageSize + } + case model.SchemeScopeChannel: + totalBroadcasts := 0 + offset := 0 + for { + channels, storeErr := a.Srv().Store().Channel().GetChannelsByScheme(scheme.Id, offset, pageSize) + if storeErr != nil { + return model.NewAppError("sendUpdatedRoleEvent", "app.role.send_updated_role_event.app_error", nil, "", http.StatusInternalServerError).Wrap(storeErr) + } + for _, channel := range channels { + publishEvent("", channel.Id) + } + totalBroadcasts += len(channels) + if len(channels) < pageSize { + break + } + if totalBroadcasts >= maxBroadcasts { + a.Log().Error("sendUpdatedRoleEvent: hit broadcast limit for channel scheme", + mlog.String("scheme_id", scheme.Id), + mlog.Int("totalBroadcasts", totalBroadcasts)) + break + } + offset += pageSize + } + case model.SchemeScopePlaybook, model.SchemeScopeRun: + // Playbook/run schemes don't map to teams or channels; broadcast globally. + publishEvent("", "") + default: + return model.NewAppError("sendUpdatedRoleEvent", "app.role.send_updated_role_event.unknown_scope", nil, fmt.Sprintf("unknown scheme scope: %s", scheme.Scope), http.StatusInternalServerError) + } return nil } diff --git a/server/channels/app/role_test.go b/server/channels/app/role_test.go index e2762cb0ed6..d89d6379b60 100644 --- a/server/channels/app/role_test.go +++ b/server/channels/app/role_test.go @@ -5,6 +5,7 @@ package app import ( "encoding/csv" + "errors" "io" "os" "slices" @@ -12,9 +13,11 @@ import ( "strings" "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" ) type permissionInheritanceTestData struct { @@ -259,3 +262,225 @@ func testPermissionInheritance(t *testing.T, testCallback func(t *testing.T, th // test 24 combinations where the higher-scoped scheme is a TEAM scheme test(teamScheme.DefaultChannelGuestRole, teamScheme.DefaultChannelUserRole, teamScheme.DefaultChannelAdminRole) } + +func TestSendUpdatedRoleEvent(t *testing.T) { + t.Run("BuiltIn role broadcasts globally without a DB lookup", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockStore.On("Scheme").Return(&mockSchemeStore) + + role := &model.Role{Name: model.TeamAdminRoleId, BuiltIn: true} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockSchemeStore.AssertNotCalled(t, "Get", mock.Anything) + }) + + t.Run("Team scheme role calls GetTeamsByScheme and emits per-team events", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + roleName := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopeTeam} + teams := []*model.Team{{Id: model.NewId()}, {Id: model.NewId()}} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockTeamStore := mocks.TeamStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockTeamStore.On("GetTeamsByScheme", schemeID, 0, 1000).Return(teams, nil) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Team").Return(&mockTeamStore) + + role := &model.Role{Name: roleName, BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockSchemeStore.AssertCalled(t, "Get", schemeID) + mockTeamStore.AssertCalled(t, "GetTeamsByScheme", schemeID, 0, 1000) + }) + + t.Run("Channel scheme role calls GetChannelsByScheme and emits per-channel events", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + roleName := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopeChannel} + channels := model.ChannelList{{Id: model.NewId()}} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockChannelStore := mocks.ChannelStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockChannelStore.On("GetChannelsByScheme", schemeID, 0, 1000).Return(channels, nil) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Channel").Return(&mockChannelStore) + + role := &model.Role{Name: roleName, BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockSchemeStore.AssertCalled(t, "Get", schemeID) + mockChannelStore.AssertCalled(t, "GetChannelsByScheme", schemeID, 0, 1000) + }) + + t.Run("Role not in any scheme broadcasts globally", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockTeamStore := mocks.TeamStore{} + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Team").Return(&mockTeamStore) + + role := &model.Role{Name: model.NewId(), BuiltIn: false, SchemeId: nil} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockSchemeStore.AssertNotCalled(t, "Get", mock.Anything) + mockTeamStore.AssertNotCalled(t, "GetTeamsByScheme", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("Playbook scope falls back to global broadcast without querying teams or channels", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + roleName := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopePlaybook} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockTeamStore := mocks.TeamStore{} + mockChannelStore := mocks.ChannelStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Team").Return(&mockTeamStore) + mockStore.On("Channel").Return(&mockChannelStore) + + role := &model.Role{Name: roleName, BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockTeamStore.AssertNotCalled(t, "GetTeamsByScheme", mock.Anything, mock.Anything, mock.Anything) + mockChannelStore.AssertNotCalled(t, "GetChannelsByScheme", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("Scheme store error is logged and skips broadcast", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + roleName := model.NewId() + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockSchemeStore.On("Get", schemeID).Return(nil, errors.New("db error")) + mockStore.On("Scheme").Return(&mockSchemeStore) + + role := &model.Role{Name: roleName, BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + }) + + t.Run("GetTeamsByScheme store error propagates as AppError", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + roleName := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopeTeam} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockTeamStore := mocks.TeamStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockTeamStore.On("GetTeamsByScheme", schemeID, 0, 1000).Return(nil, errors.New("db error")) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Team").Return(&mockTeamStore) + + role := &model.Role{Name: roleName, BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.NotNil(t, appErr) + }) + + t.Run("Team scheme paginates across multiple pages", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopeTeam} + + // Build a full first page (1000 teams) and a partial second page (2 teams). + page1 := make([]*model.Team, 1000) + for i := range page1 { + page1[i] = &model.Team{Id: model.NewId()} + } + page2 := []*model.Team{{Id: model.NewId()}, {Id: model.NewId()}} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockTeamStore := mocks.TeamStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockTeamStore.On("GetTeamsByScheme", schemeID, 0, 1000).Return(page1, nil) + mockTeamStore.On("GetTeamsByScheme", schemeID, 1000, 1000).Return(page2, nil) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Team").Return(&mockTeamStore) + + role := &model.Role{Name: model.NewId(), BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockTeamStore.AssertCalled(t, "GetTeamsByScheme", schemeID, 0, 1000) + mockTeamStore.AssertCalled(t, "GetTeamsByScheme", schemeID, 1000, 1000) + }) + + t.Run("Channel scheme paginates across multiple pages", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopeChannel} + + page1 := make(model.ChannelList, 1000) + for i := range page1 { + page1[i] = &model.Channel{Id: model.NewId()} + } + page2 := model.ChannelList{{Id: model.NewId()}, {Id: model.NewId()}, {Id: model.NewId()}} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockChannelStore := mocks.ChannelStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockChannelStore.On("GetChannelsByScheme", schemeID, 0, 1000).Return(page1, nil) + mockChannelStore.On("GetChannelsByScheme", schemeID, 1000, 1000).Return(page2, nil) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Channel").Return(&mockChannelStore) + + role := &model.Role{Name: model.NewId(), BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.Nil(t, appErr) + mockChannelStore.AssertCalled(t, "GetChannelsByScheme", schemeID, 0, 1000) + mockChannelStore.AssertCalled(t, "GetChannelsByScheme", schemeID, 1000, 1000) + }) + + t.Run("GetChannelsByScheme store error propagates as AppError", func(t *testing.T) { + mainHelper.Parallel(t) + th := SetupWithStoreMock(t) + + schemeID := model.NewId() + roleName := model.NewId() + scheme := &model.Scheme{Id: schemeID, Scope: model.SchemeScopeChannel} + + mockStore := th.App.Srv().Store().(*mocks.Store) + mockSchemeStore := mocks.SchemeStore{} + mockChannelStore := mocks.ChannelStore{} + mockSchemeStore.On("Get", schemeID).Return(scheme, nil) + mockChannelStore.On("GetChannelsByScheme", schemeID, 0, 1000).Return(nil, errors.New("db error")) + mockStore.On("Scheme").Return(&mockSchemeStore) + mockStore.On("Channel").Return(&mockChannelStore) + + role := &model.Role{Name: roleName, BuiltIn: false, SchemeId: &schemeID} + appErr := th.App.sendUpdatedRoleEvent(role) + require.NotNil(t, appErr) + }) +} diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index 66fdb3db6e5..132aeb4650f 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -307,3 +307,9 @@ channels/db/migrations/postgres/000154_drop_translation_updateat_index.down.sql channels/db/migrations/postgres/000154_drop_translation_updateat_index.up.sql channels/db/migrations/postgres/000155_create_translation_channel_updateat_index.down.sql channels/db/migrations/postgres/000155_create_translation_channel_updateat_index.up.sql +channels/db/migrations/postgres/000156_add_schemeid_to_roles.down.sql +channels/db/migrations/postgres/000156_add_schemeid_to_roles.up.sql +channels/db/migrations/postgres/000157_backfill_roles_schemeid.down.sql +channels/db/migrations/postgres/000157_backfill_roles_schemeid.up.sql +channels/db/migrations/postgres/000158_add_roles_schemeid_index.down.sql +channels/db/migrations/postgres/000158_add_roles_schemeid_index.up.sql diff --git a/server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.down.sql b/server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.down.sql new file mode 100644 index 00000000000..ac12dd12e4b --- /dev/null +++ b/server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.down.sql @@ -0,0 +1 @@ +ALTER TABLE roles DROP COLUMN IF EXISTS schemeid; diff --git a/server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.up.sql b/server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.up.sql new file mode 100644 index 00000000000..6c3612417ea --- /dev/null +++ b/server/channels/db/migrations/postgres/000156_add_schemeid_to_roles.up.sql @@ -0,0 +1 @@ +ALTER TABLE roles ADD COLUMN IF NOT EXISTS schemeid VARCHAR(26); diff --git a/server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.down.sql b/server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.down.sql new file mode 100644 index 00000000000..efb09e806d4 --- /dev/null +++ b/server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.down.sql @@ -0,0 +1 @@ +UPDATE roles SET schemeid = NULL; diff --git a/server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.up.sql b/server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.up.sql new file mode 100644 index 00000000000..1c4acba08bc --- /dev/null +++ b/server/channels/db/migrations/postgres/000157_backfill_roles_schemeid.up.sql @@ -0,0 +1,23 @@ +UPDATE roles +SET schemeid = match.scheme_id +FROM ( + SELECT DISTINCT ON (role_name) id AS scheme_id, role_name + FROM ( + SELECT id, unnest(ARRAY[ + defaultteamadminrole, + defaultteamuserrole, + defaultteamguestrole, + defaultchanneladminrole, + defaultchanneluserrole, + defaultchannelguestrole, + defaultplaybookadminrole, + defaultplaybookmemberrole, + defaultrunadminrole, + defaultrunmemberrole + ]) AS role_name + FROM schemes + ) expanded + WHERE role_name IS NOT NULL AND role_name <> '' + ORDER BY role_name, scheme_id +) match +WHERE roles.name = match.role_name; diff --git a/server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.down.sql b/server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.down.sql new file mode 100644 index 00000000000..61b69a26135 --- /dev/null +++ b/server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.down.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +DROP INDEX CONCURRENTLY IF EXISTS idx_roles_scheme_id; diff --git a/server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.up.sql b/server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.up.sql new file mode 100644 index 00000000000..16b6d4198a1 --- /dev/null +++ b/server/channels/db/migrations/postgres/000158_add_roles_schemeid_index.up.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_roles_scheme_id ON roles(schemeid); diff --git a/server/channels/store/sqlstore/role_store.go b/server/channels/store/sqlstore/role_store.go index 809d4982ced..7a48391e94a 100644 --- a/server/channels/store/sqlstore/role_store.go +++ b/server/channels/store/sqlstore/role_store.go @@ -33,6 +33,7 @@ type Role struct { Permissions string SchemeManaged bool BuiltIn bool + SchemeId *string } type channelRolesPermissions struct { @@ -66,6 +67,7 @@ func NewRoleFromModel(role *model.Role) *Role { Permissions: permissions.String(), SchemeManaged: role.SchemeManaged, BuiltIn: role.BuiltIn, + SchemeId: role.SchemeId, } } @@ -81,6 +83,7 @@ func (role Role) ToModel() *model.Role { Permissions: strings.Fields(role.Permissions), SchemeManaged: role.SchemeManaged, BuiltIn: role.BuiltIn, + SchemeId: role.SchemeId, } } @@ -90,7 +93,7 @@ func newSqlRoleStore(sqlStore *SqlStore) store.RoleStore { } s.tableSelectQuery = s.getQueryBuilder(). - Select("Id", "Name", "DisplayName", "Description", "CreateAt", "UpdateAt", "DeleteAt", "Permissions", "SchemeManaged", "BuiltIn"). + Select("Id", "Name", "DisplayName", "Description", "CreateAt", "UpdateAt", "DeleteAt", "Permissions", "SchemeManaged", "BuiltIn", "SchemeId"). From("Roles") return &s @@ -122,9 +125,10 @@ func (s *SqlRoleStore) Save(role *model.Role) (_ *model.Role, err error) { dbRole.UpdateAt = model.GetMillis() res, err := s.GetMaster().NamedExec(`UPDATE Roles - SET UpdateAt=:UpdateAt, DeleteAt=:DeleteAt, CreateAt=:CreateAt, Name=:Name, DisplayName=:DisplayName, - Description=:Description, Permissions=:Permissions, SchemeManaged=:SchemeManaged, BuiltIn=:BuiltIn - WHERE Id=:Id`, &dbRole) + SET UpdateAt=:UpdateAt, DeleteAt=:DeleteAt, CreateAt=:CreateAt, Name=:Name, DisplayName=:DisplayName, + Description=:Description, Permissions=:Permissions, SchemeManaged=:SchemeManaged, BuiltIn=:BuiltIn, + SchemeId=:SchemeId + WHERE Id=:Id`, &dbRole) if err != nil { return nil, errors.Wrap(err, "failed to update Role") @@ -155,9 +159,9 @@ func (s *SqlRoleStore) createRole(role *model.Role, transaction *sqlxTxWrapper) dbRole.UpdateAt = dbRole.CreateAt if _, err := transaction.NamedExec(`INSERT INTO Roles - (Id, Name, DisplayName, Description, Permissions, CreateAt, UpdateAt, DeleteAt, SchemeManaged, BuiltIn) + (Id, Name, DisplayName, Description, Permissions, CreateAt, UpdateAt, DeleteAt, SchemeManaged, BuiltIn, SchemeId) VALUES - (:Id, :Name, :DisplayName, :Description, :Permissions, :CreateAt, :UpdateAt, :DeleteAt, :SchemeManaged, :BuiltIn)`, dbRole); err != nil { + (:Id, :Name, :DisplayName, :Description, :Permissions, :CreateAt, :UpdateAt, :DeleteAt, :SchemeManaged, :BuiltIn, :SchemeId)`, dbRole); err != nil { return nil, errors.Wrap(err, "failed to save Role") } @@ -230,7 +234,7 @@ func (s *SqlRoleStore) GetByNames(names []string) ([]*model.Role, error) { err = rows.Scan( &role.Id, &role.Name, &role.DisplayName, &role.Description, &role.CreateAt, &role.UpdateAt, &role.DeleteAt, &role.Permissions, - &role.SchemeManaged, &role.BuiltIn) + &role.SchemeManaged, &role.BuiltIn, &role.SchemeId) if err != nil { return nil, errors.Wrap(err, "failed to scan values") } @@ -394,9 +398,10 @@ func (s *SqlRoleStore) AllChannelSchemeRoles() ([]*model.Role, error) { "Roles.Permissions", "Roles.SchemeManaged", "Roles.BuiltIn", + "Roles.SchemeId", ). - From("Schemes"). - Join("Roles ON Schemes.DefaultChannelGuestRole = Roles.Name OR Schemes.DefaultChannelUserRole = Roles.Name OR Schemes.DefaultChannelAdminRole = Roles.Name"). + From("Roles"). + Join("Schemes ON Roles.SchemeId = Schemes.Id"). Where(sq.Eq{"Schemes.Scope": model.SchemeScopeChannel}). Where(sq.Eq{"Roles.DeleteAt": 0}). Where(sq.Eq{"Schemes.DeleteAt": 0}) @@ -433,13 +438,14 @@ func (s *SqlRoleStore) ChannelRolesUnderTeamRole(roleName string) ([]*model.Role "ChannelSchemeRoles.Permissions", "ChannelSchemeRoles.SchemeManaged", "ChannelSchemeRoles.BuiltIn", + "ChannelSchemeRoles.SchemeId", ). From("Roles AS HigherScopedRoles"). Join("Schemes AS HigherScopedSchemes ON (HigherScopedRoles.Name = HigherScopedSchemes.DefaultChannelGuestRole OR HigherScopedRoles.Name = HigherScopedSchemes.DefaultChannelUserRole OR HigherScopedRoles.Name = HigherScopedSchemes.DefaultChannelAdminRole)"). Join("Teams ON Teams.SchemeId = HigherScopedSchemes.Id"). Join("Channels ON Channels.TeamId = Teams.Id"). Join("Schemes AS ChannelSchemes ON Channels.SchemeId = ChannelSchemes.Id"). - Join("Roles AS ChannelSchemeRoles ON (ChannelSchemeRoles.Name = ChannelSchemes.DefaultChannelGuestRole OR ChannelSchemeRoles.Name = ChannelSchemes.DefaultChannelUserRole OR ChannelSchemeRoles.Name = ChannelSchemes.DefaultChannelAdminRole)"). + Join("Roles AS ChannelSchemeRoles ON ChannelSchemeRoles.SchemeId = ChannelSchemes.Id"). Where(sq.Eq{"HigherScopedSchemes.Scope": model.SchemeScopeTeam}). Where(sq.Eq{"HigherScopedRoles.Name": roleName}). Where(sq.Eq{"HigherScopedRoles.DeleteAt": 0}). diff --git a/server/channels/store/sqlstore/scheme_store.go b/server/channels/store/sqlstore/scheme_store.go index 182f7739015..c42962fd82c 100644 --- a/server/channels/store/sqlstore/scheme_store.go +++ b/server/channels/store/sqlstore/scheme_store.go @@ -100,6 +100,12 @@ func (s *SqlSchemeStore) Save(scheme *model.Scheme) (_ *model.Scheme, err error) } func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxWrapper) (*model.Scheme, error) { + // Generate the scheme ID up front so it can be recorded on each created role. + scheme.Id = model.NewId() + if scheme.Name == "" { + scheme.Name = model.NewId() + } + // Fetch the default system scheme roles to populate default permissions. defaultRoleNames := []string{ model.TeamAdminRoleId, @@ -135,6 +141,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNameTeamAdmin, scheme.Name), Permissions: defaultRoles[model.TeamAdminRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err := s.SqlStore.Role().(*SqlRoleStore).createRole(teamAdminRole, transaction) @@ -149,6 +156,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNameTeamUser, scheme.Name), Permissions: defaultRoles[model.TeamUserRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err = s.SqlStore.Role().(*SqlRoleStore).createRole(teamUserRole, transaction) @@ -163,6 +171,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNameTeamGuest, scheme.Name), Permissions: defaultRoles[model.TeamGuestRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err = s.SqlStore.Role().(*SqlRoleStore).createRole(teamGuestRole, transaction) @@ -177,6 +186,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNamePlaybookAdmin, scheme.Name), Permissions: defaultRoles[model.PlaybookAdminRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err = s.SqlStore.Role().(*SqlRoleStore).createRole(playbookAdminRole, transaction) if err != nil { @@ -190,6 +200,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNamePlaybookMember, scheme.Name), Permissions: defaultRoles[model.PlaybookMemberRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err = s.SqlStore.Role().(*SqlRoleStore).createRole(playbookMemberRole, transaction) if err != nil { @@ -203,6 +214,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNameRunAdmin, scheme.Name), Permissions: defaultRoles[model.RunAdminRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err = s.SqlStore.Role().(*SqlRoleStore).createRole(runAdminRole, transaction) if err != nil { @@ -216,6 +228,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("%s %s", SchemeRoleDisplayNameRunMember, scheme.Name), Permissions: defaultRoles[model.RunMemberRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } savedRole, err = s.SqlStore.Role().(*SqlRoleStore).createRole(runMemberRole, transaction) if err != nil { @@ -231,6 +244,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("Channel Admin Role for Scheme %s", scheme.Name), Permissions: defaultRoles[model.ChannelAdminRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } if scheme.Scope == model.SchemeScopeChannel { @@ -249,6 +263,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("Channel User Role for Scheme %s", scheme.Name), Permissions: defaultRoles[model.ChannelUserRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } if scheme.Scope == model.SchemeScopeChannel { @@ -267,6 +282,7 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW DisplayName: fmt.Sprintf("Channel Guest Role for Scheme %s", scheme.Name), Permissions: defaultRoles[model.ChannelGuestRoleId].Permissions, SchemeManaged: true, + SchemeId: &scheme.Id, } if scheme.Scope == model.SchemeScopeChannel { @@ -280,10 +296,6 @@ func (s *SqlSchemeStore) createScheme(scheme *model.Scheme, transaction *sqlxTxW scheme.DefaultChannelGuestRole = savedRole.Name } - scheme.Id = model.NewId() - if scheme.Name == "" { - scheme.Name = model.NewId() - } scheme.CreateAt = model.GetMillis() scheme.UpdateAt = scheme.CreateAt @@ -363,23 +375,11 @@ func (s *SqlSchemeStore) Delete(schemeId string) (*model.Scheme, error) { s.Channel().ClearCaches() // Delete the roles belonging to the scheme. - roleNames := []string{scheme.DefaultChannelGuestRole, scheme.DefaultChannelUserRole, scheme.DefaultChannelAdminRole} - if scheme.Scope == model.SchemeScopeTeam { - roleNames = append(roleNames, scheme.DefaultTeamGuestRole, scheme.DefaultTeamUserRole, scheme.DefaultTeamAdminRole) - } - if scheme.Scope == model.SchemeScopePlaybook { - roleNames = append(roleNames, scheme.DefaultPlaybookAdminRole, scheme.DefaultPlaybookMemberRole) - } - - if scheme.Scope == model.SchemeScopeRun { - roleNames = append(roleNames, scheme.DefaultRunAdminRole, scheme.DefaultRunMemberRole) - } - time := model.GetMillis() updateQuery, args, err := s.getQueryBuilder(). Update("Roles"). - Where(sq.Eq{"Name": roleNames}). + Where(sq.Eq{"SchemeId": schemeId}). Set("UpdateAt", time). Set("DeleteAt", time). ToSql() @@ -388,7 +388,7 @@ func (s *SqlSchemeStore) Delete(schemeId string) (*model.Scheme, error) { } if _, err = s.GetMaster().Exec(updateQuery, args...); err != nil { - return nil, errors.Wrapf(err, "failed to update Roles with name in (%s)", roleNames) + return nil, errors.Wrapf(err, "failed to update Roles with SchemeId=%s", schemeId) } // Delete the scheme itself. diff --git a/server/channels/store/storetest/role_store.go b/server/channels/store/storetest/role_store.go index d7dcb8bab7f..2e33d98b90a 100644 --- a/server/channels/store/storetest/role_store.go +++ b/server/channels/store/storetest/role_store.go @@ -456,39 +456,45 @@ func testRoleStoreLowerScopedChannelSchemeRoles(t *testing.T, rctx request.CTX, actualRoles, err := ss.Role().ChannelRolesUnderTeamRole(teamScheme1.DefaultChannelGuestRole) require.NoError(t, err) - var actualRoleNames []string + roleByName := make(map[string]*model.Role, len(actualRoles)) for _, role := range actualRoles { - actualRoleNames = append(actualRoleNames, role.Name) + roleByName[role.Name] = role } - require.Contains(t, actualRoleNames, channelScheme1.DefaultChannelGuestRole) - require.NotContains(t, actualRoleNames, channelScheme2.DefaultChannelGuestRole) + require.Contains(t, roleByName, channelScheme1.DefaultChannelGuestRole) + require.NotContains(t, roleByName, channelScheme2.DefaultChannelGuestRole) + require.NotNil(t, roleByName[channelScheme1.DefaultChannelGuestRole].SchemeId) + assert.Equal(t, channelScheme1.Id, *roleByName[channelScheme1.DefaultChannelGuestRole].SchemeId) }) t.Run("user role for the right team's channels are returned", func(t *testing.T) { actualRoles, err := ss.Role().ChannelRolesUnderTeamRole(teamScheme1.DefaultChannelUserRole) require.NoError(t, err) - var actualRoleNames []string + roleByName := make(map[string]*model.Role, len(actualRoles)) for _, role := range actualRoles { - actualRoleNames = append(actualRoleNames, role.Name) + roleByName[role.Name] = role } - require.Contains(t, actualRoleNames, channelScheme1.DefaultChannelUserRole) - require.NotContains(t, actualRoleNames, channelScheme2.DefaultChannelUserRole) + require.Contains(t, roleByName, channelScheme1.DefaultChannelUserRole) + require.NotContains(t, roleByName, channelScheme2.DefaultChannelUserRole) + require.NotNil(t, roleByName[channelScheme1.DefaultChannelUserRole].SchemeId) + assert.Equal(t, channelScheme1.Id, *roleByName[channelScheme1.DefaultChannelUserRole].SchemeId) }) t.Run("admin role for the right team's channels are returned", func(t *testing.T) { actualRoles, err := ss.Role().ChannelRolesUnderTeamRole(teamScheme1.DefaultChannelAdminRole) require.NoError(t, err) - var actualRoleNames []string + roleByName := make(map[string]*model.Role, len(actualRoles)) for _, role := range actualRoles { - actualRoleNames = append(actualRoleNames, role.Name) + roleByName[role.Name] = role } - require.Contains(t, actualRoleNames, channelScheme1.DefaultChannelAdminRole) - require.NotContains(t, actualRoleNames, channelScheme2.DefaultChannelAdminRole) + require.Contains(t, roleByName, channelScheme1.DefaultChannelAdminRole) + require.NotContains(t, roleByName, channelScheme2.DefaultChannelAdminRole) + require.NotNil(t, roleByName[channelScheme1.DefaultChannelAdminRole].SchemeId) + assert.Equal(t, channelScheme1.Id, *roleByName[channelScheme1.DefaultChannelAdminRole].SchemeId) }) }) @@ -497,9 +503,9 @@ func testRoleStoreLowerScopedChannelSchemeRoles(t *testing.T, rctx request.CTX, actualRoles, err := ss.Role().AllChannelSchemeRoles() require.NoError(t, err) - var actualRoleNames []string + roleByName := make(map[string]*model.Role, len(actualRoles)) for _, role := range actualRoles { - actualRoleNames = append(actualRoleNames, role.Name) + roleByName[role.Name] = role } allRoleNames := []string{ @@ -514,7 +520,27 @@ func testRoleStoreLowerScopedChannelSchemeRoles(t *testing.T, rctx request.CTX, } for _, roleName := range allRoleNames { - require.Contains(t, actualRoleNames, roleName) + require.Contains(t, roleByName, roleName) + } + + // Roles for channelScheme1 must carry channelScheme1's ID. + for _, roleName := range []string{ + channelScheme1.DefaultChannelGuestRole, + channelScheme1.DefaultChannelUserRole, + channelScheme1.DefaultChannelAdminRole, + } { + require.NotNil(t, roleByName[roleName].SchemeId) + assert.Equal(t, channelScheme1.Id, *roleByName[roleName].SchemeId) + } + + // Roles for channelScheme2 must carry channelScheme2's ID. + for _, roleName := range []string{ + channelScheme2.DefaultChannelGuestRole, + channelScheme2.DefaultChannelUserRole, + channelScheme2.DefaultChannelAdminRole, + } { + require.NotNil(t, roleByName[roleName].SchemeId) + assert.Equal(t, channelScheme2.Id, *roleByName[roleName].SchemeId) } }) }) diff --git a/server/channels/store/storetest/scheme_store.go b/server/channels/store/storetest/scheme_store.go index 386fb86dcac..5e8f0962fc2 100644 --- a/server/channels/store/storetest/scheme_store.go +++ b/server/channels/store/storetest/scheme_store.go @@ -174,6 +174,28 @@ func testSchemeStoreSave(t *testing.T, rctx request.CTX, ss store.Store) { assert.Equal(t, role6.Permissions, []string{"read_channel", "read_channel_content", "create_post"}) assert.True(t, role6.SchemeManaged) + role7, err := ss.Role().GetByName(context.Background(), d1.DefaultPlaybookAdminRole) + assert.NoError(t, err) + assert.True(t, role7.SchemeManaged) + + role8, err := ss.Role().GetByName(context.Background(), d1.DefaultPlaybookMemberRole) + assert.NoError(t, err) + assert.True(t, role8.SchemeManaged) + + role9, err := ss.Role().GetByName(context.Background(), d1.DefaultRunAdminRole) + assert.NoError(t, err) + assert.True(t, role9.SchemeManaged) + + role10, err := ss.Role().GetByName(context.Background(), d1.DefaultRunMemberRole) + assert.NoError(t, err) + assert.True(t, role10.SchemeManaged) + + // Every role created for a scheme must carry the scheme's ID. + for _, role := range []*model.Role{role1, role2, role3, role4, role5, role6, role7, role8, role9, role10} { + require.NotNil(t, role.SchemeId) + assert.Equal(t, d1.Id, *role.SchemeId) + } + // Change the scheme description and update. d1.Description = model.NewId() diff --git a/server/i18n/en.json b/server/i18n/en.json index 4c6e2950cb6..4b32b304944 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -7674,6 +7674,14 @@ "id": "app.role.save.invalid_role.app_error", "translation": "The role was not valid." }, + { + "id": "app.role.send_updated_role_event.app_error", + "translation": "An error occurred while broadcasting the role update." + }, + { + "id": "app.role.send_updated_role_event.unknown_scope", + "translation": "An error occurred while broadcasting the role update: unknown scheme scope." + }, { "id": "app.save_config.app_error", "translation": "An error occurred saving the configuration." diff --git a/server/public/model/role.go b/server/public/model/role.go index a29ea8f0e37..3f8d48565f5 100644 --- a/server/public/model/role.go +++ b/server/public/model/role.go @@ -432,6 +432,7 @@ type Role struct { Permissions []string `json:"permissions"` SchemeManaged bool `json:"scheme_managed"` BuiltIn bool `json:"built_in"` + SchemeId *string `json:"scheme_id"` } func (r *Role) Auditable() map[string]any { @@ -446,6 +447,7 @@ func (r *Role) Auditable() map[string]any { "permissions": r.Permissions, "scheme_managed": r.SchemeManaged, "built_in": r.BuiltIn, + "scheme_id": r.SchemeId, } } @@ -466,6 +468,7 @@ func (r *Role) MarshalYAML() (any, error) { Permissions []string `yaml:"permissions"` SchemeManaged bool `yaml:"scheme_managed"` BuiltIn bool `yaml:"built_in"` + SchemeId *string `yaml:"scheme_id"` }{ Id: r.Id, Name: r.Name, @@ -477,6 +480,7 @@ func (r *Role) MarshalYAML() (any, error) { Permissions: r.Permissions, SchemeManaged: r.SchemeManaged, BuiltIn: r.BuiltIn, + SchemeId: r.SchemeId, }, nil } @@ -492,6 +496,7 @@ func (r *Role) UnmarshalYAML(unmarshal func(any) error) error { Permissions []string `yaml:"permissions"` SchemeManaged bool `yaml:"scheme_managed"` BuiltIn bool `yaml:"built_in"` + SchemeId *string `yaml:"scheme_id"` }{} err := unmarshal(&out) @@ -523,6 +528,7 @@ func (r *Role) UnmarshalYAML(unmarshal func(any) error) error { Permissions: out.Permissions, SchemeManaged: out.SchemeManaged, BuiltIn: out.BuiltIn, + SchemeId: out.SchemeId, } return nil } From 1db869731502cd7b42f71cd10df65a83f9e25118 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Mon, 16 Mar 2026 15:25:50 -0400 Subject: [PATCH 4/5] [MM-67514] Fix crash when AppDownloadLink is set to a malformed URL (#35462) This commit fixes a crash in the webapp when native link URLs are malformed. Now we catch the exception instead of crashing, and we also validate URLs on the server side to avoid malformed URLs in the first place. --- .../link_customization_e20_1_spec.js | 2 +- server/i18n/en.json | 4 ++ server/public/model/config.go | 17 ++++++ server/public/model/config_test.go | 60 +++++++++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/e2e-tests/cypress/tests/integration/channels/system_console/site_configuration/link_customization_e20_1_spec.js b/e2e-tests/cypress/tests/integration/channels/system_console/site_configuration/link_customization_e20_1_spec.js index 10bd7234dbf..cc322a28898 100644 --- a/e2e-tests/cypress/tests/integration/channels/system_console/site_configuration/link_customization_e20_1_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/system_console/site_configuration/link_customization_e20_1_spec.js @@ -130,7 +130,7 @@ describe('SupportSettings', () => { it('MM-T1038 - Customization App download link - Change to different', () => { // # Edit links in the support email field - const link = 'some_link'; + const link = 'https://github.com/mattermost/desktop/releases'; cy.findByTestId('NativeAppSettings.AppDownloadLinkinput').clear().type(link); // # Save setting then back to team view diff --git a/server/i18n/en.json b/server/i18n/en.json index 4b32b304944..a64e3266e1d 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -10468,6 +10468,10 @@ "id": "model.config.is_valid.move_thread.domain_invalid.app_error", "translation": "Invalid domain for move thread settings" }, + { + "id": "model.config.is_valid.native_app_settings.download_link.app_error", + "translation": "One or more app download links are not valid URLs." + }, { "id": "model.config.is_valid.notification_settings.invalid_event", "translation": "Invalid flagging event specified." diff --git a/server/public/model/config.go b/server/public/model/config.go index 370e3863241..2f4eb7a0b01 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -3093,6 +3093,19 @@ func (s *NativeAppSettings) SetDefaults() { } } +func (s *NativeAppSettings) AreDownloadLinksValid() *AppError { + for _, link := range []*string{s.AppDownloadLink, s.AndroidAppDownloadLink, s.IosAppDownloadLink} { + if link == nil || *link == "" { + continue + } + u, err := url.ParseRequestURI(*link) + if err != nil || u.Scheme == "" || u.Hostname() == "" { + return NewAppError("NativeAppSettings.AreDownloadLinksValid", "model.config.is_valid.native_app_settings.download_link.app_error", nil, "", http.StatusBadRequest) + } + } + return nil +} + type ElasticsearchSettings struct { ConnectionURL *string `access:"environment_elasticsearch,write_restrictable,cloud_restrictable"` Backend *string `access:"environment_elasticsearch,write_restrictable,cloud_restrictable"` @@ -4164,6 +4177,10 @@ func (o *Config) IsValid() *AppError { return appErr } + if appErr := o.NativeAppSettings.AreDownloadLinksValid(); appErr != nil { + return appErr + } + // Cross-reference validation: IntuneSettings requires either Office365 or SAML to be enabled if o.IntuneSettings.Enable != nil && *o.IntuneSettings.Enable { if o.IntuneSettings.AuthService != nil && *o.IntuneSettings.AuthService != "" { diff --git a/server/public/model/config_test.go b/server/public/model/config_test.go index 112e38d30f0..0991f8a214a 100644 --- a/server/public/model/config_test.go +++ b/server/public/model/config_test.go @@ -2905,3 +2905,63 @@ func TestConfigAccessTagsMapToValidPermissions(t *testing.T) { checkStruct(t, reflect.TypeFor[Config](), "Config") } + +func TestNativeAppSettingsIsValid(t *testing.T) { + t.Run("defaults are valid", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + require.Nil(t, cfg.NativeAppSettings.AreDownloadLinksValid()) + }) + + t.Run("empty string is valid (hides the menu item)", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + *cfg.NativeAppSettings.AppDownloadLink = "" + *cfg.NativeAppSettings.AndroidAppDownloadLink = "" + *cfg.NativeAppSettings.IosAppDownloadLink = "" + require.Nil(t, cfg.NativeAppSettings.AreDownloadLinksValid()) + }) + + t.Run("valid https URLs are accepted", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + *cfg.NativeAppSettings.AppDownloadLink = "https://example.com/download" + *cfg.NativeAppSettings.AndroidAppDownloadLink = "https://play.google.com/store/apps/details?id=com.mattermost.rn" + *cfg.NativeAppSettings.IosAppDownloadLink = "https://apps.apple.com/us/app/mattermost/id1257222717" + require.Nil(t, cfg.NativeAppSettings.AreDownloadLinksValid()) + }) + + t.Run("custom scheme URLs are accepted for enterprise use cases", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + *cfg.NativeAppSettings.AppDownloadLink = "myapp://install/mattermost" + require.Nil(t, cfg.NativeAppSettings.AreDownloadLinksValid()) + }) + + t.Run("malformed AppDownloadLink is rejected", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + *cfg.NativeAppSettings.AppDownloadLink = "http://://mattermost.com" + appErr := cfg.NativeAppSettings.AreDownloadLinksValid() + require.NotNil(t, appErr) + require.Equal(t, "model.config.is_valid.native_app_settings.download_link.app_error", appErr.Id) + }) + + t.Run("malformed AndroidAppDownloadLink is rejected", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + *cfg.NativeAppSettings.AndroidAppDownloadLink = "not-a-url" + appErr := cfg.NativeAppSettings.AreDownloadLinksValid() + require.NotNil(t, appErr) + require.Equal(t, "model.config.is_valid.native_app_settings.download_link.app_error", appErr.Id) + }) + + t.Run("malformed IosAppDownloadLink is rejected", func(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + *cfg.NativeAppSettings.IosAppDownloadLink = "not-a-url" + appErr := cfg.NativeAppSettings.AreDownloadLinksValid() + require.NotNil(t, appErr) + require.Equal(t, "model.config.is_valid.native_app_settings.download_link.app_error", appErr.Id) + }) +} From 3a2cc5e242344d41715c9024a2d5e5251b8cd0e0 Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Mon, 16 Mar 2026 15:35:02 -0400 Subject: [PATCH 5/5] ES search: include footer and author name in indexed fields. (#35603) * Include footer and author name in indexed fields. --- .../enterprise/elasticsearch/common/common.go | 4 +- .../elasticsearch/common/common_test.go | 174 ++++++++++++++++++ 2 files changed, 176 insertions(+), 2 deletions(-) diff --git a/server/enterprise/elasticsearch/common/common.go b/server/enterprise/elasticsearch/common/common.go index 255d366ee1b..0ec56b0e9ca 100644 --- a/server/enterprise/elasticsearch/common/common.go +++ b/server/enterprise/elasticsearch/common/common.go @@ -133,7 +133,7 @@ func ESPostFromPostForIndexing(post *model.PostForIndexing) *ESPost { if !mOk { continue } - for _, key := range []string{"text", "title", "pretext", "fallback"} { + for _, key := range []string{"text", "title", "pretext", "fallback", "footer", "author_name"} { if s, _ := m[key].(string); s != "" { searchAttachments = append(searchAttachments, s) } @@ -165,7 +165,7 @@ func ESPostFromPostForIndexing(post *model.PostForIndexing) *ESPost { if attachment == nil { continue } - for _, s := range []string{attachment.Text, attachment.Title, attachment.Pretext, attachment.Fallback} { + for _, s := range []string{attachment.Text, attachment.Title, attachment.Pretext, attachment.Fallback, attachment.Footer, attachment.AuthorName} { if s != "" { searchAttachments = append(searchAttachments, s) } diff --git a/server/enterprise/elasticsearch/common/common_test.go b/server/enterprise/elasticsearch/common/common_test.go index 3715a4520b8..b112ce7de70 100644 --- a/server/enterprise/elasticsearch/common/common_test.go +++ b/server/enterprise/elasticsearch/common/common_test.go @@ -4,6 +4,7 @@ package common import ( + "encoding/json" "testing" "time" @@ -319,6 +320,64 @@ func TestESPostFromPostForIndexing(t *testing.T) { assert.Contains(t, espost.Attachments, "99.5") }) + t.Run("any form indexes footer and author_name", func(t *testing.T) { + post := model.PostForIndexing{ + TeamId: model.NewId(), + Post: model.Post{ + Id: model.NewId(), + ChannelId: model.NewId(), + UserId: model.NewId(), + CreateAt: model.GetMillis(), + Message: "", + Type: "slack_attachment", + Props: map[string]any{ + model.PostPropsAttachments: []any{ + map[string]any{ + "text": "body text", + "footer": "Opportunity #OPP-000035341 • United States", + "author_name": "Salesforce", + }, + }, + }, + }, + } + + espost := ESPostFromPostForIndexing(&post) + + assert.Contains(t, espost.Attachments, "body text") + assert.Contains(t, espost.Attachments, "Opportunity #OPP-000035341 • United States") + assert.Contains(t, espost.Attachments, "Salesforce") + }) + + t.Run("MessageAttachment form indexes footer and author_name", func(t *testing.T) { + post := model.PostForIndexing{ + TeamId: model.NewId(), + Post: model.Post{ + Id: model.NewId(), + ChannelId: model.NewId(), + UserId: model.NewId(), + CreateAt: model.GetMillis(), + Message: "", + Type: "slack_attachment", + Props: map[string]any{ + model.PostPropsAttachments: []*model.MessageAttachment{ + { + Text: "body text", + Footer: "Opportunity #OPP-000035341 • United States", + AuthorName: "Salesforce", + }, + }, + }, + }, + } + + espost := ESPostFromPostForIndexing(&post) + + assert.Contains(t, espost.Attachments, "body text") + assert.Contains(t, espost.Attachments, "Opportunity #OPP-000035341 • United States") + assert.Contains(t, espost.Attachments, "Salesforce") + }) + t.Run("multiple attachments are combined", func(t *testing.T) { post := model.PostForIndexing{ TeamId: model.NewId(), @@ -353,6 +412,121 @@ func TestESPostFromPostForIndexing(t *testing.T) { }) } +// TestESPostFromPost_CreatePostJSONRoundTrip simulates the exact flow that happens +// in production: App.CreatePost converts []*MessageAttachment to []any via JSON +// marshal/unmarshal (Post.Attachments in post.go), then the search layer calls +// ESPostFromPost which does ShallowCopy → ESPostFromPostForIndexing. +func TestESPostFromPost_CreatePostJSONRoundTrip(t *testing.T) { + // Step 1: Start with typed MessageAttachment (as a plugin/webhook would create) + original := &model.Post{ + Id: model.NewId(), + ChannelId: model.NewId(), + UserId: model.NewId(), + CreateAt: model.GetMillis(), + Message: "#closedwon #renewal", + Type: "slack_attachment", + } + original.AddProp(model.PostPropsAttachments, []*model.MessageAttachment{ + { + Title: "Account: Acme Corp / Widget Industries, LLC", + Text: "Renewal ARR: $49,140.00", + Pretext: "#closedwon #renewal", + Footer: "Opportunity #OPP-000035341 • United States", + AuthorName: "CRM Bot", + Fields: []*model.MessageAttachmentField{ + {Title: "Sales Rep", Value: "Jane Smith"}, + {Title: "Account Manager", Value: "John Doe"}, + {Title: "Opportunity", Value: "Acme Corp / Widget Industries, LLC - Enterprise - 350 Seats - '26 Renewal"}, + {Title: "Seats", Value: 350.0}, + }, + }, + }) + + // Step 2: Simulate CreatePost JSON round-trip (post.go:305-315) + if attachments, ok := original.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment); ok { + jsonAttachments, err := json.Marshal(attachments) + require.NoError(t, err) + attachmentsInterface := []any{} + err = json.Unmarshal(jsonAttachments, &attachmentsInterface) + require.NoError(t, err) + original.AddProp(model.PostPropsAttachments, attachmentsInterface) + } else { + require.Fail(t, "expected []*MessageAttachment type assertion to succeed") + } + + // Step 3: Simulate ESPostFromPost (what the search layer calls) + teamId := model.NewId() + esPost, err := ESPostFromPost(original, teamId, "O") + require.NoError(t, err) + + // Step 4: Verify all attachment fields are indexed + assert.Contains(t, esPost.Attachments, "Account: Acme Corp / Widget Industries, LLC", "title should be indexed") + assert.Contains(t, esPost.Attachments, "Renewal ARR: $49,140.00", "text should be indexed") + assert.Contains(t, esPost.Attachments, "#closedwon #renewal", "pretext should be indexed") + + // Field titles and values + assert.Contains(t, esPost.Attachments, "Sales Rep", "field title should be indexed") + assert.Contains(t, esPost.Attachments, "Jane Smith", "field value should be indexed") + assert.Contains(t, esPost.Attachments, "Account Manager", "field title should be indexed") + assert.Contains(t, esPost.Attachments, "John Doe", "field value should be indexed") + assert.Contains(t, esPost.Attachments, "Opportunity", "field title should be indexed") + assert.Contains(t, esPost.Attachments, "Acme Corp / Widget Industries, LLC - Enterprise - 350 Seats - '26 Renewal", "field value should be indexed") + assert.Contains(t, esPost.Attachments, "350", "numeric field value should be indexed") + + // Footer and AuthorName + assert.Contains(t, esPost.Attachments, "Opportunity #OPP-000035341 • United States", "footer should be indexed") + assert.Contains(t, esPost.Attachments, "CRM Bot", "author_name should be indexed") + + t.Logf("Attachments field content: %s", esPost.Attachments) +} + +// TestESPostFromPost_APIJSONUnmarshal simulates a post created via the REST API +// where Props are deserialized from JSON (attachments arrive as []any directly). +func TestESPostFromPost_APIJSONUnmarshal(t *testing.T) { + // Simulate what happens when a bot POSTs JSON to /api/v4/posts + postJSON := `{ + "channel_id": "` + model.NewId() + `", + "message": "", + "props": { + "attachments": [{ + "title": "Account: Acme Corp", + "text": "Renewal ARR: $49,140.00", + "footer": "Opportunity #OPP-000035341", + "author_name": "CRM Bot", + "fields": [ + {"title": "Sales Rep", "value": "Jane Smith"}, + {"title": "Seats", "value": 350} + ] + }] + } + }` + + var post model.Post + err := json.Unmarshal([]byte(postJSON), &post) + require.NoError(t, err) + post.Id = model.NewId() + post.UserId = model.NewId() + post.CreateAt = model.GetMillis() + + // The CreatePost conversion at post.go:305-315 would NOT trigger here + // because the type is already []any, not []*MessageAttachment + _, typeOk := post.GetProp(model.PostPropsAttachments).([]*model.MessageAttachment) + assert.False(t, typeOk, "API-created post should have []any, not []*MessageAttachment") + + esPost, err := ESPostFromPost(&post, model.NewId(), "O") + require.NoError(t, err) + + assert.Contains(t, esPost.Attachments, "Account: Acme Corp", "title should be indexed") + assert.Contains(t, esPost.Attachments, "Renewal ARR: $49,140.00", "text should be indexed") + assert.Contains(t, esPost.Attachments, "Sales Rep", "field title should be indexed") + assert.Contains(t, esPost.Attachments, "Jane Smith", "field value should be indexed") + assert.Contains(t, esPost.Attachments, "350", "numeric field value should be indexed") + assert.Contains(t, esPost.Attachments, "Opportunity #OPP-000035341", "footer should be indexed") + assert.Contains(t, esPost.Attachments, "CRM Bot", "author_name should be indexed") + + t.Logf("Attachments field content: %s", esPost.Attachments) +} + func TestGetMatchesForHit(t *testing.T) { snippets := map[string][]string{ "message": {