Issue 6602 - Introduce watchextensions

watchextensions.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 """
 Track differences between versions of arbitrary extensions.
 
 Running this module direcly will attempt to fetch new versions for every
 in ~/extwatcher.ini or /etc/extwatcher.ini configured and enabled extension
 from the Chrome Web Store. When no parameters are given, you will simply be
 notified about whether a new version was found.
 
 Providing -k/--keep-repository will leave the git repository (with unstaged
 differences between the latest tracked version and the newest) on your drive,
 in order to examine the differences locally.
 
 Providing -p/--push will stage all differences and push them to the configured
 tracking repository.
-
-When the configuration files are incomplete or missing, a ConfigurationError

[Vasily Kuznetsov, 2018/05/15 16:49:31]

Nit: this line is probably not very useful for the end user of the script, it is
still printed with other help though. I'd probably remove it, because for the
developer who works on this the purpose of ConfigurationError will also be
obvious enough.

[tlucas, 2018/05/16 09:52:38]

Acknowledged.

-will be raised.
 """
 from __future__ import print_function
 
 import argparse
 from configparser import ConfigParser, NoSectionError, NoOptionError
 import json
 import logging
 import os
 import shutil
 import subprocess
+import sys
 import tempfile
 from zipfile import ZipFile
 
 try:  # Python 3 only
     from urllib import request as urllib
 except ImportError:  # Python 2 only
     import urllib
 from xml.etree import ElementTree
 
 logging.basicConfig(level=logging.INFO)
 
+OMAHA_URL = 'https://omahaproxy.appspot.com/all.json?os=win'
+SERVICE_URL = 'https://clients2.google.com/service/update2/crx'
+
 
 class SensitiveFilter(logging.Filter):

[Vasily Kuznetsov, 2018/05/15 16:49:32]

If I understand it correctly, the purpose of this class is to prevent
security-sensitive data from the repository string in the the config from
getting into the logs. First of all, it takes a bit of time to figure this out
so perhaps it would be good to document the class. Second: could we perhaps do
it with less code somehow. What if we just don't include the full repository
string into error messages, etc. Could this be simpler?

[tlucas, 2018/05/16 09:52:38]

You are right about the purpose - I added a documentation for this class.

However, the amount of code is - from what i see - as minimal as it can be (FWIW
i could get rid of self.mask() and put the logic in self.filter(), but that's
about it i guess - if we don't want to lose ALL information, that is.)

[Vasily Kuznetsov, 2018/05/16 17:54:55]

I was thinking about avoiding this filter class altogether and making sure that
nothing sensitive ends up in the log by just not writing it into the log in the
first place. I'm not sure if this would be easier, but I trust that you can
figure it out :)

In any case, as it is now, the code is easy enough to follow so it's not a big
deal.

+    """Filter sensitive ouput from the logs.
+
+    This class is meant to be registered in python's logging facility. It will
+    mask potentially sensitive data from the message that is to be logged.
+
+
+    """
+
     def __init__(self, patterns):
+        """Initialize a SensitiveFilter with the desired patterns."""
         self.patterns = patterns
         super(SensitiveFilter, self).__init__()
 
     def filter(self, record):
         msg = record.msg
         if isinstance(msg, BaseException):
             msg = str(msg)
         record.msg = self.mask(msg)
         return True
 
     def mask(self, msg):
         try:
             for pattern in self.patterns:
                 msg = msg.replace(
                     pattern,
                     '/'.join(['******', pattern.rsplit('/', 1)[-1]]),
                 )
         except AttributeError:
             pass
         return msg
 
 
+class ConfigurationError(Exception):
+    pass
+
+
 def read_config(path=None):
     config_path = path or [
         os.path.expanduser('~/watchextensions.ini'),
         '/etc/watchextensions.ini',
     ]
 
     config = ConfigParser()
     if not config.read(config_path):
         raise ConfigurationError('No configuration file was found. Please '
                                  'provide ~/watchextensions.ini or '
                                  '/etc/watchextensions.ini or specify a valid '
                                  'path.')
     return config
-
-
-class ConfigurationError(Exception):

[Vasily Kuznetsov, 2018/05/15 16:49:31]

Nit: moving this to above `read_config` would make the whole thing read a bit
more naturally. 

[tlucas, 2018/05/16 09:52:37]

Done.

-    pass
 
 
 class ExtWatcher(object):
     section = 'extensions'
 
     def __init__(self, ext_name, config, push, keep_repo):
         self.logger = logging.getLogger(name=ext_name)
         self.config = config
 
         self.ext_name = ext_name
         self.push = push
         self.keep_repo = keep_repo
 
         self._cws_ext_url = None
         self._current_ext_version = None
 
         self.downloaded_file = None
 
         super(ExtWatcher, self).__init__()
 
     def _git_cmd(self, cmds, relative=False):
         base = ['git']
         if not relative:
             base += ['-C', self.tempdir]
         suffix = []
         if not any(x in cmds for x in ['status', 'add', 'diff']):
             suffix += ['--quiet']
         return subprocess.check_output(base + list(cmds) + suffix)
 
