From 755925fb739ba40ac2486fdcfb8e90c2eaf4f35b Mon Sep 17 00:00:00 2001 From: Felipe Martin <812088+fmartingr@users.noreply.github.com> Date: Tue, 9 Jun 2026 11:18:19 +0200 Subject: [PATCH] MM-68830: Preserve unknown permissions during migrations on downgrade (#36888) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * MM-68830: Preserve unknown permissions during migrations on downgrade A server that was upgraded to a newer release (which introduced new permissions and wrote them into roles) and then downgraded fails fatally at startup: the permissions migration re-saves every role, and Role.Save() rejects any permission the older binary does not recognize, making the downgrade unrecoverable. Add RoleStore.SavePreservingUnknownPermissions, used only by doPermissionsMigration, which tolerates and preserves permissions this build does not recognize (logging a warning) instead of rejecting the role. The regular Save() — and therefore the role API path — stays strict, so unknown permissions cannot be introduced through user input. Unrecognized permissions are kept on disk so they are not lost on a later re-upgrade. * MM-68830: assert save forwarding in role cache tests Address review feedback: assert the underlying store's Save and SavePreservingUnknownPermissions are actually invoked (the cache invalidation defer fires regardless of forwarding), and check the returned errors. * MM-68830: address review feedback - Shorten log message in validateForSave - Rename validationRole -> roleCopy for clarity - Trim doc comments to describe behavior only - List all unknown permissions in IsValidWithoutId error - Assert specific error type in storetest * MM-68830: add Role.Clone and use it in validateForSave * MM-68830: add tests for Role.Clone * MM-68830: fix scheme id deep copy assertion in Role.Clone test --- server/channels/app/permissions_migrations.go | 5 +- .../app/permissions_migrations_test.go | 51 ++++++++++++- .../store/localcachelayer/main_test.go | 1 + .../store/localcachelayer/role_layer.go | 8 ++ .../store/localcachelayer/role_layer_test.go | 27 ++++++- .../channels/store/retrylayer/retrylayer.go | 21 ++++++ server/channels/store/sqlstore/role_store.go | 56 +++++++++++++- .../store/sqlstore/role_store_test.go | 34 +++++++++ server/channels/store/store.go | 4 + .../store/storetest/mocks/RoleStore.go | 30 ++++++++ server/channels/store/storetest/role_store.go | 68 +++++++++++++++++ .../channels/store/timerlayer/timerlayer.go | 16 ++++ server/public/model/role.go | 30 +++++++- server/public/model/role_test.go | 73 +++++++++++++++++++ 14 files changed, 410 insertions(+), 14 deletions(-) diff --git a/server/channels/app/permissions_migrations.go b/server/channels/app/permissions_migrations.go index 83444fe4c6a..620129dccd9 100644 --- a/server/channels/app/permissions_migrations.go +++ b/server/channels/app/permissions_migrations.go @@ -237,7 +237,10 @@ func (s *Server) doPermissionsMigration(key string, migrationMap permissionsMap, for _, role := range roles { role.Permissions = applyPermissionsMap(role, roleMap, migrationMap) - if _, err := s.Store().Role().Save(role); err != nil { + // Use SavePreservingUnknownPermissions so a server that was downgraded from a + // newer release (which wrote permissions this binary doesn't recognize) does + // not fail fatally here. Unknown permissions are logged and preserved (MM-68830). + if _, err := s.Store().Role().SavePreservingUnknownPermissions(role); err != nil { var invErr *store.ErrInvalidInput switch { case errors.As(err, &invErr): diff --git a/server/channels/app/permissions_migrations_test.go b/server/channels/app/permissions_migrations_test.go index 2ee21b23c50..3e26ca600df 100644 --- a/server/channels/app/permissions_migrations_test.go +++ b/server/channels/app/permissions_migrations_test.go @@ -4,6 +4,7 @@ package app import ( + "slices" "testing" "github.com/stretchr/testify/assert" @@ -48,7 +49,7 @@ func TestRestoreManageOAuthPermissionMigration(t *testing.T) { return system.Name == model.MigrationKeyRestoreManageOAuthPermission && system.Value == "true" })).Return(nil).Once() - roleStore.On("Save", mock.AnythingOfType("*model.Role")). + roleStore.On("SavePreservingUnknownPermissions", mock.AnythingOfType("*model.Role")). Return(func(role *model.Role) *model.Role { return role }, nil).Twice() appErr := th.App.Srv().doPermissionsMigration(model.MigrationKeyRestoreManageOAuthPermission, migrationMap, roles) @@ -61,7 +62,7 @@ func TestRestoreManageOAuthPermissionMigration(t *testing.T) { require.Nil(t, appErr) assert.Len(t, systemAdminRole.Permissions, 2) - roleStore.AssertNumberOfCalls(t, "Save", 2) + roleStore.AssertNumberOfCalls(t, "SavePreservingUnknownPermissions", 2) systemStore.AssertNumberOfCalls(t, "SaveOrUpdate", 1) } @@ -98,7 +99,7 @@ func TestAddManageAgentPermissionsMigration(t *testing.T) { return system.Name == model.MigrationKeyAddManageAgentPermissions && system.Value == "true" })).Return(nil).Once() - roleStore.On("Save", mock.AnythingOfType("*model.Role")). + roleStore.On("SavePreservingUnknownPermissions", mock.AnythingOfType("*model.Role")). Return(func(role *model.Role) *model.Role { return role }, nil).Twice() appErr := th.App.Srv().doPermissionsMigration(model.MigrationKeyAddManageAgentPermissions, migrationMap, roles) @@ -115,10 +116,52 @@ func TestAddManageAgentPermissionsMigration(t *testing.T) { assert.Len(t, systemAdminRole.Permissions, 3, "system_admin should still have 3 permissions after idempotent run") assert.Len(t, systemUserRole.Permissions, 2, "system_user should still have 2 permissions after idempotent run") - roleStore.AssertNumberOfCalls(t, "Save", 2) + roleStore.AssertNumberOfCalls(t, "SavePreservingUnknownPermissions", 2) systemStore.AssertNumberOfCalls(t, "SaveOrUpdate", 1) } +// 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 +// must preserve those unknown permissions rather than stripping them. +func TestPermissionsMigrationPreservesUnknownPermissions(t *testing.T) { + mainHelper.Parallel(t) + + th := SetupWithStoreMock(t) + + migrationMap, err := th.App.getAddManageAgentPermissionsMigration() + require.NoError(t, err) + + const unknownPermission = "manage_own_agent_from_the_future" + systemAdminRole := &model.Role{ + Name: model.SystemAdminRoleId, + Permissions: []string{model.PermissionManageSystem.Id, unknownPermission}, + } + roles := []*model.Role{systemAdminRole} + + 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.MigrationKeyAddManageAgentPermissions). + Return(nil, model.NewAppError("test", "missing", nil, "", 404)).Once() + systemStore.On("SaveOrUpdate", mock.AnythingOfType("*model.System")).Return(nil).Once() + + // The migration must route through the tolerant save, and the unknown permission + // must still be present on the role handed to the store (not stripped). + roleStore.On("SavePreservingUnknownPermissions", mock.MatchedBy(func(role *model.Role) bool { + return role.Name == model.SystemAdminRoleId && slices.Contains(role.Permissions, unknownPermission) + })).Return(func(role *model.Role) *model.Role { return role }, nil).Once() + + appErr := th.App.Srv().doPermissionsMigration(model.MigrationKeyAddManageAgentPermissions, migrationMap, roles) + require.Nil(t, appErr, "downgrade migration must not fail fatally on unknown permissions") + assert.Contains(t, systemAdminRole.Permissions, unknownPermission, "unknown permission must be preserved across the migration") + roleStore.AssertNumberOfCalls(t, "SavePreservingUnknownPermissions", 1) +} + func TestApplyPermissionsMap(t *testing.T) { mainHelper.Parallel(t) tt := []struct { diff --git a/server/channels/store/localcachelayer/main_test.go b/server/channels/store/localcachelayer/main_test.go index cd9bcaa45dc..03bb4a5c474 100644 --- a/server/channels/store/localcachelayer/main_test.go +++ b/server/channels/store/localcachelayer/main_test.go @@ -54,6 +54,7 @@ func getMockStore(t *testing.T) *mocks.Store { fakeRole2 := model.Role{Id: "456", Name: "role-name2"} mockRolesStore := mocks.RoleStore{} mockRolesStore.On("Save", &fakeRole).Return(&model.Role{}, nil) + mockRolesStore.On("SavePreservingUnknownPermissions", &fakeRole).Return(&model.Role{}, nil) mockRolesStore.On("Delete", "123").Return(&fakeRole, nil) mockRolesStore.On("GetByName", context.Background(), "role-name").Return(&fakeRole, nil) mockRolesStore.On("GetByNames", []string{"role-name"}).Return([]*model.Role{&fakeRole}, nil) diff --git a/server/channels/store/localcachelayer/role_layer.go b/server/channels/store/localcachelayer/role_layer.go index 23047dd8814..2ff6db50dc0 100644 --- a/server/channels/store/localcachelayer/role_layer.go +++ b/server/channels/store/localcachelayer/role_layer.go @@ -44,6 +44,14 @@ func (s LocalCacheRoleStore) Save(role *model.Role) (*model.Role, error) { return s.RoleStore.Save(role) } +func (s LocalCacheRoleStore) SavePreservingUnknownPermissions(role *model.Role) (*model.Role, error) { + if role.Name != "" { + defer s.rootStore.doInvalidateCacheCluster(s.rootStore.roleCache, role.Name, nil) + defer s.rootStore.doClearCacheCluster(s.rootStore.rolePermissionsCache) + } + return s.RoleStore.SavePreservingUnknownPermissions(role) +} + func (s LocalCacheRoleStore) GetByName(ctx context.Context, name string) (*model.Role, error) { var role *model.Role if err := s.rootStore.doStandardReadCache(s.rootStore.roleCache, name, &role); err == nil { diff --git a/server/channels/store/localcachelayer/role_layer_test.go b/server/channels/store/localcachelayer/role_layer_test.go index 787a023b224..2470ce3842d 100644 --- a/server/channels/store/localcachelayer/role_layer_test.go +++ b/server/channels/store/localcachelayer/role_layer_test.go @@ -46,10 +46,31 @@ func TestRoleStoreCache(t *testing.T) { cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider, logger) require.NoError(t, err) - cachedStore.Role().GetByName(context.Background(), "role-name") + _, err = cachedStore.Role().GetByName(context.Background(), "role-name") + require.NoError(t, err) mockStore.Role().(*mocks.RoleStore).AssertNumberOfCalls(t, "GetByName", 1) - cachedStore.Role().Save(&fakeRole) - cachedStore.Role().GetByName(context.Background(), "role-name") + _, err = cachedStore.Role().Save(&fakeRole) + require.NoError(t, err) + mockStore.Role().(*mocks.RoleStore).AssertNumberOfCalls(t, "Save", 1) + _, err = cachedStore.Role().GetByName(context.Background(), "role-name") + require.NoError(t, err) + mockStore.Role().(*mocks.RoleStore).AssertNumberOfCalls(t, "GetByName", 2) + }) + + t.Run("first call not cached, save preserving unknown permissions, and then not cached again", func(t *testing.T) { + mockStore := getMockStore(t) + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider, logger) + require.NoError(t, err) + + _, err = cachedStore.Role().GetByName(context.Background(), "role-name") + require.NoError(t, err) + mockStore.Role().(*mocks.RoleStore).AssertNumberOfCalls(t, "GetByName", 1) + _, err = cachedStore.Role().SavePreservingUnknownPermissions(&fakeRole) + require.NoError(t, err) + mockStore.Role().(*mocks.RoleStore).AssertNumberOfCalls(t, "SavePreservingUnknownPermissions", 1) + _, err = cachedStore.Role().GetByName(context.Background(), "role-name") + require.NoError(t, err) mockStore.Role().(*mocks.RoleStore).AssertNumberOfCalls(t, "GetByName", 2) }) diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index aa566b29604..3763a904456 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -12384,6 +12384,27 @@ func (s *RetryLayerRoleStore) Save(role *model.Role) (*model.Role, error) { } +func (s *RetryLayerRoleStore) SavePreservingUnknownPermissions(role *model.Role) (*model.Role, error) { + + tries := 0 + for { + result, err := s.RoleStore.SavePreservingUnknownPermissions(role) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerScheduledPostStore) CreateScheduledPost(scheduledPost *model.ScheduledPost) (*model.ScheduledPost, error) { tries := 0 diff --git a/server/channels/store/sqlstore/role_store.go b/server/channels/store/sqlstore/role_store.go index c6ff9d8a061..9122c36d837 100644 --- a/server/channels/store/sqlstore/role_store.go +++ b/server/channels/store/sqlstore/role_store.go @@ -13,6 +13,7 @@ import ( "github.com/pkg/errors" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/v8/channels/store" ) @@ -99,10 +100,59 @@ func newSqlRoleStore(sqlStore *SqlStore) store.RoleStore { return &s } -func (s *SqlRoleStore) Save(role *model.Role) (_ *model.Role, err error) { +func (s *SqlRoleStore) Save(role *model.Role) (*model.Role, error) { + return s.save(role, false) +} + +// SavePreservingUnknownPermissions behaves like Save but tolerates and persists +// permissions this server build does not recognize. See the RoleStore interface +// and MM-68830 for the downgrade scenario this protects against. +func (s *SqlRoleStore) SavePreservingUnknownPermissions(role *model.Role) (*model.Role, error) { + return s.save(role, true) +} + +// validateForSave validates the role before it is persisted. When +// preserveUnknownPermissions is true, permissions unknown to this server build are +// logged and excluded from the validation (but left untouched on the role so they +// are still persisted), rather than causing the save to fail. +func (s *SqlRoleStore) validateForSave(role *model.Role, preserveUnknownPermissions bool) error { + roleToValidate := role + + if preserveUnknownPermissions { + if unknown := role.UnknownPermissions(); len(unknown) > 0 { + s.Logger().Warn( + "Preserving role permissions not recognized by this server version (server likely downgraded from a newer release)", + mlog.String("role", role.Name), + mlog.Array("permissions", unknown), + ) + + unknownSet := make(map[string]bool, len(unknown)) + for _, permission := range unknown { + unknownSet[permission] = true + } + known := make([]string, 0, len(role.Permissions)) + for _, permission := range role.Permissions { + if !unknownSet[permission] { + known = append(known, permission) + } + } + + roleCopy := role.Clone() + roleCopy.Permissions = known + roleToValidate = roleCopy + } + } + + if err := roleToValidate.IsValidWithoutId(); err != nil { + return store.NewErrInvalidInput("Role", "", err.Error()) + } + return nil +} + +func (s *SqlRoleStore) save(role *model.Role, preserveUnknownPermissions bool) (*model.Role, error) { // Check the role is valid before proceeding. - if err = role.IsValidWithoutId(); err != nil { - return nil, store.NewErrInvalidInput("Role", "", err.Error()) + if err := s.validateForSave(role, preserveUnknownPermissions); err != nil { + return nil, err } if role.Id == "" { diff --git a/server/channels/store/sqlstore/role_store_test.go b/server/channels/store/sqlstore/role_store_test.go index cbbe8e6e270..edb2c7991f5 100644 --- a/server/channels/store/sqlstore/role_store_test.go +++ b/server/channels/store/sqlstore/role_store_test.go @@ -6,9 +6,43 @@ package sqlstore import ( "testing" + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/v8/channels/store" "github.com/mattermost/mattermost/server/v8/channels/store/storetest" ) func TestRoleStore(t *testing.T) { StoreTestWithSqlStore(t, storetest.TestRoleStore) } + +// TestSqlRoleStoreCreateRoleValidates guards a regression: createRole must validate +// the role itself. It is called directly by scheme_store (bypassing Save/save and +// their validation), so removing its own validation would silently let invalid roles +// through that path (see MM-68830 review). +func TestSqlRoleStoreCreateRoleValidates(t *testing.T) { + StoreTestWithSqlStore(t, func(t *testing.T, rctx request.CTX, ss store.Store, s storetest.SqlStore) { + roleStore := ss.Role().(*SqlRoleStore) + + transaction, err := roleStore.GetMaster().Begin() + require.NoError(t, err) + defer func() { _ = transaction.Rollback() }() + + // A role carrying a permission this build does not recognize must be rejected + // by createRole, just as it is by Save. createRole does not tolerate unknown + // permissions; only the migration's SavePreservingUnknownPermissions path does. + invalid := &model.Role{ + Name: model.NewId(), + DisplayName: model.NewId(), + Description: model.NewId(), + Permissions: []string{"manage_own_agent_from_the_future"}, + } + + _, err = roleStore.createRole(invalid, transaction) + require.Error(t, err, "createRole must reject unknown permissions") + var invErr *store.ErrInvalidInput + require.ErrorAs(t, err, &invErr) + }) +} diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 70d6771b9ef..7e10a960b18 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -870,6 +870,10 @@ type PluginStore interface { type RoleStore interface { Save(role *model.Role) (*model.Role, error) + // SavePreservingUnknownPermissions behaves like Save but tolerates and preserves + // permissions not recognized by this server build instead of rejecting the role. + // Unrecognized permissions are logged (see MM-68830). + SavePreservingUnknownPermissions(role *model.Role) (*model.Role, error) Get(roleID string) (*model.Role, error) GetAll() ([]*model.Role, error) GetByName(ctx context.Context, name string) (*model.Role, error) diff --git a/server/channels/store/storetest/mocks/RoleStore.go b/server/channels/store/storetest/mocks/RoleStore.go index ff16d6fafed..07cbc48f99e 100644 --- a/server/channels/store/storetest/mocks/RoleStore.go +++ b/server/channels/store/storetest/mocks/RoleStore.go @@ -304,6 +304,36 @@ func (_m *RoleStore) Save(role *model.Role) (*model.Role, error) { return r0, r1 } +// SavePreservingUnknownPermissions provides a mock function with given fields: role +func (_m *RoleStore) SavePreservingUnknownPermissions(role *model.Role) (*model.Role, error) { + ret := _m.Called(role) + + if len(ret) == 0 { + panic("no return value specified for SavePreservingUnknownPermissions") + } + + var r0 *model.Role + var r1 error + if rf, ok := ret.Get(0).(func(*model.Role) (*model.Role, error)); ok { + return rf(role) + } + if rf, ok := ret.Get(0).(func(*model.Role) *model.Role); ok { + r0 = rf(role) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.Role) + } + } + + if rf, ok := ret.Get(1).(func(*model.Role) error); ok { + r1 = rf(role) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // NewRoleStore creates a new instance of RoleStore. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewRoleStore(t interface { diff --git a/server/channels/store/storetest/role_store.go b/server/channels/store/storetest/role_store.go index 2e33d98b90a..0ab23b7decb 100644 --- a/server/channels/store/storetest/role_store.go +++ b/server/channels/store/storetest/role_store.go @@ -18,6 +18,7 @@ import ( func TestRoleStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { t.Run("Save", func(t *testing.T) { testRoleStoreSave(t, rctx, ss) }) + t.Run("SavePreservingUnknownPermissions", func(t *testing.T) { testRoleStoreSavePreservingUnknownPermissions(t, rctx, ss) }) t.Run("Get", func(t *testing.T) { testRoleStoreGet(t, rctx, ss) }) t.Run("GetAll", func(t *testing.T) { testRoleStoreGetAll(t, rctx, ss) }) t.Run("GetByName", func(t *testing.T) { testRoleStoreGetByName(t, rctx, ss) }) @@ -103,6 +104,73 @@ func testRoleStoreSave(t *testing.T, rctx request.CTX, ss store.Store) { assert.Error(t, err) } +func testRoleStoreSavePreservingUnknownPermissions(t *testing.T, _ request.CTX, ss store.Store) { + // A role whose permissions are all valid for this build saves and round-trips + // unchanged, just like Save. + t.Run("preserves known permissions like Save", func(t *testing.T) { + r := &model.Role{ + Name: model.NewId(), + DisplayName: model.NewId(), + Description: model.NewId(), + Permissions: []string{"invite_user", "add_user_to_team"}, + SchemeManaged: false, + } + + saved, err := ss.Role().SavePreservingUnknownPermissions(r) + require.NoError(t, err) + assert.Equal(t, r.Permissions, saved.Permissions) + }) + + // The downgrade scenario from MM-68830: an existing role gains a permission this + // build does not recognize (written by a newer release before the downgrade). The + // migration re-saves every role; Save would reject the unknown permission, but + // SavePreservingUnknownPermissions must keep it so it is not lost on a future upgrade. + t.Run("tolerates and persists unknown permissions on update", func(t *testing.T) { + unknown := "manage_own_agent_from_the_future" + + // Create the role as a known-good role first (the pre-downgrade state). + existing, err := ss.Role().Save(&model.Role{ + Name: model.NewId(), + DisplayName: model.NewId(), + Description: model.NewId(), + Permissions: []string{"invite_user"}, + }) + require.NoError(t, err) + + // Simulate the newer release having added an unknown permission to the role. + existing.Permissions = append(existing.Permissions, unknown) + + // Sanity check: the regular Save rejects the unknown permission. + _, err = ss.Role().Save(existing) + require.Error(t, err) + + saved, err := ss.Role().SavePreservingUnknownPermissions(existing) + require.NoError(t, err) + assert.Contains(t, saved.Permissions, unknown, "unknown permission should be preserved") + + // It must actually be persisted, not just returned. + fetched, err := ss.Role().Get(saved.Id) + require.NoError(t, err) + assert.Contains(t, fetched.Permissions, unknown) + }) + + // Tolerating unknown permissions must not mask genuine structural problems. + t.Run("still rejects structurally invalid roles", func(t *testing.T) { + r := &model.Role{ + Name: "invalid-name", + DisplayName: model.NewId(), + Description: model.NewId(), + Permissions: []string{"manage_own_agent_from_the_future"}, + SchemeManaged: false, + } + + _, err := ss.Role().SavePreservingUnknownPermissions(r) + require.Error(t, err) + var invErr *store.ErrInvalidInput + require.ErrorAs(t, err, &invErr) + }) +} + func testRoleStoreGetAll(t *testing.T, rctx request.CTX, ss store.Store) { prev, err := ss.Role().GetAll() require.NoError(t, err) diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 4cb08ea6983..5352d6bdcdf 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -9817,6 +9817,22 @@ func (s *TimerLayerRoleStore) Save(role *model.Role) (*model.Role, error) { return result, err } +func (s *TimerLayerRoleStore) SavePreservingUnknownPermissions(role *model.Role) (*model.Role, error) { + start := time.Now() + + result, err := s.RoleStore.SavePreservingUnknownPermissions(role) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("RoleStore.SavePreservingUnknownPermissions", success, elapsed) + } + return result, err +} + func (s *TimerLayerScheduledPostStore) CreateScheduledPost(scheduledPost *model.ScheduledPost) (*model.ScheduledPost, error) { start := time.Now() diff --git a/server/public/model/role.go b/server/public/model/role.go index 17f2807f3d7..0f59f34f030 100644 --- a/server/public/model/role.go +++ b/server/public/model/role.go @@ -428,6 +428,19 @@ type Role struct { SchemeId *string `json:"scheme_id"` } +func (r *Role) Clone() *Role { + rCopy := *r + if r.Permissions != nil { + rCopy.Permissions = make([]string, len(r.Permissions)) + copy(rCopy.Permissions, r.Permissions) + } + if r.SchemeId != nil { + schemeId := *r.SchemeId + rCopy.SchemeId = &schemeId + } + return &rCopy +} + func (r *Role) Auditable() map[string]any { return map[string]any{ "id": r.Id, @@ -802,6 +815,16 @@ func (r *Role) IsValidWithoutId() error { return fmt.Errorf("role description exceeds maximum length of %d", RoleDescriptionMaxLength) } + if unknown := r.UnknownPermissions(); len(unknown) > 0 { + return fmt.Errorf("unknown permissions: %s", strings.Join(unknown, ", ")) + } + + return nil +} + +// UnknownPermissions returns the permissions on the role that are not present in +// AllPermissions or DeprecatedPermissions (see MM-68830). +func (r *Role) UnknownPermissions() []string { check := func(perms []*Permission, permission string) bool { for _, p := range perms { if permission == p.Id { @@ -810,13 +833,14 @@ func (r *Role) IsValidWithoutId() error { } return false } + + var unknown []string for _, permission := range r.Permissions { if !check(AllPermissions, permission) && !check(DeprecatedPermissions, permission) { - return fmt.Errorf("unknown permission %q", permission) + unknown = append(unknown, permission) } } - - return nil + return unknown } func CleanRoleNames(roleNames []string) ([]string, bool) { diff --git a/server/public/model/role_test.go b/server/public/model/role_test.go index 0550509cbba..848d8a9a79e 100644 --- a/server/public/model/role_test.go +++ b/server/public/model/role_test.go @@ -430,6 +430,79 @@ func TestRoleIsValidWithoutId(t *testing.T) { }) } +func TestRoleUnknownPermissions(t *testing.T) { + t.Run("returns nil when all permissions are known", func(t *testing.T) { + r := &Role{ + Permissions: []string{PermissionCreatePost.Id, PermissionCreateEmojis.Id}, + } + assert.Empty(t, r.UnknownPermissions()) + }) + + t.Run("tolerates deprecated permissions", func(t *testing.T) { + require.NotEmpty(t, DeprecatedPermissions) + r := &Role{ + Permissions: []string{PermissionCreatePost.Id, DeprecatedPermissions[0].Id}, + } + assert.Empty(t, r.UnknownPermissions()) + }) + + t.Run("returns only the permissions this build does not recognize", func(t *testing.T) { + r := &Role{ + Permissions: []string{PermissionCreatePost.Id, "manage_own_agent_from_the_future", "another_unknown"}, + } + assert.ElementsMatch(t, []string{"manage_own_agent_from_the_future", "another_unknown"}, r.UnknownPermissions()) + }) + + t.Run("empty permissions yields no unknowns", func(t *testing.T) { + assert.Empty(t, (&Role{}).UnknownPermissions()) + }) +} + +func TestRoleClone(t *testing.T) { + schemeId := NewId() + original := &Role{ + Id: NewId(), + Name: "test_role", + DisplayName: "Test Role", + Description: "desc", + CreateAt: 1000, + UpdateAt: 2000, + DeleteAt: 0, + Permissions: []string{"invite_user", "add_user_to_team"}, + SchemeManaged: true, + BuiltIn: false, + SchemeId: &schemeId, + } + + t.Run("clone equals original", func(t *testing.T) { + cloned := original.Clone() + assert.Equal(t, original, cloned) + }) + + t.Run("permissions are deep copied", func(t *testing.T) { + cloned := original.Clone() + cloned.Permissions[0] = "mutated" + assert.Equal(t, "invite_user", original.Permissions[0]) + }) + + t.Run("scheme id pointer is deep copied", func(t *testing.T) { + cloned := original.Clone() + require.NotSame(t, original.SchemeId, cloned.SchemeId) + *cloned.SchemeId = NewId() + assert.Equal(t, schemeId, *original.SchemeId) + }) + + t.Run("nil permissions stays nil", func(t *testing.T) { + r := &Role{} + assert.Nil(t, r.Clone().Permissions) + }) + + t.Run("nil scheme id stays nil", func(t *testing.T) { + r := &Role{} + assert.Nil(t, r.Clone().SchemeId) + }) +} + func TestRoleIsValid(t *testing.T) { validRole := func() *Role { return &Role{