Merge lp:~cprov/launchpad/snap-privacy-take1 into lp:launchpad

[Celso Providelo]

Initial Snap.private implications.

Initial Snap.private implications.

Snaps implement privacy orthogonally to their contents or ownership, although snaps for private branches or owned by private teams must remain private.

Members of private teams or owners of private branches already create private snaps, however privatising existing snaps for public code requires Commercial/Admin permission.

[Colin Watson (cjwatson)]

Looks like a pretty good start, with a few bits missing. You need to make Snap.requestBuild only permit private archives if the snap itself is private, although I think we ought to keep the snap/archive owner mismatch check as well, at least for now. I don't think SnapBuild privacy needs any other work beyond that, as it already correctly delegates to the snap and the archive.

Last I checked, there were no snaps on production with private owners or snap builds with private archives, but we should double-check that after the upgrade and ensure that they're all explicitly marked private if need be. If you follow my suggestion for SnapBuild.is_private then I think this ordering is safe.

review: Needs Fixing

[Celso Providelo (cprov)]

Thanks Colin,

I understand and agree will all your comments, they will be addressed shortly.

[Celso Providelo (cprov)]

Colin,

There are still few issues that requires your help:

1) After checking the code behaviour, it seems to me that SnapBuilds for private archives will remain protected even if their parent Snap become public. So I don't see why ISnapSet.isValidPrivacy() would change. I am probably missing something.

2) Requiring lp.Admin for Snap.private uncovered an issue that I am not sure how to solve. Commercial_admins and ppa_admins cannot admin snaps owned by private teams, it endup allowing only LP admins to administrate private snaps. See `test_admin_snap_privacy_mismatch`.

3) Lastly, I am not happy with the code that inspect the privacy portlet (first_tag_by_class()). Is there anything better ?

[Colin Watson (cjwatson)]

On Thu, Feb 04, 2016 at 11:58:36AM -0000, Celso Providelo wrote:
> 1) After checking the code behaviour, it seems to me that SnapBuilds for private archives will remain protected even if their parent Snap become public. So I don't see why ISnapSet.isValidPrivacy() would change. I am probably missing something.

You're right; please disregard that comment on your isValidPrivacy
implementation. I was forgetting that the archive was an attribute of
the build, not the snap, so it isn't correct for us to constrain snap
privacy by archive privacy.

> 2) Requiring lp.Admin for Snap.private uncovered an issue that I am not sure how to solve. Commercial_admins and ppa_admins cannot admin snaps owned by private teams, it endup allowing only LP admins to administrate private snaps. See `test_admin_snap_privacy_mismatch`.

Commercial admins should be able to, because they can view any private
team (see ViewPublicOrPrivateTeamMembers.checkAuthenticated); but PPA
admins indeed won't be able to. I think that's fine and correct.

Are you sure you tried using commercial_admin in that test, not just
ppa_admin?

> 3) Lastly, I am not happy with the code that inspect the privacy portlet (first_tag_by_class()). Is there anything better ?

You have id="privacy" set on the div element at the top level of the
portlet, so I'd use find_tag_by_id(content, "privacy"). There are some
other tests that do this, albeit doctests: see for example
lib/lp/bugs/stories/bug-privacy/xx-presenting-private-bugs.txt.

[Celso Providelo (cprov)]

Colin,

Thanks for the suggestions, highly appreciated. Finally, this seems to have reached a good level of maturity.

Security adapters and tests make sense now, if you could take another look, perhaps playing with it locally, to make sure it matches our plans.

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2016-01-26 15:47:37 +0000
+++ lib/lp/registry/vocabularies.py	2016-02-04 13:01:27 +0000
@@ -1105,6 +1105,20 @@
         return SimpleTerm(obj, obj.name, obj.displayname)
 
 
