Merge ~pappacena/launchpad:snap-pillar-subscribe into launchpad:master

[Thiago F. Pappacena]

Adding SnapSubscription model

f3387eb... by Thiago F. Pappacena on 2021-02-18

Merge branch 'snap-pillar-reconcile-access' into snap-pillar-subscribe

7f125c7... by Thiago F. Pappacena on 2021-02-22

Merge branch 'snap-pillar-reconcile-access' into snap-pillar-subscribe

984531e... by Thiago F. Pappacena on 2021-02-22

Fixing personmerge test

4dacf01... by Thiago F. Pappacena on 2021-02-22

Removing SnapSubscriptions when deleting a Snap

6612dca... by Thiago F. Pappacena on 2021-02-22

Test fix: private snaps cannot be associated with non-moderated teams

a273e7a... by Thiago F. Pappacena on 2021-02-24

Fixing Snap.unsubscribe and improving performance on Snap.visibleByUser

858eaf1... by Thiago F. Pappacena on 2021-03-02

Merge branch 'snap-pillar-reconcile-access' into snap-pillar-subscribe

92bebbd... by Thiago F. Pappacena on 2021-03-03

Merge branch 'master' into snap-pillar-subscribe

[Colin Watson (cjwatson)]

review: Approve

f5f27aa... by Thiago F. Pappacena on 2021-03-08

Allowing snap owner to unsubscribe anyone and few minor adjustments

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
index 949e8d9..3e51005 100644
--- a/lib/lp/registry/personmerge.py
+++ b/lib/lp/registry/personmerge.py
@@ -660,18 +660,36 @@ def _mergeSnap(cur, from_person, to_person):
     existing_names = [
         s.name for s in getUtility(ISnapSet).findByOwner(to_person)]
     for snap in snaps:
-        new_name = snap.name
+        naked_snap = removeSecurityProxy(snap)
+        new_name = naked_snap.name
         count = 1
         while new_name in existing_names:
             new_name = '%s-%s' % (snap.name, count)
             count += 1
-        naked_snap = removeSecurityProxy(snap)
         naked_snap.owner = to_person
         naked_snap.name = new_name
     if not snaps.is_empty():
         IStore(snaps[0]).flush()
 
 
+def _mergeSnapSubscription(cur, from_id, to_id):
+    # Update only the SnapSubscription that will not conflict.
+    cur.execute('''
+        UPDATE SnapSubscription
+        SET person=%(to_id)d
+        WHERE person=%(from_id)d AND snap NOT IN
+            (
+            SELECT snap
+            FROM SnapSubscription
+            WHERE person = %(to_id)d
+            )
+        ''' % vars())
+    # and delete those left over.
+    cur.execute('''
+        DELETE FROM SnapSubscription WHERE person=%(from_id)d
+        ''' % vars())
+
+
 def _mergeOCIRecipe(cur, from_person, to_person):
     # This shouldn't use removeSecurityProxy
     oci_recipes = getUtility(IOCIRecipeSet).findByOwner(from_person)
@@ -917,8 +935,7 @@ def merge_people(from_person, to_person, reviewer, delete=False):
     _mergeSnap(cur, from_person, to_person)
     skip.append(('snap', 'owner'))
 
-    # XXX pappacena 2021-02-18: add tests for this once we have
-    # SnapSubscription model in place.
+    _mergeSnapSubscription(cur, from_id, to_id)
     skip.append(('snapsubscription', 'person'))
 
     _mergeOCIRecipe(cur, from_person, to_person)
