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)