-    def _assure_local_repository(self):

[Vasily Kuznetsov, 2018/05/15 16:49:31]

This name seems a bit strange. Shouldn't it be something like `_clone_repo`?

[tlucas, 2018/05/16 09:52:38]

You are right. "_assure_local_repository" is an artifact from when the script
was able to use existing repositories (and had to make sure that that repository
is actually what we wanted)

Renamed it.

+    def _clone_repository(self):
         self.logger.info('Cloning ' + self.repository)
         self._git_cmd(['clone', '-b', 'master', '--single-branch',
                        self.repository, self.tempdir],
                       relative=True)
 
     def _parse_config(self):
         err_msg = '"{}" is not fully configured!'.format(self.ext_name)
         try:
             self.ext_id = self.config.get(self.section, self.ext_name + '_id')
             self.repository = self.config.get(self.section,
@@ skipping 20 matching lines @@
             shutil.rmtree(self.tempdir, ignore_errors=True)
         else:
             print('Repository for {} available at {}'.format(self.ext_name,
                                                              self.tempdir))
         if self.downloaded_file:
             os.remove(self.downloaded_file)
 
         return True
 
     def _fetch_latest_chrome_version(self):
-        omaha_url = 'https://omahaproxy.appspot.com/all.json?os=win'

[Vasily Kuznetsov, 2018/05/15 16:49:31]

Maybe this should be a constant somewhere at the top of the file?

[tlucas, 2018/05/16 09:52:38]

Done.

-        response = urllib.urlopen(omaha_url).read()
+        response = urllib.urlopen(OMAHA_URL).read()
 
         data = json.loads(response.decode('utf-8'))
 
         stable = [x for x in data[0]['versions'] if x['channel'] == 'stable']
         return stable[0]['current_version']
 
     @property
     def cws_ext_url(self):
         if not self._cws_ext_url:

[Vasily Kuznetsov, 2018/05/15 16:49:31]

You could just slap a `functools.lru_cache` decorator on this instead of caching
it yourself... but, Python 2 doesn't have it :(

[tlucas, 2018/05/16 09:52:38]

I'll use this comment to also reply to what you asked for in your mail's body:

No, there is no particular reason (from my side) to support both versions, i'm
merely following our coding-style for python.
I guess until Python 2.7's end of life this won't change, so FWIW this is / was
also a good practice to assure compatibility.

-            service_url = 'https://clients2.google.com/service/update2/crx'
-            ext_url = service_url + '?prodversion={}&x=id%3D{}%26uc'.format(
+            ext_url = SERVICE_URL + '?prodversion={}&x=id%3D{}%26uc'.format(
                 self._fetch_latest_chrome_version(), self.ext_id,
             )
             self._cws_ext_url = ext_url
 
         return self._cws_ext_url
 
     @property
     def current_ext_version(self):
         if not self._current_ext_version:
             updatecheck_url = self.cws_ext_url + '&response=updatecheck'
@@ skipping 24 matching lines @@
 
     def _track_new_contents(self, message):
         status = self._git_cmd(['status'])
         if b'nothing to commit' not in status:
             self._git_cmd(['add', '--all'])
             self._git_cmd(['commit', '-m', message])
             self._git_cmd(['push', 'origin', 'master'])
 
     def run(self):
         self._parse_config()
-        self._assure_local_repository()
+        self._clone_repository()
 
         next_version = self.current_ext_version
         if next_version not in self._get_tracked_version():
             self.logger.info('New untracked version {} found!'
                              ''.format(next_version))
             if self.push or self.keep_repo:
                 self._download_ext_crx()
                 self._unzip_to_repository()
             if self.push:
                 self._track_new_contents(next_version)
@@ skipping 20 matching lines @@
         logging.disable(logging.INFO)
 
     try:
         config = read_config(args.config_path)
 
         for ext_name in config.options('enabled'):
             with ExtWatcher(ext_name, config, args.push,
                             args.keep_repository) as watcher:
                 watcher.run()
     except ConfigurationError as e:
-        logging.error(e.message)

[Vasily Kuznetsov, 2018/05/15 16:49:31]

Perhaps `sys.exit(e.message)` would be more appropriate. In any case it would
make sense to return a non-zero exit code.

[tlucas, 2018/05/16 09:52:37]

Done.

+        sys.exit(e.message)