diff --git a/lib/lp/registry/tests/test_personmerge.py b/lib/lp/registry/tests/test_personmerge.py
index 02fefcc..e69872f 100644
--- a/lib/lp/registry/tests/test_personmerge.py
+++ b/lib/lp/registry/tests/test_personmerge.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for merge_people."""
@@ -8,6 +8,7 @@ from operator import attrgetter
 
 import pytz
 from testtools.matchers import (
+    Equals,
     MatchesSetwise,
     MatchesStructure,
     )
@@ -48,13 +49,17 @@ from lp.services.identity.interfaces.emailaddress import (
     EmailAddressStatus,
     IEmailAddressSet,
     )
-from lp.snappy.interfaces.snap import ISnapSet
+from lp.snappy.interfaces.snap import (
+    ISnapSet,
+    SNAP_TESTING_FLAGS,
+    )
 from lp.soyuz.enums import ArchiveStatus
 from lp.soyuz.interfaces.livefs import (
     ILiveFSSet,
     LIVEFS_FEATURE_FLAG,
     )
 from lp.testing import (
+    admin_logged_in,
     celebrity_logged_in,
     login_person,
     person_logged_in,
@@ -664,6 +669,36 @@ class TestMergePeople(TestCaseWithFactory, KarmaTestMixin):
         self.assertIsNone(snaps[1].git_path)
         self.assertEqual(u'foo-1', snaps[1].name)
 
+    def test_merge_snapsubscription(self):
+        # Checks that merging users moves subscriptions.
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        duplicate = self.factory.makePerson()
+        mergee = self.factory.makePerson()
+        snap = removeSecurityProxy(self.factory.makeSnap(
+            owner=duplicate, registrant=duplicate,
+            name=u'foo', private=True))
+        with admin_logged_in():
+            # Owner should have being subscribed automatically on creation.
+            self.assertTrue(snap.visibleByUser(duplicate))
+            self.assertThat(snap._getSubscription(duplicate), MatchesStructure(
+                snap=Equals(snap),
+                person=Equals(duplicate)
+            ))
+            self.assertFalse(snap.visibleByUser(mergee))
+            self.assertIsNone(snap._getSubscription(mergee))
+
+        self._do_premerge(duplicate, mergee)
+        login_person(mergee)
+        duplicate, mergee = self._do_merge(duplicate, mergee)
+
+        self.assertTrue(snap.visibleByUser(mergee))
+        self.assertThat(snap._getSubscription(mergee), MatchesStructure(
+            snap=Equals(snap),
+            person=Equals(mergee)
+        ))
+        self.assertFalse(snap.visibleByUser(duplicate))
+        self.assertIsNone(snap._getSubscription(duplicate))
+
     def test_merge_moves_oci_recipes(self):
         # When person/teams are merged, oci recipes owned by the from
         # person are moved.
diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index 50390e9..025574c 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -59,6 +59,7 @@ from lp.code.tests.helpers import (
 from lp.registry.enums import (
     BranchSharingPolicy,
     PersonVisibility,
+    TeamMembershipPolicy,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -388,6 +389,7 @@ class TestSnapAddView(BaseTestSnapView):
         login_person(self.person)
         self.factory.makeTeam(
             name='super-private', owner=self.person,
+            membership_policy=TeamMembershipPolicy.MODERATED,
             visibility=PersonVisibility.PRIVATE)
         branch = self.factory.makeAnyBranch()
 
@@ -411,6 +413,7 @@ class TestSnapAddView(BaseTestSnapView):
         login_person(self.person)
         private_team = self.factory.makeTeam(
             name='super-private', owner=self.person,
+            membership_policy=TeamMembershipPolicy.MODERATED,
             visibility=PersonVisibility.PRIVATE)
         branch = self.factory.makeAnyBranch(
             owner=self.person, registrant=self.person,
@@ -440,6 +443,7 @@ class TestSnapAddView(BaseTestSnapView):
         login_person(self.person)
         private_team = self.factory.makeTeam(
             name='super-private', owner=self.person,
+            membership_policy=TeamMembershipPolicy.MODERATED,
             visibility=PersonVisibility.PRIVATE)
         [git_ref] = self.factory.makeGitRefs(
             owner=self.person, registrant=self.person,
@@ -762,6 +766,7 @@ class TestSnapAdminView(BaseTestSnapView):
         # Cannot make snap public if it still contains private information.
         login_person(self.person)
         team = self.factory.makeTeam(
+            membership_policy=TeamMembershipPolicy.MODERATED,
             owner=self.person, visibility=PersonVisibility.PRIVATE)
         project = self.factory.makeProduct(
             information_type=InformationType.PUBLIC,
diff --git a/lib/lp/snappy/interfaces/snapsubscription.py b/lib/lp/snappy/interfaces/snapsubscription.py
new file mode 100644
index 0000000..58206cc
--- /dev/null
+++ b/lib/lp/snappy/interfaces/snapsubscription.py
@@ -0,0 +1,42 @@
+# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Snap subscription model."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'ISnapSubscription'
+]
+
+from lazr.restful.fields import Reference
+from zope.interface import Interface
+from zope.schema import (
+    Datetime,
+    Int,
+    )
+
+from lp import _
+from lp.services.fields import PersonChoice
+from lp.snappy.interfaces.snap import ISnap
+
+
+class ISnapSubscription(Interface):
+    """A person subscription to a specific Snap recipe."""
+
+    id = Int(title=_('ID'), readonly=True, required=True)
+    person = PersonChoice(
+        title=_('Person'), required=True, vocabulary='ValidPersonOrTeam',
+        readonly=True,
+        description=_("The person subscribed to the related snap recipe."))
+    snap = Reference(ISnap, title=_("Snap"), required=True, readonly=True)
+    subscribed_by = PersonChoice(
+        title=_('Subscribed by'), required=True,
+        vocabulary='ValidPersonOrTeam', readonly=True,
+        description=_("The person who created this subscription."))
+    date_created = Datetime(
+        title=_('Date subscribed'), required=True, readonly=True)
+
+    def canBeUnsubscribedByUser(user):
+        """Can the user unsubscribe the subscriber from the snap recipe?"""
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 6b9af0a..dabc6fe 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -66,7 +66,11 @@ from lp.app.enums import (
     PRIVATE_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
     )
-from lp.app.errors import IncompatibleArguments
+from lp.app.errors import (
+    IncompatibleArguments,
+    SubscriptionPrivacyViolation,
+    UserCannotUnsubscribePerson,
+    )
 from lp.app.interfaces.security import IAuthorization
 from lp.app.interfaces.services import IService
 from lp.buildmaster.enums import BuildStatus
@@ -108,7 +112,10 @@ from lp.code.model.branchnamespace import (
 from lp.code.model.gitcollection import GenericGitCollection
 from lp.code.model.gitrepository import GitRepository
 from lp.registry.errors import PrivatePersonLinkageError
-from lp.registry.interfaces.accesspolicy import IAccessArtifactSource
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactGrantSource,
+    IAccessArtifactSource,
+    )
 from lp.registry.interfaces.person import (
     IPerson,
     IPersonSet,
@@ -203,6 +210,7 @@ from lp.snappy.interfaces.snappyseries import ISnappyDistroSeriesSet
 from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
 from lp.snappy.model.snapbuild import SnapBuild
 from lp.snappy.model.snapjob import SnapJob
+from lp.snappy.model.snapsubscription import SnapSubscription
 from lp.soyuz.interfaces.archive import ArchiveDisabled
 from lp.soyuz.model.archive import (
     Archive,
@@ -1129,25 +1137,66 @@ class Snap(Storm, WebhookTargetMixin):
 
     def visibleByUser(self, user):
         """See `ISnap`."""
+        if self.information_type in PUBLIC_INFORMATION_TYPES:
+            return True
+        if user is None:
+            return False
         store = IStore(self)
         return not store.find(
             Snap,
             Snap.id == self.id,
             get_snap_privacy_filter(user)).is_empty()
 
+    def _getSubscription(self, person):
+        """Returns person's subscription to this snap recipe, or None if no
+        subscription is available.
+        """
+        if person is None:
+            return None
+        return Store.of(self).find(
+            SnapSubscription,
+            SnapSubscription.person == person,
+            SnapSubscription.snap == self).one()
+
+    def _userCanBeSubscribed(self, person):
+        """Checks if the given person can subscribe to this snap recipe."""
+        return not (
+            self.private and
+            person.is_team and
+            person.anyone_can_join())
+
     def subscribe(self, person, subscribed_by, ignore_permissions=False):
         """See `ISnap`."""
-        # XXX pappacena 2021-02-05: We may need a "SnapSubscription" here.
+        if not self._userCanBeSubscribed(person):
+            raise SubscriptionPrivacyViolation(
+                "Open and delegated teams cannot be subscribed to private "
+                "snap recipes.")
+        subscription = self._getSubscription(person)
+        if subscription is None:
+            subscription = SnapSubscription(
+                person=person, snap=self, subscribed_by=subscribed_by)
+            Store.of(subscription).flush()
         service = getUtility(IService, "sharing")
         service.ensureAccessGrants(
             [person], subscribed_by, snaps=[self],
             ignore_permissions=ignore_permissions)
 
-    def unsubscribe(self, person, unsubscribed_by):
+    def unsubscribe(self, person, unsubscribed_by, ignore_permissions=False):
         """See `ISnap`."""
-        service = getUtility(IService, "sharing")
-        service.revokeAccessGrants(
-            self.pillar, person, unsubscribed_by, snaps=[self])
+        subscription = self._getSubscription(person)
+        if subscription is None:
+            return
+        if (not ignore_permissions
+                and not subscription.canBeUnsubscribedByUser(unsubscribed_by)):
+            raise UserCannotUnsubscribePerson(
+                '%s does not have permission to unsubscribe %s.' % (
+                    unsubscribed_by.displayname,
+                    person.displayname))
+        artifact = getUtility(IAccessArtifactSource).find([self])
+        getUtility(IAccessArtifactGrantSource).revokeByArtifact(
+            artifact, [person])
+        store = Store.of(subscription)
+        store.remove(subscription)
         IStore(self).flush()
 
     def _reconcileAccess(self):
@@ -1169,6 +1218,11 @@ class Snap(Storm, WebhookTargetMixin):
         """Delete access grants for this snap recipe prior to deleting it."""
         getUtility(IAccessArtifactSource).delete([self])
 
+    def _deleteSnapSubscriptions(self):
+        subscriptions = Store.of(self).find(
+            SnapSubscription, SnapSubscription.snap == self)
+        subscriptions.remove()
+
     def destroySelf(self):
         """See `ISnap`."""
         store = IStore(Snap)
@@ -1206,6 +1260,7 @@ class Snap(Storm, WebhookTargetMixin):
         store.find(Job, Job.id.is_in(affected_jobs)).remove()
         getUtility(IWebhookSet).delete(self.webhooks)
         self._deleteAccessGrants()
+        self._deleteSnapSubscriptions()
         store.remove(self)
         store.find(
             BuildFarmJob, BuildFarmJob.id.is_in(build_farm_job_ids)).remove()
diff --git a/lib/lp/snappy/model/snapsubscription.py b/lib/lp/snappy/model/snapsubscription.py
new file mode 100644
index 0000000..89860d7
--- /dev/null
+++ b/lib/lp/snappy/model/snapsubscription.py
@@ -0,0 +1,62 @@
+# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Snap subscription model."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'SnapSubscription'
+]
+
+import pytz
+from storm.properties import (
+    DateTime,
+    Int,
+    )
+from storm.references import Reference
+from zope.interface import implementer
+
+from lp.registry.interfaces.person import validate_person
+from lp.registry.interfaces.role import IPersonRoles
+from lp.services.database.constants import UTC_NOW
+from lp.services.database.stormbase import StormBase
+from lp.snappy.interfaces.snapsubscription import ISnapSubscription
+
+
+@implementer(ISnapSubscription)
+class SnapSubscription(StormBase):
+    """A relationship between a person and a snap recipe."""
+
+    __storm_table__ = 'SnapSubscription'
+
+    id = Int(primary=True)
+
+    person_id = Int(
+        "person", allow_none=False, validator=validate_person)
+    person = Reference(person_id, "Person.id")
+
+    snap_id = Int("snap", allow_none=False)
+    snap = Reference(snap_id, "Snap.id")
+
+    date_created = DateTime(allow_none=False, default=UTC_NOW, tzinfo=pytz.UTC)
+
+    subscribed_by_id = Int(
+        "subscribed_by", allow_none=False, validator=validate_person)
+    subscribed_by = Reference(subscribed_by_id, "Person.id")
+
+    def __init__(self, snap, person, subscribed_by):
+        super(SnapSubscription, self).__init__()
+        self.snap = snap
+        self.person = person
+        self.subscribed_by = subscribed_by
+
+    def canBeUnsubscribedByUser(self, user):
+        """See `ISnapSubscription`."""
+        if user is None:
+            return False
+        return (user.inTeam(self.snap.owner) or
+                user.inTeam(self.person) or
+                user.inTeam(self.subscribed_by) or
+                IPersonRoles(user).in_admin)
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index e8ea61e..f66949b 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -34,6 +34,7 @@ from testtools.matchers import (
     Equals,
     GreaterThan,
     Is,
+    IsInstance,
     LessThan,
     MatchesAll,
     MatchesDict,
@@ -46,6 +47,7 @@ from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
+from lp.app.errors import SubscriptionPrivacyViolation
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import (
     BuildQueueStatus,
@@ -68,6 +70,7 @@ from lp.code.tests.helpers import (
 from lp.registry.enums import (
     BranchSharingPolicy,
     PersonVisibility,
+    TeamMembershipPolicy,
     )
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.distribution import IDistributionSet
@@ -1353,6 +1356,9 @@ class TestSnapVisibility(TestCaseWithFactory):
             AccessArtifactGrant.abstract_artifact_id == AccessArtifact.id,
             *conditions)
 
+    def getSnapSubscription(self, snap, person):
+        return removeSecurityProxy(snap)._getSubscription(person)
+
     def test_only_owner_can_grant_access(self):
         owner = self.factory.makePerson()
         snap = self.factory.makeSnap(
@@ -1373,7 +1379,8 @@ class TestSnapVisibility(TestCaseWithFactory):
 
     def test_private_is_visible_by_team_member(self):
         person = self.factory.makePerson()
-        team = self.factory.makeTeam(members=[person])
+        team = self.factory.makeTeam(
+            members=[person], membership_policy=TeamMembershipPolicy.MODERATED)
         snap = self.factory.makeSnap(private=True, owner=team,
                                      registrant=person)
         with person_logged_in(team):
@@ -1388,11 +1395,33 @@ class TestSnapVisibility(TestCaseWithFactory):
         with person_logged_in(owner):
             self.assertFalse(snap.visibleByUser(person))
             snap.subscribe(person, snap.owner)
+            self.assertThat(
+                self.getSnapSubscription(snap, person),
+                MatchesStructure(
+                    person=Equals(person),
+                    snap=Equals(snap),
+                    subscribed_by=Equals(snap.owner),
+                    date_created=IsInstance(datetime)))
             # Calling again should be a no-op.
             snap.subscribe(person, snap.owner)
             self.assertTrue(snap.visibleByUser(person))
+
             snap.unsubscribe(person, snap.owner)
             self.assertFalse(snap.visibleByUser(person))
+            self.assertIsNone(self.getSnapSubscription(snap, person))
+
+    def test_snap_owner_can_unsubscribe_anyone(self):
+        person = self.factory.makePerson()
+        owner = self.factory.makePerson()
+        admin = self.factory.makeAdministrator()
+        snap = self.factory.makeSnap(
+            registrant=owner, owner=owner, private=True)
+        with person_logged_in(admin):
+            snap.subscribe(person, admin)
+            self.assertTrue(snap.visibleByUser(person))
+        with person_logged_in(owner):
+            snap.unsubscribe(person, owner)
+            self.assertFalse(snap.visibleByUser(person))
 
     def test_reconcile_set_public(self):
         owner = self.factory.makePerson()
@@ -1401,11 +1430,24 @@ class TestSnapVisibility(TestCaseWithFactory):
         another_user = self.factory.makePerson()
         with admin_logged_in():
             snap.subscribe(another_user, snap.owner)
+            self.assertEqual(1, self.getSnapGrants(snap, another_user).count())
+            self.assertThat(
+                self.getSnapSubscription(snap, another_user),
+                MatchesStructure(
+                    person=Equals(another_user),
+                    snap=Equals(snap),
+                    subscribed_by=Equals(snap.owner),
+                    date_created=IsInstance(datetime)))
 
-        self.assertEqual(1, self.getSnapGrants(snap, another_user).count())
-        with admin_logged_in():
             snap.information_type = InformationType.PUBLIC
-        self.assertEqual(0, self.getSnapGrants(snap, another_user).count())
+            self.assertEqual(0, self.getSnapGrants(snap, another_user).count())
+            self.assertThat(
+                self.getSnapSubscription(snap, another_user),
+                MatchesStructure(
+                    person=Equals(another_user),
+                    snap=Equals(snap),
+                    subscribed_by=Equals(snap.owner),
+                    date_created=IsInstance(datetime)))
 
     def test_reconcile_permissions_setting_project(self):
         owner = self.factory.makePerson()
@@ -1429,10 +1471,24 @@ class TestSnapVisibility(TestCaseWithFactory):
             snap.subscribe(another_person, owner)
             self.assertTrue(snap.visibleByUser(another_person))
             self.assertEqual(2, self.getSnapGrants(snap).count())
+            self.assertThat(
+                self.getSnapSubscription(snap, another_person),
+                MatchesStructure(
+                    person=Equals(another_person),
+                    snap=Equals(snap),
+                    subscribed_by=Equals(snap.owner),
+                    date_created=IsInstance(datetime)))
 
             snap.setProject(new_project)
             self.assertTrue(snap.visibleByUser(another_person))
             self.assertEqual(2, self.getSnapGrants(snap).count())
+            self.assertThat(
+                self.getSnapSubscription(snap, another_person),
+                MatchesStructure(
+                    person=Equals(another_person),
+                    snap=Equals(snap),
+                    subscribed_by=Equals(snap.owner),
+                    date_created=IsInstance(datetime)))
 
 
 class TestSnapSet(TestCaseWithFactory):
@@ -1527,12 +1583,23 @@ class TestSnapSet(TestCaseWithFactory):
         self.assertEqual(ref.path, snap.git_path)
         self.assertEqual(ref, snap.git_ref)
 
+    def test_create_private_snap_with_open_team_as_owner_fails(self):
+        components = self.makeSnapComponents()
+        with admin_logged_in():
+            components['owner'].membership_policy = TeamMembershipPolicy.OPEN
+            components['information_type'] = InformationType.PROPRIETARY
+        self.assertRaises(
+            SubscriptionPrivacyViolation,
+            getUtility(ISnapSet).new, **components)
+
     def test_private_snap_information_type_compatibility(self):
         login_admin()
         private = InformationType.PROPRIETARY
         public = InformationType.PUBLIC
+        components = self.makeSnapComponents()
+        components['owner'].membership_policy = TeamMembershipPolicy.MODERATED
         private_snap = getUtility(ISnapSet).new(
-            information_type=private, **self.makeSnapComponents())
+            information_type=private, **components)
         self.assertEqual(
             InformationType.PROPRIETARY, private_snap.information_type)
 
@@ -1545,7 +1612,10 @@ class TestSnapSet(TestCaseWithFactory):
         # Creating private snaps for public sources is allowed.
         [ref] = self.factory.makeGitRefs()
         components = self.makeSnapComponents(git_ref=ref)
-        components['information_type'] = InformationType.PROPRIETARY
+        with admin_logged_in():
+            components['information_type'] = InformationType.PROPRIETARY
+            components['owner'].membership_policy = (
+                TeamMembershipPolicy.MODERATED)
         components['project'] = self.factory.makeProduct(
             information_type=InformationType.PROPRIETARY,
             branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
@@ -2725,7 +2795,9 @@ class TestSnapWebservice(TestCaseWithFactory):
 
     def test_new_private(self):
         # Ensure private Snap creation works.
-        team = self.factory.makeTeam(owner=self.person)
+        team = self.factory.makeTeam(
+            membership_policy=TeamMembershipPolicy.MODERATED,
+            owner=self.person)
         distroseries = self.factory.makeDistroSeries(registrant=team)
         [ref] = self.factory.makeGitRefs()
         private_webservice = webservice_for_person(
diff --git a/lib/lp/snappy/tests/test_snapbuild.py b/lib/lp/snappy/tests/test_snapbuild.py
index e5dda05..7acff54 100644
--- a/lib/lp/snappy/tests/test_snapbuild.py
+++ b/lib/lp/snappy/tests/test_snapbuild.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test snap package build features."""
@@ -36,7 +36,10 @@ from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
 from lp.buildmaster.interfaces.processor import IProcessorSet
