CVE-2026-6046
Description
Mattermost fails to validate bot usernames, letting unprivileged users intercept private plugin messages.
AI Insight
LLM-synthesized narrative grounded in this CVE's description and references.
Mattermost fails to validate bot usernames, letting unprivileged users intercept private plugin messages.
Vulnerability
Mattermost versions 11.6.x <= 11.6.1, 11.5.x <= 11.5.4, and 10.11.x <= 10.11.16 fail to validate that a username returned during bot registration belongs to a bot account. This allows an unprivileged attacker to pre-register a user account with a predictable plugin bot username, thereby intercepting private messages sent by plugins via direct message channels [1].
Exploitation
An unprivileged attacker can pre-register a user account whose username matches a predictable plugin bot username (e.g., a well-known naming pattern). When a plugin attempts to register a bot and send private messages via direct message channels, the system incorrectly accepts the attacker-controlled account as the bot, allowing the attacker to receive those messages instead of the intended recipient [1].
Impact
A successful attacker gains the ability to read private messages sent by plugins via direct message channels. This is a confidentiality breach affecting communications that the plugin intended for a legitimate bot account. The attacker does not gain write access or elevated privileges, but can intercept sensitive data transmitted through plugin DMs [1].
Mitigation
Mattermost has released security updates addressing this issue. Affected versions should be upgraded to 11.6.2, 11.5.5, and 10.11.17 (or later). No workarounds have been disclosed. The vulnerability is tracked under Mattermost advisory ID MMSA-2026-00649 [1].
AI Insight generated on Jun 12, 2026. Synthesized from this CVE's description and the cited reference URLs; citations are validated against the source bundle.
Affected products
2<=11.6.1 || <=11.5.4 || <=10.11.15 || <=10.11.16+ 1 more
- (no CPE)range: <=11.6.1 || <=11.5.4 || <=10.11.15 || <=10.11.16
- (no CPE)range: <= 11.6.1, <= 11.5.4, <= 10.11.16
Patches
398f9778cec1eMM-68264: return error on bot username conflict (#36064) (#36318)
2 files changed · +29 −12
server/channels/app/bot.go+13 −12 modified@@ -65,18 +65,19 @@ func (a *App) EnsureBot(rctx request.CTX, pluginID string, bot *model.Bot) (stri if appErr := a.SetPluginKey(pluginID, botUserKey, []byte(user.Id)); appErr != nil { return "", fmt.Errorf("failed to set plugin key: %w", appErr) } - } else { - rctx.Logger().Error("Plugin attempted to use an account that already exists. Convert user to a bot "+ - "account in the CLI by running 'mattermost user convert <username> --bot'. If the user is an "+ - "existing user account you want to preserve, change its username and restart the Mattermost server, "+ - "after which the plugin will create a bot account with that name. For more information about bot "+ - "accounts, see https://mattermost.com/pl/default-bot-accounts", mlog.String("username", - bot.Username), - mlog.String("user_id", - user.Id), - ) - } - return user.Id, nil + return user.Id, nil + } + + rctx.Logger().Error("Plugin attempted to use an account that already exists. Convert user to a bot "+ + "account in the CLI by running 'mattermost user convert <username> --bot'. If the user is an "+ + "existing user account you want to preserve, change its username and restart the Mattermost server, "+ + "after which the plugin will create a bot account with that name. For more information about bot "+ + "accounts, see https://mattermost.com/pl/default-bot-accounts", mlog.String("username", + bot.Username), + mlog.String("user_id", + user.Id), + ) + return "", fmt.Errorf("username %q is already taken by a non-bot user", bot.Username) } createdBot, err := a.CreateBot(rctx, bot)
server/channels/app/bot_test.go+16 −0 modified@@ -147,6 +147,22 @@ func TestEnsureBot(t *testing.T) { assert.Equal(t, "another bot", bot.Description) }) + t.Run("ensure bot should fail if username belongs to a non-bot user", func(t *testing.T) { + th := Setup(t).InitBasic(t) + + pluginId := "pluginId" + + // th.BasicUser is a regular (non-bot) user created by InitBasic. + // EnsureBot must return an error — not the human user's ID. + botID, err := th.App.EnsureBot(th.Context, pluginId, &model.Bot{ + Username: th.BasicUser.Username, + Description: "a bot", + OwnerId: th.BasicUser.Id, + }) + require.Error(t, err) + assert.Empty(t, botID) + }) + t.Run("ensure bot should pass even after delete bot user", func(t *testing.T) { th := Setup(t).InitBasic(t)
202d125afa87[release-10.11] MM-68547: Tighten authorization on group syncable link and patch endpoints (#36434)
8 files changed · +1006 −23
e2e-tests/cypress/tests/integration/channels/enterprise/system_console/group_configuration_spec.js+6 −2 modified@@ -394,8 +394,10 @@ describe('group configuration', () => { // # Save settings savePage(); - // * Check the groupteam via the API to ensure its role wasn't updated + // * Check the groupteam via the API to ensure the team was + // removed (delete_at != 0) and its role wasn't updated. cy.apiGetGroupTeam(groupID, testTeam.id).then(({body}) => { + expect(body.delete_at).to.not.eq(0); expect(body.scheme_admin).to.eq(false); }); }); @@ -519,8 +521,10 @@ describe('group configuration', () => { // # Save settings savePage(); - // * Check the groupteam via the API to ensure its role wasn't updated + // * Check the groupteam via the API to ensure the channel was + // removed (delete_at != 0) and its role wasn't updated. cy.apiGetGroupChannel(groupID, testChannel.id).then(({body}) => { + expect(body.delete_at).to.not.eq(0); expect(body.scheme_admin).to.eq(false); }); });
server/channels/api4/group.go+68 −6 modified@@ -377,10 +377,35 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { return } - groupSyncable := &model.GroupSyncable{ - GroupId: c.Params.GroupId, - SyncableId: syncableID, - Type: syncableType, + appErr = verifySchemeAdminAssignmentPermission(c, syncableType, syncableID, patch) + if appErr != nil { + appErr.Where = "Api4.linkGroupSyncable" + c.Err = appErr + return + } + + // Upsert onto the existing row only when it is currently active so + // unspecified fields are preserved. A fresh link, or a re-link of a + // soft-deleted row, starts from a zero-value struct so that fields + // the caller did not (or was not authorized to) set are not carried + // over from the previous incarnation. The downstream upsert clears + // DeleteAt when re-activating. + existing, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType) + if appErr != nil && appErr.StatusCode != http.StatusNotFound { + appErr.Where = "Api4.linkGroupSyncable" + c.Err = appErr + return + } + + var groupSyncable *model.GroupSyncable + if existing != nil && existing.DeleteAt == 0 { + groupSyncable = existing + } else { + groupSyncable = &model.GroupSyncable{ + GroupId: c.Params.GroupId, + SyncableId: syncableID, + Type: syncableType, + } } groupSyncable.Patch(patch) groupSyncable, appErr = c.App.UpsertGroupSyncable(groupSyncable) @@ -392,8 +417,9 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventResultState(groupSyncable) auditRec.AddEventObjectType("group_syncable") + syncRoles := patch.SchemeAdmin != nil c.App.Srv().Go(func() { - c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId) + c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId, syncRoles) }) w.WriteHeader(http.StatusCreated) @@ -560,6 +586,13 @@ func patchGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { return } + appErr = verifySchemeAdminAssignmentPermission(c, syncableType, syncableID, patch) + if appErr != nil { + appErr.Where = "Api4.patchGroupSyncable" + c.Err = appErr + return + } + groupSyncable, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType) if appErr != nil { c.Err = appErr @@ -577,8 +610,9 @@ func patchGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventResultState(groupSyncable) auditRec.AddEventObjectType("group_syncable") + syncRoles := patch.SchemeAdmin != nil c.App.Srv().Go(func() { - c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId) + c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId, syncRoles) }) b, err := json.Marshal(groupSyncable) @@ -710,6 +744,34 @@ func verifyLinkUnlinkPermission(c *Context, syncableType model.GroupSyncableType return nil } +// verifySchemeAdminAssignmentPermission requires the caller to hold the +// role-management permission for the target syncable +// (manage_team_roles / manage_channel_roles), or the sysconsole groups +// write permission, before an explicit SchemeAdmin value in the patch is +// accepted. A nil patch.SchemeAdmin is a no-op. +func verifySchemeAdminAssignmentPermission(c *Context, syncableType model.GroupSyncableType, syncableID string, patch *model.GroupSyncablePatch) *model.AppError { + if patch == nil || patch.SchemeAdmin == nil { + return nil + } + + if c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleWriteUserManagementGroups) { + return nil + } + + switch syncableType { + case model.GroupSyncableTypeTeam: + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), syncableID, model.PermissionManageTeamRoles) { + return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionManageTeamRoles}) + } + case model.GroupSyncableTypeChannel: + if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), syncableID, model.PermissionManageChannelRoles); !ok { + return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionManageChannelRoles}) + } + } + + return nil +} + func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) { permissionErr := requireLicense(c) if permissionErr != nil {
server/channels/api4/group_test.go+733 −0 modified@@ -2847,3 +2847,736 @@ func TestDeleteMembersFromGroup(t *testing.T) { CheckBadRequestStatus(t, response) }) } + +// newSchemeAdminTestLdapGroup creates a fresh LDAP-source group with +// AllowReference=true. +func newSchemeAdminTestLdapGroup(t *testing.T, th *TestHelper) *model.Group { + t.Helper() + id := model.NewId() + g, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, + }) + require.Nil(t, appErr) + return g +} + +// findPersistedGroupSyncable returns the persisted GroupSyncable for a +// given (groupID, syncableID, syncableType) tuple, including SchemeAdmin. +func findPersistedGroupSyncable(t *testing.T, th *TestHelper, groupID, syncableID string, syncableType model.GroupSyncableType) *model.GroupSyncable { + t.Helper() + syncables, appErr := th.App.GetGroupSyncables(groupID, syncableType) + require.Nil(t, appErr) + for _, s := range syncables { + if s.SyncableId == syncableID { + return s + } + } + return nil +} + +func TestLinkGroupTeam_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + schemeAdminTrue := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + } + + t.Run("regular team user with invite_user must NOT be able to set scheme_admin: true", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) + + t.Run("system admin can still set scheme_admin: true", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user can still link with scheme_admin omitted", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + require.NotNil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user must NOT be able to link with scheme_admin: false explicitly", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) +} + +func TestLinkGroupChannel_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + // A regular user can only link a channel syncable when the group is + // already linked to the parent team, so seed the team link as sysadmin. + mkLinkedGroup := func(t *testing.T) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + return g + } + + schemeAdminTrue := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + } + + t.Run("regular channel user with manage_*_channel_members must NOT be able to set scheme_admin: true", func(t *testing.T) { + g := mkLinkedGroup(t) + + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) + + t.Run("system admin can still set scheme_admin: true", func(t *testing.T) { + g := mkLinkedGroup(t) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user can still link with scheme_admin omitted", func(t *testing.T) { + g := mkLinkedGroup(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + require.NotNil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user must NOT be able to link with scheme_admin: false explicitly", func(t *testing.T) { + g := mkLinkedGroup(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) +} + +func TestPatchGroupTeam_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + // schemeAdmin controls the seeded SchemeAdmin value on the team syncable. + setupLinkedGroup := func(t *testing.T, schemeAdmin bool) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(schemeAdmin), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + return g + } + + schemeAdminTrue := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(true), + } + + schemeAdminFalse := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(false), + } + + t.Run("regular team user with invite_user must NOT be able to patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user can still patch other fields with scheme_admin omitted", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.AutoAdd) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user must NOT be able to patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("sysconsole_write_user_management_groups holder can patch scheme_admin in either direction", func(t *testing.T) { + // system_manager bundles sysconsole_write_user_management_groups, + // the override honoured by verifySchemeAdminAssignmentPermission. + th.LoginSystemManager() + + gPromote := setupLinkedGroup(t, false) + _, response, err := th.SystemManagerClient.PatchGroupSyncable(context.Background(), gPromote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedPromote := findPersistedGroupSyncable(t, th, gPromote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persistedPromote) + assert.True(t, persistedPromote.SchemeAdmin) + + gDemote := setupLinkedGroup(t, true) + _, response, err = th.SystemManagerClient.PatchGroupSyncable(context.Background(), gDemote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedDemote := findPersistedGroupSyncable(t, th, gDemote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persistedDemote) + assert.False(t, persistedDemote.SchemeAdmin) + }) +} + +func TestPatchGroupChannel_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + // schemeAdmin controls the seeded SchemeAdmin value on the channel + // syncable. The team syncable is seeded so the channel link succeeds. + setupLinkedGroup := func(t *testing.T, schemeAdmin bool) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + _, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(schemeAdmin), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + return g + } + + schemeAdminTrue := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(true), + } + + schemeAdminFalse := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(false), + } + + t.Run("regular channel user with manage_*_channel_members must NOT be able to patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user can still patch other fields with scheme_admin omitted", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.AutoAdd) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user must NOT be able to patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("sysconsole_write_user_management_groups holder can patch scheme_admin in either direction", func(t *testing.T) { + // system_manager bundles sysconsole_write_user_management_groups, + // the override honoured by verifySchemeAdminAssignmentPermission. + th.LoginSystemManager() + + gPromote := setupLinkedGroup(t, false) + _, response, err := th.SystemManagerClient.PatchGroupSyncable(context.Background(), gPromote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedPromote := findPersistedGroupSyncable(t, th, gPromote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persistedPromote) + assert.True(t, persistedPromote.SchemeAdmin) + + gDemote := setupLinkedGroup(t, true) + _, response, err = th.SystemManagerClient.PatchGroupSyncable(context.Background(), gDemote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedDemote := findPersistedGroupSyncable(t, th, gDemote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persistedDemote) + assert.False(t, persistedDemote.SchemeAdmin) + }) +} + +func TestLinkGroupTeam_LinkOnExistingPreservesSchemeAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + seedSchemeAdminTrue := func(t *testing.T) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + require.True(t, persisted.SchemeAdmin) + return g + } + + t.Run("regular team user calling LINK with scheme_admin omitted must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user calling LINK with scheme_admin: false must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) +} + +func TestLinkGroupChannel_LinkOnExistingPreservesSchemeAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + seedSchemeAdminTrue := func(t *testing.T) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + _, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + require.True(t, persisted.SchemeAdmin) + return g + } + + t.Run("regular channel user calling LINK with scheme_admin omitted must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user calling LINK with scheme_admin: false must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) +} + +func TestLinkGroupTeam_LinkOnSoftDeletedDoesNotPreserveSchemeAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + t.Run("regular team user re-linking a soft-deleted syncable with scheme_admin omitted must persist scheme_admin: false", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + response, err = th.SystemAdminClient.UnlinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NoError(t, err) + CheckOKStatus(t, response) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, response, err = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) +} + +func TestPatchGroupTeam_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + th.UpdateUserToTeamAdmin(th.BasicUser2, th.BasicTeam) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err = th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) +} + +func TestPatchGroupChannel_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + _, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + th.MakeUserChannelAdmin(th.BasicUser2, th.BasicChannel) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err = th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + time.Sleep(2 * time.Second) + + cm, appErr := th.App.GetChannelMember(th.Context, th.BasicChannel.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, cm.SchemeAdmin) +} + +func TestLinkGroupTeam_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + th.UpdateUserToTeamAdmin(th.BasicUser2, th.BasicTeam) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) +} + +func TestLinkGroupChannel_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + th.MakeUserChannelAdmin(th.BasicUser2, th.BasicChannel) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, response, err = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + cm, appErr := th.App.GetChannelMember(th.Context, th.BasicChannel.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, cm.SchemeAdmin) +} + +func TestLinkGroupTeam_SchemeAdminTruePromotesGroupMembers(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, appErr := th.App.UpsertGroupMember(g.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) +} + +func TestLinkGroupTeam_AutoAddOnlyAddsGroupMembers(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + newUser := th.CreateUser() + _, appErr := th.App.UpsertGroupMember(g.Id, newUser.Id) + require.Nil(t, appErr) + + _, appErr = th.App.GetTeamMember(th.Context, th.BasicTeam.Id, newUser.Id) + require.NotNil(t, appErr) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, newUser.Id) + require.Nil(t, appErr) + assert.Equal(t, newUser.Id, tm.UserId) +}
server/channels/app/syncables.go+8 −6 modified@@ -268,18 +268,20 @@ func (a *App) SyncSyncableRoles(rctx request.CTX, syncableID string, syncableTyp return nil } -// SyncRolesAndMembership updates the SchemeAdmin status and membership of all of the members of the given -// syncable. -func (a *App) SyncRolesAndMembership(rctx request.CTX, syncableID string, syncableType model.GroupSyncableType, groupID string) { +// SyncRolesAndMembership updates the membership of the given syncable and, +// when syncRoles is true, also reconciles SchemeAdmin status for its members. +func (a *App) SyncRolesAndMembership(rctx request.CTX, syncableID string, syncableType model.GroupSyncableType, groupID string, syncRoles bool) { group, appErr := a.GetGroup(groupID, nil, nil) if appErr != nil { rctx.Logger().Warn("Error getting group", mlog.Err(appErr)) return } - appErr = a.SyncSyncableRoles(rctx, syncableID, syncableType) - if appErr != nil { - rctx.Logger().Warn("Error syncing syncable roles", mlog.Err(appErr)) + if syncRoles { + appErr = a.SyncSyncableRoles(rctx, syncableID, syncableType) + if appErr != nil { + rctx.Logger().Warn("Error syncing syncable roles", mlog.Err(appErr)) + } } var since int64
server/channels/app/syncables_test.go+144 −0 modified@@ -677,3 +677,147 @@ func TestSyncSyncableRoles(t *testing.T) { require.True(t, cm.SchemeAdmin) } } + +func TestSyncRolesAndMembership_RoleSyncGate(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + setup := func(t *testing.T) (*model.Team, *model.Channel, *model.Group, *model.User) { + t.Helper() + + team := th.CreateTeam() + channel := th.CreateChannel(th.Context, team) + group := th.CreateGroup() + + _, err := th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: team.Id, + Type: model.GroupSyncableTypeTeam, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + _, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: channel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + directAdmin := th.CreateUser() + _, appErr := th.App.AddTeamMember(th.Context, team.Id, directAdmin.Id) + require.Nil(t, appErr) + _, appErr = th.App.AddUserToChannel(th.Context, directAdmin, channel, false) + require.Nil(t, appErr) + + tm, storeErr := th.App.Srv().Store().Team().GetMember(th.Context, team.Id, directAdmin.Id) + require.NoError(t, storeErr) + tm.SchemeAdmin = true + _, storeErr = th.App.Srv().Store().Team().UpdateMember(th.Context, tm) + require.NoError(t, storeErr) + + cm, storeErr := th.App.Srv().Store().Channel().GetMember(th.Context.Context(), channel.Id, directAdmin.Id) + require.NoError(t, storeErr) + cm.SchemeAdmin = true + _, storeErr = th.App.Srv().Store().Channel().UpdateMember(th.Context, cm) + require.NoError(t, storeErr) + + return team, channel, group, directAdmin + } + + t.Run("syncRoles=false preserves the existing SchemeAdmin on team members", func(t *testing.T) { + team, _, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, false) + + tm, appErr := th.App.GetTeamMember(th.Context, team.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) + }) + + t.Run("syncRoles=false preserves the existing SchemeAdmin on channel members", func(t *testing.T) { + _, channel, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, false) + + cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.True(t, cm.SchemeAdmin) + }) + + t.Run("syncRoles=true reconciles team SchemeAdmin against PermittedSyncableAdmins", func(t *testing.T) { + team, _, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, true) + + tm, appErr := th.App.GetTeamMember(th.Context, team.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.False(t, tm.SchemeAdmin) + }) + + t.Run("syncRoles=true reconciles channel SchemeAdmin against PermittedSyncableAdmins", func(t *testing.T) { + _, channel, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, true) + + cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.False(t, cm.SchemeAdmin) + }) +} + +func TestSyncRolesAndMembership_AlwaysSyncsMembership(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + setup := func(t *testing.T) (*model.Team, *model.Channel, *model.Group, *model.User) { + t.Helper() + + team := th.CreateTeam() + channel := th.CreateChannel(th.Context, team) + group := th.CreateGroup() + + _, err := th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: team.Id, + Type: model.GroupSyncableTypeTeam, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + _, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: channel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + groupMember := th.CreateUser() + _, err = th.App.UpsertGroupMember(group.Id, groupMember.Id) + require.Nil(t, err) + + return team, channel, group, groupMember + } + + t.Run("syncRoles=false still adds group members to the team", func(t *testing.T) { + team, _, group, groupMember := setup(t) + + th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, false) + + tm, appErr := th.App.GetTeamMember(th.Context, team.Id, groupMember.Id) + require.Nil(t, appErr) + assert.Equal(t, groupMember.Id, tm.UserId) + }) + + t.Run("syncRoles=false still adds group members to the channel", func(t *testing.T) { + _, channel, group, groupMember := setup(t) + + th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, false) + + cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, groupMember.Id) + require.Nil(t, appErr) + assert.Equal(t, groupMember.Id, cm.UserId) + }) +}
server/channels/store/sqlstore/group_store.go+3 −1 modified@@ -777,6 +777,7 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync groupSyncable.DeleteAt = groupTeam.DeleteAt groupSyncable.UpdateAt = groupTeam.UpdateAt groupSyncable.Type = syncableType + groupSyncable.SchemeAdmin = groupTeam.SchemeAdmin case model.GroupSyncableTypeChannel: groupChannel := result.(*groupChannel) groupSyncable.SyncableId = groupChannel.ChannelId @@ -786,6 +787,7 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync groupSyncable.DeleteAt = groupChannel.DeleteAt groupSyncable.UpdateAt = groupChannel.UpdateAt groupSyncable.Type = syncableType + groupSyncable.SchemeAdmin = groupChannel.SchemeAdmin default: return nil, fmt.Errorf("unable to convert syncableType: %s", syncableType.String()) } @@ -1834,7 +1836,7 @@ func (s *SqlGroupStore) AdminRoleGroupsForSyncableMember(userID, syncableID stri func (s *SqlGroupStore) PermittedSyncableAdmins(syncableID string, syncableType model.GroupSyncableType) ([]string, error) { builder := s.getQueryBuilder().Select("UserId"). From(fmt.Sprintf("Group%ss", syncableType)). - Join(fmt.Sprintf("GroupMembers ON GroupMembers.GroupId = Group%ss.GroupId AND Group%[1]ss.SchemeAdmin = TRUE AND GroupMembers.DeleteAt = 0", syncableType.String())).Where(fmt.Sprintf("Group%[1]ss.%[1]sId = ?", syncableType.String()), syncableID) + Join(fmt.Sprintf("GroupMembers ON GroupMembers.GroupId = Group%ss.GroupId AND Group%[1]ss.SchemeAdmin = TRUE AND Group%[1]ss.DeleteAt = 0 AND GroupMembers.DeleteAt = 0", syncableType.String())).Where(fmt.Sprintf("Group%[1]ss.%[1]sId = ?", syncableType.String()), syncableID) var userIDs []string if err := s.GetMaster().SelectBuilder(&userIDs, builder); err != nil {
server/channels/store/storetest/group_store.go+28 −0 modified@@ -1616,9 +1616,19 @@ func testGetGroupSyncable(t *testing.T, rctx request.CTX, ss store.Store) { require.Equal(t, gt1.GroupId, dgt.GroupId) require.Equal(t, gt1.SyncableId, dgt.SyncableId) require.Equal(t, gt1.AutoAdd, dgt.AutoAdd) + require.Equal(t, gt1.SchemeAdmin, dgt.SchemeAdmin) require.NotZero(t, gt1.CreateAt) require.NotZero(t, gt1.UpdateAt) require.Zero(t, gt1.DeleteAt) + + // Round-trip SchemeAdmin: true through UpdateGroupSyncable and re-fetch. + dgt.SchemeAdmin = true + _, err = ss.Group().UpdateGroupSyncable(dgt) + require.NoError(t, err) + + dgt, err = ss.Group().GetGroupSyncable(groupTeam.GroupId, groupTeam.SyncableId, model.GroupSyncableTypeTeam) + require.NoError(t, err) + require.True(t, dgt.SchemeAdmin, "GetGroupSyncable must populate SchemeAdmin from the persisted row") } func testGetGroupSyncableErrors(t *testing.T, rctx request.CTX, ss store.Store) { @@ -4989,6 +4999,15 @@ func groupTestPermittedSyncableAdminsTeam(t *testing.T, rctx request.CTX, ss sto // deleted group syncable no longer includes group members _, err = ss.Group().DeleteGroupSyncable(group1.Id, team.Id, model.GroupSyncableTypeTeam) require.NoError(t, err) + + // The persisted row must still carry SchemeAdmin=true after soft-delete; + // PermittedSyncableAdmins excludes it via the DeleteAt = 0 predicate, not + // via the field having been silently cleared. + deletedSyncable, err := ss.Group().GetGroupSyncable(group1.Id, team.Id, model.GroupSyncableTypeTeam) + require.NoError(t, err) + require.True(t, deletedSyncable.SchemeAdmin) + require.NotZero(t, deletedSyncable.DeleteAt) + actualUserIDs, err = ss.Group().PermittedSyncableAdmins(team.Id, model.GroupSyncableTypeTeam) require.NoError(t, err) require.ElementsMatch(t, []string{user3.Id}, actualUserIDs) @@ -5096,6 +5115,15 @@ func groupTestPermittedSyncableAdminsChannel(t *testing.T, rctx request.CTX, ss // deleted group syncable no longer includes group members _, err = ss.Group().DeleteGroupSyncable(group1.Id, channel.Id, model.GroupSyncableTypeChannel) require.NoError(t, err) + + // The persisted row must still carry SchemeAdmin=true after soft-delete; + // PermittedSyncableAdmins excludes it via the DeleteAt = 0 predicate, not + // via the field having been silently cleared. + deletedSyncable, err := ss.Group().GetGroupSyncable(group1.Id, channel.Id, model.GroupSyncableTypeChannel) + require.NoError(t, err) + require.True(t, deletedSyncable.SchemeAdmin) + require.NotZero(t, deletedSyncable.DeleteAt) + actualUserIDs, err = ss.Group().PermittedSyncableAdmins(channel.Id, model.GroupSyncableTypeChannel) require.NoError(t, err) require.ElementsMatch(t, []string{user3.Id}, actualUserIDs)
webapp/channels/src/components/admin_console/group_settings/group_details/group_details.tsx+16 −8 modified@@ -417,17 +417,25 @@ class GroupDetails extends React.PureComponent<Props, State> { roleChangeKey = (groupTeamOrChannel: { type?: SyncableType; + id?: string; team_id?: string; channel_id?: string; }) => { - let id; - if ( - this.syncableTypeFromEntryType(groupTeamOrChannel.type) === - SyncableType.Team - ) { - id = groupTeamOrChannel.team_id; - } else { - id = groupTeamOrChannel.channel_id; + // Items in itemsToRemove use a generic `id`, while items coming from + // teamsToAdd/channelsToAdd use `team_id`/`channel_id`. The key must + // be identical regardless of source so the dedup in + // handleRemovedTeamsAndChannels and handleAddedTeamsAndChannels + // matches the key produced by onChangeRoles. + let id = groupTeamOrChannel.id; + if (!id) { + if ( + this.syncableTypeFromEntryType(groupTeamOrChannel.type) === + SyncableType.Team + ) { + id = groupTeamOrChannel.team_id; + } else { + id = groupTeamOrChannel.channel_id; + } } return `${id}/${groupTeamOrChannel.type}`; };
1ce2484a00c9MM-68547: Tighten authorization on group syncable link and patch endpoints (#36316) (#36431)
8 files changed · +1006 −23
e2e-tests/cypress/tests/integration/channels/enterprise/system_console/group_configuration_spec.js+6 −2 modified@@ -394,8 +394,10 @@ describe('group configuration', () => { // # Save settings savePage(); - // * Check the groupteam via the API to ensure its role wasn't updated + // * Check the groupteam via the API to ensure the team was + // removed (delete_at != 0) and its role wasn't updated. cy.apiGetGroupTeam(groupID, testTeam.id).then(({body}) => { + expect(body.delete_at).to.not.eq(0); expect(body.scheme_admin).to.eq(false); }); }); @@ -519,8 +521,10 @@ describe('group configuration', () => { // # Save settings savePage(); - // * Check the groupteam via the API to ensure its role wasn't updated + // * Check the groupteam via the API to ensure the channel was + // removed (delete_at != 0) and its role wasn't updated. cy.apiGetGroupChannel(groupID, testChannel.id).then(({body}) => { + expect(body.delete_at).to.not.eq(0); expect(body.scheme_admin).to.eq(false); }); });
server/channels/api4/group.go+68 −6 modified@@ -372,10 +372,35 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { return } - groupSyncable := &model.GroupSyncable{ - GroupId: c.Params.GroupId, - SyncableId: syncableID, - Type: syncableType, + appErr = verifySchemeAdminAssignmentPermission(c, syncableType, syncableID, patch) + if appErr != nil { + appErr.Where = "Api4.linkGroupSyncable" + c.Err = appErr + return + } + + // Upsert onto the existing row only when it is currently active so + // unspecified fields are preserved. A fresh link, or a re-link of a + // soft-deleted row, starts from a zero-value struct so that fields + // the caller did not (or was not authorized to) set are not carried + // over from the previous incarnation. The downstream upsert clears + // DeleteAt when re-activating. + existing, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType) + if appErr != nil && appErr.StatusCode != http.StatusNotFound { + appErr.Where = "Api4.linkGroupSyncable" + c.Err = appErr + return + } + + var groupSyncable *model.GroupSyncable + if existing != nil && existing.DeleteAt == 0 { + groupSyncable = existing + } else { + groupSyncable = &model.GroupSyncable{ + GroupId: c.Params.GroupId, + SyncableId: syncableID, + Type: syncableType, + } } groupSyncable.Patch(patch) groupSyncable, appErr = c.App.UpsertGroupSyncable(groupSyncable) @@ -387,8 +412,9 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventResultState(groupSyncable) auditRec.AddEventObjectType("group_syncable") + syncRoles := patch.SchemeAdmin != nil c.App.Srv().Go(func() { - c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId) + c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId, syncRoles) }) w.WriteHeader(http.StatusCreated) @@ -555,6 +581,13 @@ func patchGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { return } + appErr = verifySchemeAdminAssignmentPermission(c, syncableType, syncableID, patch) + if appErr != nil { + appErr.Where = "Api4.patchGroupSyncable" + c.Err = appErr + return + } + groupSyncable, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType) if appErr != nil { c.Err = appErr @@ -572,8 +605,9 @@ func patchGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddEventResultState(groupSyncable) auditRec.AddEventObjectType("group_syncable") + syncRoles := patch.SchemeAdmin != nil c.App.Srv().Go(func() { - c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId) + c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId, syncRoles) }) b, err := json.Marshal(groupSyncable) @@ -705,6 +739,34 @@ func verifyLinkUnlinkPermission(c *Context, syncableType model.GroupSyncableType return nil } +// verifySchemeAdminAssignmentPermission requires the caller to hold the +// role-management permission for the target syncable +// (manage_team_roles / manage_channel_roles), or the sysconsole groups +// write permission, before an explicit SchemeAdmin value in the patch is +// accepted. A nil patch.SchemeAdmin is a no-op. +func verifySchemeAdminAssignmentPermission(c *Context, syncableType model.GroupSyncableType, syncableID string, patch *model.GroupSyncablePatch) *model.AppError { + if patch == nil || patch.SchemeAdmin == nil { + return nil + } + + if c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleWriteUserManagementGroups) { + return nil + } + + switch syncableType { + case model.GroupSyncableTypeTeam: + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), syncableID, model.PermissionManageTeamRoles) { + return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionManageTeamRoles}) + } + case model.GroupSyncableTypeChannel: + if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), syncableID, model.PermissionManageChannelRoles); !ok { + return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionManageChannelRoles}) + } + } + + return nil +} + func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) { permissionErr := requireLicense(c) if permissionErr != nil {
server/channels/api4/group_test.go+733 −0 modified@@ -3002,3 +3002,736 @@ func TestDeleteMembersFromGroup(t *testing.T) { CheckBadRequestStatus(t, response) }) } + +// newSchemeAdminTestLdapGroup creates a fresh LDAP-source group with +// AllowReference=true. +func newSchemeAdminTestLdapGroup(t *testing.T, th *TestHelper) *model.Group { + t.Helper() + id := model.NewId() + g, appErr := th.App.CreateGroup(&model.Group{ + DisplayName: "dn_" + id, + Name: model.NewPointer("name" + id), + Source: model.GroupSourceLdap, + Description: "description_" + id, + RemoteId: model.NewPointer(model.NewId()), + AllowReference: true, + }) + require.Nil(t, appErr) + return g +} + +// findPersistedGroupSyncable returns the persisted GroupSyncable for a +// given (groupID, syncableID, syncableType) tuple, including SchemeAdmin. +func findPersistedGroupSyncable(t *testing.T, th *TestHelper, groupID, syncableID string, syncableType model.GroupSyncableType) *model.GroupSyncable { + t.Helper() + syncables, appErr := th.App.GetGroupSyncables(groupID, syncableType) + require.Nil(t, appErr) + for _, s := range syncables { + if s.SyncableId == syncableID { + return s + } + } + return nil +} + +func TestLinkGroupTeam_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + schemeAdminTrue := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + } + + t.Run("regular team user with invite_user must NOT be able to set scheme_admin: true", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) + + t.Run("system admin can still set scheme_admin: true", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user can still link with scheme_admin omitted", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + require.NotNil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user must NOT be able to link with scheme_admin: false explicitly", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) +} + +func TestLinkGroupChannel_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + // A regular user can only link a channel syncable when the group is + // already linked to the parent team, so seed the team link as sysadmin. + mkLinkedGroup := func(t *testing.T) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + return g + } + + schemeAdminTrue := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + } + + t.Run("regular channel user with manage_*_channel_members must NOT be able to set scheme_admin: true", func(t *testing.T) { + g := mkLinkedGroup(t) + + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) + + t.Run("system admin can still set scheme_admin: true", func(t *testing.T) { + g := mkLinkedGroup(t) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user can still link with scheme_admin omitted", func(t *testing.T) { + g := mkLinkedGroup(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + require.NotNil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user must NOT be able to link with scheme_admin: false explicitly", func(t *testing.T) { + g := mkLinkedGroup(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.Error(t, err) + CheckForbiddenStatus(t, response) + assert.Nil(t, groupSyncable) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + if persisted != nil { + assert.False(t, persisted.SchemeAdmin) + } + }) +} + +func TestPatchGroupTeam_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + // schemeAdmin controls the seeded SchemeAdmin value on the team syncable. + setupLinkedGroup := func(t *testing.T, schemeAdmin bool) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(schemeAdmin), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + return g + } + + schemeAdminTrue := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(true), + } + + schemeAdminFalse := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(false), + } + + t.Run("regular team user with invite_user must NOT be able to patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user can still patch other fields with scheme_admin omitted", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.AutoAdd) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user must NOT be able to patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("sysconsole_write_user_management_groups holder can patch scheme_admin in either direction", func(t *testing.T) { + // system_manager bundles sysconsole_write_user_management_groups, + // the override honoured by verifySchemeAdminAssignmentPermission. + th.LoginSystemManager(t) + + gPromote := setupLinkedGroup(t, false) + _, response, err := th.SystemManagerClient.PatchGroupSyncable(context.Background(), gPromote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedPromote := findPersistedGroupSyncable(t, th, gPromote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persistedPromote) + assert.True(t, persistedPromote.SchemeAdmin) + + gDemote := setupLinkedGroup(t, true) + _, response, err = th.SystemManagerClient.PatchGroupSyncable(context.Background(), gDemote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedDemote := findPersistedGroupSyncable(t, th, gDemote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persistedDemote) + assert.False(t, persistedDemote.SchemeAdmin) + }) +} + +func TestPatchGroupChannel_SchemeAdminRequiresElevatedPermission(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + // schemeAdmin controls the seeded SchemeAdmin value on the channel + // syncable. The team syncable is seeded so the channel link succeeds. + setupLinkedGroup := func(t *testing.T, schemeAdmin bool) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + _, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(schemeAdmin), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + return g + } + + schemeAdminTrue := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(true), + } + + schemeAdminFalse := &model.GroupSyncablePatch{ + SchemeAdmin: model.NewPointer(false), + } + + t.Run("regular channel user with manage_*_channel_members must NOT be able to patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: true", func(t *testing.T) { + g := setupLinkedGroup(t, false) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user can still patch other fields with scheme_admin omitted", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.AutoAdd) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user must NOT be able to patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse) + require.Error(t, err) + CheckForbiddenStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("system admin can still patch scheme_admin: false", func(t *testing.T) { + g := setupLinkedGroup(t, true) + + _, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) + + t.Run("sysconsole_write_user_management_groups holder can patch scheme_admin in either direction", func(t *testing.T) { + // system_manager bundles sysconsole_write_user_management_groups, + // the override honoured by verifySchemeAdminAssignmentPermission. + th.LoginSystemManager(t) + + gPromote := setupLinkedGroup(t, false) + _, response, err := th.SystemManagerClient.PatchGroupSyncable(context.Background(), gPromote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedPromote := findPersistedGroupSyncable(t, th, gPromote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persistedPromote) + assert.True(t, persistedPromote.SchemeAdmin) + + gDemote := setupLinkedGroup(t, true) + _, response, err = th.SystemManagerClient.PatchGroupSyncable(context.Background(), gDemote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse) + require.NoError(t, err) + CheckOKStatus(t, response) + persistedDemote := findPersistedGroupSyncable(t, th, gDemote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persistedDemote) + assert.False(t, persistedDemote.SchemeAdmin) + }) +} + +func TestLinkGroupTeam_LinkOnExistingPreservesSchemeAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + seedSchemeAdminTrue := func(t *testing.T) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + require.True(t, persisted.SchemeAdmin) + return g + } + + t.Run("regular team user calling LINK with scheme_admin omitted must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular team user calling LINK with scheme_admin: false must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) +} + +func TestLinkGroupChannel_LinkOnExistingPreservesSchemeAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + seedSchemeAdminTrue := func(t *testing.T) *model.Group { + t.Helper() + g := newSchemeAdminTestLdapGroup(t, th) + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + _, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + require.True(t, persisted.SchemeAdmin) + return g + } + + t.Run("regular channel user calling LINK with scheme_admin omitted must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) + + t.Run("regular channel user calling LINK with scheme_admin: false must not change persisted scheme_admin", func(t *testing.T) { + g := seedSchemeAdminTrue(t) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(false), + } + _, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel) + require.NotNil(t, persisted) + assert.True(t, persisted.SchemeAdmin) + }) +} + +func TestLinkGroupTeam_LinkOnSoftDeletedDoesNotPreserveSchemeAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + t.Run("regular team user re-linking a soft-deleted syncable with scheme_admin omitted must persist scheme_admin: false", func(t *testing.T) { + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + response, err = th.SystemAdminClient.UnlinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NoError(t, err) + CheckOKStatus(t, response) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, response, err = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam) + require.NotNil(t, persisted) + assert.False(t, persisted.SchemeAdmin) + }) +} + +func TestPatchGroupTeam_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + th.UpdateUserToTeamAdmin(t, th.BasicUser2, th.BasicTeam) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err = th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) +} + +func TestPatchGroupChannel_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + _, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + th.MakeUserChannelAdmin(t, th.BasicUser2, th.BasicChannel) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(false), + } + _, response, err = th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckOKStatus(t, response) + + time.Sleep(2 * time.Second) + + cm, appErr := th.App.GetChannelMember(th.Context, th.BasicChannel.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, cm.SchemeAdmin) +} + +func TestLinkGroupTeam_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + th.UpdateUserToTeamAdmin(t, th.BasicUser2, th.BasicTeam) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) +} + +func TestLinkGroupChannel_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + th.MakeUserChannelAdmin(t, th.BasicUser2, th.BasicChannel) + + patch := &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + } + _, response, err = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + cm, appErr := th.App.GetChannelMember(th.Context, th.BasicChannel.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, cm.SchemeAdmin) +} + +func TestLinkGroupTeam_SchemeAdminTruePromotesGroupMembers(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + _, appErr := th.App.UpsertGroupMember(g.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + SchemeAdmin: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) +} + +func TestLinkGroupTeam_AutoAddOnlyAddsGroupMembers(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(model.NewTestLicense("ldap")) + + g := newSchemeAdminTestLdapGroup(t, th) + + newUser := th.CreateUser(t) + _, appErr := th.App.UpsertGroupMember(g.Id, newUser.Id) + require.Nil(t, appErr) + + _, appErr = th.App.GetTeamMember(th.Context, th.BasicTeam.Id, newUser.Id) + require.NotNil(t, appErr) + + _, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{ + AutoAdd: model.NewPointer(true), + }) + require.NoError(t, err) + CheckCreatedStatus(t, response) + + time.Sleep(2 * time.Second) + + tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, newUser.Id) + require.Nil(t, appErr) + assert.Equal(t, newUser.Id, tm.UserId) +}
server/channels/app/syncables.go+8 −6 modified@@ -268,18 +268,20 @@ func (a *App) SyncSyncableRoles(rctx request.CTX, syncableID string, syncableTyp return nil } -// SyncRolesAndMembership updates the SchemeAdmin status and membership of all of the members of the given -// syncable. -func (a *App) SyncRolesAndMembership(rctx request.CTX, syncableID string, syncableType model.GroupSyncableType, groupID string) { +// SyncRolesAndMembership updates the membership of the given syncable and, +// when syncRoles is true, also reconciles SchemeAdmin status for its members. +func (a *App) SyncRolesAndMembership(rctx request.CTX, syncableID string, syncableType model.GroupSyncableType, groupID string, syncRoles bool) { group, appErr := a.GetGroup(groupID, nil, nil) if appErr != nil { rctx.Logger().Warn("Error getting group", mlog.Err(appErr)) return } - appErr = a.SyncSyncableRoles(rctx, syncableID, syncableType) - if appErr != nil { - rctx.Logger().Warn("Error syncing syncable roles", mlog.Err(appErr)) + if syncRoles { + appErr = a.SyncSyncableRoles(rctx, syncableID, syncableType) + if appErr != nil { + rctx.Logger().Warn("Error syncing syncable roles", mlog.Err(appErr)) + } } var since int64
server/channels/app/syncables_test.go+144 −0 modified@@ -674,3 +674,147 @@ func TestSyncSyncableRoles(t *testing.T) { require.True(t, cm.SchemeAdmin) } } + +func TestSyncRolesAndMembership_RoleSyncGate(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + setup := func(t *testing.T) (*model.Team, *model.Channel, *model.Group, *model.User) { + t.Helper() + + team := th.CreateTeam(t) + channel := th.CreateChannel(t, team) + group := th.CreateGroup(t) + + _, err := th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: team.Id, + Type: model.GroupSyncableTypeTeam, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + _, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: channel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + directAdmin := th.CreateUser(t) + _, appErr := th.App.AddTeamMember(th.Context, team.Id, directAdmin.Id) + require.Nil(t, appErr) + _, appErr = th.App.AddUserToChannel(th.Context, directAdmin, channel, false) + require.Nil(t, appErr) + + tm, storeErr := th.App.Srv().Store().Team().GetMember(th.Context, team.Id, directAdmin.Id) + require.NoError(t, storeErr) + tm.SchemeAdmin = true + _, storeErr = th.App.Srv().Store().Team().UpdateMember(th.Context, tm) + require.NoError(t, storeErr) + + cm, storeErr := th.App.Srv().Store().Channel().GetMember(th.Context, channel.Id, directAdmin.Id) + require.NoError(t, storeErr) + cm.SchemeAdmin = true + _, storeErr = th.App.Srv().Store().Channel().UpdateMember(th.Context, cm) + require.NoError(t, storeErr) + + return team, channel, group, directAdmin + } + + t.Run("syncRoles=false preserves the existing SchemeAdmin on team members", func(t *testing.T) { + team, _, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, false) + + tm, appErr := th.App.GetTeamMember(th.Context, team.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.True(t, tm.SchemeAdmin) + }) + + t.Run("syncRoles=false preserves the existing SchemeAdmin on channel members", func(t *testing.T) { + _, channel, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, false) + + cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.True(t, cm.SchemeAdmin) + }) + + t.Run("syncRoles=true reconciles team SchemeAdmin against PermittedSyncableAdmins", func(t *testing.T) { + team, _, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, true) + + tm, appErr := th.App.GetTeamMember(th.Context, team.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.False(t, tm.SchemeAdmin) + }) + + t.Run("syncRoles=true reconciles channel SchemeAdmin against PermittedSyncableAdmins", func(t *testing.T) { + _, channel, group, directAdmin := setup(t) + + th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, true) + + cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, directAdmin.Id) + require.Nil(t, appErr) + assert.False(t, cm.SchemeAdmin) + }) +} + +func TestSyncRolesAndMembership_AlwaysSyncsMembership(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + setup := func(t *testing.T) (*model.Team, *model.Channel, *model.Group, *model.User) { + t.Helper() + + team := th.CreateTeam(t) + channel := th.CreateChannel(t, team) + group := th.CreateGroup(t) + + _, err := th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: team.Id, + Type: model.GroupSyncableTypeTeam, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + _, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{ + SyncableId: channel.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: group.Id, + AutoAdd: true, + }) + require.Nil(t, err) + + groupMember := th.CreateUser(t) + _, err = th.App.UpsertGroupMember(group.Id, groupMember.Id) + require.Nil(t, err) + + return team, channel, group, groupMember + } + + t.Run("syncRoles=false still adds group members to the team", func(t *testing.T) { + team, _, group, groupMember := setup(t) + + th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, false) + + tm, appErr := th.App.GetTeamMember(th.Context, team.Id, groupMember.Id) + require.Nil(t, appErr) + assert.Equal(t, groupMember.Id, tm.UserId) + }) + + t.Run("syncRoles=false still adds group members to the channel", func(t *testing.T) { + _, channel, group, groupMember := setup(t) + + th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, false) + + cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, groupMember.Id) + require.Nil(t, appErr) + assert.Equal(t, groupMember.Id, cm.UserId) + }) +}
server/channels/store/sqlstore/group_store.go+3 −1 modified@@ -798,6 +798,7 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync groupSyncable.DeleteAt = groupTeam.DeleteAt groupSyncable.UpdateAt = groupTeam.UpdateAt groupSyncable.Type = syncableType + groupSyncable.SchemeAdmin = groupTeam.SchemeAdmin case model.GroupSyncableTypeChannel: groupChannel := result.(*groupChannel) groupSyncable.SyncableId = groupChannel.ChannelId @@ -807,6 +808,7 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync groupSyncable.DeleteAt = groupChannel.DeleteAt groupSyncable.UpdateAt = groupChannel.UpdateAt groupSyncable.Type = syncableType + groupSyncable.SchemeAdmin = groupChannel.SchemeAdmin default: return nil, fmt.Errorf("unable to convert syncableType: %s", syncableType.String()) } @@ -1836,7 +1838,7 @@ func (s *SqlGroupStore) AdminRoleGroupsForSyncableMember(userID, syncableID stri func (s *SqlGroupStore) PermittedSyncableAdmins(syncableID string, syncableType model.GroupSyncableType) ([]string, error) { builder := s.getQueryBuilder().Select("UserId"). From(fmt.Sprintf("Group%ss", syncableType)). - Join(fmt.Sprintf("GroupMembers ON GroupMembers.GroupId = Group%ss.GroupId AND Group%[1]ss.SchemeAdmin = TRUE AND GroupMembers.DeleteAt = 0", syncableType.String())).Where(fmt.Sprintf("Group%[1]ss.%[1]sId = ?", syncableType.String()), syncableID) + Join(fmt.Sprintf("GroupMembers ON GroupMembers.GroupId = Group%ss.GroupId AND Group%[1]ss.SchemeAdmin = TRUE AND Group%[1]ss.DeleteAt = 0 AND GroupMembers.DeleteAt = 0", syncableType.String())).Where(fmt.Sprintf("Group%[1]ss.%[1]sId = ?", syncableType.String()), syncableID) var userIDs []string if err := s.GetMaster().SelectBuilder(&userIDs, builder); err != nil {
server/channels/store/storetest/group_store.go+28 −0 modified@@ -1616,9 +1616,19 @@ func testGetGroupSyncable(t *testing.T, rctx request.CTX, ss store.Store) { require.Equal(t, gt1.GroupId, dgt.GroupId) require.Equal(t, gt1.SyncableId, dgt.SyncableId) require.Equal(t, gt1.AutoAdd, dgt.AutoAdd) + require.Equal(t, gt1.SchemeAdmin, dgt.SchemeAdmin) require.NotZero(t, gt1.CreateAt) require.NotZero(t, gt1.UpdateAt) require.Zero(t, gt1.DeleteAt) + + // Round-trip SchemeAdmin: true through UpdateGroupSyncable and re-fetch. + dgt.SchemeAdmin = true + _, err = ss.Group().UpdateGroupSyncable(dgt) + require.NoError(t, err) + + dgt, err = ss.Group().GetGroupSyncable(groupTeam.GroupId, groupTeam.SyncableId, model.GroupSyncableTypeTeam) + require.NoError(t, err) + require.True(t, dgt.SchemeAdmin, "GetGroupSyncable must populate SchemeAdmin from the persisted row") } func testGetGroupSyncableErrors(t *testing.T, rctx request.CTX, ss store.Store) { @@ -4979,6 +4989,15 @@ func groupTestPermittedSyncableAdminsTeam(t *testing.T, rctx request.CTX, ss sto // deleted group syncable no longer includes group members _, err = ss.Group().DeleteGroupSyncable(group1.Id, team.Id, model.GroupSyncableTypeTeam) require.NoError(t, err) + + // The persisted row must still carry SchemeAdmin=true after soft-delete; + // PermittedSyncableAdmins excludes it via the DeleteAt = 0 predicate, not + // via the field having been silently cleared. + deletedSyncable, err := ss.Group().GetGroupSyncable(group1.Id, team.Id, model.GroupSyncableTypeTeam) + require.NoError(t, err) + require.True(t, deletedSyncable.SchemeAdmin) + require.NotZero(t, deletedSyncable.DeleteAt) + actualUserIDs, err = ss.Group().PermittedSyncableAdmins(team.Id, model.GroupSyncableTypeTeam) require.NoError(t, err) require.ElementsMatch(t, []string{user3.Id}, actualUserIDs) @@ -5086,6 +5105,15 @@ func groupTestPermittedSyncableAdminsChannel(t *testing.T, rctx request.CTX, ss // deleted group syncable no longer includes group members _, err = ss.Group().DeleteGroupSyncable(group1.Id, channel.Id, model.GroupSyncableTypeChannel) require.NoError(t, err) + + // The persisted row must still carry SchemeAdmin=true after soft-delete; + // PermittedSyncableAdmins excludes it via the DeleteAt = 0 predicate, not + // via the field having been silently cleared. + deletedSyncable, err := ss.Group().GetGroupSyncable(group1.Id, channel.Id, model.GroupSyncableTypeChannel) + require.NoError(t, err) + require.True(t, deletedSyncable.SchemeAdmin) + require.NotZero(t, deletedSyncable.DeleteAt) + actualUserIDs, err = ss.Group().PermittedSyncableAdmins(channel.Id, model.GroupSyncableTypeChannel) require.NoError(t, err) require.ElementsMatch(t, []string{user3.Id}, actualUserIDs)
webapp/channels/src/components/admin_console/group_settings/group_details/group_details.tsx+16 −8 modified@@ -417,17 +417,25 @@ class GroupDetails extends React.PureComponent<Props, State> { roleChangeKey = (groupTeamOrChannel: { type?: SyncableType; + id?: string; team_id?: string; channel_id?: string; }) => { - let id; - if ( - this.syncableTypeFromEntryType(groupTeamOrChannel.type) === - SyncableType.Team - ) { - id = groupTeamOrChannel.team_id; - } else { - id = groupTeamOrChannel.channel_id; + // Items in itemsToRemove use a generic `id`, while items coming from + // teamsToAdd/channelsToAdd use `team_id`/`channel_id`. The key must + // be identical regardless of source so the dedup in + // handleRemovedTeamsAndChannels and handleAddedTeamsAndChannels + // matches the key produced by onChangeRoles. + let id = groupTeamOrChannel.id; + if (!id) { + if ( + this.syncableTypeFromEntryType(groupTeamOrChannel.type) === + SyncableType.Team + ) { + id = groupTeamOrChannel.team_id; + } else { + id = groupTeamOrChannel.channel_id; + } } return `${id}/${groupTeamOrChannel.type}`; };
Vulnerability mechanics
Root cause
"Missing validation that a username returned during bot registration belongs to a bot account allows an unprivileged attacker to intercept private messages sent by plugins via direct message channels."
Attack vector
An unprivileged attacker can pre-register a user account with a predictable plugin bot username (e.g., by guessing the username a plugin will request during `EnsureBot`). When the plugin later calls `EnsureBot` to register its bot, the vulnerable code [patch_id=5724812] returns the attacker's user ID instead of creating a new bot account. The plugin then sends private messages via a direct message channel to what it believes is its bot, but the attacker's user account receives those messages, allowing the attacker to intercept private communications intended for the bot. The CVSS vector indicates network-based exploitation with high attack complexity and low privileges required.
Affected code
The vulnerability is in the `EnsureBot` function in `server/channels/app/bot.go`. The patch modifies the logic that handles the case where a username requested by a plugin for bot registration is already taken by an existing user account. Previously, the code would log a warning and return the existing user's ID, allowing a non-bot user account to be treated as a bot. The patch changes this to return an error when the existing user is not a bot account.
What the fix does
The patch in `server/channels/app/bot.go` [patch_id=5724812] changes the `EnsureBot` function so that when a username is already taken by a non-bot user, it returns an error (`"username %q is already taken by a non-bot user"`) instead of silently returning the existing user's ID. This prevents plugins from accidentally using a human user's account as their bot. The added test case in `bot_test.go` confirms that `EnsureBot` now fails when the username belongs to a regular user. The other two patches [patch_id=5724813, patch_id=5724811] address a separate authorization issue in group syncable endpoints and are not directly related to this bot registration vulnerability.
Preconditions
- inputThe attacker must know or guess the username that a plugin will request during bot registration.
- authThe attacker must be able to register a user account with that username before the plugin calls EnsureBot.
- networkThe attacker must have network access to the Mattermost server.
Generated on Jun 12, 2026. Inputs: CWE entries + fix-commit diffs from this CVE's patches. Citations validated against bundle.
References
1News mentions
0No linked articles in our index yet.