Preference Pane for chromoting on Mac

remoting/host/me2me_preference_pane.mm

Index: remoting/host/me2me_preference_pane.mm
diff --git a/remoting/host/me2me_preference_pane.mm b/remoting/host/me2me_preference_pane.mm
new file mode 100644
index 0000000000000000000000000000000000000000..0c413403e2ea510be0f14b74db344df1817eb6a2
--- /dev/null
+++ b/remoting/host/me2me_preference_pane.mm
@@ -0,0 +1,444 @@
+// Copyright (c) 2012 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.
+
+#import <Cocoa/Cocoa.h>
+#include <launch.h>
+#import <PreferencePanes/PreferencePanes.h>
+#import <SecurityInterface/SFAuthorizationView.h>
+
+#include "base/eintr_wrapper.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/mac/authorization_util.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/launchd.h"
+#include "base/mac/mac_logging.h"
+#include "base/mac/scoped_launch_data.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/stringprintf.h"
+#include "base/sys_string_conversions.h"
+#include "remoting/host/host_config.h"
+#include "remoting/host/json_host_config.h"
+#include "remoting/protocol/me2me_host_authenticator_factory.h"
+
+namespace {
+// The name of the Remoting Host service that is registered with launchd.
+#define kServiceName "org.chromium.chromoting"
+#define kConfigDir "/Library/PrivilegedHelperTools/"
+
+// This helper script is executed as root.  It is passed a command-line option
+// (--enable or --disable), which causes it to create or remove a trigger file.
+// The trigger file (defined in the service's plist file) informs launchd
+// whether the Host service should be running.  Creating the trigger file causes
+// launchd to immediately start the service.  Deleting the trigger file has no
+// immediate effect, but it prevents the service from being restarted if it
+// becomes stopped.
+const char kHelperTool[] = kConfigDir kServiceName ".me2me.sh";
+
+bool GetTemporaryConfigFilePath(FilePath* path) {
+  if (!file_util::GetTempDir(path))
+    return false;
+  *path = path->Append(kServiceName ".json");
+  return true;
+}
+
+bool IsPinValid(const std::string& pin, const std::string& host_id,
+                const std::string& host_secret_hash) {
+  remoting::protocol::SharedSecretHash hash;
+  if (!hash.Parse(host_secret_hash)) {
+    LOG(ERROR) << "Invalid host_secret_hash.";
+    return false;
+  }
+  std::string result =
+      remoting::protocol::AuthenticationMethod::ApplyHashFunction(
+          hash.hash_function, host_id, pin);
+  return result == hash.value;
+}
+
+}  // namespace
+
+@interface Me2MePreferencePane : NSPreferencePane {
+  IBOutlet NSTextField* status_message_;
+  IBOutlet NSButton* disable_button_;
+  IBOutlet NSTextField* pin_instruction_message_;
+  IBOutlet NSTextField* email_;
+  IBOutlet NSTextField* pin_;
+  IBOutlet NSButton* apply_button_;
+  IBOutlet SFAuthorizationView* authorization_view_;
+
+  // Holds the new proposed configuration if a temporary config file is
+  // present.
+  scoped_ptr<remoting::JsonHostConfig> config_;
+
+  NSTimer* service_status_timer_;
+
+  // These flags determine the UI state.  These are computed in the
+  // update...Status methods.
+  BOOL is_service_running_;
+  BOOL is_pane_unlocked_;
+
+  // True if a new proposed config file has been loaded into memory.
+  BOOL have_new_config_;
+}
+
+- (void)mainViewDidLoad;
+- (void)willSelect;
+- (void)willUnselect;
+- (IBAction)onDisable:(id)sender;
+- (IBAction)onApply:(id)sender;
+- (void)onNewConfigFile:(NSNotification*)notification;
+- (void)refreshServiceStatus:(NSTimer*)timer;
+- (void)authorizationViewDidAuthorize:(SFAuthorizationView*)view;
+- (void)authorizationViewDidDeauthorize:(SFAuthorizationView*)view;
+- (void)updateServiceStatus;
+- (void)updateAuthorizationStatus;
+
+// Read any new config file if present.  If a config file is successfully read,
+// this deletes the file and keeps the config data loaded in memory.  If this
+// method is called a second time (when the file has been deleted), the current
+// config is remembered, so this method acts as a latch: it can change
+// |have_new_config_| from NO to YES, but never from YES to NO.
+//
+// This scheme means that this method can delete the file immediately (to avoid
+// leaving a stale file around in case of a crash), but this method can safely
+// be called multiple times without forgetting the loaded config.  To explicitly
+// forget the current config, set |have_new_config_| to NO.
+- (void)readNewConfig;
+
+// Update all UI controls according to any stored flags and loaded config.
+// This should be called after any sequence of operations that might change the
+// UI state.
+- (void)updateUI;
+
+// Alert the user to a generic error condition.
+- (void)showError;
+
+// Alert the user that the typed PIN is incorrect.
+- (void)showIncorrectPinMessage;
+
+// Save the new config to the system, and either start the service or inform
+// the currently-running service of the new config.
+- (void)applyNewServiceConfig;
+
+- (BOOL)runHelperAsRootWithCommand:(const char*)command
+                         inputData:(const std::string&)input_data;
+@end
+
+@implementation Me2MePreferencePane
+
+- (void)mainViewDidLoad {
+  [authorization_view_ setDelegate:self];
+  [authorization_view_ setString:kAuthorizationRightExecute];
+  [authorization_view_ setAutoupdate:YES];
+}
+
+- (void)willSelect {
+  have_new_config_ = NO;
+
+  NSDistributedNotificationCenter* center =
+      [NSDistributedNotificationCenter defaultCenter];
+  [center addObserver:self
+             selector:@selector(onNewConfigFile:)
+                 name:@kServiceName
+               object:nil];
+
+  service_status_timer_ =
+      [[NSTimer scheduledTimerWithTimeInterval:2.0

[Jamie, 2012/05/02 22:30:24]

How costly is the refresh operation? 2s is quite a short period of time if it
involves disk access, for example.

Longer term, could we avoid polling using another distributed notification?

[Lambros, 2012/05/03 01:47:04]

I don't think there's any disk access to worry about.  We talk to launchd over a
Unix domain socket, then ask it for the PID corresponding to a named service. 
That should be a lookup in a short list in memory.  And I don't see any extra
logging in the system console.
Possibly.  We could poll only when we're expecting the state to change shortly
(such as when the user hits "Disable" or "Apply").  Also, the service couldn't
guarantee to broadcast in the case of unclean shutdown, so we'd have to deal
with that anyway.

+                                        target:self
+                                      selector:@selector(refreshServiceStatus:)
+                                      userInfo:nil
+                                       repeats:YES] retain];
+  [self updateServiceStatus];
+  [self updateAuthorizationStatus];
+  [self readNewConfig];
+  [self updateUI];
+}
+
+- (void)willUnselect {
+  NSDistributedNotificationCenter* center =
+      [NSDistributedNotificationCenter defaultCenter];
+  [center removeObserver:self];
+
+  [service_status_timer_ invalidate];
+  [service_status_timer_ release];
+  service_status_timer_ = nil;
+}
+
+- (void)onApply:(id)sender {
+  if (!have_new_config_) {
+    // It shouldn't be possible to hit the button if there is no config to
+    // apply, but check anyway just in case it happens somehow.
+    return;
+  }
+
+  // Ensure the authorization token is up-to-date before using it.
+  [self updateAuthorizationStatus];
+  [self updateUI];
+
+  std::string pin = base::SysNSStringToUTF8([pin_ stringValue]);
+  std::string host_id, host_secret_hash;
+  if (!config_->GetString(remoting::kHostIdConfigPath, &host_id) ||
+      !config_->GetString(remoting::kHostSecretHashConfigPath,
+                          &host_secret_hash)) {
+    LOG(ERROR) << "Failed to get config data for PIN verification";
+    [self showError];
+    return;
+  }
+  if (!IsPinValid(pin, host_id, host_secret_hash)) {
+    [self showIncorrectPinMessage];
+    return;
+  }
+
+  [self applyNewServiceConfig];
+  [self updateUI];
+}
+
+- (void)onDisable:(id)sender {
+  // Ensure the authorization token is up-to-date before using it.
+  [self updateAuthorizationStatus];

[Jamie, 2012/05/02 22:30:24]

Can this fail? If so, shouldn't we return early?

[Lambros, 2012/05/03 01:47:04]

Done.

+  [self updateUI];
+
+  if (![self runHelperAsRootWithCommand:"--disable"
+                              inputData:""]) {
+    LOG(ERROR) << "Failed to run the helper tool";
+    [self showError];
+    return;
+  }
+
+  // Stop the launchd job.  This cannot easily be done by the helper tool,
+  // since the launchd job runs in the current user's context.
+  base::mac::ScopedLaunchData response(
+      base::mac::MessageForJob(kServiceName, LAUNCH_KEY_STOPJOB));
+  if (!response) {
+    LOG(ERROR) << "Failed to send message to launchd";
+    [self showError];
+    return;
+  }
+
+  // Expect a response of type LAUNCH_DATA_ERRNO.
+  launch_data_type_t type = launch_data_get_type(response.get());
+  if (type != LAUNCH_DATA_ERRNO) {
+    LOG(ERROR) << "launchd returned unexpected type: " << type;
+    [self showError];
+    return;
+  }
+
+  int error = launch_data_get_errno(response.get());
+  if (error) {
+    LOG(ERROR) << "launchd returned error: " << error;
+    [self showError];
+  }
+}
+
+- (void)onNewConfigFile:(NSNotification*)notification {
+  [self readNewConfig];
+  [self updateUI];
+}
+
+- (void)refreshServiceStatus:(NSTimer*)timer {
+  BOOL was_running = is_service_running_;
+  [self updateServiceStatus];
+  if (was_running != is_service_running_)
+    [self updateUI];
+}
+
+- (void)authorizationViewDidAuthorize:(SFAuthorizationView*)view {
+  [self updateAuthorizationStatus];
+  [self updateUI];
+}
+
+- (void)authorizationViewDidDeauthorize:(SFAuthorizationView*)view {
+  [self updateAuthorizationStatus];
+  [self updateUI];
+}
+
+- (void)updateServiceStatus {
+  pid_t job_pid = base::mac::PIDForJob(kServiceName);
+  is_service_running_ = (job_pid > 0);
+}
+
+- (void)updateAuthorizationStatus {
+  is_pane_unlocked_ = [authorization_view_ updateStatus:authorization_view_];
+}
+
+- (void)readNewConfig {
+  FilePath file;
+  if (!GetTemporaryConfigFilePath(&file)) {
+    LOG(ERROR) << "Failed to get path of configuration data.";
+    [self showError];
+    return;
+  }
+  if (!file_util::PathExists(file)) {
+    return;
+  }
+  scoped_ptr<remoting::JsonHostConfig> new_config_(
+      new remoting::JsonHostConfig(file));
+  if (new_config_->Read()) {
+    file_util::Delete(file, false);
+    config_.swap(new_config_);
+    have_new_config_ = YES;
+  } else {
+    // Report the error, because the file exists but couldn't be read.  The
+    // case of non-existence is normal and expected.
+    LOG(ERROR) << "Error reading configuration data from " << file.value();
+    [self showError];
+  }
+}
+
+- (void)updateUI {
+  if (is_service_running_) {
+    [status_message_ setStringValue:@"Me2Me is enabled"];

[Jamie, 2012/05/02 22:30:24]

Is this user-facing? If so then it should say Chrome Remote Desktop. Ideally it
should be branded if possible.

[Lambros, 2012/05/03 01:47:04]

Changed to Chrome Remote Desktop for now.  Needs to be localized as well as
branded, so maybe this can wait?

[Jamie, 2012/05/03 16:32:25]

It should just be an ifdef. If it's more complex than that then it can wait, but
if it's easy it might as well go in now.

[Lambros, 2012/05/03 18:24:30]

Done.

+  } else {
+    [status_message_ setStringValue:@"Me2Me is disabled"];
+  }
+
+  std::string email;
+  if (config_.get()) {
+    if (!config_->GetString(remoting::kXmppLoginConfigPath, &email)) {
+      LOG(ERROR) << "Failed to get config item:"
+                 << remoting::kXmppLoginConfigPath;

[Jamie, 2012/05/02 22:30:24]

No user-visible error here? Also, you've already started updating the UI here.
It would be better to validate the config first, before updating the UI. You can
delete if it it's invalid so that it only gets reported once.

[Lambros, 2012/05/03 01:47:04]

Done.

[Jamie, 2012/05/03 16:32:25]

Perhaps I'm missing something, but I think both of my points are still valid
with the new code.

[Lambros, 2012/05/03 18:24:30]

The new code moves the config validation out of updateUI, and into the
readNewConfig method.  So:

The config is thrown away if invalid, and not kept in |config_|.
The user is alerted in readNewConfig if an invalid config is read.
The config is validated "before updating the UI", as requested.
The invalid config is only reported once, and not every time updateUI is called.
The updateUI method assumes |config_| is either valid or nil, so the DCHECK is
appropriate when pulling out the strings.

I think this is better than moving the validation to the top of updateUI - it
means the config is either valid or nil, and this invariant holds independently
of when updateUI gets called.

+    }
+  }
+  [email_ setStringValue:base::SysUTF8ToNSString(email)];
+
+  [disable_button_ setEnabled:(is_pane_unlocked_ && is_service_running_)];
+  [pin_instruction_message_ setEnabled:have_new_config_];
+  [email_ setEnabled:have_new_config_];
+  [pin_ setEnabled:have_new_config_];
+  [apply_button_ setEnabled:(is_pane_unlocked_ && have_new_config_)];
+}
+
+- (void)showError {
+  NSAlert* alert = [[NSAlert alloc] init];
+  [alert setMessageText:@"An unexpected error occurred."];
+  [alert setInformativeText:@"Check the system log for more information."];
+  [alert setAlertStyle:NSWarningAlertStyle];
+  [alert beginSheetModalForWindow:[[self mainView] window]
+                    modalDelegate:nil
+                   didEndSelector:nil
+                      contextInfo:nil];
+  [alert release];
+}
+
+- (void)showIncorrectPinMessage {
+  NSAlert* alert = [[NSAlert alloc] init];
+  [alert setMessageText:@"Incorrect PIN entered."];
+  [alert setAlertStyle:NSWarningAlertStyle];
+  [alert beginSheetModalForWindow:[[self mainView] window]
+                    modalDelegate:nil
+                   didEndSelector:nil
+                      contextInfo:nil];
+  [alert release];
+}
+
+- (void)applyNewServiceConfig {
+  [self updateServiceStatus];
+  std::string serialized_config = config_->GetSerializedData();
+  const char* command = is_service_running_ ? "--save-config" : "--enable";
+  if (![self runHelperAsRootWithCommand:command
+                              inputData:serialized_config]) {
+    LOG(ERROR) << "Failed to run the helper tool";
+    [self showError];
+    return;
+  }
+  have_new_config_ = NO;
+
+  // If the service was previously running, send a signal to cause it to reload
+  // its configuration.
+  if (is_service_running_) {
+    pid_t job_pid = base::mac::PIDForJob(kServiceName);
+    if (job_pid > 0) {
+      kill(job_pid, SIGHUP);
+    } else {
+      LOG(ERROR) << "Failed to obtain PID of service " << kServiceName;
+      [self showError];
+    }
+  }
+}
+
+- (BOOL)runHelperAsRootWithCommand:(const char*)command
+                         inputData:(const std::string&)input_data {
+  if (!file_util::VerifyPathControlledByAdmin(FilePath(kHelperTool))) {
+    LOG(ERROR) << "Security check failed for: " << kHelperTool;
+    return NO;
+  }
+
+  AuthorizationRef authorization =
+      [[authorization_view_ authorization] authorizationRef];
+  if (!authorization) {
+    LOG(ERROR) << "Failed to obtain authorizationRef";
+    return NO;

[Jamie, 2012/05/02 22:30:24]

Will this show "An unexpected error occurred" if the user cancels the dialog?
That's not great UX, but it doesn't need fixing in this CL.

[Lambros, 2012/05/03 01:47:04]

It shouldn't do.  If the user cancels the elevation prompt, then
|is_pane_unlocked_| should remain false, and this method shouldn't get called.

+  }
+
+  const char* arguments[] = { command, NULL };
+  FILE* pipe = NULL;
+  pid_t pid;
+  OSStatus status = base::mac::ExecuteWithPrivilegesAndGetPID(
+      authorization,
+      kHelperTool,
+      kAuthorizationFlagDefaults,
+      arguments,
+      &pipe,
+      &pid);
+  if (status != errAuthorizationSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "AuthorizationExecuteWithPrivileges";
+    return NO;
+  }
+  if (pid == -1) {
+    LOG(ERROR) << "Failed to get child PID";
+    if (pipe)
+      fclose(pipe);

[Jamie, 2012/05/02 22:30:24]

Can this be merged with the general error-detection logic below so that there's
only one call to fclose?

If not, then please be consistent w.r.t. braces on single-line ifs.

[Lambros, 2012/05/03 01:47:04]

I couldn't see an easy way to do it.
Done

+    return NO;
+  }
+  if (!pipe) {
+    LOG(ERROR) << "Unexpected NULL pipe";
+    return NO;
+  }
+
+  // Some cleanup is needed (closing the pipe and waiting for the child
+  // process), so flag any errors before returning.
+  BOOL error = NO;
+
+  if (!input_data.empty()) {
+    size_t bytes_written = fwrite(input_data.data(), sizeof(char),
+                                  input_data.size(), pipe);
+    // According to the fwrite manpage, a partial count is returned only if a
+    // write error has occurred.
+    if (bytes_written != input_data.size()) {
+      LOG(ERROR) << "Failed to write data to child process";
+      error = YES;
+    }
+  }
+
+  // In all cases, fclose() should be called with the returned FILE*.  In the
+  // case of sending data to the child, this needs to be done before calling
+  // waitpid(), since the child reads until EOF on its stdin, so calling
+  // waitpid() first would result in deadlock.
+  if (fclose(pipe) != 0) {
+    PLOG(ERROR) << "fclose";
+    error = YES;
+  }
+
+  int exit_status;
+  pid_t wait_result = HANDLE_EINTR(waitpid(pid, &exit_status, 0));
+  if (wait_result != pid) {
+    PLOG(ERROR) << "waitpid";
+    error = YES;
+  }
+
+  // No more cleanup needed.
+  if (error) {
+    return NO;
+  }
+
+  if (WIFEXITED(exit_status) && WEXITSTATUS(exit_status) == 0) {
+    return YES;
+  } else {
+    LOG(ERROR) << kHelperTool << " failed with exit status " << exit_status;
+    return NO;
+  }
+}
+
+@end