-from lp.registry.enums import PersonVisibility
+from lp.registry.enums import (
+    PersonVisibility,
+    TeamMembershipPolicy,
+    )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.authserver.xmlrpc import AuthServerAPIView
 from lp.services.config import config
@@ -172,6 +175,7 @@ class TestSnapBuild(TestCaseWithFactory):
         # A SnapBuild is private iff its Snap and archive are.
         self.assertFalse(self.build.is_private)
         private_team = self.factory.makeTeam(
+            membership_policy=TeamMembershipPolicy.MODERATED,
             visibility=PersonVisibility.PRIVATE)
         with person_logged_in(private_team.teamowner):
             build = self.factory.makeSnapBuild(
@@ -775,6 +779,7 @@ class TestSnapBuildWebservice(TestCaseWithFactory):
     def test_private_snap(self):
         # A SnapBuild with a private Snap is private.
         db_team = self.factory.makeTeam(
+            membership_policy=TeamMembershipPolicy.MODERATED,
             owner=self.person, visibility=PersonVisibility.PRIVATE)
         with person_logged_in(self.person):
             db_build = self.factory.makeSnapBuild(
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 6948419..034095d 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -4764,7 +4764,14 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         if registrant is None:
             registrant = self.makePerson()
         if owner is None:
-            owner = self.makeTeam(registrant)
+            is_private_snap = (
+                private or information_type not in PUBLIC_INFORMATION_TYPES)
+            # Private snaps cannot be owned by non-moderated teams.
+            membership_policy = (
+                TeamMembershipPolicy.OPEN if not is_private_snap
+                else TeamMembershipPolicy.MODERATED)
+            owner = self.makeTeam(
+                registrant, membership_policy=membership_policy)
         if distroseries is _DEFAULT:
             distroseries = self.makeDistroSeries()
         if name is None:
@@ -4778,7 +4785,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
             if auto_build_pocket is None:
                 auto_build_pocket = PackagePublishingPocket.UPDATES
         if private and project is _DEFAULT:
-            # If we are creating a private snap and didn't explictly set a
+            # If we are creating a private snap and didn't explicitly set a
             # pillar for it, we must create a pillar.
             branch_sharing = (
                 BranchSharingPolicy.PUBLIC_OR_PROPRIETARY if not private