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/command.go b/server/command.go index 5d3e0c93..0227fdc6 100644 --- a/server/command.go +++ b/server/command.go @@ -338,8 +338,8 @@ 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 + 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 70b2e4e6..6b28becc 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -762,18 +762,32 @@ 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, "Please specify the instance name.") + 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") }) }) } diff --git a/server/configuration.go b/server/configuration.go index 0319af38..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 @@ -200,14 +205,27 @@ 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) + if previousEncryptionKey != "" && configuration.EncryptionKey != "" && + previousEncryptionKey != configuration.EncryptionKey { + configuration.PreviousEncryptionKey = previousEncryptionKey + } + p.setConfiguration(configuration, serverConfiguration) + if configuration.PreviousEncryptionKey != "" { + newKey := configuration.EncryptionKey + prevKey := configuration.PreviousEncryptionKey + go p.reEncryptUserData(newKey, prevKey) + } + if hadConfig && p.BotUserID != "" && newGitlabGroup != "" && newGitlabGroup != previousGitlabGroup { p.notifyUsersOfDisallowedSubscriptions() } diff --git a/server/plugin.go b/server/plugin.go index ca587fa9..387422d1 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. @@ -469,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 { @@ -527,9 +552,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 +1020,193 @@ 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.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. +// 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 { + p.client.Log.Warn("Failed to read token during re-encryption, force-disconnecting user", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID) + return false, err + } + 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 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) + 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 + } + + 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", + "user_id", userID, "error", err.Error()) + p.forceDisconnectUser(userID) + return false, err + } + + 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", appErr.Error()) + p.forceDisconnectUser(userID) + 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 +} + +// 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_disconnected", + ); 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..d0a8d363 --- /dev/null +++ b/server/reencrypt_test.go @@ -0,0 +1,612 @@ +// 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 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) + + 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") + }) +} + +// --------------------------------------------------------------------------- +// 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("KVCompareAndSet", kvKey, tokenBytes, isNonNilBytes). + Run(func(args mock.Arguments) { + storedBytes = args.Get(2).([]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, "KVCompareAndSet", kvKey, mock.Anything, mock.Anything) + 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("KVCompareAndSet", kvKey, tokenBytes, isNonNilBytes). + 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) +} + +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 +// --------------------------------------------------------------------------- + +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")). + 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 +} + +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("KVCompareAndSet", user1Key, token1Bytes, isNonNilBytes).Return(true, nil).Once() + api.On("KVCompareAndSet", user2Key, token2Bytes, isNonNilBytes).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() + + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + api.On("KVGet", mock.AnythingOfType("string")).Return(tokenBytes, 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) { + 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() + + tokenBytes := encryptedTokenWithKey(t, testOldEncryptionKey) + api.On("KVGet", mock.AnythingOfType("string")).Return(tokenBytes, 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) { + 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("KVCompareAndSet", user2Key, token2Bytes, isNonNilBytes).Return(true, nil).Once() + + p.reEncryptUserData(testNewEncryptionKey, testOldEncryptionKey) + + 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() + + 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) +} + +// --------------------------------------------------------------------------- +// 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) + + api.AssertExpectations(t) +} + +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) + + 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 }