Merge lp:~maxiberta/launchpad/snap-name-extraction into lp:launchpad

[Maximiliano Bertacchini]

Extract Snap.store_name from snapcraft.yaml (Git only).

Extract Snap.store_name from snapcraft.yaml (Git only).

[Maximiliano Bertacchini (maxiberta)]

Should mark https://code.launchpad.net/~cjwatson/launchpad/snap-store-add-edit-views/+merge/294524 as prerequisite? Will probably conflict (just a little); but I thought maybe this change is small enough to be merged directly to devel first.

[Colin Watson (cjwatson)]

You should set a prerequisite branch if and only if you actually branched from the unmerged branch in question; otherwise you will get very confusing results.

However, you really will need to redo this on top of snap-store-add-edit-views, I'm afraid. The reason is that Snap.name is unique per person, but the same name in snapcraft.yaml may be associated with a snap that exists in multiple series. Defaulting name from what's in snapcraft.yaml will therefore fill in an initial form value that fails validation in some cases, and we shouldn't do that. We must only use this to pick a default for Snap.store_name, not Snap.name.

review: Needs Resubmitting

[Colin Watson (cjwatson)]

review: Approve

[Maximiliano Bertacchini (maxiberta)]

Replied to inline comment. Thanks

[Celso Providelo (cprov)]

Thanks Maxi,

We have discussed alternatives for the mock/call-counting strategy for testing the fact we are using yaml.safe_load(). Turns out that actually checking a malicious yaml content can't cause any harm was too involving for this test. So I am happy with the current state.

We also mentioned that, theoretically, we should be able to warn users in the form about repositories missing sufficient (only name for now) "snapcraft.yaml" content and prevent future errors. However, what we have right now is more friendly to initial setups (empty repo, for instance)

review: Approve

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2016-05-27 16:04:55 +0000
+++ lib/lp/snappy/browser/snap.py	2016-05-28 00:35:28 +0000
@@ -25,6 +25,7 @@
     use_template,
     )
 from pymacaroons import Macaroon
+import yaml
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import Interface
@@ -52,12 +53,14 @@
     LaunchpadRadioWidget,
     )
 from lp.code.browser.widgets.gitref import GitRefWidget
+from lp.code.errors import GitRepositoryScanFault
 from lp.code.interfaces.gitref import IGitRef
 from lp.registry.enums import VCSType
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.features import getFeatureFlag
 from lp.services.helpers import english_list
 from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
+from lp.services.scripts import log
 from lp.services.webapp import (
     canonical_url,
     ContextMenu,
@@ -372,12 +375,32 @@
 
     @property
     def initial_values(self):
+        store_name = None
+        if self.has_snappy_distro_series and IGitRef.providedBy(self.context):
+            # Try to extract Snap store name from snapcraft.yaml file.
+            try:
+                blob = self.context.repository.getBlob(
+                    'snapcraft.yaml', self.context.name)
+                # Beware of unsafe yaml.load()!
+                store_name = yaml.safe_load(blob).get('name')
+            except GitRepositoryScanFault:
+                log.exception("Failed to get Snap manifest from Git %s",
+                              self.context.unique_name)
+            except (AttributeError, yaml.YAMLError):
+                # Ignore parsing errors from invalid, user-supplied YAML
+                pass
+            except Exception as e:
+                log.exception(
+                    "Failed to extract name from Snap manifest at Git %s: %s",
+                    self.context.unique_name, unicode(e))
+
         # XXX cjwatson 2015-09-18: Hack to ensure that we don't end up
         # accidentally selecting ubuntu-rtm/14.09 or similar.
         # ubuntu.currentseries will always be in BuildableDistroSeries.
         series = getUtility(ILaunchpadCelebrities).ubuntu.currentseries
         sds_set = getUtility(ISnappyDistroSeriesSet)
         return {
+            'store_name': store_name,
             'owner': self.user,
             'store_distro_series': sds_set.getByDistroSeries(series).first(),
             }

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2016-05-27 22:05:53 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2016-05-28 00:35:28 +0000
@@ -24,6 +24,7 @@
     HTTMock,
     )
 from mechanize import LinkNotFoundError
+import mock
 from pymacaroons import Macaroon
 import pytz
 import soupmatchers
@@ -40,6 +41,7 @@
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
+from lp.code.errors import GitRepositoryScanFault
 from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -73,6 +75,7 @@
     TestCaseWithFactory,
     time_counter,
     )
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -84,8 +87,8 @@
 from lp.testing.pages import (
     extract_text,
     find_main_content,
+    find_tag_by_id,
     find_tags_by_class,
-    find_tag_by_id,
     get_feedback_messages,
     )
 from lp.testing.publication import test_traverse
@@ -360,6 +363,41 @@
             }
         self.assertEqual(expected_args, parse_qs(parsed_location[3]))
 
+    def test_initial_name_extraction_git(self):
+        [git_ref] = self.factory.makeGitRefs()
+        git_ref.repository.getBlob = FakeMethod(result='name: test-snap')
+        view = create_initialized_view(git_ref, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertEqual('test-snap', initial_values['store_name'])
+
+    def test_initial_name_extraction_git_repo_error(self):
+        [git_ref] = self.factory.makeGitRefs()
+        git_ref.repository.getBlob = FakeMethod(failure=GitRepositoryScanFault)
+        view = create_initialized_view(git_ref, "+new-snap")
+        initial_values = view.initial_values
+        self.assertIn('store_name', initial_values)
+        self.assertIsNone(initial_values['store_name'])
+
+    def test_initial_name_extraction_git_invalid_data(self):
+        for invalid_result in (None, 123, '', '[][]', '#name:test', ']'):
+            [git_ref] = self.factory.makeGitRefs()
+            git_ref.repository.getBlob = FakeMethod(result=invalid_result)
+            view = create_initialized_view(git_ref, "+new-snap")
+            initial_values = view.initial_values
+            self.assertIn('store_name', initial_values)
+            self.assertIsNone(initial_values['store_name'])
+
+    def test_initial_name_extraction_git_safe_yaml(self):
+        [git_ref] = self.factory.makeGitRefs()
+        git_ref.repository.getBlob = FakeMethod(result='Malicious YAML!')
+        view = create_initialized_view(git_ref, "+new-snap")
+        with mock.patch('yaml.load') as unsafe_load:
+            with mock.patch('yaml.safe_load') as safe_load:
+                initial_values = view.initial_values
+        self.assertEqual(0, unsafe_load.call_count)
+        self.assertEqual(1, safe_load.call_count)
+
 
 class TestSnapAdminView(BrowserTestCase):
 

=== modified file 'setup.py'
--- setup.py	2016-05-11 11:23:41 +0000
+++ setup.py	2016-05-28 00:35:28 +0000
@@ -83,6 +83,7 @@
         'python-memcached',
         'python-openid',
         'pytz',
+        'PyYAML',
         'rabbitfixture',
         'requests',
         'requests-toolbelt',