From 7f58e2004a0b7e1bfd6cb9717f2b753acb1af2fd Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Tue, 10 Mar 2026 18:20:45 +0200 Subject: [PATCH 1/8] MM-67613 Added handling of encryption key rotation with force disconnect fallback --- server/plugin/configuration.go | 10 +- server/plugin/plugin.go | 108 ++++++++++++ server/plugin/plugin_test.go | 302 +++++++++++++++++++++++++++++++++ 3 files changed, 419 insertions(+), 1 deletion(-) create mode 100644 server/plugin/plugin_test.go diff --git a/server/plugin/configuration.go b/server/plugin/configuration.go index 9886560d1..2de6eb96e 100644 --- a/server/plugin/configuration.go +++ b/server/plugin/configuration.go @@ -207,10 +207,18 @@ func (p *Plugin) OnConfigurationChange() error { configuration.sanitize() - p.sendWebsocketEventIfNeeded(p.getConfiguration(), configuration) + previousConfig := p.getConfiguration() + previousEncryptionKey := previousConfig.EncryptionKey + + p.sendWebsocketEventIfNeeded(previousConfig, configuration) p.setConfiguration(configuration) + if previousEncryptionKey != "" && configuration.EncryptionKey != "" && + previousEncryptionKey != configuration.EncryptionKey { + p.reEncryptUserData(previousEncryptionKey) + } + command, err := p.getCommand(configuration) if err != nil { return errors.Wrap(err, "failed to get command") diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index fcab70252..07baeaf85 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -719,6 +719,114 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { ) } +// reEncryptUserData re-encrypts all connected users' access tokens when +// the encryption key changes. Users whose tokens cannot be migrated are +// force-disconnected and notified to reconnect. +func (p *Plugin) reEncryptUserData(previousEncryptionKey string) { + checker := func(key string) (keep bool, err error) { + return strings.HasSuffix(key, githubTokenKey), nil + } + + var allKeys []string + for page := 0; ; page++ { + keys, err := p.store.ListKeys(page, keysPerPage, pluginapi.WithChecker(checker)) + if err != nil { + p.client.Log.Warn("Encryption key changed but failed to list user keys for re-encryption", + "page", fmt.Sprintf("%d", page), "error", err.Error()) + return + } + allKeys = append(allKeys, keys...) + if len(keys) < keysPerPage { + break + } + } + + if len(allKeys) == 0 { + return + } + + p.client.Log.Info("Encryption key changed, re-encrypting user tokens", + "user_count", fmt.Sprintf("%d", len(allKeys))) + + for _, key := range allKeys { + userID := strings.TrimSuffix(key, githubTokenKey) + + githubUsername, err := p.reEncryptUserToken(key, previousEncryptionKey) + if err != nil { + p.client.Log.Warn("Failed to re-encrypt user token during encryption key rotation", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID, githubUsername) + } + } +} + +// reEncryptUserToken decrypts a single user's token with the old key and +// re-encrypts it with the current key. Returns the GitHub username (best-effort, +// may be empty) and any error encountered. +func (p *Plugin) reEncryptUserToken(kvKey, previousEncryptionKey string) (string, error) { + var userInfo *GitHubUserInfo + if err := p.store.Get(kvKey, &userInfo); err != nil { + return "", errors.Wrap(err, "could not load user info") + } + if userInfo == nil { + return "", errors.New("user info not found") + } + + if userInfo.Token == nil || userInfo.Token.AccessToken == "" { + return userInfo.GitHubUsername, errors.New("user has no token to re-encrypt") + } + + plainToken, err := decrypt([]byte(previousEncryptionKey), userInfo.Token.AccessToken) + if err != nil { + return userInfo.GitHubUsername, errors.Wrap(err, "could not decrypt token with previous key") + } + + userInfo.Token.AccessToken = plainToken + if err := p.storeGitHubUserInfo(userInfo); err != nil { + return userInfo.GitHubUsername, errors.Wrap(err, "could not store re-encrypted token") + } + + return userInfo.GitHubUsername, nil +} + +// forceDisconnectUser performs a best-effort cleanup of a user's encrypted +// data and notifies them to reconnect +func (p *Plugin) forceDisconnectUser(userID, githubUsername string) { + if err := p.store.Delete(userID + githubTokenKey); err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to delete github token", + "user_id", userID, "error", err.Error()) + } + + if githubUsername != "" { + if err := p.store.Delete(githubUsername + githubUsernameKey); err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to delete username mapping", + "user_id", userID, "error", err.Error()) + } + } + + user, err := p.client.User.Get(userID) + if err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to get user props", + "user_id", userID, "error", err.Error()) + } else if _, ok := user.Props["git_user"]; ok { + delete(user.Props, "git_user") + if err := p.client.User.Update(user); err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to update user props", + "user_id", userID, "error", err.Error()) + } + } + + p.client.Frontend.PublishWebSocketEvent( + wsEventDisconnect, + nil, + &model.WebsocketBroadcast{UserId: userID}, + ) + + p.CreateBotDMPost(userID, + "Your GitHub connection has been reset due to a change in the plugin configuration. Please reconnect your account using `/github connect`.", + "custom_git_disconnect") +} + func (p *Plugin) openIssueCreateModal(userID string, channelID string, title string) { p.client.Frontend.PublishWebSocketEvent( wsEventCreateIssue, diff --git a/server/plugin/plugin_test.go b/server/plugin/plugin_test.go new file mode 100644 index 000000000..8da523d41 --- /dev/null +++ b/server/plugin/plugin_test.go @@ -0,0 +1,302 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "encoding/json" + "testing" + + "github.com/golang/mock/gomock" + "github.com/pkg/errors" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" + "github.com/mattermost/mattermost/server/public/pluginapi" + + "github.com/mattermost/mattermost-plugin-github/server/mocks" +) + +const ( + testOldKey = "old_encryption_key_32chX" // 24 bytes = valid AES-192 key + testNewKey = "new_encryption_key_32chX" // 24 bytes = valid AES-192 key +) + +func setupRotationTest(t *testing.T) (*Plugin, *plugintest.API, *mocks.MockKvStore, *gomock.Controller) { + t.Helper() + ctrl := gomock.NewController(t) + mockKvStore := mocks.NewMockKvStore(ctrl) + + api := &plugintest.API{} + p := NewPlugin() + p.store = mockKvStore + p.BotUserID = MockBotID + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + + p.setConfiguration(&Configuration{EncryptionKey: testNewKey}) + + return p, api, mockKvStore, ctrl +} + +func TestReEncryptUserData_HappyPath(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + encryptedToken, err := encrypt([]byte(testOldKey), MockAccessToken) + require.NoError(t, err) + + userInfo := &GitHubUserInfo{ + UserID: "user1", + GitHubUsername: "ghuser1", + Token: &oauth2.Token{AccessToken: encryptedToken}, + Settings: &UserSettings{}, + } + userInfoBytes, err := json.Marshal(userInfo) + require.NoError(t, err) + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return([]string{"user1" + githubTokenKey}, nil) + + mockKvStore.EXPECT().Get("user1"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, out any) error { + return json.Unmarshal(userInfoBytes, out) + }, + ) + + mockKvStore.EXPECT().Set("user1"+githubTokenKey, gomock.Any()).Return(true, nil) + + api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", + "user_count", "1").Times(1) + + p.reEncryptUserData(testOldKey) + + api.AssertExpectations(t) +} + +func TestReEncryptUserData_DecryptFailure(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + userInfo := &GitHubUserInfo{ + UserID: "user1", + GitHubUsername: "ghuser1", + Token: &oauth2.Token{AccessToken: "not-valid-base64-ciphertext!@#$"}, + Settings: &UserSettings{}, + } + userInfoBytes, err := json.Marshal(userInfo) + require.NoError(t, err) + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return([]string{"user1" + githubTokenKey}, nil) + + mockKvStore.EXPECT().Get("user1"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, out any) error { + return json.Unmarshal(userInfoBytes, out) + }, + ) + + // forceDisconnectUser expectations + mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) + + api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("GetUser", "user1").Return(&model.User{ + Id: "user1", + Props: model.StringMap{}, + }, nil) + api.On("GetDirectChannel", "user1", MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil) + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil) + api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), + &model.WebsocketBroadcast{UserId: "user1"}).Times(1) + + p.reEncryptUserData(testOldKey) + + api.AssertExpectations(t) +} + +func TestReEncryptUserData_StoreFailure(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + encryptedToken, err := encrypt([]byte(testOldKey), MockAccessToken) + require.NoError(t, err) + + userInfo := &GitHubUserInfo{ + UserID: "user1", + GitHubUsername: "ghuser1", + Token: &oauth2.Token{AccessToken: encryptedToken}, + Settings: &UserSettings{}, + } + userInfoBytes, err := json.Marshal(userInfo) + require.NoError(t, err) + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return([]string{"user1" + githubTokenKey}, nil) + + mockKvStore.EXPECT().Get("user1"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, out any) error { + return json.Unmarshal(userInfoBytes, out) + }, + ) + + // storeGitHubUserInfo fails + mockKvStore.EXPECT().Set("user1"+githubTokenKey, gomock.Any()).Return(false, errors.New("KV store write error")) + + // forceDisconnectUser expectations + mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) + + api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("GetUser", "user1").Return(&model.User{ + Id: "user1", + Props: model.StringMap{}, + }, nil) + api.On("GetDirectChannel", "user1", MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil) + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil) + api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), + &model.WebsocketBroadcast{UserId: "user1"}).Times(1) + + p.reEncryptUserData(testOldKey) + + api.AssertExpectations(t) +} + +func TestReEncryptUserData_NoConnectedUsers(t *testing.T) { + p, _, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return([]string{}, nil) + + p.reEncryptUserData(testOldKey) +} + +func TestReEncryptUserData_ListKeysError(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return(nil, errors.New("KV list error")) + + api.On("LogWarn", "Encryption key changed but failed to list user keys for re-encryption", + "page", "0", "error", "KV list error").Times(1) + + p.reEncryptUserData(testOldKey) + + api.AssertExpectations(t) +} + +func TestReEncryptUserData_MultipleUsers(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + enc1, err := encrypt([]byte(testOldKey), "token_user1") + require.NoError(t, err) + enc2, err := encrypt([]byte(testOldKey), "token_user2") + require.NoError(t, err) + + user1 := &GitHubUserInfo{ + UserID: "user1", + GitHubUsername: "ghuser1", + Token: &oauth2.Token{AccessToken: enc1}, + Settings: &UserSettings{}, + } + user2 := &GitHubUserInfo{ + UserID: "user2", + GitHubUsername: "ghuser2", + Token: &oauth2.Token{AccessToken: enc2}, + Settings: &UserSettings{}, + } + u1bytes, _ := json.Marshal(user1) + u2bytes, _ := json.Marshal(user2) + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return( + []string{"user1" + githubTokenKey, "user2" + githubTokenKey}, nil) + + mockKvStore.EXPECT().Get("user1"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, out any) error { return json.Unmarshal(u1bytes, out) }, + ) + mockKvStore.EXPECT().Get("user2"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, out any) error { return json.Unmarshal(u2bytes, out) }, + ) + + mockKvStore.EXPECT().Set("user1"+githubTokenKey, gomock.Any()).Return(true, nil) + mockKvStore.EXPECT().Set("user2"+githubTokenKey, gomock.Any()).Return(true, nil) + + api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", + "user_count", "2").Times(1) + + p.reEncryptUserData(testOldKey) + + api.AssertExpectations(t) +} + +func TestForceDisconnectUser_CleansUpAndNotifies(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) + + api.On("GetUser", "user1").Return(&model.User{ + Id: "user1", + Props: model.StringMap{"git_user": "ghuser1"}, + }, nil) + api.On("UpdateUser", mock.MatchedBy(func(u *model.User) bool { + _, hasGitUser := u.Props["git_user"] + return u.Id == "user1" && !hasGitUser + })).Return(&model.User{Id: "user1", Props: model.StringMap{}}, nil) + api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), + &model.WebsocketBroadcast{UserId: "user1"}).Times(1) + api.On("GetDirectChannel", "user1", MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil) + api.On("CreatePost", mock.MatchedBy(func(post *model.Post) bool { + return post.UserId == MockBotID && + post.ChannelId == "dmchannel" && + post.Type == "custom_git_disconnect" + })).Return(&model.Post{}, nil) + + p.forceDisconnectUser("user1", "ghuser1") + + api.AssertExpectations(t) +} + +func TestForceDisconnectUser_NoGitHubUsername(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + // Should NOT call Delete for the username mapping when empty + + api.On("GetUser", "user1").Return(&model.User{ + Id: "user1", + Props: model.StringMap{}, + }, nil) + api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), + &model.WebsocketBroadcast{UserId: "user1"}).Times(1) + api.On("GetDirectChannel", "user1", MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil) + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil) + + p.forceDisconnectUser("user1", "") + + api.AssertExpectations(t) +} + +func TestForceDisconnectUser_DeleteErrors(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(errors.New("delete failed")) + mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(errors.New("delete failed")) + + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("GetUser", "user1").Return(nil, &model.AppError{Message: "user not found"}) + api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), + &model.WebsocketBroadcast{UserId: "user1"}).Times(1) + api.On("GetDirectChannel", "user1", MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil) + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil) + + p.forceDisconnectUser("user1", "ghuser1") + + api.AssertExpectations(t) +} From 6262378a71ba2264245d9c1dfc8339a6bc9dcc86 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Tue, 10 Mar 2026 18:36:40 +0200 Subject: [PATCH 2/8] Added force disconnect fallback on normal disconnect flow - Ensuring that even if an environment somehow bypasses the OnConfigurationChanged hook, a user can still disconnect and re-connect their accounts instead of being locked --- server/plugin/plugin.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 07baeaf85..6b0d1d9a0 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -685,8 +685,21 @@ func (p *Plugin) getGitHubToUsernameMapping(githubUsername string) string { } func (p *Plugin) disconnectGitHubAccount(userID string) { - userInfo, _ := p.getGitHubUserInfo(userID) - if userInfo == nil { + userInfo, apiErr := p.getGitHubUserInfo(userID) + if apiErr != nil { + if apiErr.ID == apiErrorIDNotConnected { + return + } + + p.client.Log.Warn("Failed to load user info for disconnect, falling back to force-disconnect", + "user_id", userID, "error", apiErr.Message) + var rawInfo *GitHubUserInfo + _ = p.store.Get(userID+githubTokenKey, &rawInfo) + githubUsername := "" + if rawInfo != nil { + githubUsername = rawInfo.GitHubUsername + } + p.forceDisconnectUser(userID, githubUsername) return } @@ -695,7 +708,7 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { } if err := p.store.Delete(userInfo.GitHubUsername + githubUsernameKey); err != nil { - p.client.Log.Warn("Failed to delete github token from KV store", "userID", userID, "error", err.Error()) + p.client.Log.Warn("Failed to delete github username mapping from KV store", "userID", userID, "error", err.Error()) } user, err := p.client.User.Get(userID) @@ -707,7 +720,7 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { delete(user.Props, "git_user") err := p.client.User.Update(user) if err != nil { - p.client.Log.Warn("Failed to get update user props", "userID", userID, "error", err.Error()) + p.client.Log.Warn("Failed to update user props", "userID", userID, "error", err.Error()) } } } From 5c972b944961dabd3e9712b18cd0890f21cfbbee Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Tue, 10 Mar 2026 20:32:53 +0200 Subject: [PATCH 3/8] Ensuring any failure to fetch a page of KV entries doesn't abort the entire rotation flow --- server/plugin/plugin.go | 6 +++--- server/plugin/plugin_test.go | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 6b0d1d9a0..e7158b6b3 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -744,9 +744,9 @@ func (p *Plugin) reEncryptUserData(previousEncryptionKey string) { for page := 0; ; page++ { keys, err := p.store.ListKeys(page, keysPerPage, pluginapi.WithChecker(checker)) if err != nil { - p.client.Log.Warn("Encryption key changed but failed to list user keys for re-encryption", - "page", fmt.Sprintf("%d", page), "error", err.Error()) - return + p.client.Log.Warn("Encryption key changed but failed to list user keys for re-encryption, proceeding with keys collected so far", + "page", fmt.Sprintf("%d", page), "keys_collected", fmt.Sprintf("%d", len(allKeys)), "error", err.Error()) + break } allKeys = append(allKeys, keys...) if len(keys) < keysPerPage { diff --git a/server/plugin/plugin_test.go b/server/plugin/plugin_test.go index 8da523d41..612b318df 100644 --- a/server/plugin/plugin_test.go +++ b/server/plugin/plugin_test.go @@ -66,7 +66,16 @@ func TestReEncryptUserData_HappyPath(t *testing.T) { }, ) - mockKvStore.EXPECT().Set("user1"+githubTokenKey, gomock.Any()).Return(true, nil) + mockKvStore.EXPECT().Set("user1"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, value any, opts ...pluginapi.KVSetOption) (bool, error) { + storedInfo, ok := value.(*GitHubUserInfo) + require.True(t, ok, "expected *GitHubUserInfo") + decrypted, decErr := decrypt([]byte(testNewKey), storedInfo.Token.AccessToken) + require.NoError(t, decErr) + require.Equal(t, MockAccessToken, decrypted) + return true, nil + }, + ) api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", "user_count", "1").Times(1) @@ -179,8 +188,8 @@ func TestReEncryptUserData_ListKeysError(t *testing.T) { mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return(nil, errors.New("KV list error")) - api.On("LogWarn", "Encryption key changed but failed to list user keys for re-encryption", - "page", "0", "error", "KV list error").Times(1) + api.On("LogWarn", "Encryption key changed but failed to list user keys for re-encryption, proceeding with keys collected so far", + "page", "0", "keys_collected", "0", "error", "KV list error").Times(1) p.reEncryptUserData(testOldKey) From ec8c14e697cde4b09b7927287604e91de870a015 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Tue, 10 Mar 2026 22:08:50 +0200 Subject: [PATCH 4/8] PR review fixes --- server/plugin/plugin.go | 29 ++++++++++++++++++----------- server/plugin/plugin_test.go | 29 ++++++++++++++++++++++++++--- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index e7158b6b3..e67f69b29 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -810,21 +810,28 @@ func (p *Plugin) forceDisconnectUser(userID, githubUsername string) { "user_id", userID, "error", err.Error()) } - if githubUsername != "" { - if err := p.store.Delete(githubUsername + githubUsernameKey); err != nil { - p.client.Log.Warn("forceDisconnectUser: failed to delete username mapping", - "user_id", userID, "error", err.Error()) - } - } - user, err := p.client.User.Get(userID) if err != nil { p.client.Log.Warn("forceDisconnectUser: failed to get user props", "user_id", userID, "error", err.Error()) - } else if _, ok := user.Props["git_user"]; ok { - delete(user.Props, "git_user") - if err := p.client.User.Update(user); err != nil { - p.client.Log.Warn("forceDisconnectUser: failed to update user props", + } else { + if githubUsername == "" { + if gitUser, ok := user.Props["git_user"]; ok { + githubUsername = gitUser + } + } + if _, ok := user.Props["git_user"]; ok { + delete(user.Props, "git_user") + if err := p.client.User.Update(user); err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to update user props", + "user_id", userID, "error", err.Error()) + } + } + } + + if githubUsername != "" { + if err := p.store.Delete(githubUsername + githubUsernameKey); err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to delete username mapping", "user_id", userID, "error", err.Error()) } } diff --git a/server/plugin/plugin_test.go b/server/plugin/plugin_test.go index 612b318df..4a614ac51 100644 --- a/server/plugin/plugin_test.go +++ b/server/plugin/plugin_test.go @@ -111,7 +111,7 @@ func TestReEncryptUserData_DecryptFailure(t *testing.T) { mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything).Maybe() - api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() api.On("GetUser", "user1").Return(&model.User{ Id: "user1", Props: model.StringMap{}, @@ -270,12 +270,35 @@ func TestForceDisconnectUser_CleansUpAndNotifies(t *testing.T) { api.AssertExpectations(t) } -func TestForceDisconnectUser_NoGitHubUsername(t *testing.T) { +func TestForceDisconnectUser_NoGitHubUsername_FallbackFromProps(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + // Username recovered from user props, so the mapping delete should happen + mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) + + api.On("GetUser", "user1").Return(&model.User{ + Id: "user1", + Props: model.StringMap{"git_user": "ghuser1"}, + }, nil) + api.On("UpdateUser", mock.Anything).Return(&model.User{Id: "user1", Props: model.StringMap{}}, nil) + api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), + &model.WebsocketBroadcast{UserId: "user1"}).Times(1) + api.On("GetDirectChannel", "user1", MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil) + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil) + + p.forceDisconnectUser("user1", "") + + api.AssertExpectations(t) +} + +func TestForceDisconnectUser_NoGitHubUsername_NoPropsFallback(t *testing.T) { p, api, mockKvStore, ctrl := setupRotationTest(t) defer ctrl.Finish() mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) - // Should NOT call Delete for the username mapping when empty + // No username available anywhere, so no mapping delete api.On("GetUser", "user1").Return(&model.User{ Id: "user1", From e0a4cb240b185aa8cd0882414ead21dc14db41aa Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Tue, 10 Mar 2026 23:16:04 +0200 Subject: [PATCH 5/8] Improving behavior in clustered environments to prevent race conditions --- server/plugin/plugin.go | 20 ++++++++++++++++++-- server/plugin/plugin_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index e67f69b29..eeb11a3f4 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -23,6 +23,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin" "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/mattermost/mattermost/server/public/pluginapi/cluster" "github.com/mattermost/mattermost/server/public/pluginapi/experimental/bot/logger" "github.com/mattermost/mattermost/server/public/pluginapi/experimental/bot/poster" @@ -35,8 +36,9 @@ const ( githubUsernameKey = "_githubusername" githubPrivateRepoKey = "_githubprivate" - mm34646MutexKey = "mm34646_token_reset_mutex" - mm34646DoneKey = "mm34646_token_reset_done" + mm34646MutexKey = "mm34646_token_reset_mutex" + mm34646DoneKey = "mm34646_token_reset_done" + reEncryptMutexKey = "reencrypt_user_data_mutex" wsEventConnect = "connect" wsEventDisconnect = "disconnect" @@ -735,7 +737,16 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { // reEncryptUserData re-encrypts all connected users' access tokens when // the encryption key changes. Users whose tokens cannot be migrated are // force-disconnected and notified to reconnect. +// A cluster mutex ensures only one node performs the migration in HA setups. func (p *Plugin) reEncryptUserData(previousEncryptionKey string) { + m, err := cluster.NewMutex(p.API, reEncryptMutexKey) + if err != nil { + p.client.Log.Warn("Failed to create cluster mutex for encryption key rotation", "error", err.Error()) + return + } + m.Lock() + defer m.Unlock() + checker := func(key string) (keep bool, err error) { return strings.HasSuffix(key, githubTokenKey), nil } @@ -789,6 +800,11 @@ func (p *Plugin) reEncryptUserToken(kvKey, previousEncryptionKey string) (string return userInfo.GitHubUsername, errors.New("user has no token to re-encrypt") } + config := p.getConfiguration() + if _, err := decrypt([]byte(config.EncryptionKey), userInfo.Token.AccessToken); err == nil { + return userInfo.GitHubUsername, nil + } + plainToken, err := decrypt([]byte(previousEncryptionKey), userInfo.Token.AccessToken) if err != nil { return userInfo.GitHubUsername, errors.Wrap(err, "could not decrypt token with previous key") diff --git a/server/plugin/plugin_test.go b/server/plugin/plugin_test.go index 4a614ac51..4375dd0df 100644 --- a/server/plugin/plugin_test.go +++ b/server/plugin/plugin_test.go @@ -39,6 +39,9 @@ func setupRotationTest(t *testing.T) (*Plugin, *plugintest.API, *mocks.MockKvSto p.setConfiguration(&Configuration{EncryptionKey: testNewKey}) + api.On("KVSetWithOptions", mock.Anything, mock.Anything, mock.Anything).Return(true, nil).Maybe() + api.On("LogError", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + return p, api, mockKvStore, ctrl } @@ -241,6 +244,38 @@ func TestReEncryptUserData_MultipleUsers(t *testing.T) { api.AssertExpectations(t) } +func TestReEncryptUserData_AlreadyMigratedToken(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(t) + defer ctrl.Finish() + + encryptedWithNewKey, err := encrypt([]byte(testNewKey), MockAccessToken) + require.NoError(t, err) + + userInfo := &GitHubUserInfo{ + UserID: "user1", + GitHubUsername: "ghuser1", + Token: &oauth2.Token{AccessToken: encryptedWithNewKey}, + Settings: &UserSettings{}, + } + userInfoBytes, err := json.Marshal(userInfo) + require.NoError(t, err) + + mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return([]string{"user1" + githubTokenKey}, nil) + + mockKvStore.EXPECT().Get("user1"+githubTokenKey, gomock.Any()).DoAndReturn( + func(key string, out any) error { + return json.Unmarshal(userInfoBytes, out) + }, + ) + + api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", + "user_count", "1").Times(1) + + p.reEncryptUserData(testOldKey) + + api.AssertExpectations(t) +} + func TestForceDisconnectUser_CleansUpAndNotifies(t *testing.T) { p, api, mockKvStore, ctrl := setupRotationTest(t) defer ctrl.Finish() From 3fc10d67d4c74b06299c51cf9f8d173034b4aad4 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Wed, 11 Mar 2026 15:07:30 +0200 Subject: [PATCH 6/8] Addressing multiple issues with rotation - Making sure rotation is non-blocking - Passing correct references to remove risk of race conditions - Applying stricter unpad logic --- server/plugin/api.go | 6 ++--- server/plugin/command.go | 2 +- server/plugin/configuration.go | 6 ++--- server/plugin/mm_34646_token_refresh.go | 2 +- server/plugin/plugin.go | 33 ++++++++++++++++--------- server/plugin/plugin_test.go | 20 +++++++++------ server/plugin/utils.go | 13 ++++++++-- 7 files changed, 53 insertions(+), 29 deletions(-) diff --git a/server/plugin/api.go b/server/plugin/api.go index 05e32da5f..7250372da 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -446,7 +446,7 @@ func (p *Plugin) completeConnectUserToGitHub(c *Context, w http.ResponseWriter, MM34646ResetTokenDone: true, } - if err = p.storeGitHubUserInfo(userInfo); err != nil { + if err = p.storeGitHubUserInfo(userInfo, p.getConfiguration().EncryptionKey); err != nil { c.Log.WithError(err).Errorf("Failed to store GitHub user info") p.writeAPIError(w, &APIErrorResponse{Message: "unable to connect user to GitHub", StatusCode: http.StatusInternalServerError}) return @@ -642,7 +642,7 @@ func (p *Plugin) getConnected(c *Context, w http.ResponseWriter, r *http.Request c.Log.WithError(err).Warnf("Failed to create GitHub todo message") } info.LastToDoPostAt = now - if err := p.storeGitHubUserInfo(info); err != nil { + if err := p.storeGitHubUserInfo(info, p.getConfiguration().EncryptionKey); err != nil { c.Log.WithError(err).Warnf("Failed to store github info for new user") } } @@ -1098,7 +1098,7 @@ func (p *Plugin) updateSettings(c *UserContext, w http.ResponseWriter, r *http.R info := c.GHInfo info.Settings = settings - if err := p.storeGitHubUserInfo(info); err != nil { + if err := p.storeGitHubUserInfo(info, p.getConfiguration().EncryptionKey); err != nil { c.Log.WithError(err).Errorf("Failed to store GitHub user info") p.writeAPIError(w, &APIErrorResponse{Message: "error occurred while updating settings", StatusCode: http.StatusInternalServerError}) return diff --git a/server/plugin/command.go b/server/plugin/command.go index 9c4d273a0..771bebb9b 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -787,7 +787,7 @@ func (p *Plugin) handleSettings(_ *plugin.Context, _ *model.CommandArgs, paramet } } - err := p.storeGitHubUserInfo(userInfo) + err := p.storeGitHubUserInfo(userInfo, p.getConfiguration().EncryptionKey) if err != nil { p.client.Log.Warn("Failed to store github user info", "error", err.Error()) return "Failed to store settings" diff --git a/server/plugin/configuration.go b/server/plugin/configuration.go index 2de6eb96e..919fbe5d6 100644 --- a/server/plugin/configuration.go +++ b/server/plugin/configuration.go @@ -212,13 +212,13 @@ func (p *Plugin) OnConfigurationChange() error { p.sendWebsocketEventIfNeeded(previousConfig, configuration) - p.setConfiguration(configuration) - if previousEncryptionKey != "" && configuration.EncryptionKey != "" && previousEncryptionKey != configuration.EncryptionKey { - p.reEncryptUserData(previousEncryptionKey) + go p.reEncryptUserData(configuration.EncryptionKey, previousEncryptionKey) } + p.setConfiguration(configuration) + command, err := p.getCommand(configuration) if err != nil { return errors.Wrap(err, "failed to get command") diff --git a/server/plugin/mm_34646_token_refresh.go b/server/plugin/mm_34646_token_refresh.go index e1bf63341..5a963e3fe 100644 --- a/server/plugin/mm_34646_token_refresh.go +++ b/server/plugin/mm_34646_token_refresh.go @@ -115,7 +115,7 @@ func (p *Plugin) forceResetUserTokenMM34646(ctx context.Context, config *Configu info.Token.AccessToken = *a.Token info.MM34646ResetTokenDone = true - err = p.storeGitHubUserInfo(info) + err = p.storeGitHubUserInfo(info, p.getConfiguration().EncryptionKey) if err != nil { return "", errors.Wrap(err, "failed to store updated GitHubUserInfo") } diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index eeb11a3f4..28aea36fd 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -616,10 +616,8 @@ type UserSettings struct { Notifications bool `json:"notifications"` } -func (p *Plugin) storeGitHubUserInfo(info *GitHubUserInfo) error { - config := p.getConfiguration() - - encryptedToken, err := encrypt([]byte(config.EncryptionKey), info.Token.AccessToken) +func (p *Plugin) storeGitHubUserInfo(info *GitHubUserInfo, encryptionKey string) error { + encryptedToken, err := encrypt([]byte(encryptionKey), info.Token.AccessToken) if err != nil { return errors.Wrap(err, "error occurred while encrypting access token") } @@ -696,7 +694,10 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { p.client.Log.Warn("Failed to load user info for disconnect, falling back to force-disconnect", "user_id", userID, "error", apiErr.Message) var rawInfo *GitHubUserInfo - _ = p.store.Get(userID+githubTokenKey, &rawInfo) + if err := p.store.Get(userID+githubTokenKey, &rawInfo); err != nil { + p.client.Log.Warn("Failed to load raw user info during fallback disconnect", + "user_id", userID, "error", err.Error()) + } githubUsername := "" if rawInfo != nil { githubUsername = rawInfo.GitHubUsername @@ -713,6 +714,10 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { p.client.Log.Warn("Failed to delete github username mapping from KV store", "userID", userID, "error", err.Error()) } + if err := p.store.Delete(userID + githubPrivateRepoKey); err != nil { + p.client.Log.Warn("Failed to delete github private repo key from KV store", "userID", userID, "error", err.Error()) + } + user, err := p.client.User.Get(userID) if err != nil { p.client.Log.Warn("Failed to get user props", "userID", userID, "error", err.Error()) @@ -738,7 +743,7 @@ func (p *Plugin) disconnectGitHubAccount(userID string) { // the encryption key changes. Users whose tokens cannot be migrated are // force-disconnected and notified to reconnect. // A cluster mutex ensures only one node performs the migration in HA setups. -func (p *Plugin) reEncryptUserData(previousEncryptionKey string) { +func (p *Plugin) reEncryptUserData(newEncryptionKey, previousEncryptionKey string) { m, err := cluster.NewMutex(p.API, reEncryptMutexKey) if err != nil { p.client.Log.Warn("Failed to create cluster mutex for encryption key rotation", "error", err.Error()) @@ -775,7 +780,7 @@ func (p *Plugin) reEncryptUserData(previousEncryptionKey string) { for _, key := range allKeys { userID := strings.TrimSuffix(key, githubTokenKey) - githubUsername, err := p.reEncryptUserToken(key, previousEncryptionKey) + githubUsername, err := p.reEncryptUserToken(key, newEncryptionKey, previousEncryptionKey) if err != nil { p.client.Log.Warn("Failed to re-encrypt user token during encryption key rotation", "user_id", userID, "error", err.Error()) @@ -785,9 +790,9 @@ func (p *Plugin) reEncryptUserData(previousEncryptionKey string) { } // reEncryptUserToken decrypts a single user's token with the old key and -// re-encrypts it with the current key. Returns the GitHub username (best-effort, +// re-encrypts it with the new key. Returns the GitHub username (best-effort, // may be empty) and any error encountered. -func (p *Plugin) reEncryptUserToken(kvKey, previousEncryptionKey string) (string, error) { +func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionKey string) (string, error) { var userInfo *GitHubUserInfo if err := p.store.Get(kvKey, &userInfo); err != nil { return "", errors.Wrap(err, "could not load user info") @@ -800,8 +805,7 @@ func (p *Plugin) reEncryptUserToken(kvKey, previousEncryptionKey string) (string return userInfo.GitHubUsername, errors.New("user has no token to re-encrypt") } - config := p.getConfiguration() - if _, err := decrypt([]byte(config.EncryptionKey), userInfo.Token.AccessToken); err == nil { + if _, err := decrypt([]byte(newEncryptionKey), userInfo.Token.AccessToken); err == nil { return userInfo.GitHubUsername, nil } @@ -811,7 +815,7 @@ func (p *Plugin) reEncryptUserToken(kvKey, previousEncryptionKey string) (string } userInfo.Token.AccessToken = plainToken - if err := p.storeGitHubUserInfo(userInfo); err != nil { + if err := p.storeGitHubUserInfo(userInfo, newEncryptionKey); err != nil { return userInfo.GitHubUsername, errors.Wrap(err, "could not store re-encrypted token") } @@ -826,6 +830,11 @@ func (p *Plugin) forceDisconnectUser(userID, githubUsername string) { "user_id", userID, "error", err.Error()) } + if err := p.store.Delete(userID + githubPrivateRepoKey); err != nil { + p.client.Log.Warn("forceDisconnectUser: failed to delete github private repo key", + "user_id", userID, "error", err.Error()) + } + user, err := p.client.User.Get(userID) if err != nil { p.client.Log.Warn("forceDisconnectUser: failed to get user props", diff --git a/server/plugin/plugin_test.go b/server/plugin/plugin_test.go index 4375dd0df..003a17617 100644 --- a/server/plugin/plugin_test.go +++ b/server/plugin/plugin_test.go @@ -83,7 +83,7 @@ func TestReEncryptUserData_HappyPath(t *testing.T) { api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", "user_count", "1").Times(1) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) api.AssertExpectations(t) } @@ -111,6 +111,7 @@ func TestReEncryptUserData_DecryptFailure(t *testing.T) { // forceDisconnectUser expectations mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(nil) mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything).Maybe() @@ -124,7 +125,7 @@ func TestReEncryptUserData_DecryptFailure(t *testing.T) { api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), &model.WebsocketBroadcast{UserId: "user1"}).Times(1) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) api.AssertExpectations(t) } @@ -158,6 +159,7 @@ func TestReEncryptUserData_StoreFailure(t *testing.T) { // forceDisconnectUser expectations mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(nil) mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything).Maybe() @@ -171,7 +173,7 @@ func TestReEncryptUserData_StoreFailure(t *testing.T) { api.On("PublishWebSocketEvent", wsEventDisconnect, map[string]any(nil), &model.WebsocketBroadcast{UserId: "user1"}).Times(1) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) api.AssertExpectations(t) } @@ -182,7 +184,7 @@ func TestReEncryptUserData_NoConnectedUsers(t *testing.T) { mockKvStore.EXPECT().ListKeys(0, keysPerPage, gomock.Any()).Return([]string{}, nil) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) } func TestReEncryptUserData_ListKeysError(t *testing.T) { @@ -194,7 +196,7 @@ func TestReEncryptUserData_ListKeysError(t *testing.T) { api.On("LogWarn", "Encryption key changed but failed to list user keys for re-encryption, proceeding with keys collected so far", "page", "0", "keys_collected", "0", "error", "KV list error").Times(1) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) api.AssertExpectations(t) } @@ -239,7 +241,7 @@ func TestReEncryptUserData_MultipleUsers(t *testing.T) { api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", "user_count", "2").Times(1) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) api.AssertExpectations(t) } @@ -271,7 +273,7 @@ func TestReEncryptUserData_AlreadyMigratedToken(t *testing.T) { api.On("LogInfo", "Encryption key changed, re-encrypting user tokens", "user_count", "1").Times(1) - p.reEncryptUserData(testOldKey) + p.reEncryptUserData(testNewKey, testOldKey) api.AssertExpectations(t) } @@ -281,6 +283,7 @@ func TestForceDisconnectUser_CleansUpAndNotifies(t *testing.T) { defer ctrl.Finish() mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(nil) mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) api.On("GetUser", "user1").Return(&model.User{ @@ -310,6 +313,7 @@ func TestForceDisconnectUser_NoGitHubUsername_FallbackFromProps(t *testing.T) { defer ctrl.Finish() mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(nil) // Username recovered from user props, so the mapping delete should happen mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(nil) @@ -333,6 +337,7 @@ func TestForceDisconnectUser_NoGitHubUsername_NoPropsFallback(t *testing.T) { defer ctrl.Finish() mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(nil) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(nil) // No username available anywhere, so no mapping delete api.On("GetUser", "user1").Return(&model.User{ @@ -354,6 +359,7 @@ func TestForceDisconnectUser_DeleteErrors(t *testing.T) { defer ctrl.Finish() mockKvStore.EXPECT().Delete("user1" + githubTokenKey).Return(errors.New("delete failed")) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(errors.New("delete failed")) mockKvStore.EXPECT().Delete("ghuser1" + githubUsernameKey).Return(errors.New("delete failed")) api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() diff --git a/server/plugin/utils.go b/server/plugin/utils.go index 1d5f0e771..828cd82a3 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -74,12 +74,21 @@ func pad(src []byte) []byte { func unpad(src []byte) ([]byte, error) { length := len(src) - unpadding := int(src[length-1]) + if length == 0 { + return nil, errors.New("unpad error: empty input") + } - if unpadding > length { + unpadding := int(src[length-1]) + if unpadding < 1 || unpadding > aes.BlockSize || unpadding > length { return nil, errors.New("unpad error. This could happen when incorrect encryption key is used") } + for i := length - unpadding; i < length; i++ { + if src[i] != byte(unpadding) { + return nil, errors.New("unpad error. This could happen when incorrect encryption key is used") + } + } + return src[:(length - unpadding)], nil } From ccf92684c3d904b62ba83c55df4d496d2a79ca81 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Wed, 11 Mar 2026 15:36:23 +0200 Subject: [PATCH 7/8] Added audit logging to migration process --- server/plugin/audit.go | 28 ++++++++++++++++++++++++++++ server/plugin/plugin.go | 19 +++++++++++++++++++ server/plugin/plugin_test.go | 1 + 3 files changed, 48 insertions(+) create mode 100644 server/plugin/audit.go diff --git a/server/plugin/audit.go b/server/plugin/audit.go new file mode 100644 index 000000000..612b3a438 --- /dev/null +++ b/server/plugin/audit.go @@ -0,0 +1,28 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +// ReEncryptUserDataAuditParams holds request audit data for the reEncryptUserData transaction. +type ReEncryptUserDataAuditParams struct { + TotalUsers int `json:"total_users"` +} + +func (p ReEncryptUserDataAuditParams) Auditable() map[string]any { + return map[string]any{ + "total_users": p.TotalUsers, + } +} + +// ReEncryptUserDataAuditResult holds the outcome of the reEncryptUserData transaction. +type ReEncryptUserDataAuditResult struct { + Migrated int `json:"migrated"` + ForceDisconnected int `json:"force_disconnected"` +} + +func (p ReEncryptUserDataAuditResult) Auditable() map[string]any { + return map[string]any{ + "migrated": p.Migrated, + "force_disconnected": p.ForceDisconnected, + } +} diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 28aea36fd..9551a6bd9 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -774,9 +774,16 @@ func (p *Plugin) reEncryptUserData(newEncryptionKey, previousEncryptionKey strin return } + auditRec := plugin.MakeAuditRecord("reEncryptUserData", model.AuditStatusFail) + defer p.API.LogAuditRec(auditRec) + model.AddEventParameterAuditableToAuditRec(auditRec, "re_encrypt_user_data", ReEncryptUserDataAuditParams{ + TotalUsers: len(allKeys), + }) + p.client.Log.Info("Encryption key changed, re-encrypting user tokens", "user_count", fmt.Sprintf("%d", len(allKeys))) + var migrated, forceDisconnected int for _, key := range allKeys { userID := strings.TrimSuffix(key, githubTokenKey) @@ -784,9 +791,21 @@ func (p *Plugin) reEncryptUserData(newEncryptionKey, previousEncryptionKey strin if err != nil { p.client.Log.Warn("Failed to re-encrypt user token during encryption key rotation", "user_id", userID, "error", err.Error()) + auditRec.AddErrorDesc(fmt.Sprintf("user %s: %s", userID, err.Error())) p.forceDisconnectUser(userID, githubUsername) + forceDisconnected++ + } else { + migrated++ } } + + if forceDisconnected == 0 { + auditRec.Success() + } + auditRec.AddEventResultState(ReEncryptUserDataAuditResult{ + Migrated: migrated, + ForceDisconnected: forceDisconnected, + }) } // reEncryptUserToken decrypts a single user's token with the old key and diff --git a/server/plugin/plugin_test.go b/server/plugin/plugin_test.go index 003a17617..0791c97cc 100644 --- a/server/plugin/plugin_test.go +++ b/server/plugin/plugin_test.go @@ -41,6 +41,7 @@ func setupRotationTest(t *testing.T) (*Plugin, *plugintest.API, *mocks.MockKvSto api.On("KVSetWithOptions", mock.Anything, mock.Anything, mock.Anything).Return(true, nil).Maybe() api.On("LogError", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogAuditRec", mock.Anything).Maybe() return p, api, mockKvStore, ctrl } From 97cc27066541609d39b6dcb47db8abb342a1197e Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Wed, 25 Mar 2026 22:40:38 +0200 Subject: [PATCH 8/8] Fixing issue with force disconnect not working --- server/plugin/command.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/plugin/command.go b/server/plugin/command.go index 771bebb9b..e9a96d18a 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -1072,6 +1072,12 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*mo return &model.CommandResponse{}, nil } + if action == "disconnect" { + p.disconnectGitHubAccount(args.UserId) + p.postCommandResponse(args, "Disconnected your GitHub account.") + return &model.CommandResponse{}, nil + } + info, apiErr := p.getGitHubUserInfo(args.UserId) if apiErr != nil { text := "Unknown error."