diff --git a/server/channels/api4/channel.go b/server/channels/api4/channel.go index f34293cd8bc..a2f85ba7a66 100644 --- a/server/channels/api4/channel.go +++ b/server/channels/api4/channel.go @@ -2124,7 +2124,7 @@ func updateChannelMemberRoles(c *Context, w http.ResponseWriter, r *http.Request props := model.MapFromJSON(r.Body) newRoles := props["roles"] - if !(model.IsValidUserRoles(newRoles)) { + if !model.IsValidChannelMemberRoles(newRoles) { c.SetInvalidParam("roles") return } diff --git a/server/channels/api4/channel_test.go b/server/channels/api4/channel_test.go index ea9e54007b8..ba8340b4faa 100644 --- a/server/channels/api4/channel_test.go +++ b/server/channels/api4/channel_test.go @@ -5001,6 +5001,84 @@ func TestUpdateChannelRoles(t *testing.T) { CheckForbiddenStatus(t, resp) } +func TestUpdateChannelMemberRolesRejectsNonChannelScopedRoles(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + client := th.Client + + const channelAdmin = "channel_user channel_admin" + const channelMember = "channel_user" + + channel := th.CreatePublicChannel(t) + + _, appErr := th.App.AddUserToChannel(th.Context, th.BasicUser2, channel, false) + require.Nil(t, appErr) + + invalidRoles := []struct { + name string + roles string + }{ + {name: "system manager with channel user", roles: channelMember + " " + model.SystemManagerRoleId}, + {name: "system user manager with channel user", roles: channelMember + " " + model.SystemUserManagerRoleId}, + {name: "system admin with channel admin", roles: channelAdmin + " " + model.SystemAdminRoleId}, + {name: "team user with channel user", roles: channelMember + " " + model.TeamUserRoleId}, + {name: "team admin with channel user", roles: channelMember + " " + model.TeamAdminRoleId}, + {name: "team post all with channel user", roles: channelMember + " " + model.TeamPostAllRoleId}, + {name: "system post all with channel user", roles: channelMember + " " + model.SystemPostAllRoleId}, + {name: "system read only admin with channel user", roles: channelMember + " " + model.SystemReadOnlyAdminRoleId}, + {name: "custom group user with channel user", roles: channelMember + " " + model.CustomGroupUserRoleId}, + } + + for _, tc := range invalidRoles { + t.Run("rejects "+tc.name, func(t *testing.T) { + memberBefore, _, err := client.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + rolesBefore := memberBefore.Roles + + resp, err := client.UpdateChannelRoles(context.Background(), channel.Id, th.BasicUser2.Id, tc.roles) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + + memberAfter, _, err := client.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + require.Equal(t, rolesBefore, memberAfter.Roles) + }) + } + + validRoles := []struct { + name string + roles string + }{ + {name: "channel member", roles: channelMember}, + {name: "channel admin", roles: channelAdmin}, + } + + for _, tc := range validRoles { + t.Run("accepts "+tc.name, func(t *testing.T) { + _, err := client.UpdateChannelRoles(context.Background(), channel.Id, th.BasicUser2.Id, tc.roles) + require.NoError(t, err) + + member, _, err := client.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + require.Equal(t, tc.roles, member.Roles) + }) + } + + t.Run("rejects system manager assigned by system admin", func(t *testing.T) { + memberBefore, _, err := th.SystemAdminClient.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + rolesBefore := memberBefore.Roles + + resp, err := th.SystemAdminClient.UpdateChannelRoles(context.Background(), channel.Id, th.BasicUser2.Id, channelMember+" "+model.SystemManagerRoleId) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + + memberAfter, _, err := th.SystemAdminClient.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + require.Equal(t, rolesBefore, memberAfter.Roles) + }) +} + func TestUpdateChannelMemberSchemeRoles(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index ed50bdbfedc..45070309ba8 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -1380,7 +1380,10 @@ func (a *App) updateChannelMemberRolesInternal(rctx request.CTX, channelID strin } if !role.SchemeManaged { - // The role is not scheme-managed, so it's OK to apply it to the explicit roles field. + if model.IsBuiltInRole(roleName) && !model.IsChannelScopedBuiltInRole(roleName) { + err = model.NewAppError("UpdateChannelMemberRoles", "api.channel.update_channel_member_roles.scheme_role.app_error", nil, "role_name="+roleName, http.StatusBadRequest) + return nil, err + } newExplicitRoles = append(newExplicitRoles, roleName) } else { // The role is scheme-managed, so need to check if it is part of the scheme for this channel or not. diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index 38d3e330bfb..ffd3db6f98f 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -1677,6 +1677,30 @@ func TestUpdateChannelMemberRolesChangingGuest(t *testing.T) { }) } +func TestUpdateChannelMemberRolesRejectsOutOfScopeBuiltInRoles(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + user := model.User{Email: strings.ToLower(model.NewId()) + "success+test@example.com", Nickname: "Tester", Username: "tester" + model.NewId(), Password: model.NewTestPassword(), AuthService: ""} + ruser, _ := th.App.CreateUser(th.Context, &user) + + _, _, appErr := th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, ruser.Id, "") + require.Nil(t, appErr) + + _, appErr = th.App.AddUserToChannel(th.Context, ruser, th.BasicChannel, false) + require.Nil(t, appErr) + + // CustomGroupUserRoleId is a built-in role whose role.BuiltIn flag is false, so it must + // be rejected via the shared model.IsBuiltInRole predicate rather than the BuiltIn flag. + // This guards the app-layer path used by plugins and bulk import, which bypass the API check. + for _, roleName := range []string{model.SystemManagerRoleId, model.SystemCustomGroupAdminRoleId, model.CustomGroupUserRoleId} { + _, appErr = th.App.UpdateChannelMemberRoles(th.Context, th.BasicChannel.Id, ruser.Id, model.ChannelUserRoleId+" "+roleName) + require.NotNilf(t, appErr, "expected rejection for role %s", roleName) + require.Equal(t, "api.channel.update_channel_member_roles.scheme_role.app_error", appErr.Id) + require.Equal(t, http.StatusBadRequest, appErr.StatusCode) + } +} + func TestUpdateChannelMemberRolesRequireUser(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/public/model/role.go b/server/public/model/role.go index 0f59f34f030..d4a39284906 100644 --- a/server/public/model/role.go +++ b/server/public/model/role.go @@ -49,6 +49,7 @@ func init() { ChannelAdminRoleId, CustomGroupUserRoleId, + SystemCustomGroupAdminRoleId, PlaybookAdminRoleId, PlaybookMemberRoleId, @@ -56,6 +57,11 @@ func init() { RunMemberRoleId, }, NewSystemRoleIDs...) + builtInRoleSet = make(map[string]bool, len(BuiltInSchemeManagedRoleIDs)) + for _, id := range BuiltInSchemeManagedRoleIDs { + builtInRoleSet[id] = true + } + // When updating the values here, the values in mattermost-redux must also be updated. SysconsoleAncillaryPermissions = map[string][]*Permission{ PermissionSysconsoleReadAboutEditionAndLicense.Id: { @@ -872,6 +878,44 @@ func IsValidRoleName(roleName string) bool { return true } +// builtInRoleSet is the O(1) lookup set for BuiltInSchemeManagedRoleIDs, built in init(). +// Despite its name, BuiltInSchemeManagedRoleIDs is the canonical list of built-in role +// IDs and not all of its entries are scheme-managed (roughly half have SchemeManaged: false, +// e.g. custom_group_user). It is used as the single source of truth for "is this a built-in +// role", independent of the per-role BuiltIn/SchemeManaged flags. +var builtInRoleSet map[string]bool + +// IsBuiltInRole reports whether roleName is a built-in role, using +// BuiltInSchemeManagedRoleIDs as the source of truth. This is the predicate shared by +// IsValidChannelMemberRoles and the app-layer channel member role validation so both +// layers agree on which roles are built-in. +func IsBuiltInRole(roleName string) bool { + return builtInRoleSet[roleName] +} + +// IsChannelScopedBuiltInRole returns true for the three built-in roles that are +// valid inside a channel-member role list. +func IsChannelScopedBuiltInRole(roleName string) bool { + return roleName == ChannelGuestRoleId || roleName == ChannelUserRoleId || roleName == ChannelAdminRoleId +} + +// IsValidChannelMemberRoles reports whether roles are valid for a channel member. +// IsValidUserRoles is format validation only; this additionally rejects any built-in +// role (per IsBuiltInRole) that is not channel-scoped. +func IsValidChannelMemberRoles(channelMemberRoles string) bool { + if !IsValidUserRoles(channelMemberRoles) { + return false + } + + for roleName := range strings.FieldsSeq(channelMemberRoles) { + if IsBuiltInRole(roleName) && !IsChannelScopedBuiltInRole(roleName) { + return false + } + } + + return true +} + func MakeDefaultRoles() map[string]*Role { roles := make(map[string]*Role) diff --git a/server/public/model/role_test.go b/server/public/model/role_test.go index 848d8a9a79e..cb01b4a29a4 100644 --- a/server/public/model/role_test.go +++ b/server/public/model/role_test.go @@ -536,6 +536,59 @@ func TestRoleIsValid(t *testing.T) { }) } +func TestIsValidChannelMemberRoles(t *testing.T) { + tests := []struct { + name string + roles string + valid bool + }{ + {name: "channel user only", roles: ChannelUserRoleId, valid: true}, + {name: "channel user and admin", roles: ChannelUserRoleId + " " + ChannelAdminRoleId, valid: true}, + {name: "channel guest only", roles: ChannelGuestRoleId, valid: true}, + {name: "custom role with channel user", roles: ChannelUserRoleId + " custom_role", valid: true}, + {name: "prefixed custom team role with channel user", roles: ChannelUserRoleId + " team_custom", valid: true}, + {name: "prefixed custom system role with channel user", roles: ChannelUserRoleId + " system_custom", valid: true}, + {name: "team user with channel user", roles: ChannelUserRoleId + " " + TeamUserRoleId, valid: false}, + {name: "team post all with channel user", roles: ChannelUserRoleId + " " + TeamPostAllRoleId, valid: false}, + {name: "system user with channel user", roles: ChannelUserRoleId + " " + SystemUserRoleId, valid: false}, + {name: "system manager with channel user", roles: ChannelUserRoleId + " " + SystemManagerRoleId, valid: false}, + {name: "system post all with channel user", roles: ChannelUserRoleId + " " + SystemPostAllRoleId, valid: false}, + {name: "system read only admin with channel user", roles: ChannelUserRoleId + " " + SystemReadOnlyAdminRoleId, valid: false}, + {name: "custom group user with channel user", roles: ChannelUserRoleId + " " + CustomGroupUserRoleId, valid: false}, + {name: "system custom group admin with channel user", roles: ChannelUserRoleId + " " + SystemCustomGroupAdminRoleId, valid: false}, + {name: "invalid role name", roles: "invalid-role", valid: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.valid, IsValidChannelMemberRoles(tc.roles)) + }) + } +} + +func TestIsBuiltInRole(t *testing.T) { + tests := []struct { + name string + roleName string + builtIn bool + }{ + {name: "channel user", roleName: ChannelUserRoleId, builtIn: true}, + {name: "system manager", roleName: SystemManagerRoleId, builtIn: true}, + {name: "system custom group admin", roleName: SystemCustomGroupAdminRoleId, builtIn: true}, + // custom_group_user is built-in even though its role.BuiltIn flag is false. + {name: "custom group user", roleName: CustomGroupUserRoleId, builtIn: true}, + {name: "custom role", roleName: "custom_role", builtIn: false}, + {name: "prefixed custom system role", roleName: "system_custom", builtIn: false}, + {name: "empty", roleName: "", builtIn: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.builtIn, IsBuiltInRole(tc.roleName)) + }) + } +} + func TestManageAgentPermissionsDefaultRoles(t *testing.T) { roles := MakeDefaultRoles()