Don't use auth.json in me2me_virtual_host.

remoting/tools/me2me_virtual_host.py

Index: remoting/tools/me2me_virtual_host.py
diff --git a/remoting/tools/me2me_virtual_host.py b/remoting/tools/me2me_virtual_host.py
index bdb483720400a3b646e89d0aa6137723aea696cc..e5402ef1307de2c33466ff04ca28e64a7be02512 100755
--- a/remoting/tools/me2me_virtual_host.py
+++ b/remoting/tools/me2me_virtual_host.py
@@ -74,12 +74,63 @@ os.environ["XAUTHORITY"] = X_AUTH_FILE
 g_desktops = []
 g_pidfile = None
 
+class Config:
+  def __init__(self, path):
+    self.path = path
+    self.data = {}
+    self.changed = False
+
+  def load(self):
+    try:
+      settings_file = open(self.path, 'r')
+      self.data = json.load(settings_file)
+      self.changed = False
+      settings_file.close()
+    except:
+      return False
+    return True
+
+  def save(self):
+    if not self.changed:
+      return True
+    try:
+      old_umask = os.umask(0066)
+      settings_file = open(self.path, 'w')
+      settings_file.write(json.dumps(self.data, indent=2))
+      settings_file.close()
+      os.umask(old_umask)
+    except:
+      return False
+    self.changed = False
+    return True
+
+  def copy_from(self, config):
+    """ Copy all parameters from |config|.
+    Args:
+      config: Config object to copy from
+    """
+    self.data.update(config.data)
+    self.changed = True
+
+  def get(self, key):
+    return self.data.get(key)
+
+  def __getitem__(self, key):
+    return self.data[key]
+
+  def __setitem__(self, key, value):
+    self.data[key] = value
+    self.changed = True
+
+  def __delitem__(self, key):
+    del self.data[key]
+    self.changed = True
 
 class Authentication:
   """Manage authentication tokens for Chromoting/xmpp"""
 
-  def __init__(self, config_file):
-    self.config_file = config_file
+  def __init__(self, config):
+    self.config = config
 
   def generate_tokens(self):
     """Prompt for username/password and use them to generate new authentication
@@ -102,29 +153,22 @@ class Authentication:
 
   def load_config(self):
     try:
-      settings_file = open(self.config_file, 'r')
-      data = json.load(settings_file)
-      settings_file.close()
-      self.login = data["xmpp_login"]
-      self.chromoting_auth_token = data["chromoting_auth_token"]
-      self.xmpp_auth_token = data["xmpp_auth_token"]
-    except:
+      self.login = self.config["xmpp_login"]
+      self.chromoting_auth_token = self.config["chromoting_auth_token"]
+      self.xmpp_auth_token = self.config["xmpp_auth_token"]
+    except KeyError:
       return False
     return True
 
   def save_config(self):
-    data = {
-        "xmpp_login": self.login,
-        "chromoting_auth_token": self.chromoting_auth_token,
-        "xmpp_auth_token": self.xmpp_auth_token,
-    }
-    # File will contain private keys, so deny read/write access to others.
-    old_umask = os.umask(0066)
-    settings_file = open(self.config_file, 'w')
-    settings_file.write(json.dumps(data, indent=2))
-    settings_file.close()
-    os.umask(old_umask)
+    self.config["xmpp_login"] = self.login
+    self.config["chromoting_auth_token"] = self.chromoting_auth_token
+    self.config["xmpp_auth_token"] = self.xmpp_auth_token
 
+  def clear_config(self):
+    del self.config["xmpp_login"]
+    del self.config["chromoting_auth_token"]
+    del self.config["xmpp_auth_token"]
 
 class Host:
   """This manages the configuration for a host.
@@ -145,14 +189,14 @@ class Host:
   server = 'www.googleapis.com'
   url = 'https://' + server + '/chromoting/v1/@me/hosts'
 
