From 7fab3d923e820325dafce13640037666e8670813 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Wed, 1 Apr 2026 16:06:57 +0300 Subject: [PATCH 1/7] Added encryption key rotation and fallback to gitlab plugin --- server/audit.go | 22 ++ server/configuration.go | 8 + server/plugin.go | 162 ++++++++++++- server/reencrypt_test.go | 495 +++++++++++++++++++++++++++++++++++++++ server/utils.go | 13 +- 5 files changed, 695 insertions(+), 5 deletions(-) create mode 100644 server/reencrypt_test.go diff --git a/server/audit.go b/server/audit.go index 89c858ef..c7dda461 100644 --- a/server/audit.go +++ b/server/audit.go @@ -63,3 +63,25 @@ type AttachCommentToIssueAuditResult struct { func (p AttachCommentToIssueAuditResult) Auditable() map[string]any { return map[string]any{"note_id": p.NoteID} } + +// ReEncryptUserDataAuditParams holds request audit data for the reEncryptUserData operation. +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 operation. +type ReEncryptUserDataAuditResult struct { + MigratedCount int `json:"migrated_count"` + ForceDisconnectCount int `json:"force_disconnect_count"` +} + +func (p ReEncryptUserDataAuditResult) Auditable() map[string]any { + return map[string]any{ + "migrated_count": p.MigratedCount, + "force_disconnect_count": p.ForceDisconnectCount, + } +} diff --git a/server/configuration.go b/server/configuration.go index 0319af38..05eba897 100644 --- a/server/configuration.go +++ b/server/configuration.go @@ -200,14 +200,22 @@ func (p *Plugin) OnConfigurationChange() error { p.configurationLock.RLock() hadConfig := p.configuration != nil var previousGitlabGroup string + var previousEncryptionKey string if hadConfig { previousGitlabGroup = strings.TrimSpace(p.configuration.GitlabGroup) + previousEncryptionKey = p.configuration.EncryptionKey } p.configurationLock.RUnlock() newGitlabGroup := strings.TrimSpace(configuration.GitlabGroup) p.setConfiguration(configuration, serverConfiguration) + if previousEncryptionKey != "" && configuration.EncryptionKey != "" && + previousEncryptionKey != configuration.EncryptionKey { + newKey := configuration.EncryptionKey + go p.reEncryptUserData(newKey, previousEncryptionKey) + } + if hadConfig && p.BotUserID != "" && newGitlabGroup != "" && newGitlabGroup != previousGitlabGroup { p.notifyUsersOfDisallowedSubscriptions() } diff --git a/server/plugin.go b/server/plugin.go index ca587fa9..38bd422e 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -527,9 +527,13 @@ func (p *Plugin) getGitlabIDToUsernameMapping(gitlabUserID string) string { } func (p *Plugin) disconnectGitlabAccount(userID string) { - userInfo, err := p.getGitlabUserInfoByMattermostID(userID) - if err != nil { - p.client.Log.Warn("can't get GitLab user info from mattermost id", "err", err.Message) + userInfo, apiErr := p.getGitlabUserInfoByMattermostID(userID) + if apiErr != nil { + p.client.Log.Warn("can't get GitLab user info from mattermost id, attempting best-effort cleanup", + "err", apiErr.Message, "user_id", userID) + // Fall back to best-effort cleanup using only the userID, in case the user is + // in a partial state (e.g., _userinfo is missing but a token key still exists). + p.forceDisconnectUser(userID) return } if userInfo == nil { @@ -991,6 +995,158 @@ func (p *Plugin) handleRevokedToken(info *gitlab.UserInfo) { } } +// reEncryptUserData re-encrypts all stored user tokens from previousEncryptionKey to newEncryptionKey. +// It is safe to call concurrently; a cluster mutex prevents parallel executions. +func (p *Plugin) reEncryptUserData(newEncryptionKey, previousEncryptionKey string) { + auditRec := plugin.MakeAuditRecord("reEncryptUserData", model.AuditStatusFail) + defer p.API.LogAuditRec(auditRec) + + mutex, err := cluster.NewMutex(p.API, "gitlab-reencrypt-lock") + if err != nil { + p.client.Log.Warn("Failed to acquire re-encryption mutex", "error", err.Error()) + auditRec.AddErrorDesc(err.Error()) + return + } + mutex.Lock() + defer mutex.Unlock() + + p.client.Log.Info("Encryption key changed, re-encrypting user tokens") + + const keysPerPage = 1000 + var allKeys []string + for page := 0; ; page++ { + keys, listErr := p.client.KV.ListKeys(page, keysPerPage, pluginapi.WithChecker(func(key string) (bool, error) { + return strings.HasSuffix(key, GitlabUserTokenKey), nil + })) + if listErr != nil { + p.client.Log.Warn("Failed to list KV keys during re-encryption, continuing with already-collected keys", + "page", page, "error", listErr.Error()) + break + } + allKeys = append(allKeys, keys...) + if len(keys) < keysPerPage { + break + } + } + + params := ReEncryptUserDataAuditParams{TotalUsers: len(allKeys)} + model.AddEventParameterAuditableToAuditRec(auditRec, "re_encrypt_user_data", params) + + result := ReEncryptUserDataAuditResult{} + + for _, key := range allKeys { + migrated, reErr := p.reEncryptUserToken(key, newEncryptionKey, previousEncryptionKey) + if reErr != nil { + result.ForceDisconnectCount++ + continue + } + if migrated { + result.MigratedCount++ + } + } + + auditRec.Success() + auditRec.AddEventResultState(result) +} + +// reEncryptUserToken re-encrypts a single user token KV entry from previousEncryptionKey to newEncryptionKey. +// Returns true if the token was re-encrypted, false if it was already migrated (idempotent). +// On failure, calls forceDisconnectUser and returns an error. +func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionKey string) (bool, error) { + userID := strings.TrimSuffix(kvKey, GitlabUserTokenKey) + + var tokenBytes []byte + if err := p.client.KV.Get(kvKey, &tokenBytes); err != nil || tokenBytes == nil { + return false, nil + } + + tokenStr := string(tokenBytes) + + // Idempotency check: if the token can already be decrypted with the new key, it was already migrated. + if _, err := decrypt([]byte(newEncryptionKey), tokenStr); err == nil { + return false, nil + } + + plainJSON, err := decrypt([]byte(previousEncryptionKey), tokenStr) + if err != nil { + p.client.Log.Warn("Failed to decrypt token with previous key during re-encryption, force-disconnecting user", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID) + return false, err + } + + reEncrypted, err := encrypt([]byte(newEncryptionKey), plainJSON) + if err != nil { + p.client.Log.Warn("Failed to re-encrypt token with new key, force-disconnecting user", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID) + return false, err + } + + if _, err := p.client.KV.Set(kvKey, []byte(reEncrypted)); err != nil { + p.client.Log.Warn("Failed to store re-encrypted token, force-disconnecting user", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID) + return false, err + } + + return true, nil +} + +// forceDisconnectUser performs a best-effort cleanup of a user's encrypted data and notifies them to reconnect. +func (p *Plugin) forceDisconnectUser(userID string) { + // Fetch user info for GitLab-specific mapping cleanup. User info is not encrypted, + // so this should succeed even after a key rotation. + userInfo, apiErr := p.getGitlabUserInfoByMattermostID(userID) + if apiErr != nil { + p.client.Log.Warn("Failed to load user info for force-disconnect", + "user_id", userID, "error", apiErr.Message) + } + + if err := p.client.KV.Delete(userID + GitlabUserTokenKey); err != nil { + p.client.Log.Warn("Failed to delete user token during force-disconnect", "user_id", userID, "error", err.Error()) + } + if err := p.client.KV.Delete(userID + GitlabUserInfoKey); err != nil { + p.client.Log.Warn("Failed to delete user info during force-disconnect", "user_id", userID, "error", err.Error()) + } + + if userInfo != nil { + if userInfo.GitlabUsername != "" { + if err := p.deleteGitlabToUserIDMapping(userInfo.GitlabUsername); err != nil { + p.client.Log.Warn("Failed to delete GitLab username mapping during force-disconnect", + "user_id", userID, "gitlab_username", userInfo.GitlabUsername, "error", err.Error()) + } + } + if userInfo.GitlabUserID != 0 { + if err := p.deleteGitlabIDToUserIDMapping(userInfo.GitlabUserID); err != nil { + p.client.Log.Warn("Failed to delete GitLab ID mapping during force-disconnect", + "user_id", userID, "gitlab_user_id", userInfo.GitlabUserID, "error", err.Error()) + } + } + } + + if user, err := p.client.User.Get(userID); err == nil && user.Props != nil && len(user.Props["git_user"]) > 0 { + delete(user.Props, "git_user") + if err := p.client.User.Update(user); err != nil { + p.client.Log.Warn("Failed to update user props during force-disconnect", "user_id", userID, "error", err.Error()) + } + } + + p.client.Frontend.PublishWebSocketEvent( + WsEventDisconnect, + nil, + &model.WebsocketBroadcast{UserId: userID}, + ) + + if err := p.CreateBotDMPost( + userID, + "Your GitLab account has been disconnected because the encryption key was rotated and your token could not be re-encrypted. Please reconnect your account using the `/gitlab connect` command.", + "custom_git_force_disconnect", + ); err != nil { + p.client.Log.Warn("Failed to send force-disconnect DM", "user_id", userID, "error", err.Error()) + } +} + func (p *Plugin) getOrRefreshTokenWithMutex(info *gitlab.UserInfo) (*oauth2.Token, error) { token, apiErr := p.getGitlabUserTokenByMattermostID(info.UserID) diff --git a/server/reencrypt_test.go b/server/reencrypt_test.go new file mode 100644 index 00000000..1b933bca --- /dev/null +++ b/server/reencrypt_test.go @@ -0,0 +1,495 @@ +// Copyright (c) 2019-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package main + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" + "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/mattermost/mattermost-plugin-gitlab/server/gitlab" +) + +const ( + // 32-byte keys required by AES. + testOldEncryptionKey = "oldKey0123456789012345678901234x" + testNewEncryptionKey = "newKey0123456789012345678901234x" + testUserID = "testUserID" + testGitlabUsername = "testGitlabUser" + testGitlabUserID = 42 +) + +// isNilBytes matches KVSetWithOptions calls with nil payload (i.e. Delete operations). +var isNilBytes = mock.MatchedBy(func(b []byte) bool { return b == nil }) + +// isNonNilBytes matches KVSetWithOptions calls with a non-nil payload (i.e. Set operations). +var isNonNilBytes = mock.MatchedBy(func(b []byte) bool { return b != nil }) + +func makeReencryptPlugin(t *testing.T, api *plugintest.API) *Plugin { + t.Helper() + p := &Plugin{ + configuration: &configuration{ + EncryptionKey: testNewEncryptionKey, + }, + BotUserID: "bot-user-id", + } + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + return p +} + +func encryptedTokenWithKey(t *testing.T, key string) []byte { + t.Helper() + token := &oauth2.Token{AccessToken: "test-access-token", TokenType: "Bearer"} + tokenJSON, err := json.Marshal(token) + require.NoError(t, err) + encrypted, err := encrypt([]byte(key), string(tokenJSON)) + require.NoError(t, err) + return []byte(encrypted) +} + +func marshaledUserInfo(t *testing.T) []byte { + t.Helper() + info := gitlab.UserInfo{ + UserID: testUserID, + GitlabUsername: testGitlabUsername, + GitlabUserID: testGitlabUserID, + } + b, err := json.Marshal(info) + require.NoError(t, err) + return b +} + +// mockForceDisconnectUser sets up the mocks for a forceDisconnectUser call where user info +// is available. All expectations use Maybe() since forceDisconnectUser is best-effort. +func mockForceDisconnectUser(t *testing.T, api *plugintest.API, userID string) { + t.Helper() + infoKey := userID + GitlabUserInfoKey + api.On("KVGet", infoKey).Return(marshaledUserInfo(t), nil).Maybe() + // Delete token and info via KVSetWithOptions with nil payload. + api.On("KVSetWithOptions", userID+GitlabUserTokenKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + api.On("KVSetWithOptions", userID+GitlabUserInfoKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + // Username and GitLab ID mapping deletes. + api.On("KVSetWithOptions", testGitlabUsername+GitlabUsernameKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + api.On("KVSetWithOptions", fmt.Sprintf("%d%s", testGitlabUserID, GitlabIDUsernameKey), isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + api.On("GetUser", userID).Return(&model.User{Props: model.StringMap{"git_user": testGitlabUsername}}, nil).Maybe() + api.On("UpdateUser", mock.Anything).Return(&model.User{}, nil).Maybe() + api.On("PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything).Return(nil).Maybe() + api.On("GetDirectChannel", userID, "bot-user-id").Return(&model.Channel{Id: "dm-ch"}, nil).Maybe() + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Maybe() +} + +// mockCommonLogCalls silences log calls that are not under test. +func mockCommonLogCalls(api *plugintest.API) { + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() +} + +// --------------------------------------------------------------------------- +// TestUnpad_StrictValidation +// --------------------------------------------------------------------------- + +func TestUnpad_StrictValidation(t *testing.T) { + t.Run("empty input returns error", func(t *testing.T) { + _, err := unpad([]byte{}) + require.Error(t, err) + }) + + t.Run("padding value zero is rejected", func(t *testing.T) { + src := []byte{0x41, 0x42, 0x00} + _, err := unpad(src) + require.Error(t, err) + assert.Contains(t, err.Error(), "unpad error") + }) + + t.Run("padding value exceeding block size is rejected", func(t *testing.T) { + // Last byte claims 17 bytes of padding (> aes.BlockSize=16). + src := make([]byte, 32) + src[31] = 17 + _, err := unpad(src) + require.Error(t, err) + }) + + t.Run("mismatched padding bytes are rejected", func(t *testing.T) { + // Last byte claims 4 bytes of padding, but the third-from-last byte is wrong. + src := []byte{0x41, 0x42, 0x03, 0x04, 0x04, 0x04} + _, err := unpad(src) + require.Error(t, err) + }) + + t.Run("valid PKCS7 padding succeeds", func(t *testing.T) { + src := []byte{0x41, 0x42, 0x03, 0x03, 0x03} + result, err := unpad(src) + require.NoError(t, err) + assert.Equal(t, []byte{0x41, 0x42}, result) + }) + + t.Run("wrong-key decrypt garbage does not pass unpad", func(t *testing.T) { + // Encrypt with old key, attempt to decrypt with new key. The stricter unpad + // validation should reliably reject the garbage output. + token := &oauth2.Token{AccessToken: "secret-token", TokenType: "Bearer"} + tokenJSON, marshalErr := json.Marshal(token) + require.NoError(t, marshalErr) + ciphertext, encErr := encrypt([]byte(testOldEncryptionKey), string(tokenJSON)) + require.NoError(t, encErr) + + _, err := decrypt([]byte(testNewEncryptionKey), ciphertext) + assert.Error(t, err, "decrypting with the wrong key should fail with the stricter unpad") + }) +} + +// --------------------------------------------------------------------------- +// TestReEncryptUserToken +// --------------------------------------------------------------------------- + +func TestReEncryptUserToken_HappyPath(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + kvKey := testUserID + GitlabUserTokenKey + + api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() + var storedBytes []byte + api.On("KVSetWithOptions", kvKey, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). + Run(func(args mock.Arguments) { + storedBytes = args.Get(1).([]byte) + }). + Return(true, nil).Once() + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + require.NoError(t, err) + assert.True(t, migrated) + + // Verify the stored ciphertext can be decrypted with the new key and yields the original token. + require.NotNil(t, storedBytes) + decrypted, decErr := decrypt([]byte(testNewEncryptionKey), string(storedBytes)) + require.NoError(t, decErr) + var tok oauth2.Token + require.NoError(t, json.Unmarshal([]byte(decrypted), &tok)) + assert.Equal(t, "test-access-token", tok.AccessToken) + + api.AssertExpectations(t) +} + +func TestReEncryptUserToken_AlreadyMigrated(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + + // Token already encrypted with the new key. + tokenBytes := encryptedTokenWithKey(t, testNewEncryptionKey) + kvKey := testUserID + GitlabUserTokenKey + + api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + require.NoError(t, err) + assert.False(t, migrated, "should report not migrated when token is already using the new key") + + api.AssertNotCalled(t, "KVSetWithOptions", kvKey, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")) + api.AssertExpectations(t) +} + +func TestReEncryptUserToken_NilToken(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + + kvKey := testUserID + GitlabUserTokenKey + api.On("KVGet", kvKey).Return(nil, nil).Once() + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + require.NoError(t, err) + assert.False(t, migrated) + api.AssertExpectations(t) +} + +func TestReEncryptUserToken_DecryptFailure(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + kvKey := testUserID + GitlabUserTokenKey + // Random garbage that can't be decrypted by either key. + api.On("KVGet", kvKey).Return([]byte("bm90LXZhbGlkLWJhc2U2NA=="), nil).Once() + + mockForceDisconnectUser(t, api, testUserID) + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + assert.Error(t, err) + assert.False(t, migrated) + + // Verify disconnect side-effects were triggered. + api.AssertCalled(t, "PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything) + api.AssertExpectations(t) +} + +func TestReEncryptUserToken_StoreFailure(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + kvKey := testUserID + GitlabUserTokenKey + + api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() + // Re-encrypt store fails. + api.On("KVSetWithOptions", kvKey, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). + Return(false, model.NewAppError("test", "test.store_error", nil, "kv store error", 500)).Once() + + mockForceDisconnectUser(t, api, testUserID) + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + assert.Error(t, err) + assert.False(t, migrated) + + api.AssertCalled(t, "PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything) + api.AssertExpectations(t) +} + +// --------------------------------------------------------------------------- +// TestReEncryptUserData +// --------------------------------------------------------------------------- + +func setupReEncryptUserDataPlugin(t *testing.T, api *plugintest.API) *Plugin { + t.Helper() + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + // Cluster mutex lock/unlock: cluster.NewMutex prepends "mutex_" to the key. + api.On("KVSetWithOptions", "mutex_gitlab-reencrypt-lock", isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). + Return(true, nil).Maybe() + api.On("KVSetWithOptions", "mutex_gitlab-reencrypt-lock", isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). + Return(true, nil).Maybe() + return p +} + +func TestReEncryptUserData_NoUsers(t *testing.T) { + api := &plugintest.API{} + p := setupReEncryptUserDataPlugin(t, api) + + api.On("KVList", 0, 1000).Return([]string{}, nil).Once() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + api.AssertExpectations(t) +} + +func TestReEncryptUserData_HappyPath(t *testing.T) { + api := &plugintest.API{} + p := setupReEncryptUserDataPlugin(t, api) + + user1Key := "user1" + GitlabUserTokenKey + user2Key := "user2" + GitlabUserTokenKey + + api.On("KVList", 0, 1000).Return([]string{user1Key, user2Key}, nil).Once() + + token1Bytes := encryptedTokenWithKey(t, testOldEncryptionKey) + token2Bytes := encryptedTokenWithKey(t, testOldEncryptionKey) + + api.On("KVGet", user1Key).Return(token1Bytes, nil).Once() + api.On("KVGet", user2Key).Return(token2Bytes, nil).Once() + + api.On("KVSetWithOptions", user1Key, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", user2Key, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + api.AssertExpectations(t) +} + +func TestReEncryptUserData_MultiplePages(t *testing.T) { + api := &plugintest.API{} + p := setupReEncryptUserDataPlugin(t, api) + + page0Keys := make([]string, 1000) + for i := range page0Keys { + page0Keys[i] = fmt.Sprintf("user%d%s", i, GitlabUserTokenKey) + } + api.On("KVList", 0, 1000).Return(page0Keys, nil).Once() + api.On("KVList", 1, 1000).Return([]string{}, nil).Once() + + // Use Maybe() for per-token operations — the pagination behavior is what this test verifies. + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + api.On("KVGet", mock.AnythingOfType("string")).Return(tokenBytes, nil).Maybe() + api.On("KVSetWithOptions", mock.AnythingOfType("string"), isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + // Verify both pages were fetched. + api.AssertCalled(t, "KVList", 0, 1000) + api.AssertCalled(t, "KVList", 1, 1000) +} + +func TestReEncryptUserData_ListKeysError(t *testing.T) { + api := &plugintest.API{} + p := setupReEncryptUserDataPlugin(t, api) + + api.On("KVList", 0, 1000).Return(nil, model.NewAppError("test", "test.list_error", nil, "kv list error", 500)).Once() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + // No token operations should be attempted when the key list fails. + api.AssertNotCalled(t, "KVGet", testUserID+GitlabUserTokenKey) + api.AssertExpectations(t) +} + +func TestReEncryptUserData_PageErrorAfterFirstPage(t *testing.T) { + api := &plugintest.API{} + p := setupReEncryptUserDataPlugin(t, api) + + page0Keys := make([]string, 1000) + for i := range page0Keys { + page0Keys[i] = fmt.Sprintf("user%d%s", i, GitlabUserTokenKey) + } + api.On("KVList", 0, 1000).Return(page0Keys, nil).Once() + api.On("KVList", 1, 1000).Return(nil, model.NewAppError("test", "test.list_error", nil, "page 1 error", 500)).Once() + + // Use Maybe() for per-token operations — the key assertion is that page 0 keys + // are processed even when page 1 returns an error. + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + api.On("KVGet", mock.AnythingOfType("string")).Return(tokenBytes, nil).Maybe() + api.On("KVSetWithOptions", mock.AnythingOfType("string"), isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + // Verify page 0 was fetched and page 1 was attempted. + api.AssertCalled(t, "KVList", 0, 1000) + api.AssertCalled(t, "KVList", 1, 1000) +} + +func TestReEncryptUserData_DecryptFailureContinuesOtherUsers(t *testing.T) { + api := &plugintest.API{} + p := setupReEncryptUserDataPlugin(t, api) + + user1Key := "user1" + GitlabUserTokenKey + user2Key := "user2" + GitlabUserTokenKey + + api.On("KVList", 0, 1000).Return([]string{user1Key, user2Key}, nil).Once() + + // user1: corrupted ciphertext triggers force disconnect. + api.On("KVGet", user1Key).Return([]byte("bm90LXZhbGlkLWJhc2U2NA=="), nil).Once() + mockForceDisconnectUser(t, api, "user1") + + // user2: succeeds. + token2Bytes := encryptedTokenWithKey(t, testOldEncryptionKey) + api.On("KVGet", user2Key).Return(token2Bytes, nil).Once() + api.On("KVSetWithOptions", user2Key, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + api.AssertExpectations(t) +} + +// --------------------------------------------------------------------------- +// TestForceDisconnectUser +// --------------------------------------------------------------------------- + +func TestForceDisconnectUser_HappyPath(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + infoKey := testUserID + GitlabUserInfoKey + api.On("KVGet", infoKey).Return(marshaledUserInfo(t), nil).Once() + + api.On("KVSetWithOptions", testUserID+GitlabUserTokenKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", testUserID+GitlabUserInfoKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", testGitlabUsername+GitlabUsernameKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", fmt.Sprintf("%d%s", testGitlabUserID, GitlabIDUsernameKey), isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("GetUser", testUserID).Return(&model.User{Props: model.StringMap{"git_user": testGitlabUsername}}, nil).Once() + api.On("UpdateUser", mock.Anything).Return(&model.User{}, nil).Once() + api.On("PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything).Return(nil).Once() + api.On("GetDirectChannel", testUserID, "bot-user-id").Return(&model.Channel{Id: "dm-ch"}, nil).Once() + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Once() + + p.forceDisconnectUser(testUserID) + + api.AssertExpectations(t) +} + +func TestForceDisconnectUser_UserInfoNotFound(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + // User info missing — partial cleanup using only userID. + infoKey := testUserID + GitlabUserInfoKey + api.On("KVGet", infoKey).Return(nil, nil).Once() + // getGitlabUserInfoByMattermostID falls back to the migration key when _userinfo is nil. + api.On("KVGet", testUserID+GitlabMigrationTokenKey).Return(nil, nil).Once() + + api.On("KVSetWithOptions", testUserID+GitlabUserTokenKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", testUserID+GitlabUserInfoKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + // No username/ID mapping deletes since userInfo is nil (GitlabUsername="" and GitlabUserID=0). + api.On("GetUser", testUserID).Return(&model.User{Props: model.StringMap{}}, nil).Once() + api.On("PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything).Return(nil).Once() + api.On("GetDirectChannel", testUserID, "bot-user-id").Return(&model.Channel{Id: "dm-ch"}, nil).Once() + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Once() + + p.forceDisconnectUser(testUserID) + + // Mapping deletes must NOT be called when we have no GitLab identity info. + api.AssertNotCalled(t, "KVSetWithOptions", testGitlabUsername+GitlabUsernameKey, isNilBytes, mock.Anything) + api.AssertExpectations(t) +} + +func TestForceDisconnectUser_DMFailure(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + infoKey := testUserID + GitlabUserInfoKey + api.On("KVGet", infoKey).Return(marshaledUserInfo(t), nil).Once() + api.On("KVSetWithOptions", testUserID+GitlabUserTokenKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", testUserID+GitlabUserInfoKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", testGitlabUsername+GitlabUsernameKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", fmt.Sprintf("%d%s", testGitlabUserID, GitlabIDUsernameKey), isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("GetUser", testUserID).Return(&model.User{Props: model.StringMap{}}, nil).Once() + api.On("PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything).Return(nil).Once() + + // DM channel lookup fails — must be handled gracefully without panicking. + api.On("GetDirectChannel", testUserID, "bot-user-id").Return(nil, &model.AppError{Message: "channel not found"}).Once() + + p.forceDisconnectUser(testUserID) + + api.AssertExpectations(t) +} + +// --------------------------------------------------------------------------- +// TestDisconnectGitlabAccount_FallbackWhenUserInfoMissing +// --------------------------------------------------------------------------- + +func TestDisconnectGitlabAccount_FallbackWhenUserInfoMissing(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + // getGitlabUserInfoByMattermostID reads _userinfo then _gitlabtoken; both return nil → not connected. + // This is called twice: once in disconnectGitlabAccount and once inside forceDisconnectUser. + api.On("KVGet", testUserID+GitlabUserInfoKey).Return(nil, nil) + api.On("KVGet", testUserID+GitlabMigrationTokenKey).Return(nil, nil) + + // forceDisconnectUser best-effort cleanup (no user info available). + api.On("KVSetWithOptions", testUserID+GitlabUserTokenKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVSetWithOptions", testUserID+GitlabUserInfoKey, isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("GetUser", testUserID).Return(&model.User{Props: model.StringMap{"git_user": "someUser"}}, nil).Once() + api.On("UpdateUser", mock.Anything).Return(&model.User{}, nil).Once() + api.On("PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything).Return(nil).Once() + api.On("GetDirectChannel", testUserID, "bot-user-id").Return(&model.Channel{Id: "dm-ch"}, nil).Once() + api.On("CreatePost", mock.Anything).Return(&model.Post{}, nil).Once() + + p.disconnectGitlabAccount(testUserID) + + // The websocket disconnect event must still be published despite user info being missing. + api.AssertCalled(t, "PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything) + api.AssertExpectations(t) +} diff --git a/server/utils.go b/server/utils.go index 00ccd507..dd9b3624 100644 --- a/server/utils.go +++ b/server/utils.go @@ -32,12 +32,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 7cd5fc95eedb9100f07f56f6e77573f9f1b1651f Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Wed, 1 Apr 2026 16:45:53 +0300 Subject: [PATCH 2/7] Fixing requirement for connect so it's backwards compatible - Also fixing DMs --- server/command.go | 16 +++++++--------- server/command_test.go | 2 +- server/plugin.go | 18 +++++++++++++++++- server/reencrypt_test.go | 2 ++ 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/server/command.go b/server/command.go index 4ffb6c76..afcdfdc6 100644 --- a/server/command.go +++ b/server/command.go @@ -338,15 +338,13 @@ func (p *Plugin) handleAbout(args *model.CommandArgs, parameters []string) (*mod } func (p *Plugin) handleConnect(args *model.CommandArgs, parameters []string) (*model.CommandResponse, *model.AppError) { - if len(parameters) < 1 { - return p.getCommandResponse(args, "Please specify the instance name.", true), nil - } - - // Set the default instance for the user before connecting - instanceName := strings.TrimSpace(strings.Join(parameters, " ")) - err := p.setDefaultInstance(instanceName) - if err != nil { - return p.getCommandResponse(args, err.Error(), true), nil + if len(parameters) >= 1 { + instanceName := strings.TrimSpace(strings.Join(parameters, " ")) + if err := p.setDefaultInstance(instanceName); err != nil { + return p.getCommandResponse(args, err.Error(), true), nil + } + } else if !p.canConnect() { + return p.getCommandResponse(args, "No instance is configured. Please specify an instance name or ask your system administrator to configure the plugin.", true), nil } pluginURL := getPluginURL(p.client) diff --git a/server/command_test.go b/server/command_test.go index 56d25c13..6823e36f 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -780,7 +780,7 @@ func TestInstanceCommands(t *testing.T) { t.Run("no parameters", func(t *testing.T) { p, msg, _ := setupInstanceCommandTest(t, nil, nil) _, _ = p.handleConnect(args, []string{}) - assert.Contains(t, *msg, "Please specify the instance name.") + assert.Contains(t, *msg, "No instance is configured") }) }) } diff --git a/server/plugin.go b/server/plugin.go index 38bd422e..2039bec2 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -276,6 +276,22 @@ func (p *Plugin) getOAuthConfig() (*oauth2.Config, error) { }, nil } +// canConnect checks whether the plugin has enough configuration to allow a user to connect, +// either via a KV-backed instance (default instance) or via legacy plugin settings. +func (p *Plugin) canConnect() bool { + config := p.getConfiguration() + + if config.DefaultInstanceName != "" { + if _, err := p.getInstance(config.DefaultInstanceName); err == nil { + return true + } + } + + return config.GitlabOAuthClientID != "" && + config.GitlabOAuthClientSecret != "" && + isValidURL(config.GitlabURL) == nil +} + // resolveOAuthCredentials returns OAuth client credentials and the parsed GitLab base URL // by first trying the KV-backed instance configuration, then falling back to legacy plugin // settings for backwards compatibility with upgrades from v1.11 and earlier. @@ -1141,7 +1157,7 @@ func (p *Plugin) forceDisconnectUser(userID string) { if err := p.CreateBotDMPost( userID, "Your GitLab account has been disconnected because the encryption key was rotated and your token could not be re-encrypted. Please reconnect your account using the `/gitlab connect` command.", - "custom_git_force_disconnect", + "custom_git_disconnected", ); err != nil { p.client.Log.Warn("Failed to send force-disconnect DM", "user_id", userID, "error", err.Error()) } diff --git a/server/reencrypt_test.go b/server/reencrypt_test.go index 1b933bca..d1704785 100644 --- a/server/reencrypt_test.go +++ b/server/reencrypt_test.go @@ -269,6 +269,8 @@ func setupReEncryptUserDataPlugin(t *testing.T, api *plugintest.API) *Plugin { Return(true, nil).Maybe() api.On("KVSetWithOptions", "mutex_gitlab-reencrypt-lock", isNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). Return(true, nil).Maybe() + // Audit logging. + api.On("LogAuditRec", mock.Anything).Maybe() return p } From 955c63efd56282e72e6c55d8a303b4eb4b46322d Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Mon, 6 Apr 2026 15:53:15 +0300 Subject: [PATCH 3/7] Updating connect tests --- server/command_test.go | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/server/command_test.go b/server/command_test.go index 7e20d6f9..6b28becc 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -762,15 +762,29 @@ func TestInstanceCommands(t *testing.T) { }) t.Run("connect", func(t *testing.T) { - for _, tc := range instanceNameTestCases { - t.Run("connect "+tc.name, func(t *testing.T) { - p, msg, api := setupInstanceCommandTest(t, nil, nil) - _, _ = p.handleConnect(args, tc.parameters) - assert.Contains(t, *msg, "Click here to link your GitLab account") - api.AssertNotCalled(t, "SavePluginConfig", mock.Anything) - }) - } - t.Run("no parameters", func(t *testing.T) { + t.Run("with default instance configured", func(t *testing.T) { + instanceList := []string{"production"} + instanceConfig := map[string]InstanceConfiguration{ + "production": { + GitlabURL: "https://gitlab.example.com", + GitlabOAuthClientID: "client-id", + GitlabOAuthClientSecret: "client-secret", + }, + } + p, msg, _ := setupInstanceCommandTest(t, instanceList, instanceConfig) + p.configuration.DefaultInstanceName = "production" + _, _ = p.handleConnect(args, []string{}) + assert.Contains(t, *msg, "Click here to link your GitLab account") + }) + t.Run("with legacy plugin settings", func(t *testing.T) { + p, msg, _ := setupInstanceCommandTest(t, nil, nil) + p.configuration.GitlabURL = "https://gitlab.example.com" + p.configuration.GitlabOAuthClientID = "client-id" + p.configuration.GitlabOAuthClientSecret = "client-secret" + _, _ = p.handleConnect(args, []string{}) + assert.Contains(t, *msg, "Click here to link your GitLab account") + }) + t.Run("not configured", func(t *testing.T) { p, msg, _ := setupInstanceCommandTest(t, nil, nil) _, _ = p.handleConnect(args, []string{}) assert.Contains(t, *msg, "No instance is configured") From d3ade686dfd877481b67cf4fa544b1c9298039fd Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Thu, 9 Apr 2026 18:43:04 +0300 Subject: [PATCH 4/7] Fixing issues raised in PR reviews --- server/configuration.go | 16 +++++++-- server/plugin.go | 49 ++++++++++++++++++++++---- server/reencrypt_test.go | 76 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 9 deletions(-) diff --git a/server/configuration.go b/server/configuration.go index 05eba897..1782b596 100644 --- a/server/configuration.go +++ b/server/configuration.go @@ -40,6 +40,11 @@ type configuration struct { EnableCodePreview string `json:"enablecodepreview"` UsePreregisteredApplication bool `json:"usepreregisteredapplication"` EnableChildPipelineNotifications bool `json:"enablechildpipelinenotifications"` + + // PreviousEncryptionKey is set internally during key rotation so that token + // reads can fall back to the old key while background re-encryption runs. + // It is never persisted to the plugin settings. + PreviousEncryptionKey string `json:"-"` } // Clone shallow copies the configuration. Your implementation may require a deep copy if @@ -208,12 +213,17 @@ func (p *Plugin) OnConfigurationChange() error { p.configurationLock.RUnlock() newGitlabGroup := strings.TrimSpace(configuration.GitlabGroup) - p.setConfiguration(configuration, serverConfiguration) - if previousEncryptionKey != "" && configuration.EncryptionKey != "" && previousEncryptionKey != configuration.EncryptionKey { + configuration.PreviousEncryptionKey = previousEncryptionKey + } + + p.setConfiguration(configuration, serverConfiguration) + + if configuration.PreviousEncryptionKey != "" { newKey := configuration.EncryptionKey - go p.reEncryptUserData(newKey, previousEncryptionKey) + prevKey := configuration.PreviousEncryptionKey + go p.reEncryptUserData(newKey, prevKey) } if hadConfig && p.BotUserID != "" && newGitlabGroup != "" && newGitlabGroup != previousGitlabGroup { diff --git a/server/plugin.go b/server/plugin.go index 2039bec2..7af55b4f 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -485,8 +485,17 @@ func (p *Plugin) getGitlabUserTokenByMattermostID(userID string) (*oauth2.Token, unencryptedToken, err := decrypt([]byte(config.EncryptionKey), string(tokenBytes)) if err != nil { - p.client.Log.Warn("can't decrypt token", "err", err.Error()) - return nil, &APIErrorResponse{ID: "", Message: "Unable to decrypt access token.", StatusCode: http.StatusInternalServerError} + prevKey := config.PreviousEncryptionKey + if prevKey != "" { + if fallback, fbErr := decrypt([]byte(prevKey), string(tokenBytes)); fbErr == nil { + unencryptedToken = fallback + err = nil + } + } + if err != nil { + p.client.Log.Warn("can't decrypt token", "err", err.Error()) + return nil, &APIErrorResponse{ID: "", Message: "Unable to decrypt access token.", StatusCode: http.StatusInternalServerError} + } } if err := json.Unmarshal([]byte(unencryptedToken), &token); err != nil { @@ -1061,8 +1070,20 @@ func (p *Plugin) reEncryptUserData(newEncryptionKey, previousEncryptionKey strin } } - auditRec.Success() auditRec.AddEventResultState(result) + + if result.ForceDisconnectCount > 0 { + auditRec.AddErrorDesc(fmt.Sprintf("%d users were force-disconnected during re-encryption", result.ForceDisconnectCount)) + } else { + auditRec.Success() + } + + // Clear the fallback key now that all tokens have been migrated. + p.configurationLock.Lock() + if p.configuration != nil && p.configuration.PreviousEncryptionKey == previousEncryptionKey { + p.configuration.PreviousEncryptionKey = "" + } + p.configurationLock.Unlock() } // reEncryptUserToken re-encrypts a single user token KV entry from previousEncryptionKey to newEncryptionKey. @@ -1072,15 +1093,23 @@ func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionK userID := strings.TrimSuffix(kvKey, GitlabUserTokenKey) var tokenBytes []byte - if err := p.client.KV.Get(kvKey, &tokenBytes); err != nil || tokenBytes == nil { + if err := p.client.KV.Get(kvKey, &tokenBytes); err != nil { + p.client.Log.Warn("Failed to read token during re-encryption, skipping user", + "user_id", userID, "error", err.Error()) + return false, nil + } + if tokenBytes == nil { return false, nil } tokenStr := string(tokenBytes) // Idempotency check: if the token can already be decrypted with the new key, it was already migrated. - if _, err := decrypt([]byte(newEncryptionKey), tokenStr); err == nil { - return false, nil + if plain, decErr := decrypt([]byte(newEncryptionKey), tokenStr); decErr == nil { + var tok oauth2.Token + if json.Unmarshal([]byte(plain), &tok) == nil { + return false, nil + } } plainJSON, err := decrypt([]byte(previousEncryptionKey), tokenStr) @@ -1091,6 +1120,14 @@ func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionK return false, err } + var tok oauth2.Token + if err := json.Unmarshal([]byte(plainJSON), &tok); err != nil { + p.client.Log.Warn("Decrypted token is not valid JSON, force-disconnecting user", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID) + return false, err + } + reEncrypted, err := encrypt([]byte(newEncryptionKey), plainJSON) if err != nil { p.client.Log.Warn("Failed to re-encrypt token with new key, force-disconnecting user", diff --git a/server/reencrypt_test.go b/server/reencrypt_test.go index d1704785..9802560a 100644 --- a/server/reencrypt_test.go +++ b/server/reencrypt_test.go @@ -263,6 +263,7 @@ func TestReEncryptUserToken_StoreFailure(t *testing.T) { func setupReEncryptUserDataPlugin(t *testing.T, api *plugintest.API) *Plugin { t.Helper() p := makeReencryptPlugin(t, api) + p.configuration.PreviousEncryptionKey = testOldEncryptionKey mockCommonLogCalls(api) // Cluster mutex lock/unlock: cluster.NewMutex prepends "mutex_" to the key. api.On("KVSetWithOptions", "mutex_gitlab-reencrypt-lock", isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). @@ -391,6 +392,81 @@ func TestReEncryptUserData_DecryptFailureContinuesOtherUsers(t *testing.T) { api.AssertExpectations(t) } +func TestReEncryptUserToken_InvalidJSON(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + kvKey := testUserID + GitlabUserTokenKey + + // Encrypt non-JSON data with the old key — decrypt succeeds but JSON validation fails. + notJSON := "this is not json" + encrypted, err := encrypt([]byte(testOldEncryptionKey), notJSON) + require.NoError(t, err) + api.On("KVGet", kvKey).Return([]byte(encrypted), nil).Once() + + mockForceDisconnectUser(t, api, testUserID) + + migrated, reErr := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + assert.Error(t, reErr) + assert.False(t, migrated) + + api.AssertCalled(t, "PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything) + api.AssertExpectations(t) +} + +func TestReEncryptUserToken_KVGetError(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + kvKey := testUserID + GitlabUserTokenKey + api.On("KVGet", kvKey).Return(nil, model.NewAppError("test", "test.kv_error", nil, "kv error", 500)).Once() + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + require.NoError(t, err) + assert.False(t, migrated, "should skip user on KV read error without force-disconnect") + + api.AssertNotCalled(t, "PublishWebSocketEvent", mock.Anything, mock.Anything, mock.Anything) + api.AssertExpectations(t) +} + +// --------------------------------------------------------------------------- +// TestGetGitlabUserTokenByMattermostID (fallback decryption) +// --------------------------------------------------------------------------- + +func TestGetGitlabUserTokenByMattermostID_FallbackToPreviousKey(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + p.configuration.PreviousEncryptionKey = testOldEncryptionKey + mockCommonLogCalls(api) + + kvKey := testUserID + GitlabUserTokenKey + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() + + tok, apiErr := p.getGitlabUserTokenByMattermostID(testUserID) + require.Nil(t, apiErr) + require.NotNil(t, tok) + assert.Equal(t, "test-access-token", tok.AccessToken) +} + +func TestGetGitlabUserTokenByMattermostID_BothKeysFail(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + p.configuration.PreviousEncryptionKey = testOldEncryptionKey + mockCommonLogCalls(api) + + kvKey := testUserID + GitlabUserTokenKey + // Garbage that neither key can decrypt. + api.On("KVGet", kvKey).Return([]byte("bm90LXZhbGlkLWJhc2U2NA=="), nil).Once() + + tok, apiErr := p.getGitlabUserTokenByMattermostID(testUserID) + require.NotNil(t, apiErr) + assert.Nil(t, tok) + assert.Equal(t, "Unable to decrypt access token.", apiErr.Message) +} + // --------------------------------------------------------------------------- // TestForceDisconnectUser // --------------------------------------------------------------------------- From 8ce021566f1e169bfb055997e571dbb0e5d736f5 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Thu, 9 Apr 2026 20:16:36 +0300 Subject: [PATCH 5/7] Addressing issue with test assertion --- server/reencrypt_test.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/server/reencrypt_test.go b/server/reencrypt_test.go index 9802560a..b47ab7d4 100644 --- a/server/reencrypt_test.go +++ b/server/reencrypt_test.go @@ -133,17 +133,24 @@ func TestUnpad_StrictValidation(t *testing.T) { assert.Equal(t, []byte{0x41, 0x42}, result) }) - t.Run("wrong-key decrypt garbage does not pass unpad", func(t *testing.T) { - // Encrypt with old key, attempt to decrypt with new key. The stricter unpad - // validation should reliably reject the garbage output. + t.Run("wrong-key decrypt is caught by unpad or JSON validation", func(t *testing.T) { + // Encrypt with old key, attempt to decrypt with new key. In the vast majority + // of cases unpad rejects the garbage. On the ~0.4% chance that random garbage + // has valid PKCS7 padding, the result will not be valid JSON. token := &oauth2.Token{AccessToken: "secret-token", TokenType: "Bearer"} tokenJSON, marshalErr := json.Marshal(token) require.NoError(t, marshalErr) ciphertext, encErr := encrypt([]byte(testOldEncryptionKey), string(tokenJSON)) require.NoError(t, encErr) - _, err := decrypt([]byte(testNewEncryptionKey), ciphertext) - assert.Error(t, err, "decrypting with the wrong key should fail with the stricter unpad") + plain, decErr := decrypt([]byte(testNewEncryptionKey), ciphertext) + if decErr != nil { + return // unpad correctly rejected the garbage — most common path + } + // Padding happened to be valid; the decrypted bytes must not parse as a token. + var tok oauth2.Token + assert.Error(t, json.Unmarshal([]byte(plain), &tok), + "wrong-key garbage that passes unpad must not be valid JSON") }) } From 992090e539d89f9bad83518f5752d06c36f6d6f3 Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Thu, 9 Apr 2026 20:35:27 +0300 Subject: [PATCH 6/7] Addressing findings in PR --- server/plugin.go | 14 ++++++++--- server/reencrypt_test.go | 54 +++++++++++++++++++++++++++++++--------- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/server/plugin.go b/server/plugin.go index 7af55b4f..58e003c1 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -1121,7 +1121,7 @@ func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionK } var tok oauth2.Token - if err := json.Unmarshal([]byte(plainJSON), &tok); err != nil { + if err = json.Unmarshal([]byte(plainJSON), &tok); err != nil { p.client.Log.Warn("Decrypted token is not valid JSON, force-disconnecting user", "user_id", userID, "error", err.Error()) p.forceDisconnectUser(userID) @@ -1136,11 +1136,17 @@ func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionK return false, err } - if _, err := p.client.KV.Set(kvKey, []byte(reEncrypted)); err != nil { + swapped, appErr := p.API.KVCompareAndSet(kvKey, tokenBytes, []byte(reEncrypted)) + if appErr != nil { p.client.Log.Warn("Failed to store re-encrypted token, force-disconnecting user", - "user_id", userID, "error", err.Error()) + "user_id", userID, "error", appErr.Error()) p.forceDisconnectUser(userID) - return false, err + return false, appErr + } + if !swapped { + p.client.Log.Info("Token was modified concurrently during re-encryption, skipping user", + "user_id", userID) + return false, nil } return true, nil diff --git a/server/reencrypt_test.go b/server/reencrypt_test.go index b47ab7d4..17340003 100644 --- a/server/reencrypt_test.go +++ b/server/reencrypt_test.go @@ -168,9 +168,9 @@ func TestReEncryptUserToken_HappyPath(t *testing.T) { api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() var storedBytes []byte - api.On("KVSetWithOptions", kvKey, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). + api.On("KVCompareAndSet", kvKey, tokenBytes, isNonNilBytes). Run(func(args mock.Arguments) { - storedBytes = args.Get(1).([]byte) + storedBytes = args.Get(2).([]byte) }). Return(true, nil).Once() @@ -203,7 +203,7 @@ func TestReEncryptUserToken_AlreadyMigrated(t *testing.T) { require.NoError(t, err) assert.False(t, migrated, "should report not migrated when token is already using the new key") - api.AssertNotCalled(t, "KVSetWithOptions", kvKey, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")) + api.AssertNotCalled(t, "KVCompareAndSet", kvKey, mock.Anything, mock.Anything) api.AssertExpectations(t) } @@ -250,7 +250,7 @@ func TestReEncryptUserToken_StoreFailure(t *testing.T) { api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() // Re-encrypt store fails. - api.On("KVSetWithOptions", kvKey, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")). + api.On("KVCompareAndSet", kvKey, tokenBytes, isNonNilBytes). Return(false, model.NewAppError("test", "test.store_error", nil, "kv store error", 500)).Once() mockForceDisconnectUser(t, api, testUserID) @@ -263,6 +263,27 @@ func TestReEncryptUserToken_StoreFailure(t *testing.T) { api.AssertExpectations(t) } +func TestReEncryptUserToken_CASConflict(t *testing.T) { + api := &plugintest.API{} + p := makeReencryptPlugin(t, api) + mockCommonLogCalls(api) + + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + kvKey := testUserID + GitlabUserTokenKey + + api.On("KVGet", kvKey).Return(tokenBytes, nil).Once() + // CAS returns false with no error — the token was modified concurrently. + api.On("KVCompareAndSet", kvKey, tokenBytes, isNonNilBytes).Return(false, nil).Once() + + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) + require.NoError(t, err) + assert.False(t, migrated, "should skip user when token was modified concurrently") + + // Must NOT force-disconnect the user on a benign CAS conflict. + api.AssertNotCalled(t, "PublishWebSocketEvent", mock.Anything, mock.Anything, mock.Anything) + api.AssertExpectations(t) +} + // --------------------------------------------------------------------------- // TestReEncryptUserData // --------------------------------------------------------------------------- @@ -308,8 +329,8 @@ func TestReEncryptUserData_HappyPath(t *testing.T) { api.On("KVGet", user1Key).Return(token1Bytes, nil).Once() api.On("KVGet", user2Key).Return(token2Bytes, nil).Once() - api.On("KVSetWithOptions", user1Key, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() - api.On("KVSetWithOptions", user2Key, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVCompareAndSet", user1Key, token1Bytes, isNonNilBytes).Return(true, nil).Once() + api.On("KVCompareAndSet", user2Key, token2Bytes, isNonNilBytes).Return(true, nil).Once() p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) @@ -327,16 +348,19 @@ func TestReEncryptUserData_MultiplePages(t *testing.T) { api.On("KVList", 0, 1000).Return(page0Keys, nil).Once() api.On("KVList", 1, 1000).Return([]string{}, nil).Once() - // Use Maybe() for per-token operations — the pagination behavior is what this test verifies. tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) api.On("KVGet", mock.AnythingOfType("string")).Return(tokenBytes, nil).Maybe() - api.On("KVSetWithOptions", mock.AnythingOfType("string"), isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + casCount := 0 + api.On("KVCompareAndSet", mock.AnythingOfType("string"), tokenBytes, isNonNilBytes). + Run(func(_ mock.Arguments) { casCount++ }). + Return(true, nil).Maybe() p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) // Verify both pages were fetched. api.AssertCalled(t, "KVList", 0, 1000) api.AssertCalled(t, "KVList", 1, 1000) + assert.Equal(t, len(page0Keys), casCount, "all page-0 keys should have been re-encrypted") } func TestReEncryptUserData_ListKeysError(t *testing.T) { @@ -363,17 +387,19 @@ func TestReEncryptUserData_PageErrorAfterFirstPage(t *testing.T) { api.On("KVList", 0, 1000).Return(page0Keys, nil).Once() api.On("KVList", 1, 1000).Return(nil, model.NewAppError("test", "test.list_error", nil, "page 1 error", 500)).Once() - // Use Maybe() for per-token operations — the key assertion is that page 0 keys - // are processed even when page 1 returns an error. tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) api.On("KVGet", mock.AnythingOfType("string")).Return(tokenBytes, nil).Maybe() - api.On("KVSetWithOptions", mock.AnythingOfType("string"), isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Maybe() + casCount := 0 + api.On("KVCompareAndSet", mock.AnythingOfType("string"), tokenBytes, isNonNilBytes). + Run(func(_ mock.Arguments) { casCount++ }). + Return(true, nil).Maybe() p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) // Verify page 0 was fetched and page 1 was attempted. api.AssertCalled(t, "KVList", 0, 1000) api.AssertCalled(t, "KVList", 1, 1000) + assert.Equal(t, len(page0Keys), casCount, "all page-0 keys should have been re-encrypted despite page-1 error") } func TestReEncryptUserData_DecryptFailureContinuesOtherUsers(t *testing.T) { @@ -392,7 +418,7 @@ func TestReEncryptUserData_DecryptFailureContinuesOtherUsers(t *testing.T) { // user2: succeeds. token2Bytes := encryptedTokenWithKey(t, testOldEncryptionKey) api.On("KVGet", user2Key).Return(token2Bytes, nil).Once() - api.On("KVSetWithOptions", user2Key, isNonNilBytes, mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil).Once() + api.On("KVCompareAndSet", user2Key, token2Bytes, isNonNilBytes).Return(true, nil).Once() p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) @@ -456,6 +482,8 @@ func TestGetGitlabUserTokenByMattermostID_FallbackToPreviousKey(t *testing.T) { require.Nil(t, apiErr) require.NotNil(t, tok) assert.Equal(t, "test-access-token", tok.AccessToken) + + api.AssertExpectations(t) } func TestGetGitlabUserTokenByMattermostID_BothKeysFail(t *testing.T) { @@ -472,6 +500,8 @@ func TestGetGitlabUserTokenByMattermostID_BothKeysFail(t *testing.T) { require.NotNil(t, apiErr) assert.Nil(t, tok) assert.Equal(t, "Unable to decrypt access token.", apiErr.Message) + + api.AssertExpectations(t) } // --------------------------------------------------------------------------- From fe4ffcae3852adfe03ca312b7c1f82cf10a2600b Mon Sep 17 00:00:00 2001 From: avasconcelos114 Date: Thu, 9 Apr 2026 21:30:42 +0300 Subject: [PATCH 7/7] Ensuring users get disconnected in case of KV read error --- server/plugin.go | 5 +++-- server/reencrypt_test.go | 8 +++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/server/plugin.go b/server/plugin.go index 58e003c1..387422d1 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -1094,9 +1094,10 @@ func (p *Plugin) reEncryptUserToken(kvKey, newEncryptionKey, previousEncryptionK var tokenBytes []byte if err := p.client.KV.Get(kvKey, &tokenBytes); err != nil { - p.client.Log.Warn("Failed to read token during re-encryption, skipping user", + p.client.Log.Warn("Failed to read token during re-encryption, force-disconnecting user", "user_id", userID, "error", err.Error()) - return false, nil + p.forceDisconnectUser(userID) + return false, err } if tokenBytes == nil { return false, nil diff --git a/server/reencrypt_test.go b/server/reencrypt_test.go index 17340003..d0a8d363 100644 --- a/server/reencrypt_test.go +++ b/server/reencrypt_test.go @@ -456,11 +456,13 @@ func TestReEncryptUserToken_KVGetError(t *testing.T) { kvKey := testUserID + GitlabUserTokenKey api.On("KVGet", kvKey).Return(nil, model.NewAppError("test", "test.kv_error", nil, "kv error", 500)).Once() + mockForceDisconnectUser(t, api, testUserID) + migrated, err := p.reEncryptUserToken(kvKey, testNewEncryptionKey, testOldEncryptionKey) - require.NoError(t, err) - assert.False(t, migrated, "should skip user on KV read error without force-disconnect") + assert.Error(t, err) + assert.False(t, migrated) - api.AssertNotCalled(t, "PublishWebSocketEvent", mock.Anything, mock.Anything, mock.Anything) + api.AssertCalled(t, "PublishWebSocketEvent", WsEventDisconnect, mock.Anything, mock.Anything) api.AssertExpectations(t) }