MM-67613 Adding encryption key rotation with graceful fallback

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,
+	}
+}

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)

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")
 		})
 	})
 }

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()
 	}

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)