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/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/command.go b/server/plugin/command.go index 9c4d273a0..e9a96d18a 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" @@ -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." diff --git a/server/plugin/configuration.go b/server/plugin/configuration.go index 9886560d1..919fbe5d6 100644 --- a/server/plugin/configuration.go +++ b/server/plugin/configuration.go @@ -207,7 +207,15 @@ func (p *Plugin) OnConfigurationChange() error { configuration.sanitize() - p.sendWebsocketEventIfNeeded(p.getConfiguration(), configuration) + previousConfig := p.getConfiguration() + previousEncryptionKey := previousConfig.EncryptionKey + + p.sendWebsocketEventIfNeeded(previousConfig, configuration) + + if previousEncryptionKey != "" && configuration.EncryptionKey != "" && + previousEncryptionKey != configuration.EncryptionKey { + go p.reEncryptUserData(configuration.EncryptionKey, previousEncryptionKey) + } p.setConfiguration(configuration) 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 fcab70252..9551a6bd9 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" @@ -614,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") } @@ -685,8 +685,24 @@ 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 + 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 + } + p.forceDisconnectUser(userID, githubUsername) return } @@ -695,7 +711,11 @@ 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()) + } + + 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) @@ -707,7 +727,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()) } } } @@ -719,6 +739,158 @@ 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(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()) + return + } + m.Lock() + defer m.Unlock() + + 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, 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 { + break + } + } + + if len(allKeys) == 0 { + 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) + + 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()) + 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 +// 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, 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") + } + 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") + } + + if _, err := decrypt([]byte(newEncryptionKey), 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") + } + + userInfo.Token.AccessToken = plainToken + if err := p.storeGitHubUserInfo(userInfo, newEncryptionKey); 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 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", + "user_id", userID, "error", err.Error()) + } 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()) + } + } + + 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..0791c97cc --- /dev/null +++ b/server/plugin/plugin_test.go @@ -0,0 +1,376 @@ +// 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}) + + 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 +} + +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()).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) + + p.reEncryptUserData(testNewKey, 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("user1" + githubPrivateRepoKey).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(testNewKey, 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("user1" + githubPrivateRepoKey).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(testNewKey, 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(testNewKey, 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, proceeding with keys collected so far", + "page", "0", "keys_collected", "0", "error", "KV list error").Times(1) + + p.reEncryptUserData(testNewKey, 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(testNewKey, testOldKey) + + 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(testNewKey, 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("user1" + githubPrivateRepoKey).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_FallbackFromProps(t *testing.T) { + p, api, mockKvStore, ctrl := setupRotationTest(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) + + 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) + mockKvStore.EXPECT().Delete("user1" + githubPrivateRepoKey).Return(nil) + // No username available anywhere, so no mapping delete + + 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("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() + 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) +} 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 }