-  def __init__(self, config_file, auth):
+  def __init__(self, config, auth):
     """
     Args:
-      config_file: Host configuration file path
+      config: Host configuration object
       auth: Authentication object with credentials for authenticating with the
         Directory service.
     """
-    self.config_file = config_file
+    self.config = config
     self.auth = auth
     self.host_id = str(uuid.uuid1())
     self.host_name = socket.gethostname()
@@ -224,34 +268,25 @@ class Host:
 
   def load_config(self):

[Lambros, 2012/08/08 22:11:29]

Now that load_config no longer pulls data from a file, the semantics don't make
much sense.  It now looks as if this object has two internal copies of all its
state, and this method just copies one part to another part.  It would be better
if this method accepted a Config object as a parameter (instead of as a member
variable), and then the method "loads" data from the supplied Config object. 
Alternatively, have Config as a member variable (self.config), and don't copy
state - just have the fields "host_id", "host_name",... as getters (and setters)
that operate on self.config, or say that callers are allowed to operate on
self.config directly.

[Sergey Ulanov, 2012/08/08 22:41:03]

You can look at it as if self.config is just a reference to the config object
and not internal state of this object (and it's shared between auth and host
objects). But yes, I agree it's better to just pass the config to this method.
Done.

     try:
-      settings_file = open(self.config_file, 'r')
-      data = json.load(settings_file)
-      settings_file.close()
-    except:
-      logging.info("Failed to load: " + self.config_file)
+      self.host_id = self.config["host_id"]
+      self.host_name = self.config["host_name"]
+      self.host_secret_hash = self.config.get("host_secret_hash")
+      self.private_key = self.config["private_key"]
+    except KeyError:
       return False
-    self.host_id = data["host_id"]
-    self.host_name = data["host_name"]
-    self.host_secret_hash = data.get("host_secret_hash")
-    self.private_key = data["private_key"]
     return True
 
   def save_config(self):

[Lambros, 2012/08/08 22:11:29]

Same applies as for load_config.

[Sergey Ulanov, 2012/08/08 22:41:03]

Done.

-    data = {
-        "host_id": self.host_id,
-        "host_name": self.host_name,
-        "host_secret_hash": self.host_secret_hash,
-        "private_key": self.private_key,
-    }
-    if self.host_secret_hash:
-      data["host_secret_hash"] = self.host_secret_hash
-
-    old_umask = os.umask(0066)
-    settings_file = open(self.config_file, 'w')
-    settings_file.write(json.dumps(data, indent=2))
-    settings_file.close()
-    os.umask(old_umask)
+    self.config["host_id"] = self.host_id
+    self.config["host_name"] = self.host_name
+    self.config["host_secret_hash"] = self.host_secret_hash
+    self.config["private_key"] = self.private_key
 
+  def clear_config(self):

[Lambros, 2012/08/08 22:11:29]

Similar thing here.  clear_config operates on self.config, but not on
self.host_id etc - so you have the paradox where self appears to have valid
config data, but self.config is cleared out.

[Sergey Ulanov, 2012/08/08 22:41:03]

Moved this function to Config class.

+    del self.config["host_id"]
+    del self.config["host_name"]
+    del self.config["host_secret_hash"]
+    del self.config["private_key"]
 
 class Desktop:
   """Manage a single virtual desktop"""
@@ -383,9 +418,7 @@ class Desktop:
   def launch_host(self, host):
     # Start remoting host
     args = [locate_executable(REMOTING_COMMAND),
-            "--host-config=%s" % (host.config_file)]
-    if host.auth.config_file != host.config_file:
-      args.append("--auth-config=%s" % (host.auth.config_file))
+            "--host-config=%s" % (host.config.path)]
     self.host_proc = subprocess.Popen(args, env=self.child_env)
     logging.info(args)
     if not self.host_proc.pid:
@@ -702,20 +735,20 @@ def main():
   if not os.path.exists(CONFIG_DIR):
     os.makedirs(CONFIG_DIR, mode=0700)
 
