[remoting host][win] Delay IsUrlForwarderSetUp if user not logged in

It appears that in uncurtained session, WTS creates a single console
session whenever the login screen is shown, and the same session will
be used after the user has signed in. This is problematic for the
client to determine the URL forwarder state when the user has not
logged in, since:

* The host can't determine the state when the user is in login screen
  due to the lack of the user context
* DesktopSessionAgent isn't recreated after the user is logged in, so
  it won't report the URL forwarder state at that point where the user
  context becomes known

This CL prevents this issue by making the IsUrlForwarderSetUp call wait
for the user to log in. In curtain mode, a new session will be created
when the user logs in, so this logic isn't very helpful for that case,
but it still prevents the desktop process from reporting NOT_SET_UP
when the user context is not known; the relaunched desktop process will
report the correct state.

This will also require client side change that removes the timeout and
hides the checkbox when the IsUrlForwarderSetUp response is pending.

Bug: b:183135000
Change-Id: Ic492e0d8361d9d9ae77f1032c7f964bc5f182bd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3108516
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#914936}

remoting/host/url_forwarder_configurator_win.cc

@@ -24,7 +24,10 @@
 #include "base/task/thread_pool.h"
 #include "base/time/time.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/windows_types.h"
+#include "remoting/base/logging.h"
 #include "remoting/host/switches.h"
