diff --git a/server/channels/api4/team.go b/server/channels/api4/team.go index f8b6ec6f10c..021c3c6dab6 100644 --- a/server/channels/api4/team.go +++ b/server/channels/api4/team.go @@ -1375,7 +1375,7 @@ func updateTeamMemberRoles(c *Context, w http.ResponseWriter, r *http.Request) { props := model.MapFromJSON(r.Body) newRoles := props["roles"] - if !model.IsValidUserRoles(newRoles) { + if !model.IsValidTeamMemberRoles(newRoles) { c.SetInvalidParam("team_member_roles") return } diff --git a/server/channels/api4/team_test.go b/server/channels/api4/team_test.go index 735b4b86d31..10610eaaecb 100644 --- a/server/channels/api4/team_test.go +++ b/server/channels/api4/team_test.go @@ -3872,6 +3872,63 @@ func TestUpdateTeamMemberRolesRejectsGuestAndAdmin(t *testing.T) { require.Equal(t, rolesBefore, memberAfter.Roles) } +func TestUpdateTeamMemberRolesRejectsNonTeamScopedRoles(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + SystemAdminClient := th.SystemAdminClient + + const teamMember = "team_user" + + invalidRoles := []struct { + name string + roles string + }{ + {name: "system manager with team user", roles: teamMember + " " + model.SystemManagerRoleId}, + {name: "system user manager with team user", roles: teamMember + " " + model.SystemUserManagerRoleId}, + {name: "system read only admin with team user", roles: teamMember + " " + model.SystemReadOnlyAdminRoleId}, + {name: "system post all with team user", roles: teamMember + " " + model.SystemPostAllRoleId}, + {name: "team post all with team user", roles: teamMember + " " + model.TeamPostAllRoleId}, + {name: "team post all public with team user", roles: teamMember + " " + model.TeamPostAllPublicRoleId}, + {name: "channel user with team user", roles: teamMember + " " + model.ChannelUserRoleId}, + {name: "custom group user with team user", roles: teamMember + " " + model.CustomGroupUserRoleId}, + } + + for _, tc := range invalidRoles { + t.Run("rejects "+tc.name, func(t *testing.T) { + memberBefore, _, err := SystemAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + rolesBefore := memberBefore.Roles + + resp, err := SystemAdminClient.UpdateTeamMemberRoles(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, tc.roles) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + + memberAfter, _, err := SystemAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + require.Equal(t, rolesBefore, memberAfter.Roles) + }) + } + + validRoles := []struct { + name string + roles string + }{ + {name: "team member", roles: teamMember}, + {name: "team admin", roles: "team_user team_admin"}, + } + + for _, tc := range validRoles { + t.Run("accepts "+tc.name, func(t *testing.T) { + _, err := SystemAdminClient.UpdateTeamMemberRoles(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, tc.roles) + require.NoError(t, err) + + member, _, err := SystemAdminClient.GetTeamMember(context.Background(), th.BasicTeam.Id, th.BasicUser2.Id, "") + require.NoError(t, err) + require.ElementsMatch(t, strings.Fields(tc.roles), strings.Fields(member.Roles)) + }) + } +} + func TestUpdateTeamMemberSchemeRoles(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 e0327e8fac3..8380d19a6a5 100644 --- a/server/public/model/role.go +++ b/server/public/model/role.go @@ -5,6 +5,7 @@ package model import ( "fmt" + "slices" "strings" "github.com/mattermost/mattermost/server/public/utils/timeutils" @@ -893,22 +894,38 @@ func IsBuiltInRole(roleName string) bool { return builtInRoleSet[roleName] } +// channelScopedBuiltInRoles are the built-in roles valid inside a channel-member role list. +var channelScopedBuiltInRoles = []string{ChannelGuestRoleId, ChannelUserRoleId, ChannelAdminRoleId} + +// teamScopedBuiltInRoles are the built-in roles valid inside a team-member role list. +var teamScopedBuiltInRoles = []string{TeamGuestRoleId, TeamUserRoleId, TeamAdminRoleId} + // 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 + return slices.Contains(channelScopedBuiltInRoles, roleName) } // 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 IsValidMemberRoles(channelMemberRoles, channelScopedBuiltInRoles) +} + +// IsValidTeamMemberRoles reports whether roles are valid for a team member. +func IsValidTeamMemberRoles(teamMemberRoles string) bool { + return IsValidMemberRoles(teamMemberRoles, teamScopedBuiltInRoles) +} + +// IsValidMemberRoles reports whether memberRoles are valid for a member. IsValidUserRoles +// is format validation only; this additionally rejects any built-in role (per IsBuiltInRole) +// that is not in validRoles. +func IsValidMemberRoles(memberRoles string, validRoles []string) bool { + if !IsValidUserRoles(memberRoles) { return false } - for roleName := range strings.FieldsSeq(channelMemberRoles) { - if IsBuiltInRole(roleName) && !IsChannelScopedBuiltInRole(roleName) { + for roleName := range strings.FieldsSeq(memberRoles) { + if IsBuiltInRole(roleName) && !slices.Contains(validRoles, roleName) { return false } } diff --git a/server/public/model/role_test.go b/server/public/model/role_test.go index cb01b4a29a4..abd1ecbf1fc 100644 --- a/server/public/model/role_test.go +++ b/server/public/model/role_test.go @@ -566,6 +566,37 @@ func TestIsValidChannelMemberRoles(t *testing.T) { } } +func TestIsValidTeamMemberRoles(t *testing.T) { + tests := []struct { + name string + roles string + valid bool + }{ + {name: "team user only", roles: TeamUserRoleId, valid: true}, + {name: "team user and admin", roles: TeamUserRoleId + " " + TeamAdminRoleId, valid: true}, + {name: "team guest only", roles: TeamGuestRoleId, valid: true}, + {name: "team post all with team user", roles: TeamUserRoleId + " " + TeamPostAllRoleId, valid: false}, + {name: "team post all public with team user", roles: TeamUserRoleId + " " + TeamPostAllPublicRoleId, valid: false}, + {name: "custom role with team user", roles: TeamUserRoleId + " custom_role", valid: true}, + {name: "prefixed custom team role with team user", roles: TeamUserRoleId + " team_custom", valid: true}, + {name: "prefixed custom system role with team user", roles: TeamUserRoleId + " system_custom", valid: true}, + {name: "channel user with team user", roles: TeamUserRoleId + " " + ChannelUserRoleId, valid: false}, + {name: "system user with team user", roles: TeamUserRoleId + " " + SystemUserRoleId, valid: false}, + {name: "system manager with team user", roles: TeamUserRoleId + " " + SystemManagerRoleId, valid: false}, + {name: "system post all with team user", roles: TeamUserRoleId + " " + SystemPostAllRoleId, valid: false}, + {name: "system read only admin with team user", roles: TeamUserRoleId + " " + SystemReadOnlyAdminRoleId, valid: false}, + {name: "custom group user with team user", roles: TeamUserRoleId + " " + CustomGroupUserRoleId, valid: false}, + {name: "system custom group admin with team user", roles: TeamUserRoleId + " " + 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, IsValidTeamMemberRoles(tc.roles)) + }) + } +} + func TestIsBuiltInRole(t *testing.T) { tests := []struct { name string