mirror of
https://github.com/mattermost/mattermost.git
synced 2026-10-13 09:27:38 +08:00
[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 <matty-code@mattermost.com> * 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 <matty-code@mattermost.com> * 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 <matty-code@mattermost.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: mattermost-code <matty-code@mattermost.com>
This commit is contained in:
co-authored by
mattermost-code
Cursor Agent
parent
01fd4cfe26
commit
240b9bed8b
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user