+#include "remoting/host/win/wts_session_change_observer.h"
 
 namespace remoting {
 
@@ -48,13 +51,11 @@ base::win::ScopedHandle GetCurrentUserToken() {
 
 // If |switch_name| is empty, the process will be launched with no extra
 // switches.
-bool LaunchConfiguratorProcess(const std::string& switch_name = std::string()) {
+bool LaunchConfiguratorProcess(const std::string& switch_name,
+                               base::win::ScopedHandle user_token) {
+  DCHECK(user_token.IsValid());
   base::LaunchOptions launch_options;
-  auto current_user = GetCurrentUserToken();
-  if (!current_user.IsValid()) {
-    return false;
-  }
-  launch_options.as_user = current_user.Get();
+  launch_options.as_user = user_token.Get();
   // The remoting_desktop.exe binary (where this code runs) has extra manifest
   // flags (uiAccess and requireAdministrator) that are undesirable for the
   // url_forwarder_configurator child process, so remoting_host.exe is used
@@ -90,9 +91,38 @@ void UrlForwarderConfiguratorWin::IsUrlForwarderSetUp(
     IsUrlForwarderSetUpCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  io_task_runner_->PostTaskAndReplyWithResult(
-      FROM_HERE, base::BindOnce(&LaunchConfiguratorProcess, std::string()),
-      std::move(callback));
+  auto user_token = GetCurrentUserToken();
+  if (user_token.IsValid()) {
+    io_task_runner_->PostTaskAndReplyWithResult(
+        FROM_HERE,
+        base::BindOnce(&LaunchConfiguratorProcess, std::string(),
+                       std::move(user_token)),
+        std::move(callback));
+    return;
+  }
+
+  if (GetLastError() != ERROR_NO_TOKEN) {
+    std::move(callback).Run(false);
+    return;
+  }
+
+  if (is_url_forwarder_set_up_callback_) {
+    LOG(ERROR) << "Query is already in progress.";
+    std::move(callback).Run(false);
+    return;
+  }
+  DCHECK(!wts_session_change_observer_);
+
+  HOST_LOG << "Can't determine URL Forwarder state as no user is signed in. "
+           << "Will check again after user signs in.";
+  is_url_forwarder_set_up_callback_ = std::move(callback);
+  wts_session_change_observer_ = std::make_unique<WtsSessionChangeObserver>();
+  bool start_success = wts_session_change_observer_->Start(
+      base::BindRepeating(&UrlForwarderConfiguratorWin::OnWtsSessionChange,
+                          base::Unretained(this)));
+  if (!start_success) {
+    std::move(is_url_forwarder_set_up_callback_).Run(false);
+  }
 }
 
 void UrlForwarderConfiguratorWin::SetUpUrlForwarder(
@@ -109,9 +139,15 @@ void UrlForwarderConfiguratorWin::SetUpUrlForwarder(
   report_user_intervention_required_timer_.Start(
       FROM_HERE, kReportUserInterventionRequiredDelay, this,
       &UrlForwarderConfiguratorWin::OnReportUserInterventionRequired);
+  auto user_token = GetCurrentUserToken();
+  if (!user_token.IsValid()) {
+    OnSetUpResponse(false);
+    return;
+  }
   io_task_runner_->PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&LaunchConfiguratorProcess, kSetUpUrlForwarderSwitchName),
+      base::BindOnce(&LaunchConfiguratorProcess, kSetUpUrlForwarderSwitchName,
+                     std::move(user_token)),
       base::BindOnce(&UrlForwarderConfiguratorWin::OnSetUpResponse,
                      weak_factory_.GetWeakPtr()));
 }
@@ -133,6 +169,30 @@ void UrlForwarderConfiguratorWin::OnReportUserInterventionRequired() {
       SetUpUrlForwarderResponse::USER_INTERVENTION_REQUIRED);
 }
 
+void UrlForwarderConfiguratorWin::OnWtsSessionChange(uint32_t event,
+                                                     uint32_t session_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (event != WTS_SESSION_LOGON) {
+    return;
+  }
+
+  DCHECK(is_url_forwarder_set_up_callback_);
+
+  HOST_LOG << "User logged in. Checking URL forwarder setup state now.";
+  auto user_token = GetCurrentUserToken();
+  if (!user_token.IsValid()) {
+    std::move(is_url_forwarder_set_up_callback_).Run(false);
+    return;
+  }
+  io_task_runner_->PostTaskAndReplyWithResult(
+      FROM_HERE,
+      base::BindOnce(&LaunchConfiguratorProcess, std::string(),
+                     std::move(user_token)),
+      std::move(is_url_forwarder_set_up_callback_));
+  wts_session_change_observer_.reset();
+}
+
 // static
 std::unique_ptr<UrlForwarderConfigurator> UrlForwarderConfigurator::Create() {
   return std::make_unique<UrlForwarderConfiguratorWin>();

remoting/host/url_forwarder_configurator_win.h

@@ -5,6 +5,9 @@
 #ifndef REMOTING_HOST_URL_FORWARDER_CONFIGURATOR_WIN_H_
 #define REMOTING_HOST_URL_FORWARDER_CONFIGURATOR_WIN_H_
 
+#include <stdint.h>
+#include <memory>
+
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
@@ -15,6 +18,8 @@
 
 namespace remoting {
 
+class WtsSessionChangeObserver;
+
 // Windows implementation of UrlForwarderConfigurator.
 // Note that this class requires elevated privileges.
 class UrlForwarderConfiguratorWin final : public UrlForwarderConfigurator {
@@ -32,10 +37,26 @@ class UrlForwarderConfiguratorWin final : public UrlForwarderConfigurator {
  private:
   void OnSetUpResponse(bool success);
   void OnReportUserInterventionRequired();
+  void OnWtsSessionChange(uint32_t event, uint32_t session_id);
 
   SEQUENCE_CHECKER(sequence_checker_);
 
   scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
+
+  // For uncurtained sessions, Windows reuses the same console session before
+  // and after the user logs in. So we use these members to delay checking the
+  // URL forwarder state until the user is logged in. They are valid (non-null)
+  // only when the session is not logged in.
+  // For curtained sessions, Windows creates a new session whenever the user
+  // logs in, in which case the desktop process will be destroyed along with the
+  // URL configurator and the pending requests, and a new desktop process will
+  // be launched and IsUrlForwarderSetUp() will be called with a valid user
+  // context.
+  std::unique_ptr<WtsSessionChangeObserver> wts_session_change_observer_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  IsUrlForwarderSetUpCallback is_url_forwarder_set_up_callback_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+
   SetUpUrlForwarderCallback set_up_url_forwarder_callback_
       GUARDED_BY_CONTEXT(sequence_checker_);
   base::OneShotTimer report_user_intervention_required_timer_

remoting/host/win/BUILD.gn

@@ -133,6 +133,8 @@ source_set("win") {
     "windows_event_logger.h",
     "worker_process_launcher.cc",
     "worker_process_launcher.h",
+    "wts_session_change_observer.cc",
+    "wts_session_change_observer.h",
     "wts_session_process_delegate.h",
     "wts_terminal_monitor.cc",
     "wts_terminal_monitor.h",

remoting/host/win/wts_session_change_observer.cc

@@ -0,0 +1,56 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/win/wts_session_change_observer.h"
+
+#include <windows.h>
+#include <winuser.h>
+#include <wtsapi32.h>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/sequence_checker.h"
+
+namespace remoting {
+
+WtsSessionChangeObserver::WtsSessionChangeObserver() = default;
+
+WtsSessionChangeObserver::~WtsSessionChangeObserver() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
+bool WtsSessionChangeObserver::Start(const SessionChangeCallback& callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!message_window_.Create(base::BindRepeating(
+          &WtsSessionChangeObserver::HandleMessage, base::Unretained(this)))) {
+    PLOG(ERROR) << "Failed to create the WTS session notification window";
+    return false;
+  }
+
+  if (!WTSRegisterSessionNotification(message_window_.hwnd(),
+                                      NOTIFY_FOR_THIS_SESSION)) {
+    PLOG(ERROR) << "Failed to register WTS session notification";
+    return false;
+  }
+  callback_ = callback;
+  return true;
+}
+
+bool WtsSessionChangeObserver::HandleMessage(UINT message,
+                                             WPARAM wparam,
+                                             LPARAM lparam,
+                                             LRESULT* result) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (message == WM_WTSSESSION_CHANGE) {
+    callback_.Run(wparam, lparam);
+    *result = 0;
+    return true;
+  }
+
+  return false;
+}
+
+}  // namespace remoting

remoting/host/win/wts_session_change_observer.h

@@ -0,0 +1,50 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_WIN_WTS_SESSION_CHANGE_OBSERVER_H_
+#define REMOTING_HOST_WIN_WTS_SESSION_CHANGE_OBSERVER_H_
+
+#include <stdint.h>
+
+#include "base/callback.h"
+#include "base/sequence_checker.h"
+#include "base/thread_annotations.h"
+#include "base/win/message_window.h"
+
+namespace remoting {
+
+// Helper class to observe WM_WTSSESSION_CHANGE.
+class WtsSessionChangeObserver final {
+ public:
+  using SessionChangeCallback =
+      base::RepeatingCallback<void(uint32_t event, uint32_t session_id)>;
+
+  WtsSessionChangeObserver();
+  ~WtsSessionChangeObserver();
+
+  // Starts observing and calls |callback| whenever a WM_WTSSESSION_CHANGE
+  // message is received. The callback will be discarded once |this| is
+  // destroyed.
+  // Returns true if the observer is started successfully, false otherwise.
+  bool Start(const SessionChangeCallback& callback);
+
+  WtsSessionChangeObserver(const WtsSessionChangeObserver&) = delete;
+  WtsSessionChangeObserver& operator=(const WtsSessionChangeObserver&) = delete;
+
+ private:
+  bool HandleMessage(UINT message,
+                     WPARAM wparam,
+                     LPARAM lparam,
+                     LRESULT* result);
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  base::win::MessageWindow message_window_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  SessionChangeCallback callback_ GUARDED_BY_CONTEXT(sequence_checker_);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_WIN_WTS_SESSION_CHANGE_OBSERVER_H_