+class AllUserTeamsParticipationPlusSelfSimpleDisplayVocabulary(
+    AllUserTeamsParticipationPlusSelfVocabulary):
+    """Like `AllUserTeamsParticipationPlusSelfVocabulary` but the term title is
+    the person.displayname rather than unique_displayname.
+
+    See `UserTeamsParticipationPlusSelfSimpleDisplayVocabulary` for more
+    information on usage.
+    """
+
+    def toTerm(self, obj):
+        """See `IVocabulary`."""
+        return SimpleTerm(obj, obj.name, obj.displayname)
+
+
 @implementer(IHugeVocabulary)
 class ProductReleaseVocabulary(SQLObjectVocabularyBase):
     """All `IProductRelease` objects vocabulary."""

=== modified file 'lib/lp/registry/vocabularies.zcml'
--- lib/lp/registry/vocabularies.zcml	2014-01-03 07:39:34 +0000
+++ lib/lp/registry/vocabularies.zcml	2016-02-04 13:01:27 +0000
@@ -328,6 +328,20 @@
     <allow interface="lp.services.webapp.vocabulary.IHugeVocabulary"/>
   </class>
 
+
+  <securedutility
+    name="AllUserTeamsParticipationPlusSelfSimpleDisplay"
+    component="lp.registry.vocabularies.AllUserTeamsParticipationPlusSelfSimpleDisplayVocabulary"
+    provides="zope.schema.interfaces.IVocabularyFactory"
+    >
+    <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+  </securedutility>
+
+  <class class="lp.registry.vocabularies.AllUserTeamsParticipationPlusSelfSimpleDisplayVocabulary">
+    <allow interface="lp.services.webapp.vocabulary.IHugeVocabulary"/>
+  </class>
+
+
   <securedutility
     name="ActiveMailingList"
     component="lp.registry.vocabularies.ActiveMailingListVocabulary"

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2016-01-26 15:47:37 +0000
+++ lib/lp/security.py	2016-02-04 13:01:27 +0000
@@ -3112,12 +3112,18 @@
             obj, obj.webhook, 'launchpad.View')
 
 
-class ViewSnap(DelegatedAuthorization):
+class ViewSnap(AuthorizationBase):
+    """Private snaps are only visible to their owners and admins."""
     permission = 'launchpad.View'
     usedfor = ISnap
 
-    def __init__(self, obj):
-        super(ViewSnap, self).__init__(obj, obj.owner, 'launchpad.View')
+    def checkUnauthenticated(self):
+        return not self.obj.private
+
+    def checkAuthenticated(self, user):
+        return (
+            not self.obj.private or
+            self.forwardCheckAuthenticated(user, self.obj.owner))
 
 
 class EditSnap(AuthorizationBase):

=== modified file 'lib/lp/snappy/browser/configure.zcml'
--- lib/lp/snappy/browser/configure.zcml	2015-09-18 13:32:09 +0000
+++ lib/lp/snappy/browser/configure.zcml	2016-02-04 13:01:27 +0000
@@ -31,6 +31,12 @@
             module="lp.snappy.browser.snap"
             classes="SnapNavigation" />
         <browser:page
+            for="lp.snappy.interfaces.snap.ISnap"
+            class="lp.snappy.browser.snap.SnapView"
+            permission="launchpad.View"
+            name="+portlet-privacy"
+            template="../templates/snap-portlet-privacy.pt"/>
+        <browser:page
             for="lp.code.interfaces.branch.IBranch"
             class="lp.snappy.browser.snap.SnapAddView"
             permission="launchpad.AnyPerson"

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2015-10-08 13:58:57 +0000
+++ lib/lp/snappy/browser/snap.py	2016-02-04 13:01:27 +0000
@@ -146,7 +146,7 @@
     def person_picker(self):
         field = copy_field(
             ISnap['owner'],
-            vocabularyName='UserTeamsParticipationPlusSelfSimpleDisplay')
+            vocabularyName='AllUserTeamsParticipationPlusSelfSimpleDisplay')
         return InlinePersonEditPickerWidget(
             self.context, field, format_link(self.context.owner),
             header='Change owner', step_title='Select a new owner')