-  host_config_file = os.path.join(CONFIG_DIR, "host#%s.json" % host_hash)
+  host_config = Config(os.path.join(CONFIG_DIR, "host#%s.json" % host_hash))
+  host_config.load()
 
-  # --silent option is specified when we are started from WebApp UI. Don't use
-  # separate auth file in that case.
-  # TODO(sergeyu): Always use host config for auth parameters.
-  if options.silent:
-    auth_config_file = host_config_file
-  else:
-    auth_config_file = os.path.join(CONFIG_DIR, "auth.json")
-
-  auth = Authentication(auth_config_file)
+  auth = Authentication(host_config)
   auth_config_loaded = auth.load_config()
-
-  host = Host(host_config_file, auth)
+  if not auth_config_loaded and not options.silent:
+    # If we failed to load authentication parameters from the host config
+    # then try loading them from the legacy auth.json file.
+    auth_config = Config(os.path.join(CONFIG_DIR, "auth.json"))
+    if auth_config.load():
+      host_config.copy_from(auth_config)
+      auth_config_loaded = auth.load_config()
+
+  host = Host(host_config, auth)
   host_config_loaded = host.load_config()
 
   if options.silent:
@@ -741,33 +774,37 @@ def main():
     # previously-saved auth tokens (from a previous run of this script), which
     # may require re-prompting for username & password.
     while True:
-      try:
-        if need_auth_tokens:
+      if need_auth_tokens:
+        try:
           auth.generate_tokens()
           auth.save_config()

[Lambros, 2012/08/08 22:11:29]

Since save_config() no longer writes to disk, this is a change of behavior if
you get to "return 1" below "need_register_host:" - the auth data is no longer
saved in this case.  It's not a big deal, but it would be nice if we could save
the auth data if this happens, so the user doesn't have to re-enter their
details.

[Sergey Ulanov, 2012/08/08 22:41:03]

Done.

           need_auth_tokens = False
-      except Exception:
-        logging.error("Authentication failed")
-        return 1
+        except Exception:
+          logging.error("Authentication failed")
+          return 1
 
-      try:
-        if need_register_host:
+      if need_register_host:
+        try:
           host.register()
           host.save_config()
-      except urllib2.HTTPError, err:
-        if err.getcode() == 401:
-          # Authentication failed - re-prompt for username & password.
-          need_auth_tokens = True
-          continue
-        else:
-          # Not an authentication error.
-          logging.error("Directory returned error: " + str(err))
-          logging.error(err.read())
-          return 1
+        except urllib2.HTTPError, err:
+          if err.getcode() == 401:
+            # Authentication failed - re-prompt for username & password.
+            need_auth_tokens = True
+            continue
+          else:
+            # Not an authentication error.
+            logging.error("Directory returned error: " + str(err))
+            logging.error(err.read())
+            return 1
 
       # |auth| and |host| are both set up, so break out of the loop.
       break
 
+    if not host_config.save():
+      logging.error("Faled to save host configuration.")
+      return 1
+
   global g_pidfile
   g_pidfile = PidFile(pid_filename)
   running, pid = g_pidfile.check()
@@ -866,25 +903,19 @@ def main():
       # will be created and registered.
       if os.WEXITSTATUS(status) == 2:
         logging.info("Host configuration is invalid - exiting.")
-        try:
-          os.remove(host.config_file)
-          os.remove(auth.config_file)
-        except:
-          pass
+        auth.clear_config()
+        host.clear_config()
+        host_config.save()
         return 0
       elif os.WEXITSTATUS(status) == 3:
         logging.info("Host ID has been deleted - exiting.")
-        try:
-          os.remove(host.config_file)
-        except:
-          pass
+        host.clear_config()
+        host_config.save()
         return 0
       elif os.WEXITSTATUS(status) == 4:
         logging.info("OAuth credentials are invalid - exiting.")
-        try:
-          os.remove(auth.config_file)
-        except:
-          pass
+        auth.clear_config()
+        host_config.save()
         return 0
       elif os.WEXITSTATUS(status) == 5:
         logging.info("Host domain is blocked by policy - exiting.")