The user's consent to crash dumps reporting can now be set via the UI (Windows only). The checkbox …

remoting/host/plugin/daemon_controller_win.cc

Index: remoting/host/plugin/daemon_controller_win.cc
diff --git a/remoting/host/plugin/daemon_controller_win.cc b/remoting/host/plugin/daemon_controller_win.cc
index 5325c22b1d7a525d0d7ebdcebbcdb0801d0063a2..efecb74791258bbdc00daacb84a8a1bf9bd50534 100644
--- a/remoting/host/plugin/daemon_controller_win.cc
+++ b/remoting/host/plugin/daemon_controller_win.cc
@@ -27,6 +27,7 @@
 #include "base/win/windows_version.h"
 #include "remoting/base/scoped_sc_handle_win.h"
 #include "remoting/host/branding.h"
+#include "remoting/host/breakpad.h"
 #include "remoting/host/plugin/daemon_installer_win.h"
 
 // MIDL-generated declarations and definitions.
@@ -83,6 +84,9 @@ class DaemonControllerWin : public remoting::DaemonController {
   virtual void Stop(const CompletionCallback& done_callback) OVERRIDE;
   virtual void SetWindow(void* window_handle) OVERRIDE;
   virtual void GetVersion(const GetVersionCallback& done_callback) OVERRIDE;
+  virtual void GetUsageStatsConsent(
+      const GetUsageStatsConsentCallback& done_callback) OVERRIDE;
+  virtual void SetUsageStatsConsent(bool consent) OVERRIDE;
 
  private:
   // Activates an unprivileged instance of the daemon controller and caches it.
@@ -125,10 +129,14 @@ class DaemonControllerWin : public remoting::DaemonController {
   void DoStop(const CompletionCallback& done_callback);
   void DoSetWindow(void* window_handle);
   void DoGetVersion(const GetVersionCallback& callback);
+  void DoGetUsageStatsConsent(
+      const GetUsageStatsConsentCallback& callback);
+  void DoSetUsageStatsConsent(bool consent);
 
-  // |control_| holds a reference to an instance of the daemon controller
-  // to prevent a UAC prompt on every operation.
+  // |control_| and |control2_| hold references to an instance of the daemon
+  // controller to prevent a UAC prompt on every operation.
   ScopedComPtr<IDaemonControl> control_;
+  ScopedComPtr<IDaemonControl2> control2_;
 
   // True if |control_| holds a reference to an elevated instance of the daemon
   // controller.
@@ -258,6 +266,21 @@ void DaemonControllerWin::GetVersion(const GetVersionCallback& callback) {
                  base::Unretained(this), callback));
 }
 
+void DaemonControllerWin::GetUsageStatsConsent(
+    const GetUsageStatsConsentCallback& callback) {
+  worker_thread_.message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&DaemonControllerWin::DoGetUsageStatsConsent,
+                 base::Unretained(this), callback));
+}
+
+void DaemonControllerWin::SetUsageStatsConsent(bool consent) {
+  worker_thread_.message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&DaemonControllerWin::DoSetUsageStatsConsent,
+                 base::Unretained(this), consent));
+}
+
 HRESULT DaemonControllerWin::ActivateController() {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
@@ -273,6 +296,9 @@ HRESULT DaemonControllerWin::ActivateController() {
     if (FAILED(hr)) {
       return hr;
     }
+
+    // Ignore the error. IID_IDaemonControl2 is optional.
+    control_.QueryInterface(IID_IDaemonControl2, control2_.ReceiveVoid());
   }
 
   return S_OK;
@@ -302,6 +328,9 @@ HRESULT DaemonControllerWin::ActivateElevatedController() {
       return hr;
     }
 
+    // Ignore the error. IID_IDaemonControl2 is optional.
+    control_.QueryInterface(IID_IDaemonControl2, control2_.ReceiveVoid());

[Jamie, 2012/06/14 23:43:56]

Does this result in an elevated IDaemonControl2 instance? That seems bad if
we're saying that changing this setting should not require admin.

[alexeypa (please no reviews), 2012/06/19 23:27:29]

Yes, but the UAC prompt has been shown to the user. So it is legit. :-)
The way the code work is:

- The COM controller will run under identity of the caller unless elevation
happened.
- The UAC prompt is shown if elevation is requested. If the user agreed to if we
can do admin tasks until the timeout expires. Currently 15 minutes.
- Omaha sets permissions on ClientStateMeduim key where we store the flag so
that interactive users can change values. It means that any user can enable and
disable crash reporting. It is assumed that it is safe enough.

[Jamie, 2012/06/21 18:47:52]

My point is that if the code is designed to be runnable as non-admin, then we
should never run it as admin. Can the code be rewritten to ensure this?

[alexeypa (please no reviews), 2012/06/21 23:30:25]

The code was designed to be running as either non-admin or admin. non-admin
clients will have less functionality available because they will not be able to
do certain operations. It is all controlled by the ACLs at the end. The calling
code should not make decisions whether an operation will succeed or fail
depending on the its identity.

+
     // Note that we hold a reference to an elevated instance now.
     control_is_elevated_ = true;
 
@@ -320,6 +349,7 @@ void DaemonControllerWin::ReleaseController() {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
   control_.Release();
+  control2_.Release();
   release_timer_.reset();
   control_is_elevated_ = false;
 }
@@ -592,6 +622,45 @@ void DaemonControllerWin::DoGetVersion(const GetVersionCallback& callback) {
       string16(static_cast<BSTR>(version), version.Length())));
 }
 
+void DaemonControllerWin::DoGetUsageStatsConsent(
+    const GetUsageStatsConsentCallback& callback) {
+  DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
+
+  // Activate the Daemon Controller and see if it supports |IDaemonControl2|.
+  HRESULT hr = ActivateController();
+  if (FAILED(hr) || control2_.get() == NULL) {
+    callback.Run(false, false);

[Jamie, 2012/06/14 23:43:56]

There should be a mechanism for reporting "not implemented" and we should use it
here.

[alexeypa (please no reviews), 2012/06/19 23:27:29]

Done.

+    return;
+  }
+
+  // Get the recorded user's consent.
+  BOOL set_by_policy;
+  BOOL allowed;
+  hr = control2_->GetUsageStatsConsent(&set_by_policy, &allowed);
+  if (FAILED(hr)) {
+    callback.Run(false, false);
+    return;
+  }
+
+  callback.Run(!!set_by_policy, !!allowed);
+}
+
+void DaemonControllerWin::DoSetUsageStatsConsent(bool consent) {
+  DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
+
+  // Activate the Daemon Controller and see if it supports |IDaemonControl2|.
+  HRESULT hr = ActivateController();
+  if (FAILED(hr) || control2_.get() == NULL) {
+    return;

[Jamie, 2012/06/14 23:43:56]

It would be better to report failure to the caller, and propagate it to the
web-app, even if we just log an error for now.

[alexeypa (please no reviews), 2012/06/19 23:27:29]

Done.

+  }
+
+  // Record the user's consent.
+  hr = control2_->SetUsageStatsConsent(consent);
+  if (FAILED(hr)) {
+    return;
+  }
+}
+
 }  // namespace
 
 scoped_ptr<DaemonController> remoting::DaemonController::Create() {