@@ -273,6 +273,7 @@
     use_template(ISnap, include=[
         'owner',
         'name',
+        'private',
         'require_virtualized',
         ])
     distro_series = Choice(
@@ -321,9 +322,11 @@
             kwargs = {'git_ref': self.context}
         else:
             kwargs = {'branch': self.context}
+        private = not getUtility(
+            ISnapSet).isValidPrivacy(False, data['owner'], **kwargs)
         snap = getUtility(ISnapSet).new(
             self.user, data['owner'], data['distro_series'], data['name'],
-            **kwargs)
+            private=private, **kwargs)
         self.next_url = canonical_url(snap)
 
     def validate(self, data):
@@ -404,7 +407,20 @@
 
     page_title = 'Administer'
 
-    field_names = ['require_virtualized']
+    field_names = ['private', 'require_virtualized']
+
+    def validate(self, data):
+        super(SnapAdminView, self).validate(data)
+        private = data.get('private', None)
+        if private is not None:
+            if not getUtility(ISnapSet).isValidPrivacy(
+                    private, self.context.owner, self.context.branch,
+                    self.context.git_ref):
+                self.setFieldError(
+                    'private',
+                    u'This snap contains private information and cannot '
+                    u'be public.'
+                )
 
 
 class SnapEditView(BaseSnapEditView, EnableProcessorsMixin):

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2015-11-26 15:46:38 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2016-02-04 13:01:27 +0000
@@ -27,6 +27,7 @@
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
+from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.database.constants import UTC_NOW
@@ -63,6 +64,7 @@
     extract_text,
     find_main_content,
     find_tags_by_class,
+    find_tag_by_id,
     )
 from lp.testing.publication import test_traverse
 from lp.testing.views import (
@@ -197,6 +199,43 @@
             ["Test Person (test-person)", "Test Team (test-team)"],
             sorted(str(option) for option in options))
 
+    def test_create_new_snap_public(self):
+        # Public owner implies in public snap.
+        branch = self.factory.makeAnyBranch()
+
+        browser = self.getViewBrowser(
+            branch, view_name="+new-snap", user=self.person)
+        browser.getControl("Name").value = "public-snap"
+        browser.getControl("Create snap package").click()
+
+        content = find_main_content(browser.contents)
+        self.assertEqual("public-snap", extract_text(content.h1))
+        self.assertEqual(
+            'This snap contains Public information',
+            extract_text(find_tag_by_id(browser.contents, "privacy"))
+        )
+
+    def test_create_new_snap_private(self):
+        # Private teams will automatically create private snaps.
+        login_person(self.person)
+        self.factory.makeTeam(
+            name='super-private', owner=self.person,
+            visibility=PersonVisibility.PRIVATE)
+        branch = self.factory.makeAnyBranch()
+
+        browser = self.getViewBrowser(
+            branch, view_name="+new-snap", user=self.person)
+        browser.getControl("Name").value = "private-snap"
+        browser.getControl("Owner").value = ['super-private']
+        browser.getControl("Create snap package").click()
+
+        content = find_main_content(browser.contents)
+        self.assertEqual("private-snap", extract_text(content.h1))
+        self.assertEqual(
+            'This snap contains Private information',
+            extract_text(find_tag_by_id(browser.contents, "privacy"))
+        )
+
 
 class TestSnapAdminView(BrowserTestCase):
 
@@ -222,19 +261,43 @@
             user=self.person)
 
     def test_admin_snap(self):
-        # Admins can change require_virtualized.
+        # Admins can change require_virtualized and privacy.
         login("[e-mail]")
         ppa_admin = self.factory.makePerson(
             member_of=[getUtility(ILaunchpadCelebrities).ppa_admin])
         login_person(self.person)
         snap = self.factory.makeSnap(registrant=self.person)
         self.assertTrue(snap.require_virtualized)
+        self.assertFalse(snap.private)
+
         browser = self.getViewBrowser(snap, user=ppa_admin)
         browser.getLink("Administer snap package").click()
         browser.getControl("Require virtualized builders").selected = False
+        browser.getControl("Private").selected = True
         browser.getControl("Update snap package").click()
+
         login_person(self.person)
         self.assertFalse(snap.require_virtualized)
+        self.assertTrue(snap.private)
+
+    def test_admin_snap_privacy_mismatch(self):
+        # Cannot make snap public if it still contains private information.
+        login_person(self.person)
+        team = self.factory.makeTeam(
+            owner=self.person, visibility=PersonVisibility.PRIVATE)
+        snap = self.factory.makeSnap(
+            registrant=self.person, owner=team, private=True)
+        # Note that only LP admins or, in this case, commercial_admins
+        # can reach this snap because it's owned by a private team.
+        commercial_admin = self.factory.makePerson(
+            member_of=[getUtility(ILaunchpadCelebrities).commercial_admin])
+        browser = self.getViewBrowser(snap, user=commercial_admin)
+        browser.getLink("Administer snap package").click()
+        browser.getControl("Private").selected = False
+        browser.getControl("Update snap package").click()
+        self.assertEqual(
+            'This snap contains private information and cannot be public.',
+            extract_text(find_tags_by_class(browser.contents, "message")[1]))
 
     def test_admin_snap_sets_date_last_modified(self):
         # Administering a snap package sets the date_last_modified property.

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2015-09-25 17:26:03 +0000
+++ lib/lp/snappy/interfaces/snap.py	2016-02-04 13:01:27 +0000
@@ -21,6 +21,7 @@
     'SnapBuildDisallowedArchitecture',
     'SnapFeatureDisabled',
     'SnapNotOwner',
+    'SnapPrivacyMismatch',
     ]
 
 import httplib
@@ -66,6 +67,7 @@
     )
 
 from lp import _
+from lp.app.interfaces.launchpad import IPrivacy
 from lp.app.errors import NameLookupFailed
 from lp.app.validators.name import name_validator
 from lp.buildmaster.interfaces.processor import IProcessor
@@ -164,6 +166,15 @@
 
 
 @error_status(httplib.BAD_REQUEST)
+class SnapPrivacyMismatch(Exception):
+    """Snap package privacy does not match its content."""
+
+    def __init__(self):
+        super(SnapPrivacyMismatch, self).__init__(
+            "Snap contains private information and cannot be public.")
+
+
+@error_status(httplib.BAD_REQUEST)
 class CannotDeleteSnap(Exception):
     """This snap package cannot be deleted."""
 
@@ -332,6 +343,11 @@
 
     These attributes need launchpad.View to see, and launchpad.Admin to change.
     """
+
+    private = exported(Bool(
+        title=_("Private"), required=False, readonly=False,
+        description=_("Whether or not this snap is private.")))
+
     require_virtualized = exported(Bool(
         title=_("Require virtualized builders"), required=True, readonly=False,
         description=_("Only build this snap package on virtual builders.")))
@@ -345,7 +361,8 @@
 
 
 class ISnap(
-    ISnapView, ISnapEdit, ISnapEditableAttributes, ISnapAdminAttributes):
+    ISnapView, ISnapEdit, ISnapEditableAttributes, ISnapAdminAttributes,
+    IPrivacy):
     """A buildable snap package."""
 
     # XXX cjwatson 2015-07-17 bug=760849: "beta" is a lie to get WADL
@@ -363,16 +380,19 @@
     @export_factory_operation(
         ISnap, [
             "owner", "distro_series", "name", "description", "branch",
-            "git_ref"])
+            "git_ref", "private"])
     @operation_for_version("devel")
     def new(registrant, owner, distro_series, name, description=None,
             branch=None, git_ref=None, require_virtualized=True,
-            processors=None, date_created=None):
+            processors=None, date_created=None, private=False):
         """Create an `ISnap`."""
 
     def exists(owner, name):
         """Check to see if a matching snap exists."""
 
+    def isValidPrivacy(private, owner, branch=None, git_ref=None):
+        """Whether or not the privacy context is valid."""
+
     @operation_parameters(
         owner=Reference(IPerson, title=_("Owner"), required=True),
         name=TextLine(title=_("Snap name"), required=True))

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2015-10-08 14:18:29 +0000
+++ lib/lp/snappy/model/snap.py	2016-02-04 13:01:27 +0000
@@ -26,6 +26,7 @@
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import PRIVATE_INFORMATION_TYPES
 from lp.app.interfaces.security import IAuthorization
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
@@ -45,6 +46,7 @@
 from lp.code.model.branchcollection import GenericBranchCollection
 from lp.code.model.gitcollection import GenericGitCollection
 from lp.code.model.gitrepository import GitRepository
+from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.person import (
     IPerson,
     IPersonSet,
@@ -84,6 +86,7 @@
     SnapBuildDisallowedArchitecture,
     SnapFeatureDisabled,
     SnapNotOwner,
+    SnapPrivacyMismatch,
     )
 from lp.snappy.interfaces.snapbuild import ISnapBuildSet
 from lp.snappy.model.snapbuild import SnapBuild
@@ -140,9 +143,12 @@
 
     require_virtualized = Bool(name='require_virtualized')
 
+    private = Bool(name='private')
+
     def __init__(self, registrant, owner, distro_series, name,
                  description=None, branch=None, git_ref=None,
-                 require_virtualized=True, date_created=DEFAULT):
+                 require_virtualized=True, date_created=DEFAULT,
+                 private=False):
         """Construct a `Snap`."""
         if not getFeatureFlag(SNAP_FEATURE_FLAG):
             raise SnapFeatureDisabled
@@ -158,6 +164,7 @@
         self.require_virtualized = require_virtualized
         self.date_created = date_created
         self.date_last_modified = date_created
+        self.private = private
 
     @property
     def git_ref(self):
@@ -366,7 +373,7 @@
 
     def new(self, registrant, owner, distro_series, name, description=None,
             branch=None, git_ref=None, require_virtualized=True,
-            processors=None, date_created=DEFAULT):
+            processors=None, date_created=DEFAULT, private=False):
         """See `ISnapSet`."""
         if not registrant.inTeam(owner):
             if owner.is_team:
@@ -383,11 +390,15 @@
         if self.exists(owner, name):
             raise DuplicateSnapName
 
+        if not self.isValidPrivacy(private, owner, branch, git_ref):
+            raise SnapPrivacyMismatch
+
         store = IMasterStore(Snap)
         snap = Snap(
             registrant, owner, distro_series, name, description=description,
             branch=branch, git_ref=git_ref,
-            require_virtualized=require_virtualized, date_created=date_created)
+            require_virtualized=require_virtualized, date_created=date_created,
+            private=private)
         store.add(snap)
 
         if processors is None:
@@ -398,6 +409,23 @@
 
         return snap
 
+    def isValidPrivacy(self, private, owner, branch=None, git_ref=None):
+        """See `ISnapSet`."""
+        # Private snaps may contain anything.
+        if private:
+            return True
+
+        # Public snaps with private sources are not allowed.
+        source_ref = branch or git_ref
+        if source_ref.information_type in PRIVATE_INFORMATION_TYPES:
+            return False
+
+        # Public snaps owned by private teams are not allowed.
+        if owner.is_team and owner.visibility == PersonVisibility.PRIVATE:
+            return False
+
+        return True
+
     def _getByName(self, owner, name):
         return IStore(Snap).find(
             Snap, Snap.owner == owner, Snap.name == name).one()

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2015-09-16 13:30:33 +0000
+++ lib/lp/snappy/model/snapbuild.py	2016-02-04 13:01:27 +0000
@@ -163,7 +163,11 @@
     @property
     def is_private(self):
         """See `IBuildFarmJob`."""
-        return self.snap.owner.private or self.archive.private
+        return (
+            self.snap.private or
+            self.snap.owner.private or
+            self.archive.private
+        )
 
     @property
     def title(self):

=== modified file 'lib/lp/snappy/templates/snap-index.pt'
--- lib/lp/snappy/templates/snap-index.pt	2015-09-18 13:32:09 +0000
+++ lib/lp/snappy/templates/snap-index.pt	2016-02-04 13:01:27 +0000
@@ -18,6 +18,7 @@
   </metal:registering>
 
   <metal:side fill-slot="side">
+    <div tal:replace="structure context/@@+portlet-privacy" />
     <div tal:replace="structure context/@@+global-actions"/>
   </metal:side>
 

=== added file 'lib/lp/snappy/templates/snap-portlet-privacy.pt'
--- lib/lp/snappy/templates/snap-portlet-privacy.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/templates/snap-portlet-privacy.pt	2016-02-04 13:01:27 +0000
@@ -0,0 +1,16 @@
+<div
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+  id="privacy"
+  tal:attributes="
+    class python: path('context/private') and 'portlet private' or 'portlet public'
+  "
+>
+    <span tal:attributes="class python: path('context/private') and 'sprite private' or 'sprite public'"
+      >This snap contains <strong
+        tal:content="python: path('context/private') and 'Private' or 'Public'"
+      ></strong> information</span>
+</div>
+
+

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2015-11-26 15:46:38 +0000
+++ lib/lp/snappy/tests/test_snap.py	2016-02-04 13:01:27 +0000
@@ -15,6 +15,7 @@
 from zope.event import notify
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import (
     BuildQueueStatus,
@@ -23,6 +24,7 @@
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.database.constants import (
@@ -43,6 +45,7 @@
     SnapBuildAlreadyPending,
     SnapBuildDisallowedArchitecture,
     SnapFeatureDisabled,
+    SnapPrivacyMismatch,
     )
 from lp.snappy.interfaces.snapbuild import ISnapBuild
 from lp.testing import (
@@ -404,6 +407,7 @@
         self.assertIsNone(snap.git_path)
         self.assertIsNone(snap.git_ref)
         self.assertTrue(snap.require_virtualized)
+        self.assertFalse(snap.private)
 
     def test_creation_git(self):
         # The metadata entries supplied when a Snap is created for a Git
@@ -420,6 +424,65 @@
         self.assertEqual(ref.path, snap.git_path)
         self.assertEqual(ref, snap.git_ref)
         self.assertTrue(snap.require_virtualized)
+        self.assertFalse(snap.private)
+
+    def test_private_snap_for_public_sources(self):
+        # Creating private snaps for public sources is allowed.
+        [ref] = self.factory.makeGitRefs()
+        components = self.makeSnapComponents(git_ref=ref)
+        components['private'] = True
+        snap = getUtility(ISnapSet).new(**components)
+        with person_logged_in(components['owner']):
+            self.assertTrue(snap.private)
+
+    def test_private_git_requires_private_snap(self):
+        # Snaps for a private Git branch cannot be public.
+        owner = self.factory.makePerson()
+        with person_logged_in(owner):
+            [git_ref] = self.factory.makeGitRefs(
+                owner=owner, information_type=InformationType.PRIVATESECURITY)
+            components = dict(
+                registrant=owner,
+                owner=owner,
+                git_ref=git_ref,
+                distro_series=self.factory.makeDistroSeries(),
+                name=self.factory.getUniqueString(u"snap-name"),
+            )
+            self.assertRaises(
+                SnapPrivacyMismatch, getUtility(ISnapSet).new, **components)
+
+    def test_private_bzr_requires_private_snap(self):
+        # Snaps for a private Bzr branch cannot be public.
+        owner = self.factory.makePerson()
+        with person_logged_in(owner):
+            branch = self.factory.makeAnyBranch(
+                owner=owner, information_type=InformationType.PRIVATESECURITY)
+            components = dict(
+                registrant=owner,
+                owner=owner,
+                branch=branch,
+                distro_series=self.factory.makeDistroSeries(),
+                name=self.factory.getUniqueString(u"snap-name"),
+            )
+            self.assertRaises(
+                SnapPrivacyMismatch, getUtility(ISnapSet).new, **components)
+
+    def test_private_team_requires_private_snap(self):
+        # Snaps owned by private teams cannot be public.
+        registrant = self.factory.makePerson()
+        with person_logged_in(registrant):
+            private_team = self.factory.makeTeam(
+                owner=registrant, visibility=PersonVisibility.PRIVATE)
+            [git_ref] = self.factory.makeGitRefs()
+            components = dict(
+                registrant=registrant,
+                owner=private_team,
+                git_ref=git_ref,
+                distro_series=self.factory.makeDistroSeries(),
+                name=self.factory.getUniqueString(u"snap-name"),
+            )
+            self.assertRaises(
+                SnapPrivacyMismatch, getUtility(ISnapSet).new, **components)
 
     def test_creation_no_source(self):
         # Attempting to create a Snap with neither a Bazaar branch nor a Git
@@ -703,7 +766,8 @@
         return self.webservice.getAbsoluteUrl(api_url(obj))
 
     def makeSnap(self, owner=None, distroseries=None, branch=None,
-                 git_ref=None, processors=None, webservice=None):
+                 git_ref=None, processors=None, webservice=None,
+                 private=False):
         if owner is None:
             owner = self.person
         if distroseries is None:
@@ -726,7 +790,7 @@
         logout()
         response = webservice.named_post(
             "/+snaps", "new", owner=owner_url, distro_series=distroseries_url,
-            name="mir", **kwargs)
+            name="mir", private=private, **kwargs)
         self.assertEqual(201, response.status)
         return webservice.get(response.getHeader("Location")).jsonBody()
 
@@ -775,6 +839,21 @@
             self.assertEqual(self.getURL(ref), snap["git_ref_link"])
             self.assertTrue(snap["require_virtualized"])
 
+    def test_new_private(self):
+        # Ensure private Snap creation works.
+        team = self.factory.makeTeam(owner=self.person)
+        distroseries = self.factory.makeDistroSeries(registrant=team)
+        [ref] = self.factory.makeGitRefs()
+        private_webservice = webservice_for_person(
+            self.person, permission=OAuthPermission.WRITE_PRIVATE)
+        private_webservice.default_api_version = "devel"
+        login(ANONYMOUS)
+        snap = self.makeSnap(
+            owner=team, distroseries=distroseries, git_ref=ref,
+            webservice=private_webservice, private=True)
+        with person_logged_in(self.person):
+            self.assertTrue(snap["private"])
+
     def test_duplicate(self):
         # An attempt to create a duplicate Snap fails.
         team = self.factory.makeTeam(owner=self.person)

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2015-09-11 12:20:23 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2016-02-04 13:01:27 +0000
@@ -127,7 +127,7 @@
             visibility=PersonVisibility.PRIVATE)
         with person_logged_in(private_team.teamowner):
             build = self.factory.makeSnapBuild(
-                requester=private_team.teamowner, owner=private_team)
+                requester=private_team.teamowner, owner=private_team, private=True)
             self.assertTrue(build.is_private)
         private_archive = self.factory.makeArchive(private=True)
         with person_logged_in(private_archive.owner):
@@ -366,7 +366,7 @@
             owner=self.person, visibility=PersonVisibility.PRIVATE)
         with person_logged_in(self.person):
             db_build = self.factory.makeSnapBuild(
-                requester=self.person, owner=db_team)
+                requester=self.person, owner=db_team, private=True)
             build_url = api_url(db_build)
         unpriv_webservice = webservice_for_person(
             self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2016-01-26 15:14:01 +0000
+++ lib/lp/testing/factory.py	2016-02-04 13:01:27 +0000
@@ -4554,7 +4554,7 @@
     def makeSnap(self, registrant=None, owner=None, distroseries=None,
                  name=None, branch=None, git_ref=None,
                  require_virtualized=True, processors=None,
-                 date_created=DEFAULT):
+                 date_created=DEFAULT, private=False):
         """Make a new Snap."""
         if registrant is None:
             registrant = self.makePerson()
@@ -4569,7 +4569,8 @@
         snap = getUtility(ISnapSet).new(
             registrant, owner, distroseries, name,
             require_virtualized=require_virtualized, processors=processors,
-            date_created=date_created, branch=branch, git_ref=git_ref)
+            date_created=date_created, branch=branch, git_ref=git_ref,
+            private=private)
         IStore(snap).flush()
         return snap