[analysis_server] Support LSP executeCommand over DTD to support code actions

There are a few things in this CL that were difficult to separate testing of (because they're all part of the same feature):

1. sets `requiresTrustedCaller=false` on the `executeCommand` handler so that commands can be called over DTD. It adds the same flag to the command handlers themselves, so a command can control whether it can be called over DTD or not (right now we allow everything except `logAction` and `sendWorkspaceEdit` which are both commands used internally and not appropriate for DTD clients to call).

2. Removes the allow-list on DTD methods, allowing all LSP shared methods to be available over DTD

3. Extends the integration test classes to support reverse-requests so we can verify the edits being sent back to the editor when calling the code actions commands over DTD

4. It also fixes a few bugs where we read the callers capabilities instead of the editors capabilities (which until now would always be the same in those places, but with this change are not).

Change-Id: I6d271ddad6dc1b00a98b10b735763a368c91af7a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428784
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/handler/legacy/lsp_over_legacy_handler.dart

@@ -69,6 +69,7 @@ class LspOverLegacyHandler extends LegacyHandler {
       performance: performance,
       clientCapabilities: server.editorClientCapabilities,
       timeSinceRequest: request.timeSinceRequest,
+      isTrustedCaller: true,
     );
 
     ErrorOr<Object?> result;

pkg/analysis_server/lib/src/lsp/handlers/commands/apply_code_action.dart

@@ -33,6 +33,9 @@ class ApplyCodeActionCommandHandler
   @override
   bool get recordsOwnAnalytics => true;
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart

@@ -22,6 +22,9 @@ class FixAllCommandHandler extends SimpleEditCommandHandler<LspAnalysisServer> {
   @override
   String get commandName => 'Fix All';
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all_in_workspace.dart

@@ -24,6 +24,9 @@ abstract class AbstractFixAllInWorkspaceCommandHandler
   /// user can choose which changes to apply.
   bool get requireConfirmation;
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/log_action.dart

@@ -15,6 +15,10 @@ class LogActionCommandHandler
   @override
   bool get recordsOwnAnalytics => true;
 
+  @override
+  // We only currently expect this to be called by the editor.
+  bool get requiresTrustedCaller => true;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart

@@ -16,6 +16,9 @@ class OrganizeImportsCommandHandler extends SimpleEditCommandHandler {
   @override
   String get commandName => 'Organize Imports';
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart

@@ -28,6 +28,9 @@ class PerformRefactorCommandHandler extends AbstractRefactorCommandHandler {
   @override
   bool get recordsOwnAnalytics => true;
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   FutureOr<ErrorOr<void>> execute(
     String path,
@@ -40,6 +43,12 @@ class PerformRefactorCommandHandler extends AbstractRefactorCommandHandler {
     ProgressReporter reporter,
     int? docVersion,
   ) async {
+    var editorCapabilities = server.editorClientCapabilities;
+    if (editorCapabilities == null) {
+      // This should not happen unless a client misbehaves.
+      return serverNotInitializedError;
+    }
+
     var actionName = 'dart.refactor.${kind.toLowerCase()}';
     server.analyticsManager.executedCommand(actionName);
 
@@ -102,7 +111,7 @@ class PerformRefactorCommandHandler extends AbstractRefactorCommandHandler {
             return fileModifiedError;
           }
 
-          var edit = createWorkspaceEdit(server, clientCapabilities, change);
+          var edit = createWorkspaceEdit(server, editorCapabilities, change);
           return await sendWorkspaceEditToClient(edit);
         } finally {
           manager.end(cancelableToken);

pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler.dart

@@ -27,6 +27,9 @@ class RefactorCommandHandler extends SimpleEditCommandHandler<AnalysisServer> {
 
   RefactorCommandHandler(super.server, this.commandName, this.generator);
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart

@@ -22,6 +22,11 @@ class SendWorkspaceEditCommandHandler extends SimpleEditCommandHandler {
   @override
   String get commandName => 'Send Workspace Edit';
 
+  @override
+  // This command is used internally to work around some limitations in LSP
+  // and is not expected to ever be called by a DTD client.
+  bool get requiresTrustedCaller => true;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart

@@ -17,6 +17,9 @@ class SortMembersCommandHandler extends SimpleEditCommandHandler {
   @override
   String get commandName => 'Sort Members';
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   Future<ErrorOr<void>> handle(
     MessageInfo message,

pkg/analysis_server/lib/src/lsp/handlers/commands/validate_refactor.dart

@@ -22,6 +22,9 @@ class ValidateRefactorCommandHandler extends AbstractRefactorCommandHandler {
   @override
   bool get recordsOwnAnalytics => true;
 
+  @override
+  bool get requiresTrustedCaller => false;
+
   @override
   FutureOr<ErrorOr<ValidateRefactorResult>> execute(
     String path,

pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart

@@ -2,7 +2,9 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-import 'package:analysis_server/lsp_protocol/protocol.dart';
+import 'dart:async';
+
+import 'package:analysis_server/lsp_protocol/protocol.dart' hide MessageType;
 import 'package:analysis_server/src/analysis_server.dart';
 import 'package:analysis_server/src/lsp/constants.dart';
 import 'package:analysis_server/src/lsp/error_or.dart';
@@ -65,10 +67,9 @@ class ExecuteCommandHandler
       ExecuteCommandParams.jsonHandler;
 
   @override
-  // TODO(dantup): This will need to be relaxed to support calling over DTD,
-  //  but at that point we must also add this flag to Commands so that we can
-  //  control which commands require trusted callers.
-  bool get requiresTrustedCaller => true;
+  /// This handler does not require a trusted caller, however some of the
+  /// commands might (which are checked on the handler during execution).
+  bool get requiresTrustedCaller => false;
 
   @override
   Future<ErrorOr<Object?>> handle(
@@ -84,6 +85,13 @@ class ExecuteCommandHandler
       );
     }
 
+    if (handler.requiresTrustedCaller && !message.isTrustedCaller) {
+      return error(
+        ServerErrorCodes.UnknownCommand,
+        '${params.command} can only be called by the owning process',
+      );
+    }
+
     if (!handler.recordsOwnAnalytics) {
       server.analyticsManager.executedCommand(params.command);
     }

pkg/analysis_server/lib/src/lsp/handlers/handler_will_rename_files.dart

@@ -39,8 +39,10 @@ class WillRenameFilesHandler
     MessageInfo message,
     CancellationToken token,
   ) async {
-    var clientCapabilities = message.clientCapabilities;
-    if (clientCapabilities == null) {
+    // Use editor capabilities because this is used to compute to edits to send
+    // to the client.
+    var editorCapabilities = server.editorClientCapabilities;
+    if (editorCapabilities == null) {
       return serverNotInitializedError;
     }
 
@@ -61,11 +63,11 @@ class WillRenameFilesHandler
         pathMapping[oldPath] = newPath;
       });
     }
-    return _renameFiles(clientCapabilities, pathMapping, token);
+    return _renameFiles(editorCapabilities, pathMapping, token);
   }
 
   Future<ErrorOr<WorkspaceEdit?>> _renameFiles(
-    LspClientCapabilities clientCapabilities,
+    LspClientCapabilities editorCapabilities,
     Map<String, String> renames,
     CancellationToken token,
   ) async {
@@ -103,7 +105,7 @@ class WillRenameFilesHandler
 
     server.checkConsistency(sessions);
 
-    var edit = createWorkspaceEdit(server, clientCapabilities, change);
+    var edit = createWorkspaceEdit(server, editorCapabilities, change);
     return success(edit);
   }
 }

pkg/analysis_server/lib/src/lsp/handlers/handlers.dart

@@ -50,6 +50,10 @@ abstract class CommandHandler<P, R, S extends AnalysisServer>
   /// can record a more specific command name.
   bool get recordsOwnAnalytics => false;
 
+  /// Whether or not this handler can only be called by the owner of the
+  /// analysis server process (for example the editor).
+  bool get requiresTrustedCaller;
+
   Future<ErrorOr<Object?>> handle(
     MessageInfo message,
     Map<String, Object?> parameters,
@@ -455,13 +459,18 @@ class MessageInfo {
   /// shown.
   final Completer<void>? completer;
 
+  /// Whether or not this message came from the the owner of the analysis server
+  /// process (for example the editor).
+  final bool isTrustedCaller;
+
   MessageInfo({
     required this.performance,
     // TODO(dantup): Consider a version of this that has a non-nullable
     //  `LspClientCapabilities` since the majority of handlers first check this
     //  is non-null and reject the request. Only a small number of handlers need
     //  to run without, so it would remove a bunch of boilerplate in the others.
     required this.clientCapabilities,
+    required this.isTrustedCaller,
     this.timeSinceRequest,
     this.completer,
   });

pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart

@@ -509,6 +509,7 @@ class LspAnalysisServer extends AnalysisServer {
               clientCapabilities: editorClientCapabilities,
               timeSinceRequest: message.timeSinceRequest,
               completer: completer,
+              isTrustedCaller: true,
             );
 
             if (message is RequestMessage) {

pkg/analysis_server/lib/src/services/dart_tooling_daemon/dtd_services.dart

@@ -7,7 +7,6 @@ import 'dart:async';
 import 'package:analysis_server/lsp_protocol/protocol.dart';
 import 'package:analysis_server/src/analysis_server.dart';
 import 'package:analysis_server/src/lsp/client_capabilities.dart';
-import 'package:analysis_server/src/lsp/constants.dart';
 import 'package:analysis_server/src/lsp/error_or.dart';
 import 'package:analysis_server/src/lsp/handlers/handler_states.dart';
 import 'package:analysis_server/src/lsp/handlers/handlers.dart';
@@ -36,21 +35,6 @@ enum DtdConnectionState {
 /// A connection to DTD that exposes some analysis services (such as a subset
 /// of LSP) to other DTD clients.
 class DtdServices {
-  /// The set of LSP methods currently exposed over DTD.
-  ///
-  /// Eventually all "shared" methods will be exposed, but during initial
-  /// development and testing this will be restricted to selected methods (in
-  /// particular, those with well defined results that are not affected by
-  /// differences in client capabilities).
-  static final allowedLspMethods = <Method>{
-    // TODO(dantup): Remove this allow list so that all shared methods are
-    //  available over DTD.
-    CustomMethods.experimentalEcho,
-    CustomMethods.dartTextDocumentEditableArguments,
-    CustomMethods.dartTextDocumentEditArgument,
-    Method.textDocument_hover,
-  };
-
   /// The name of the DTD service that methods will be registered under.
   static const _lspServiceName = 'Lsp';
 
@@ -92,6 +76,7 @@ class DtdServices {
       // capabilities so that the responses don't change in format based on the
       // owning editor.
       clientCapabilities: fixedBasicLspClientCapabilities,
+      isTrustedCaller: false,
     );
     var token = NotCancelableToken(); // We don't currently support cancel.
 
@@ -230,8 +215,7 @@ class DtdServices {
   ) async {
     await Future.wait([
       for (var lspHandler in handler.messageHandlers.values)
-        if (allowedLspMethods.contains(lspHandler.handlesMessage) &&
-            (registerExperimentalHandlers || !lspHandler.isExperimental))
+        if (registerExperimentalHandlers || !lspHandler.isExperimental)
           _registerLspService(lspHandler, dtd),
     ]);
 
@@ -240,7 +224,8 @@ class DtdServices {
     await dtd.postEvent(_lspStreamName, 'initialized', {});
   }
 
-  /// Registers a single message handler to DTD if it allows untrusted callers.
+  /// Registers a single message handler to DTD only if it allows untrusted
+  /// callers.
   Future<void> _registerLspService(
     MessageHandler<Object?, Object?, AnalysisServer> messageHandler,
     DartToolingDaemon dtd,

pkg/analysis_server/lib/src/services/user_prompts/dart_fix_prompt_manager.dart

@@ -256,6 +256,7 @@ class DartFixPromptManager {
       MessageInfo(
         performance: OperationPerformanceImpl(''),
         clientCapabilities: clientCapabilities,
+        isTrustedCaller: true,
       ),
       NotCancelableToken(),
     );

pkg/analysis_server/test/integration/dtd/dtd_test.dart

@@ -33,6 +33,10 @@ class DtdTest
   Future<void> initializeServer() async {
     await standardAnalysisSetup();
     await analysisFinished;
+    await sendServerSetClientCapabilities(
+      [],
+      lspCapabilities: editorClientCapabilities.raw,
+    );
   }
 
   @override