From 240b9bed8b371430dd112e2ff51eaf336ce4fd0f Mon Sep 17 00:00:00 2001 From: "cursor[bot]" <206951365+cursor[bot]@users.noreply.github.com> Date: Fri, 4 Sep 2026 00:00:47 -0400 Subject: [PATCH] [MM-70274] Add permissions migration so upgraded installs can access the System Console Recaps page (#38058) * [MM-70274] Add permissions migration for AI Recaps sysconsole permissions The sysconsole_read_ai_recaps and sysconsole_write_ai_recaps permissions were only added to the SysconsoleReadPermissions/SysconsoleWritePermissions slices. Those slices are applied to system_admin by the one-time add_system_console_permissions migration, which has already run on every existing install, so upgraded servers never received the new permissions and the System Console Recaps page stayed hidden for system admins. Add a dedicated add_ai_recaps_permissions migration that grants both permissions to the system_admin role, matching fresh-install defaults. Co-authored-by: mattermost-code * Add tests for the AI Recaps permissions migration Cover the transformation itself (system_admin gains both permissions, other roles untouched, idempotent on re-run) and guard that the migration is actually registered in the migration list, which is the mistake that left existing installs without the permissions. Co-authored-by: mattermost-code * Tighten the AI Recaps migration registration test Assert the exact permission count so a migration key that collides with an already-completed key, which would never run on a real upgrade, fails the test. Drop the unreachable store expectations. Co-authored-by: mattermost-code --------- Co-authored-by: Cursor Agent Co-authored-by: mattermost-code --- server/channels/app/permissions_migrations.go | 13 +++ .../app/permissions_migrations_test.go | 109 ++++++++++++++++++ server/channels/testlib/store.go | 1 + server/public/model/migration.go | 1 + 4 files changed, 124 insertions(+) diff --git a/server/channels/app/permissions_migrations.go b/server/channels/app/permissions_migrations.go index c8ef2f62f8f..e302c2673bf 100644 --- a/server/channels/app/permissions_migrations.go +++ b/server/channels/app/permissions_migrations.go @@ -1354,6 +1354,18 @@ func (a *App) getAddDiscoverableChannelPermissionsMigration() (permissionsMap, e }, nil } +func (a *App) getAddAIRecapsPermissionsMigration() (permissionsMap, error) { + return permissionsMap{ + permissionTransformation{ + On: isExactRole(model.SystemAdminRoleId), + Add: []string{ + model.PermissionSysconsoleReadAiRecaps.Id, + model.PermissionSysconsoleWriteAiRecaps.Id, + }, + }, + }, nil +} + // DoPermissionsMigrations execute all the permissions migrations need by the current version. func (a *App) DoPermissionsMigrations() error { return a.Srv().doPermissionsMigrations() @@ -1418,6 +1430,7 @@ func (s *Server) doPermissionsMigrations() error { {Key: model.MigrationKeyAddEditFileAttachmentPermission, Migration: a.getAddEditFileAttachmentPermissionMigration}, {Key: model.MigrationKeyAddDiscoverableChannelPermissions, Migration: a.getAddDiscoverableChannelPermissionsMigration}, {Key: model.MigrationRemoveImportTeamPermission, Migration: a.removeImportTeamPermissionMigration}, + {Key: model.MigrationKeyAddAIRecapsPermissions, Migration: a.getAddAIRecapsPermissionsMigration}, } roles, err := s.Store().Role().GetAll() diff --git a/server/channels/app/permissions_migrations_test.go b/server/channels/app/permissions_migrations_test.go index 3e26ca600df..3c0aa8d95cd 100644 --- a/server/channels/app/permissions_migrations_test.go +++ b/server/channels/app/permissions_migrations_test.go @@ -120,6 +120,115 @@ func TestAddManageAgentPermissionsMigration(t *testing.T) { systemStore.AssertNumberOfCalls(t, "SaveOrUpdate", 1) } +func TestAddAIRecapsPermissionsMigration(t *testing.T) { + mainHelper.Parallel(t) + + th := SetupWithStoreMock(t) + + migrationMap, err := th.App.getAddAIRecapsPermissionsMigration() + require.NoError(t, err) + + systemAdminRole := &model.Role{ + Name: model.SystemAdminRoleId, + Permissions: []string{model.PermissionManageSystem.Id}, + } + systemReadOnlyAdminRole := &model.Role{ + Name: model.SystemReadOnlyAdminRoleId, + Permissions: []string{model.PermissionSysconsoleReadSiteCustomization.Id}, + } + systemUserRole := &model.Role{ + Name: model.SystemUserRoleId, + Permissions: []string{model.PermissionCreateDirectChannel.Id}, + } + roles := []*model.Role{systemAdminRole, systemReadOnlyAdminRole, systemUserRole} + + mockStore := th.App.Srv().Store().(*mocks.Store) + roleStore := mocks.RoleStore{} + systemStore := mocks.SystemStore{} + + mockStore.On("Role").Return(&roleStore) + mockStore.On("System").Return(&systemStore) + + systemStore.On("GetByName", model.MigrationKeyAddAIRecapsPermissions). + Return(nil, model.NewAppError("test", "missing", nil, "", 404)).Once() + systemStore.On("GetByName", model.MigrationKeyAddAIRecapsPermissions). + Return(&model.System{Name: model.MigrationKeyAddAIRecapsPermissions, Value: "true"}, nil).Once() + systemStore.On("SaveOrUpdate", mock.MatchedBy(func(system *model.System) bool { + return system.Name == model.MigrationKeyAddAIRecapsPermissions && system.Value == "true" + })).Return(nil).Once() + + roleStore.On("SavePreservingUnknownPermissions", mock.AnythingOfType("*model.Role")). + Return(func(role *model.Role) *model.Role { return role }, nil) + + appErr := th.App.Srv().doPermissionsMigration(model.MigrationKeyAddAIRecapsPermissions, migrationMap, roles) + require.Nil(t, appErr) + assert.Contains(t, systemAdminRole.Permissions, model.PermissionSysconsoleReadAiRecaps.Id) + assert.Contains(t, systemAdminRole.Permissions, model.PermissionSysconsoleWriteAiRecaps.Id) + assert.Len(t, systemAdminRole.Permissions, 3) + + // The permissions are not part of the default permissions of any other role, + // so the migration must leave every other role untouched. + assert.Equal(t, []string{model.PermissionSysconsoleReadSiteCustomization.Id}, systemReadOnlyAdminRole.Permissions) + assert.Equal(t, []string{model.PermissionCreateDirectChannel.Id}, systemUserRole.Permissions) + + appErr = th.App.Srv().doPermissionsMigration(model.MigrationKeyAddAIRecapsPermissions, migrationMap, roles) + require.Nil(t, appErr) + assert.Len(t, systemAdminRole.Permissions, 3, "system_admin should still have 3 permissions after idempotent run") + + roleStore.AssertNumberOfCalls(t, "SavePreservingUnknownPermissions", len(roles)) + systemStore.AssertNumberOfCalls(t, "SaveOrUpdate", 1) +} + +// TestAIRecapsPermissionsMigrationIsRegistered guards against the MM-70274 root +// cause: sysconsole permissions added to the SysconsoleRead/WritePermissions +// slices are only applied to system_admin by the one-time +// add_system_console_permissions migration, so a new sysconsole permission needs +// its own registered migration to reach existing installs. +func TestAIRecapsPermissionsMigrationIsRegistered(t *testing.T) { + mainHelper.Parallel(t) + + th := SetupWithStoreMock(t) + + systemAdminRole := &model.Role{ + Name: model.SystemAdminRoleId, + Permissions: []string{model.PermissionManageSystem.Id}, + } + + mockStore := th.App.Srv().Store().(*mocks.Store) + roleStore := mocks.RoleStore{} + systemStore := mocks.SystemStore{} + schemeStore := mocks.SchemeStore{} + + mockStore.On("Role").Return(&roleStore) + mockStore.On("System").Return(&systemStore) + mockStore.On("Scheme").Return(&schemeStore) + + schemeStore.On("GetAllPage", model.SchemeScopeTeam, mock.AnythingOfType("int"), mock.AnythingOfType("int")). + Return([]*model.Scheme{}, nil) + roleStore.On("GetAll").Return([]*model.Role{systemAdminRole}, nil) + roleStore.On("SavePreservingUnknownPermissions", mock.AnythingOfType("*model.Role")). + Return(func(role *model.Role) *model.Role { return role }, nil) + systemStore.On("SaveOrUpdate", mock.AnythingOfType("*model.System")).Return(nil) + + // Every migration but the AI recaps one has already run on this install, which + // is the state of any server upgrading onto the release that introduced the + // AI recaps sysconsole permissions. + systemStore.On("GetByName", mock.MatchedBy(func(name string) bool { + return name != model.MigrationKeyAddAIRecapsPermissions + })).Return(func(name string) *model.System { + return &model.System{Name: name, Value: "true"} + }, nil) + systemStore.On("GetByName", model.MigrationKeyAddAIRecapsPermissions). + Return(nil, model.NewAppError("test", "missing", nil, "", 404)) + + require.NoError(t, th.App.Srv().doPermissionsMigrations()) + assert.Contains(t, systemAdminRole.Permissions, model.PermissionSysconsoleReadAiRecaps.Id) + assert.Contains(t, systemAdminRole.Permissions, model.PermissionSysconsoleWriteAiRecaps.Id) + // A key that collides with an already-completed migration would never run on a + // real upgrade, so make sure the permissions came from this migration alone. + assert.Len(t, systemAdminRole.Permissions, 3) +} + // TestPermissionsMigrationPreservesUnknownPermissions is the regression test for // MM-68830: a server downgraded from a newer release holds permissions the older // binary does not recognize. The permissions migration must not fail fatally, and diff --git a/server/channels/testlib/store.go b/server/channels/testlib/store.go index 22524512203..3e91ff9c6d3 100644 --- a/server/channels/testlib/store.go +++ b/server/channels/testlib/store.go @@ -106,6 +106,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store { systemStore.On("GetByName", model.MigrationKeyAddEditFileAttachmentPermission).Return(&model.System{Name: model.MigrationKeyAddEditFileAttachmentPermission, Value: "true"}, nil) systemStore.On("GetByName", model.MigrationKeyAddDiscoverableChannelPermissions).Return(&model.System{Name: model.MigrationKeyAddDiscoverableChannelPermissions, Value: "true"}, nil) systemStore.On("GetByName", model.MigrationRemoveImportTeamPermission).Return(&model.System{Name: model.MigrationRemoveImportTeamPermission, Value: "true"}, nil) + systemStore.On("GetByName", model.MigrationKeyAddAIRecapsPermissions).Return(&model.System{Name: model.MigrationKeyAddAIRecapsPermissions, Value: "true"}, nil) systemStore.On("InsertIfExists", mock.AnythingOfType("*model.System")).Return(&model.System{}, nil).Once() systemStore.On("Save", mock.AnythingOfType("*model.System")).Return(nil) diff --git a/server/public/model/migration.go b/server/public/model/migration.go index 065062067e9..b2bafdd7b74 100644 --- a/server/public/model/migration.go +++ b/server/public/model/migration.go @@ -67,4 +67,5 @@ const ( MigrationKeyAddEditFileAttachmentPermission = "add_edit_file_attachment_permission" MigrationKeyAddDiscoverableChannelPermissions = "add_discoverable_channel_permissions" MigrationRemoveImportTeamPermission = "remove_import_team_permission" + MigrationKeyAddAIRecapsPermissions = "add_ai_recaps_permissions" )