From 30140c0a278039cd02561c4c9cd9e9207b87ab56 Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Mon, 10 Jul 2023 13:28:40 -0600 Subject: [PATCH] MM-53098 Fix for checking bot and user permissions on shared endpoints (#23751) * temp commit * update test to allow bot creation * add bot check to updateUser and deleteUser * add more unit tests * lint fixes * lint fix * update based on doc * add more unit tests * lint fixes * fix unit tests * fix unit tests --------- Co-authored-by: Mattermost Build --- server/channels/api4/bot_test.go | 1 + server/channels/api4/user.go | 12 +- server/channels/api4/user_test.go | 58 +++++++ server/channels/app/authorization.go | 12 +- server/channels/app/authorization_test.go | 178 ++++++++++++++++++++++ 5 files changed, 251 insertions(+), 10 deletions(-) diff --git a/server/channels/api4/bot_test.go b/server/channels/api4/bot_test.go index e7a495d33d..dd97d58179 100644 --- a/server/channels/api4/bot_test.go +++ b/server/channels/api4/bot_test.go @@ -106,6 +106,7 @@ func TestCreateBot(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) th.AddPermissionToRole(model.PermissionCreateBot.Id, model.TeamUserRoleId) + th.AddPermissionToRole(model.PermissionManageBots.Id, model.TeamUserRoleId) th.AddPermissionToRole(model.PermissionEditOtherUsers.Id, model.TeamUserRoleId) th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.TeamUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index b010489e66..cdaa4b9d5a 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -428,7 +428,7 @@ func setProfileImage(c *Context, w http.ResponseWriter, r *http.Request) { return } - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), c.Params.UserId) { + if !c.App.SessionHasPermissionToUserOrBot(*c.AppContext.Session(), c.Params.UserId) { c.SetPermissionError(model.PermissionEditOtherUsers) return } @@ -499,7 +499,7 @@ func setDefaultProfileImage(c *Context, w http.ResponseWriter, r *http.Request) return } - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), c.Params.UserId) { + if !c.App.SessionHasPermissionToUserOrBot(*c.AppContext.Session(), c.Params.UserId) { c.SetPermissionError(model.PermissionEditOtherUsers) return } @@ -1249,7 +1249,7 @@ func updateUser(c *Context, w http.ResponseWriter, r *http.Request) { return } - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), user.Id) { + if !c.App.SessionHasPermissionToUserOrBot(*c.AppContext.Session(), user.Id) { c.SetPermissionError(model.PermissionEditOtherUsers) return } @@ -1259,6 +1259,7 @@ func updateUser(c *Context, w http.ResponseWriter, r *http.Request) { c.Err = err return } + // Cannot update a system admin unless user making request is a systemadmin also. if ouser.IsSystemAdmin() && !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { c.SetPermissionError(model.PermissionManageSystem) @@ -1325,7 +1326,7 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) { audit.AddEventParameterAuditable(auditRec, "user_patch", &patch) defer c.LogAuditRec(auditRec) - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), c.Params.UserId) { + if !c.App.SessionHasPermissionToUserOrBot(*c.AppContext.Session(), c.Params.UserId) { c.SetPermissionError(model.PermissionEditOtherUsers) return } @@ -1335,6 +1336,7 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) { c.SetInvalidParam("user_id") return } + auditRec.AddEventPriorState(ouser) auditRec.AddEventObjectType("user") @@ -1402,7 +1404,7 @@ func deleteUser(c *Context, w http.ResponseWriter, r *http.Request) { audit.AddEventParameter(auditRec, "user_id", c.Params.UserId) defer c.LogAuditRec(auditRec) - if !c.App.SessionHasPermissionToUser(*c.AppContext.Session(), userId) { + if !c.App.SessionHasPermissionToUserOrBot(*c.AppContext.Session(), userId) { c.SetPermissionError(model.PermissionEditOtherUsers) return } diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index e133eaa821..c12312b0a8 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -1932,6 +1932,27 @@ func TestUpdateAdminUser(t *testing.T) { require.Equal(t, user.Email, u2.Email) } +func TestUpdateBotUser(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.EnableBotAccountCreation = true + }) + + bot := th.CreateBotWithSystemAdminClient() + botUser, _, err := th.SystemAdminClient.GetUser(context.Background(), bot.UserId, "") + require.NoError(t, err) + + updateUser, _, err := th.SystemAdminClient.UpdateUser(context.Background(), botUser) + require.NoError(t, err) + require.Equal(t, botUser.Id, updateUser.Id) + + _, resp, err := th.Client.UpdateUser(context.Background(), botUser) + require.Error(t, err) + CheckForbiddenStatus(t, resp) +} + func TestPatchUser(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -2043,6 +2064,27 @@ func TestPatchUser(t *testing.T) { require.NoError(t, err) } +func TestPatchBotUser(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.EnableBotAccountCreation = true + }) + + bot := th.CreateBotWithSystemAdminClient() + patch := &model.UserPatch{} + patch.Email = model.NewString("newemail@test.com") + + user, _, err := th.SystemAdminClient.PatchUser(context.Background(), bot.UserId, patch) + require.NoError(t, err) + require.Equal(t, bot.UserId, user.Id) + + _, resp, err := th.Client.PatchUser(context.Background(), bot.UserId, patch) + require.Error(t, err) + CheckForbiddenStatus(t, resp) +} + func TestPatchAdminUser(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -2063,6 +2105,7 @@ func TestPatchAdminUser(t *testing.T) { _, _, err = th.SystemAdminClient.PatchUser(context.Background(), user.Id, patch) require.NoError(t, err) } + func TestUserUnicodeNames(t *testing.T) { th := Setup(t) defer th.TearDown() @@ -2229,6 +2272,21 @@ func TestDeleteUser(t *testing.T) { require.NoError(t, err) } +func TestDeleteBotUser(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(c *model.Config) { + *c.ServiceSettings.EnableBotAccountCreation = true + }) + + bot := th.CreateBotWithSystemAdminClient() + + _, err := th.Client.DeleteUser(context.Background(), bot.UserId) + require.Error(t, err) + require.Equal(t, err.Error(), ": You do not have the appropriate permissions.") +} + func TestPermanentDeleteUser(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/server/channels/app/authorization.go b/server/channels/app/authorization.go index 2688903561..5ef7b80833 100644 --- a/server/channels/app/authorization.go +++ b/server/channels/app/authorization.go @@ -258,14 +258,16 @@ func (a *App) SessionHasPermissionToUserOrBot(session model.Session, userID stri if session.IsUnrestricted() { return true } - if a.SessionHasPermissionToUser(session, userID) { + + err := a.SessionHasPermissionToManageBot(session, userID) + if err == nil { return true } - - if err := a.SessionHasPermissionToManageBot(session, userID); err == nil { - return true + if err.Id == "store.sql_bot.get.missing.app_error" && err.Unwrap() != nil { + if a.SessionHasPermissionToUser(session, userID) { + return true + } } - return false } diff --git a/server/channels/app/authorization_test.go b/server/channels/app/authorization_test.go index 04e2f06488..7b36e6e90d 100644 --- a/server/channels/app/authorization_test.go +++ b/server/channels/app/authorization_test.go @@ -113,6 +113,184 @@ func TestSessionHasPermissionToChannel(t *testing.T) { }) } +func TestHasPermissionToUser(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + assert.True(t, th.App.HasPermissionToUser(th.SystemAdminUser.Id, th.BasicUser.Id)) + assert.True(t, th.App.HasPermissionToUser(th.BasicUser.Id, th.BasicUser.Id)) + assert.False(t, th.App.HasPermissionToUser(th.BasicUser.Id, th.BasicUser2.Id)) +} + +func TestSessionHasPermissionToManageBot(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + bot, err := th.App.CreateBot(th.Context, &model.Bot{ + Username: "username", + Description: "a bot", + OwnerId: th.BasicUser.Id, + }) + require.Nil(t, err) + defer th.App.PermanentDeleteBot(bot.UserId) + assert.NotNil(t, bot) + + t.Run("test my bot", func(t *testing.T) { + session := model.Session{ + UserId: th.BasicUser.Id, + Roles: model.SystemUserRoleId, + } + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.NotNil(t, err) + assert.Equal(t, "store.sql_bot.get.missing.app_error", err.Id) + assert.NoError(t, err.Unwrap()) + + th.AddPermissionToRole(model.PermissionReadBots.Id, model.SystemUserRoleId) + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.NotNil(t, err) + assert.Equal(t, "api.context.permissions.app_error", err.Id) + assert.NoError(t, err.Unwrap()) + + th.AddPermissionToRole(model.PermissionManageBots.Id, model.SystemUserRoleId) + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.Nil(t, err) + + th.RemovePermissionFromRole(model.PermissionReadBots.Id, model.SystemUserRoleId) + th.RemovePermissionFromRole(model.PermissionManageBots.Id, model.SystemUserRoleId) + }) + + t.Run("test others bot", func(t *testing.T) { + session := model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserRoleId, + } + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.NotNil(t, err) + assert.Equal(t, "store.sql_bot.get.missing.app_error", err.Id) + assert.NoError(t, err.Unwrap()) + + th.AddPermissionToRole(model.PermissionReadOthersBots.Id, model.SystemUserRoleId) + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.NotNil(t, err) + assert.Equal(t, "api.context.permissions.app_error", err.Id) + assert.NoError(t, err.Unwrap()) + + th.AddPermissionToRole(model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.Nil(t, err) + + th.RemovePermissionFromRole(model.PermissionReadOthersBots.Id, model.SystemUserRoleId) + th.RemovePermissionFromRole(model.PermissionManageOthersBots.Id, model.SystemUserRoleId) + }) + + t.Run("test sysadmin role", func(t *testing.T) { + session := model.Session{ + UserId: th.SystemAdminUser.Id, + Roles: model.SystemAdminRoleId, + } + err = th.App.SessionHasPermissionToManageBot(session, bot.UserId) + assert.Nil(t, err) + }) + + t.Run("test non bot ", func(t *testing.T) { + session := model.Session{ + UserId: th.SystemAdminUser.Id, + Roles: model.SystemUserRoleId, + } + err = th.App.SessionHasPermissionToManageBot(session, "12345") + assert.NotNil(t, err) + assert.Equal(t, "store.sql_bot.get.missing.app_error", err.Id) + assert.Error(t, err.Unwrap()) + }) +} + +func TestSessionHasPermissionToUser(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + t.Run("test my user access", func(t *testing.T) { + session := model.Session{ + UserId: th.BasicUser.Id, + Roles: model.SystemUserRoleId, + } + assert.True(t, th.App.SessionHasPermissionToUser(session, th.BasicUser.Id)) + assert.False(t, th.App.SessionHasPermissionToUser(session, th.BasicUser2.Id)) + }) + + t.Run("test user manager access", func(t *testing.T) { + session := model.Session{ + UserId: th.BasicUser.Id, + Roles: model.SystemUserManagerRoleId, + } + assert.False(t, th.App.SessionHasPermissionToUser(session, th.BasicUser2.Id)) + + th.AddPermissionToRole(model.PermissionEditOtherUsers.Id, model.SystemUserManagerRoleId) + assert.True(t, th.App.SessionHasPermissionToUser(session, th.BasicUser2.Id)) + th.RemovePermissionFromRole(model.PermissionEditOtherUsers.Id, model.SystemUserManagerRoleId) + }) + + t.Run("test admin user access", func(t *testing.T) { + session := model.Session{ + UserId: th.SystemAdminUser.Id, + Roles: model.SystemAdminRoleId, + } + assert.True(t, th.App.SessionHasPermissionToUser(session, th.BasicUser.Id)) + assert.True(t, th.App.SessionHasPermissionToUser(session, th.BasicUser2.Id)) + }) +} + +func TestSessionHasPermissionToManageUserOrBot(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + bot, err := th.App.CreateBot(th.Context, &model.Bot{ + Username: "username", + Description: "a bot", + OwnerId: th.BasicUser.Id, + }) + require.Nil(t, err) + defer th.App.PermanentDeleteBot(bot.UserId) + + t.Run("test basic user access", func(t *testing.T) { + session := model.Session{ + UserId: th.BasicUser.Id, + Roles: model.SystemUserRoleId, + } + assert.True(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser.Id)) + assert.False(t, th.App.SessionHasPermissionToUserOrBot(session, bot.UserId)) + assert.False(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser2.Id)) + }) + + t.Run("test user manager access", func(t *testing.T) { + session := model.Session{ + UserId: th.BasicUser2.Id, + Roles: model.SystemUserManagerRoleId, + } + assert.False(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser.Id)) + assert.True(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser2.Id)) + assert.False(t, th.App.SessionHasPermissionToUserOrBot(session, bot.UserId)) + + th.AddPermissionToRole(model.PermissionEditOtherUsers.Id, model.SystemUserManagerRoleId) + assert.True(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser.Id)) + assert.False(t, th.App.SessionHasPermissionToUserOrBot(session, bot.UserId)) + th.RemovePermissionFromRole(model.PermissionEditOtherUsers.Id, model.SystemUserManagerRoleId) + + th.AddPermissionToRole(model.PermissionManageOthersBots.Id, model.SystemUserManagerRoleId) + assert.False(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser.Id)) + assert.True(t, th.App.SessionHasPermissionToUserOrBot(session, bot.UserId)) + th.RemovePermissionFromRole(model.PermissionManageOthersBots.Id, model.SystemUserManagerRoleId) + }) + + t.Run("test system admin access", func(t *testing.T) { + session := model.Session{ + UserId: th.SystemAdminUser.Id, + Roles: model.SystemAdminRoleId, + } + assert.True(t, th.App.SessionHasPermissionToUserOrBot(session, bot.UserId)) + assert.True(t, th.App.SessionHasPermissionToUserOrBot(session, th.BasicUser.Id)) + }) +} + func TestHasPermissionToCategory(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown()