mirror of
https://github.com/mattermost/mattermost.git
synced 2026-08-30 17:06:34 +08:00
[MM-69895] Delete bot access tokens when permanently deleting a bot (#37907)
* [MM-69895] Delete bot access tokens on permanent bot deletion App.PermanentDeleteBot removed the bot and user rows but left the bot's UserAccessToken rows (and their sessions) orphaned, since the UserAccessTokens table has no FK cascade to Users. Call UserAccessToken().DeleteAllForUser to match PermanentDeleteUser. Co-authored-by: mattermost-code <matty-code@mattermost.com> * [MM-69895] Strengthen bot access token deletion regression test Assert specific not-found errors, cover sessions for every bot token, and add a control bot to prove deletion is scoped to the deleted bot. Co-authored-by: mattermost-code <matty-code@mattermost.com> * [MM-69895] Assert not-found status on deleted bot tokens Co-authored-by: mattermost-code <matty-code@mattermost.com> * [MM-69895] Clear session cache when permanently deleting a bot Deleting the access token rows via DeleteAllForUser is plain SQL and never clears the in-memory session cache, so the bot's tokens kept authenticating after PermanentDeleteBot. Mirror PermanentDeleteUser: delete sessions, delete tokens, then clear the session cache (which also broadcasts to the cluster). --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: mattermost-code <matty-code@mattermost.com>
This commit is contained in:
@@ -467,6 +467,18 @@ func (a *App) PermanentDeleteBot(rctx request.CTX, botUserId string) *model.AppE
|
||||
}
|
||||
}
|
||||
|
||||
if err := a.Srv().Store().Session().PermanentDeleteSessionsByUser(botUserId); err != nil {
|
||||
return model.NewAppError("PermanentDeleteBot", "app.session.permanent_delete_sessions_by_user.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
|
||||
}
|
||||
|
||||
if err := a.Srv().Store().UserAccessToken().DeleteAllForUser(botUserId); err != nil {
|
||||
return model.NewAppError("PermanentDeleteBot", "app.user_access_token.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
|
||||
}
|
||||
|
||||
// Sessions are cached in memory by token, so clearing the cache is required
|
||||
// to stop the deleted bot's access tokens from continuing to authenticate.
|
||||
a.ClearSessionCacheForUser(botUserId)
|
||||
|
||||
if err := a.Srv().Store().User().PermanentDelete(rctx, botUserId); err != nil {
|
||||
return model.NewAppError("PermanentDeleteBot", "app.user.permanent_delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
|
||||
}
|
||||
|
||||
@@ -13,6 +13,7 @@ import (
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/mattermost/mattermost/server/public/model"
|
||||
"github.com/mattermost/mattermost/server/v8/channels/store"
|
||||
)
|
||||
|
||||
func TestCreateBot(t *testing.T) {
|
||||
@@ -682,6 +683,104 @@ func TestPermanentDeleteBot(t *testing.T) {
|
||||
require.Equal(t, "store.sql_bot.get.missing.app_error", err.Id)
|
||||
}
|
||||
|
||||
func TestPermanentDeleteBotDeletesAccessTokens(t *testing.T) {
|
||||
mainHelper.Parallel(t)
|
||||
th := Setup(t).InitBasic(t)
|
||||
|
||||
bot, err := th.App.CreateBot(th.Context, &model.Bot{
|
||||
Username: "token_bot",
|
||||
Description: "a bot with tokens",
|
||||
OwnerId: th.BasicUser.Id,
|
||||
})
|
||||
require.Nil(t, err)
|
||||
|
||||
token1, err := th.App.CreateUserAccessToken(th.Context, &model.UserAccessToken{
|
||||
UserId: bot.UserId,
|
||||
Description: "token 1",
|
||||
})
|
||||
require.Nil(t, err)
|
||||
|
||||
token2, err := th.App.CreateUserAccessToken(th.Context, &model.UserAccessToken{
|
||||
UserId: bot.UserId,
|
||||
Description: "token 2",
|
||||
})
|
||||
require.Nil(t, err)
|
||||
|
||||
// Each token gets a backing session so we can verify the sessions are
|
||||
// deleted alongside the tokens.
|
||||
session1, err := th.App.GetSession(token1.Token)
|
||||
require.Nil(t, err)
|
||||
require.NotEmpty(t, session1.Id)
|
||||
|
||||
session2, err := th.App.GetSession(token2.Token)
|
||||
require.Nil(t, err)
|
||||
require.NotEmpty(t, session2.Id)
|
||||
|
||||
// A second bot whose tokens/sessions must survive, proving the deletion is
|
||||
// scoped to the deleted bot's user ID.
|
||||
otherBot, err := th.App.CreateBot(th.Context, &model.Bot{
|
||||
Username: "other_token_bot",
|
||||
Description: "an unrelated bot",
|
||||
OwnerId: th.BasicUser.Id,
|
||||
})
|
||||
require.Nil(t, err)
|
||||
|
||||
otherToken, err := th.App.CreateUserAccessToken(th.Context, &model.UserAccessToken{
|
||||
UserId: otherBot.UserId,
|
||||
Description: "keep me",
|
||||
})
|
||||
require.Nil(t, err)
|
||||
|
||||
otherSession, err := th.App.GetSession(otherToken.Token)
|
||||
require.Nil(t, err)
|
||||
require.NotEmpty(t, otherSession.Id)
|
||||
|
||||
tokens, err := th.App.GetUserAccessTokensForUser(bot.UserId, 0, 100)
|
||||
require.Nil(t, err)
|
||||
require.Len(t, tokens, 2)
|
||||
|
||||
require.Nil(t, th.App.PermanentDeleteBot(th.Context, bot.UserId))
|
||||
|
||||
tokens, err = th.App.GetUserAccessTokensForUser(bot.UserId, 0, 100)
|
||||
require.Nil(t, err)
|
||||
require.Empty(t, tokens, "bot access tokens should be deleted with the bot")
|
||||
|
||||
_, err = th.App.GetUserAccessToken(token1.Id, false)
|
||||
require.NotNil(t, err, "token 1 should be deleted with the bot")
|
||||
require.Equal(t, http.StatusNotFound, err.StatusCode)
|
||||
_, err = th.App.GetUserAccessToken(token2.Id, false)
|
||||
require.NotNil(t, err, "token 2 should be deleted with the bot")
|
||||
require.Equal(t, http.StatusNotFound, err.StatusCode)
|
||||
|
||||
var nfErr *store.ErrNotFound
|
||||
_, nErr := th.App.Srv().Store().Session().Get(th.Context, session1.Id)
|
||||
require.ErrorAs(t, nErr, &nfErr, "session backed by the bot's first token should be deleted")
|
||||
_, nErr = th.App.Srv().Store().Session().Get(th.Context, session2.Id)
|
||||
require.ErrorAs(t, nErr, &nfErr, "session backed by the bot's second token should be deleted")
|
||||
|
||||
// Sessions are cached in memory by token, so the tokens must no longer
|
||||
// authenticate once the bot is deleted. This exercises the session cache
|
||||
// clearing, which plain SQL token deletion alone does not cover.
|
||||
_, err = th.App.GetSession(token1.Token)
|
||||
require.NotNil(t, err, "the deleted bot's first token must no longer authenticate")
|
||||
require.Equal(t, http.StatusUnauthorized, err.StatusCode)
|
||||
_, err = th.App.GetSession(token2.Token)
|
||||
require.NotNil(t, err, "the deleted bot's second token must no longer authenticate")
|
||||
require.Equal(t, http.StatusUnauthorized, err.StatusCode)
|
||||
|
||||
// The unrelated bot's credentials must be untouched.
|
||||
otherTokens, err := th.App.GetUserAccessTokensForUser(otherBot.UserId, 0, 100)
|
||||
require.Nil(t, err)
|
||||
require.Len(t, otherTokens, 1, "an unrelated bot's tokens must not be deleted")
|
||||
|
||||
_, nErr = th.App.Srv().Store().Session().Get(th.Context, otherSession.Id)
|
||||
require.NoError(t, nErr, "an unrelated bot's session must survive")
|
||||
|
||||
otherSessionAfter, err := th.App.GetSession(otherToken.Token)
|
||||
require.Nil(t, err, "an unrelated bot's token must still authenticate")
|
||||
require.Equal(t, otherSession.Id, otherSessionAfter.Id)
|
||||
}
|
||||
|
||||
func TestDisableUserBots(t *testing.T) {
|
||||
mainHelper.Parallel(t)
|
||||
th := Setup(t)
|
||||
|
||||
Reference in New Issue
Block a user