From 8000e5933526f4fd66b92131db3a1b1f4520dbae Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Mon, 4 May 2026 17:04:37 +0200 Subject: [PATCH] Automated cherry pick of #36197 (#36382) * [MM-68393] Tighten protected role patch authorization (#36197) * [MM-68393] Tighten protected role patch authorization Harden role patch authorization for protected system roles and cover the restricted paths with focused API tests. Made-with: Cursor * [MM-68393] Fix role patch test shadowing Rename shadowing response variables in the protected role patch tests so govet passes in core and enterprise check-style jobs. Made-with: Cursor * [MM-68393] Block privileged role permissions Made-with: Cursor (cherry picked from commit 99b73d4c4acf5ff3546c2548a5aaa804c2aa1b04) * Fix role patch tests for release-10.11 context and LoginSystemManager APIs Co-authored-by: Cursor --------- Co-authored-by: Nick Misasi Co-authored-by: Cursor --- server/channels/api4/role.go | 4 +-- server/channels/api4/role_test.go | 57 +++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/server/channels/api4/role.go b/server/channels/api4/role.go index bfc1088ad5..a29e6f1dfa 100644 --- a/server/channels/api4/role.go +++ b/server/channels/api4/role.go @@ -151,9 +151,9 @@ func patchRole(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventPriorState(oldRole) auditRec.AddEventObjectType("role") - // manage_system permission is required to patch system_admin + // manage_system permission is required to patch system_admin and other protected system roles. requiredPermission := model.PermissionSysconsoleWriteUserManagementPermissions - specialProtectedSystemRoles := append(model.NewSystemRoleIDs, model.SystemAdminRoleId) + specialProtectedSystemRoles := append(append([]string{}, model.NewSystemRoleIDs...), model.SystemAdminRoleId, model.SystemUserRoleId, model.SystemGuestRoleId) for _, roleID := range specialProtectedSystemRoles { if oldRole.Name == roleID { requiredPermission = model.PermissionManageSystem diff --git a/server/channels/api4/role_test.go b/server/channels/api4/role_test.go index 95c2e26c71..feec09c886 100644 --- a/server/channels/api4/role_test.go +++ b/server/channels/api4/role_test.go @@ -330,6 +330,63 @@ func TestPatchRole(t *testing.T) { Permissions: &[]string{"create_direct_channel", "manage_incoming_webhooks", "manage_outgoing_webhooks"}, } + t.Run("system manager cannot patch system_user", func(t *testing.T) { + systemUserRole, appErr := th.App.GetRoleByName(context.Background(), model.SystemUserRoleId) + require.Nil(t, appErr) + + originalPermissions := append([]string{}, systemUserRole.Permissions...) + require.NotContains(t, originalPermissions, model.PermissionEditOtherUsers.Id) + + patchedPermissions := append([]string{}, originalPermissions...) + patchedPermissions = append(patchedPermissions, model.PermissionEditOtherUsers.Id) + + th.LoginSystemManager() + + _, systemUserResp, err := th.SystemManagerClient.PatchRole(context.Background(), systemUserRole.Id, &model.RolePatch{ + Permissions: &patchedPermissions, + }) + if assert.Error(t, err, "system_manager must not be able to patch system_user") { + CheckForbiddenStatus(t, systemUserResp) + } + + systemUserRole, appErr = th.App.GetRoleByName(context.Background(), model.SystemUserRoleId) + require.Nil(t, appErr) + assert.ElementsMatch(t, originalPermissions, systemUserRole.Permissions) + assert.NotContains(t, systemUserRole.Permissions, model.PermissionEditOtherUsers.Id, "system_manager must not be able to inject privileged permissions into system_user") + }) + + t.Run("system manager cannot patch system_guest", func(t *testing.T) { + license := model.NewTestLicense() + license.Features.GuestAccountsPermissions = model.NewPointer(true) + th.App.Srv().SetLicense(license) + t.Cleanup(func() { + th.App.Srv().SetLicense(nil) + }) + + systemGuestRole, appErr := th.App.GetRoleByName(context.Background(), model.SystemGuestRoleId) + require.Nil(t, appErr) + + originalPermissions := append([]string{}, systemGuestRole.Permissions...) + require.NotContains(t, originalPermissions, model.PermissionEditOtherUsers.Id) + + patchedPermissions := append([]string{}, originalPermissions...) + patchedPermissions = append(patchedPermissions, model.PermissionEditOtherUsers.Id) + + th.LoginSystemManager() + + _, systemGuestResp, err := th.SystemManagerClient.PatchRole(context.Background(), systemGuestRole.Id, &model.RolePatch{ + Permissions: &patchedPermissions, + }) + if assert.Error(t, err, "system_manager must not be able to patch system_guest") { + CheckForbiddenStatus(t, systemGuestResp) + } + + systemGuestRole, appErr = th.App.GetRoleByName(context.Background(), model.SystemGuestRoleId) + require.Nil(t, appErr) + assert.ElementsMatch(t, originalPermissions, systemGuestRole.Permissions) + assert.NotContains(t, systemGuestRole.Permissions, model.PermissionEditOtherUsers.Id, "system_manager must not be able to inject privileged permissions into system_guest") + }) + th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { received, _, err := client.PatchRole(context.Background(), role.Id, patch) require.NoError(t, err)