From dd5051fa529e0935fe279279d39cb00cbf856c14 Mon Sep 17 00:00:00 2001
From: Edward Lemur <ehmaldonado@chromium.org>
Date: Wed, 8 Aug 2018 00:56:41 +0000
Subject: [PATCH] metrics: Deal with permission denied when writing
 metrics.cfg.

If we don't have permission to create the metrics.cfg file, print a
notice and disable metrics collection.

Bug: 870231
Change-Id: I784e988ed021daef0fb07c08f1da44718581b1b9
Reviewed-on: https://chromium-review.googlesource.com/1166322
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
---
 metrics.py            | 18 +++++++++++----
 tests/metrics_test.py | 53 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/metrics.py b/metrics.py
index 2bf999dcaf..c7edc365be 100644
--- a/metrics.py
+++ b/metrics.py
@@ -3,6 +3,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+from __future__ import print_function
+
 import contextlib
 import functools
 import json
@@ -28,8 +30,12 @@ DISABLE_METRICS_COLLECTION = os.environ.get('DEPOT_TOOLS_METRICS') == '0'
 DEFAULT_COUNTDOWN = 10
 
 INVALID_CONFIG_WARNING = (
-    'WARNING: Your metrics.cfg file was invalid or nonexistent. A new one has '
-    'been created'
+    'WARNING: Your metrics.cfg file was invalid or nonexistent. A new one will '
+    'been created.'
+)
+PERMISSION_DENIED_WARNING = (
+    'Could not write the metrics collection config:\n\t%s\n'
+    'Metrics collection will be disabled.'
 )
 
 
@@ -65,13 +71,17 @@ class _Config(object):
     self._config.setdefault('opt-in', None)
 
     if config != self._config:
-      print INVALID_CONFIG_WARNING
+      print(INVALID_CONFIG_WARNING, file=sys.stderr)
       self._write_config()
 
     self._initialized = True
 
   def _write_config(self):
-    gclient_utils.FileWrite(CONFIG_FILE, json.dumps(self._config))
+    try:
+      gclient_utils.FileWrite(CONFIG_FILE, json.dumps(self._config))
+    except IOError as e:
+      print(PERMISSION_DENIED_WARNING % e, file=sys.stderr)
+      self._config['opt-in'] = False
 
   @property
   def is_googler(self):
diff --git a/tests/metrics_test.py b/tests/metrics_test.py
index 6627681eca..ab440473fc 100644
--- a/tests/metrics_test.py
+++ b/tests/metrics_test.py
@@ -72,6 +72,51 @@ class MetricsCollectorTest(unittest.TestCase):
 
     self.addCleanup(mock.patch.stopall)
 
+  def assert_writes_file(self, expected_filename, expected_content):
+    self.assertEqual(len(self.FileWrite.mock_calls), 1)
+    filename, content = self.FileWrite.mock_calls[0][1]
+
+    self.assertEqual(filename, expected_filename)
+    self.assertEqual(json.loads(content), expected_content)
+
+  def test_writes_config_if_not_exists(self):
+    self.FileRead.side_effect = [IOError(2, "No such file or directory")]
+    mock_response = mock.Mock()
+    self.urllib2.urlopen.side_effect = [mock_response]
+    mock_response.getcode.side_effect = [200]
+
+    self.assertTrue(self.collector.config.is_googler)
+    self.assertIsNone(self.collector.config.opted_in)
+    self.assertEqual(self.collector.config.countdown, 10)
+
+    self.assert_writes_file(
+        self.config_file, {'is-googler': True, 'countdown': 10, 'opt-in': None})
+
+  def test_writes_config_if_not_exists_non_googler(self):
+    self.FileRead.side_effect = [IOError(2, "No such file or directory")]
+    mock_response = mock.Mock()
+    self.urllib2.urlopen.side_effect = [mock_response]
+    mock_response.getcode.side_effect = [403]
+
+    self.assertFalse(self.collector.config.is_googler)
+    self.assertIsNone(self.collector.config.opted_in)
+    self.assertEqual(self.collector.config.countdown, 10)
+
+    self.assert_writes_file(
+        self.config_file,
+        {'is-googler': False, 'countdown': 10, 'opt-in': None})
+
+  def test_disables_metrics_if_cant_write_config(self):
+    self.FileRead.side_effect = [IOError(2, 'No such file or directory')]
+    mock_response = mock.Mock()
+    self.urllib2.urlopen.side_effect = [mock_response]
+    mock_response.getcode.side_effect = [200]
+    self.FileWrite.side_effect = [IOError(13, 'Permission denied.')]
+
+    self.assertTrue(self.collector.config.is_googler)
+    self.assertFalse(self.collector.config.opted_in)
+    self.assertEqual(self.collector.config.countdown, 10)
+
   def assert_collects_metrics(self, update_metrics=None):
     expected_metrics = self.default_metrics
     self.default_metrics.update(update_metrics or {})
@@ -357,12 +402,8 @@ class MetricsCollectorTest(unittest.TestCase):
     self.assertEqual(self.collector.config.countdown, 9)
     self.print_notice.assert_called_once_with(10)
 
-    self.assertEqual(len(self.FileWrite.mock_calls), 1)
-    config_file, config = self.FileWrite.mock_calls[0][1]
-
-    self.assertEqual(config_file, self.config_file)
-    self.assertEqual(json.loads(config),
-                     {'is-googler': True, 'countdown': 9, 'opt-in': None})
+    self.assert_writes_file(
+        self.config_file, {'is-googler': True, 'countdown': 9, 'opt-in': None})
 
   def test_nested_functions(self):
     """Tests that a function can call another function for which metrics are