Merge pull request #450 from ABCbum/develop

Use https and http appropriately when connecting to MusicBrainz

whipper/command/main.py

@@ -20,7 +20,13 @@ logger = logging.getLogger(__name__)
 
 def main():
     server = config.Config().get_musicbrainz_server()
-    musicbrainzngs.set_hostname(server)
+    https_enabled = server['scheme'] == 'https'
+    try:
+        musicbrainzngs.set_hostname(server['netloc'], https_enabled)
+    # Parameter 'use_https' is missing in versions of musicbrainzngs < 0.7
+    except TypeError as e:
+        logger.warning(e)
+        musicbrainzngs.set_hostname(server['netloc'])
 
     # Find whipper's plugins paths (local paths have higher priority)
     plugins_p = [directory.data_path('plugins')]  # local path (in $HOME)

whipper/common/config.py

@@ -75,11 +75,12 @@ class Config:
     # musicbrainz section
 
     def get_musicbrainz_server(self):
-        server = self.get('musicbrainz', 'server') or 'musicbrainz.org'
-        server_url = urlparse('//' + server)
-        if server_url.scheme != '' or server_url.path != '':
-            raise KeyError('Invalid MusicBrainz server: %s' % server)
-        return server
+        conf = self.get('musicbrainz', 'server') or 'https://musicbrainz.org'
+        if not conf.startswith(('http://', 'https://')):
+            raise KeyError('Invalid MusicBrainz server: unsupported '
+                           'or missing scheme')
+        scheme, netloc, _, _, _, _ = urlparse(conf)
+        return {'scheme': scheme, 'netloc': netloc}
 
     # drive sections
 

whipper/image/table.py

@@ -351,7 +351,7 @@ class Table:
         return disc.id
 
     def getMusicBrainzSubmitURL(self):
-        host = config.Config().get_musicbrainz_server()
+        serv = config.Config().get_musicbrainz_server()
 
         discid = self.getMusicBrainzDiscId()
         values = self._getMusicBrainzValues()
@@ -363,7 +363,7 @@ class Table:
         ])
 
         return urlunparse((
-            'https', host, '/cdtoc/attach', '', query, ''))
+            serv['scheme'], serv['netloc'], '/cdtoc/attach', '', query, ''))
 
     def getFrameLength(self, data=False):
         """

whipper/test/test_common_config.py

@@ -69,25 +69,25 @@ class ConfigTestCase(tcommon.TestCase):
 
     def test_get_musicbrainz_server(self):
         self.assertEqual(self._config.get_musicbrainz_server(),
-                         'musicbrainz.org',
+                         {'scheme': 'https', 'netloc': 'musicbrainz.org'},
                          msg='Default value is correct')
 
         self._config._parser.add_section('musicbrainz')
 
         self._config._parser.set('musicbrainz', 'server',
-                                 '192.168.2.141:5000')
+                                 'http://192.168.2.141:5000')
         self._config.write()
         self.assertEqual(self._config.get_musicbrainz_server(),
-                         '192.168.2.141:5000',
+                         {'scheme': 'http', 'netloc': '192.168.2.141:5000'},
                          msg='Correctly returns user-set value')
 
-        self._config._parser.set('musicbrainz', 'server',
-                                 '192.168.2.141:5000/hello/world')
+        # Test for unsupported scheme
+        self._config._parser.set('musicbrainz', 'server', 'ftp://example.com')
         self._config.write()
         self.assertRaises(KeyError, self._config.get_musicbrainz_server)
 
-        self._config._parser.set('musicbrainz', 'server',
-                                 'http://192.168.2.141:5000')
+        # Test for absent scheme
+        self._config._parser.set('musicbrainz', 'server', 'example.com')
         self._config.write()
         self.assertRaises(KeyError, self._config.get_musicbrainz_server)
 

whipper/test/test_image_table.py

@@ -1,7 +1,11 @@
 # -*- Mode: Python; test-case-name: whipper.test.test_image_table -*-
 # vi:si:et:sw=4:sts=4:ts=4
 
+from os import environ
+from shutil import rmtree
+from tempfile import mkdtemp
 from whipper.image import table
+from whipper.common import config
 
 from whipper.test import common as tcommon
 
@@ -58,8 +62,31 @@ class LadyhawkeTestCase(tcommon.TestCase):
         # 177832&tracks=12&id=KnpGsLhvH.lPrNc1PBL21lb9Bg4-
         # however, not (yet) in MusicBrainz database
 
+        # setup to test if MusicBrainz submit URL is hardcoded to use https
+        env_original = environ.get('XDG_CONFIG_HOME')
+        tmp_conf = mkdtemp(suffix='.config')
+        # HACK: hijack env var to avoid overwriting user's whipper config file
+        # This works because directory.config_path() builds the location where
+        # whipper's conf will reside based on the value of env XDG_CONFIG_HOME
+        environ['XDG_CONFIG_HOME'] = tmp_conf
+        self.config = config.Config()
+        self.config._parser.add_section('musicbrainz')
+        self.config._parser.set('musicbrainz', 'server',
+                                'http://musicbrainz.org')
+        self.config.write()
+        self.assertEqual(self.table.getMusicBrainzSubmitURL(),
+                         "http://musicbrainz.org/cdtoc/attach?toc=1+12+1958"
+                         "56+150+15687+31841+51016+66616+81352+99559+116070+13"
+                         "3243+149997+161710+177832&tracks=12&id=KnpGsLhvH.lPr"
+                         "Nc1PBL21lb9Bg4-")
+        # HACK: continuation - restore original env value (if defined)
+        if env_original is not None:
+            environ['XDG_CONFIG_HOME'] = env_original
+        else:
+            environ.pop('XDG_CONFIG_HOME', None)
         self.assertEqual(self.table.getMusicBrainzDiscId(),
                          "KnpGsLhvH.lPrNc1PBL21lb9Bg4-")
+        rmtree(tmp_conf)
 
     def testAccurateRip(self):
         self.assertEqual(self.table.accuraterip_ids(), (