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

[Thiago F. Pappacena]

Remove dangling SnapSubscription when changing Snap pillar's sharing policy

6ead4b4... by Thiago F. Pappacena on 2021-02-22

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

96846ab... by Thiago F. Pappacena on 2021-02-22

Fixing snap privacy filter usage

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

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

0efaed4... by Thiago F. Pappacena on 2021-03-02

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

d256e68... by Thiago F. Pappacena on 2021-03-03

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

[Colin Watson (cjwatson)]

review: Approve

76b720d... by Thiago F. Pappacena on 2021-03-08

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

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

Refactoring and setting proper db permission for job

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes, and added a comment about `get_snap_privacy_filter` method.

920b88c... by Thiago F. Pappacena on 2021-03-09

Not granting commercial admin view permisson on every snap

[Thiago F. Pappacena (pappacena)]

Pushed the changes for admin visibility after talking to wgrant and cjwatson.

[Colin Watson (cjwatson)]

review: Approve

df76888... by Thiago F. Pappacena on 2021-03-11

Merge branch 'master' into snap-pillar-subscribe-removal-job

5f729ee... by Thiago F. Pappacena on 2021-03-11

Removing unecessary shortcut for admin permission on snaps

Unmerged commits

5f729ee... by Thiago F. Pappacena on 2021-03-11

Removing unecessary shortcut for admin permission on snaps

df76888... by Thiago F. Pappacena on 2021-03-11

Merge branch 'master' into snap-pillar-subscribe-removal-job

920b88c... by Thiago F. Pappacena on 2021-03-09

Not granting commercial admin view permisson on every snap

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

Refactoring and setting proper db permission for job

76b720d... by Thiago F. Pappacena on 2021-03-08

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

d256e68... by Thiago F. Pappacena on 2021-03-03

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

0efaed4... by Thiago F. Pappacena on 2021-03-02

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

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

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

96846ab... by Thiago F. Pappacena on 2021-02-22

Fixing snap privacy filter usage

6ead4b4... by Thiago F. Pappacena on 2021-02-22

Merge branch 'snap-pillar-subscribe' into snap-pillar-subscribe-removal-job

diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index e9dcc62..2f4ffdf 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -2111,6 +2111,8 @@ public.job                              = SELECT, INSERT, UPDATE
 public.person                           = SELECT
 public.product                          = SELECT
 public.sharingjob                       = SELECT, INSERT, UPDATE
+public.snap                             = SELECT
+public.snapsubscription                 = SELECT, UPDATE, DELETE
 public.specification                    = SELECT
 public.specificationsubscription        = SELECT, DELETE
 public.teamparticipation                = SELECT
diff --git a/lib/lp/blueprints/model/specification.py b/lib/lp/blueprints/model/specification.py
index e0d4001..55ce9d5 100644
--- a/lib/lp/blueprints/model/specification.py
+++ b/lib/lp/blueprints/model/specification.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).
 
 __metaclass__ = type
@@ -755,7 +755,7 @@ class Specification(SQLBase, BugLinkTargetMixin, InformationTypeMixin):
             # Grant the subscriber access if they can't see the
             # specification.
             service = getUtility(IService, 'sharing')
-            _, _, _, shared_specs = service.getVisibleArtifacts(
+            _, _, _, _, shared_specs = service.getVisibleArtifacts(
                 person, specifications=[self], ignore_permissions=True)
             if not shared_specs:
                 service.ensureAccessGrants(
diff --git a/lib/lp/blueprints/tests/test_specification.py b/lib/lp/blueprints/tests/test_specification.py
index 8de08c9..86c4e4a 100644
--- a/lib/lp/blueprints/tests/test_specification.py
+++ b/lib/lp/blueprints/tests/test_specification.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for Specification."""
@@ -492,7 +492,7 @@ class SpecificationTests(TestCaseWithFactory):
                 product=product, information_type=InformationType.PROPRIETARY)
             spec.subscribe(user, subscribed_by=owner)
             service = getUtility(IService, 'sharing')
-            _, _, _, shared_specs = service.getVisibleArtifacts(
+            _, _, _, _, shared_specs = service.getVisibleArtifacts(
                 user, specifications=[spec])
             self.assertEqual([spec], shared_specs)
             # The spec is also returned by getSharedSpecifications(),
@@ -509,7 +509,7 @@ class SpecificationTests(TestCaseWithFactory):
             service.sharePillarInformation(
                 product, user_2, owner, permissions)
             spec.subscribe(user_2, subscribed_by=owner)
-            _, _, _, shared_specs = service.getVisibleArtifacts(
+            _, _, _, _, shared_specs = service.getVisibleArtifacts(
                 user_2, specifications=[spec])
             self.assertEqual([spec], shared_specs)
             self.assertEqual(
@@ -529,7 +529,7 @@ class SpecificationTests(TestCaseWithFactory):
             spec.subscribe(user, subscribed_by=owner)
             spec.unsubscribe(user, unsubscribed_by=owner)
             service = getUtility(IService, 'sharing')
-            _, _, _, shared_specs = service.getVisibleArtifacts(
+            _, _, _, _, shared_specs = service.getVisibleArtifacts(
                 user, specifications=[spec])
             self.assertEqual([], shared_specs)
 
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index a9b177c..f7ebc86 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.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).
 
 """Launchpad bug-related database table classes."""
@@ -875,7 +875,7 @@ class Bug(SQLBase, InformationTypeMixin):
         # there is at least one bugtask for which access can be checked.
         if self.default_bugtask:
             service = getUtility(IService, 'sharing')
-            bugs, _, _, _ = service.getVisibleArtifacts(
+            bugs, _, _, _, _ = service.getVisibleArtifacts(
                 person, bugs=[self], ignore_permissions=True)
             if not bugs:
                 service.ensureAccessGrants(
@@ -1819,7 +1819,7 @@ class Bug(SQLBase, InformationTypeMixin):
         if information_type in PRIVATE_INFORMATION_TYPES:
             service = getUtility(IService, 'sharing')
             for person in (who, self.owner):
-                bugs, _, _, _ = service.getVisibleArtifacts(
+                bugs, _, _, _, _ = service.getVisibleArtifacts(
                     person, bugs=[self], ignore_permissions=True)
                 if not bugs:
                     # subscribe() isn't sufficient if a subscription
diff --git a/lib/lp/code/browser/branchsubscription.py b/lib/lp/code/browser/branchsubscription.py
index fbd98bc..cee4504 100644
--- a/lib/lp/code/browser/branchsubscription.py
+++ b/lib/lp/code/browser/branchsubscription.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).
 
 __metaclass__ = type
@@ -276,7 +276,7 @@ class BranchSubscriptionEditView(LaunchpadEditFormView):
         url = canonical_url(self.branch)
         # If the subscriber can no longer see the branch, redirect them away.
         service = getUtility(IService, 'sharing')
-        _, branches, _, _ = service.getVisibleArtifacts(
+        _, branches, _, _, _ = service.getVisibleArtifacts(
             self.person, branches=[self.branch], ignore_permissions=True)
         if not branches:
             url = canonical_url(self.branch.target)
diff --git a/lib/lp/code/browser/gitsubscription.py b/lib/lp/code/browser/gitsubscription.py
index 3eda78c..a18d593 100644
--- a/lib/lp/code/browser/gitsubscription.py
+++ b/lib/lp/code/browser/gitsubscription.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 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).
 
 __metaclass__ = type
@@ -280,7 +280,7 @@ class GitSubscriptionEditView(LaunchpadEditFormView):
         # If the subscriber can no longer see the repository, redirect them
         # away.
         service = getUtility(IService, "sharing")
-        _, _, repositories, _ = service.getVisibleArtifacts(
+        _, _, repositories, _, _ = service.getVisibleArtifacts(
             self.person, gitrepositories=[self.repository],
             ignore_permissions=True)
         if not repositories:
diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index 947aaf1..278db40 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.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).
 
 __metaclass__ = type
@@ -1041,7 +1041,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             subscription.review_level = code_review_level
         # Grant the subscriber access if they can't see the branch.
         service = getUtility(IService, 'sharing')
-        _, branches, _, _ = service.getVisibleArtifacts(
+        _, branches, _, _, _ = service.getVisibleArtifacts(
             person, branches=[self], ignore_permissions=True)
         if not branches:
             service.ensureAccessGrants(
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 74d94ec..589dcbf 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -1002,7 +1002,7 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
             subscription.review_level = code_review_level
         # Grant the subscriber access if they can't see the repository.
         service = getUtility(IService, "sharing")
-        _, _, repositories, _ = service.getVisibleArtifacts(
+        _, _, repositories, _, _ = service.getVisibleArtifacts(
             person, gitrepositories=[self], ignore_permissions=True)
         if not repositories:
             service.ensureAccessGrants(
diff --git a/lib/lp/code/model/tests/test_branchsubscription.py b/lib/lp/code/model/tests/test_branchsubscription.py
index 6a15cd0..b5b234e 100644
--- a/lib/lp/code/model/tests/test_branchsubscription.py
+++ b/lib/lp/code/model/tests/test_branchsubscription.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the BranchSubscription model object."""
@@ -134,7 +134,7 @@ class TestBranchSubscriptions(TestCaseWithFactory):
                 None, CodeReviewNotificationLevel.NOEMAIL, owner)
             # The stacked on branch should be visible.
             service = getUtility(IService, 'sharing')
-            _, visible_branches, _, _ = service.getVisibleArtifacts(
+            _, visible_branches, _, _, _ = service.getVisibleArtifacts(
                 grantee, branches=[private_stacked_on_branch])
             self.assertContentEqual(
                 [private_stacked_on_branch], visible_branches)
@@ -162,7 +162,7 @@ class TestBranchSubscriptions(TestCaseWithFactory):
                 grantee, BranchSubscriptionNotificationLevel.NOEMAIL,
                 None, CodeReviewNotificationLevel.NOEMAIL, owner)
             # The stacked on branch should not be visible.
-            _, visible_branches, _, _ = service.getVisibleArtifacts(
+            _, visible_branches, _, _, _ = service.getVisibleArtifacts(
                 grantee, branches=[private_stacked_on_branch])
             self.assertContentEqual([], visible_branches)
             self.assertIn(
diff --git a/lib/lp/registry/model/sharingjob.py b/lib/lp/registry/model/sharingjob.py
index 7d10fa9..81e6a9e 100644
--- a/lib/lp/registry/model/sharingjob.py
+++ b/lib/lp/registry/model/sharingjob.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Job classes related to the sharing feature are in here."""
@@ -91,6 +91,12 @@ from lp.services.job.model.job import (
     )
 from lp.services.job.runner import BaseRunnableJob
 from lp.services.mail.sendmail import format_address_for_person
+from lp.snappy.interfaces.snap import ISnap
+from lp.snappy.model.snap import (
+    get_snap_privacy_filter,
+    Snap,
+    )
+from lp.snappy.model.snapsubscription import SnapSubscription
 
 
 class SharingJobType(DBEnumeratedType):
@@ -263,6 +269,7 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
         bug_ids = []
         branch_ids = []
         gitrepository_ids = []
+        snap_ids = []
         specification_ids = []
         if artifacts:
             for artifact in artifacts:
@@ -272,6 +279,8 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
                     branch_ids.append(artifact.id)
                 elif IGitRepository.providedBy(artifact):
                     gitrepository_ids.append(artifact.id)
+                elif ISnap.providedBy(artifact):
+                    snap_ids.append(artifact.id)
                 elif ISpecification.providedBy(artifact):
                     specification_ids.append(artifact.id)
                 else:
@@ -284,6 +293,7 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
             'bug_ids': bug_ids,
             'branch_ids': branch_ids,
             'gitrepository_ids': gitrepository_ids,
+            'snap_ids': snap_ids,
             'specification_ids': specification_ids,
             'information_types': information_types,
             'requestor.id': requestor.id
@@ -320,6 +330,10 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
         return self.metadata.get('gitrepository_ids', [])
 
     @property
+    def snap_ids(self):
+        return self.metadata.get('snap_ids', [])
+
+    @property
     def specification_ids(self):
         return self.metadata.get('specification_ids', [])
 
@@ -349,6 +363,7 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
             'bug_ids': self.bug_ids,
             'branch_ids': self.branch_ids,
             'gitrepository_ids': self.gitrepository_ids,
+            'snap_ids': self.snap_ids,
             'specification_ids': self.specification_ids,
             'pillar': getattr(self.pillar, 'name', None),
             'grantee': getattr(self.grantee, 'name', None)
@@ -365,6 +380,7 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
         bug_filters = []
         branch_filters = []
         gitrepository_filters = []
+        snap_filters = []
         specification_filters = []
 
         if self.branch_ids:
@@ -372,6 +388,8 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
         if self.gitrepository_ids:
             gitrepository_filters.append(GitRepository.id.is_in(
                 self.gitrepository_ids))
+        if self.snap_ids:
+            snap_filters.append(Snap.id.is_in(self.snap_ids))
         if self.specification_ids:
             specification_filters.append(Specification.id.is_in(
                 self.specification_ids))
@@ -387,6 +405,8 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
                 gitrepository_filters.append(
                     GitRepository.information_type.is_in(
                         self.information_types))
+                snap_filters.append(Snap._information_type.is_in(
+                    self.information_types))
                 specification_filters.append(
                     Specification.information_type.is_in(
                         self.information_types))
@@ -423,6 +443,11 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
                     Select(
                         TeamParticipation.personID,
                         where=TeamParticipation.team == self.grantee)))
+            snap_filters.append(
+                In(SnapSubscription.person_id,
+                   Select(
+                       TeamParticipation.personID,
+                       where=TeamParticipation.team == self.grantee)))
             specification_filters.append(
                 In(SpecificationSubscription.person_id,
                     Select(
@@ -466,6 +491,16 @@ class RemoveArtifactSubscriptionsJob(SharingJobDerived):
             for sub in gitrepository_subscriptions:
                 sub.repository.unsubscribe(
                     sub.person, self.requestor, ignore_permissions=True)
+        if snap_filters:
+            snap_filters.append(
+                Not(get_snap_privacy_filter(SnapSubscription.person_id)))
+            snap_subscriptions = IStore(SnapSubscription).using(
+                SnapSubscription,
+                Join(Snap, Snap.id == SnapSubscription.snap_id)
+            ).find(SnapSubscription, *snap_filters).config(distinct=True)
+            for sub in snap_subscriptions:
+                sub.snap.unsubscribe(
+                    sub.person, self.requestor, ignore_permissions=True)
         if specification_filters:
             specification_filters.append(Not(*get_specification_privacy_filter(
                 SpecificationSubscription.person_id)))
diff --git a/lib/lp/registry/services/sharingservice.py b/lib/lp/registry/services/sharingservice.py
index 01e90da..4909604 100644
--- a/lib/lp/registry/services/sharingservice.py
+++ b/lib/lp/registry/services/sharingservice.py
@@ -81,10 +81,7 @@ from lp.services.webapp.authorization import (
     available_with_permission,
     check_permission,
     )
-from lp.snappy.interfaces.snap import (
-    ISnap,
-    ISnapSet,
-    )
+from lp.snappy.interfaces.snap import ISnapSet
 
 
 @implementer(ISharingService)
@@ -332,6 +329,7 @@ class SharingService:
         bug_ids = []
         branch_ids = []
         gitrepository_ids = []
+        snap_ids = []
         for bug in bugs or []:
             if (not ignore_permissions
                 and not check_permission('launchpad.View', bug)):
@@ -344,9 +342,14 @@ class SharingService:
             branch_ids.append(branch.id)
         for gitrepository in gitrepositories or []:
             if (not ignore_permissions
-                and not check_permission('launchpad.View', gitrepository)):
+                    and not check_permission('launchpad.View', gitrepository)):
                 raise Unauthorized
             gitrepository_ids.append(gitrepository.id)
+        for snap in snaps or []:
+            if (not ignore_permissions
+                and not check_permission('launchpad.View', snap)):
+                raise Unauthorized
+            snap_ids.append(snap.id)
         for spec in specifications or []:
             if (not ignore_permissions
                 and not check_permission('launchpad.View', spec)):
@@ -376,6 +379,12 @@ class SharingService:
             visible_gitrepositories = list(
                 wanted_gitrepositories.getRepositories())
 
+        # Load the Snaps.
+        visible_snaps = []
+        if snap_ids:
+            visible_snaps = list(getUtility(ISnapSet).findByIds(
+                snap_ids, visible_by_user=person))
+
         # Load the specifications.
         visible_specs = []
         if specifications:
@@ -387,7 +396,7 @@ class SharingService:
 
         return (
             visible_bugs, visible_branches, visible_gitrepositories,
-            visible_specs)
+            visible_snaps, visible_specs)
 
     def getInvisibleArtifacts(self, person, bugs=None, branches=None,
                               gitrepositories=None):
@@ -800,12 +809,6 @@ class SharingService:
         getUtility(IAccessArtifactGrantSource).revokeByArtifact(
             artifacts_to_delete, [grantee])
 
-        # XXX: Pappacena 2021-02-05: snaps should not trigger this job,
-        # since we do not have a "SnapSubscription" yet.
-        artifacts = [i for i in artifacts if not ISnap.providedBy(i)]
-        if not artifacts:
-            return
-
         # Create a job to remove subscriptions for artifacts the grantee can no
         # longer see.
         return getUtility(IRemoveArtifactSubscriptionsJobSource).create(
diff --git a/lib/lp/registry/services/tests/test_sharingservice.py b/lib/lp/registry/services/tests/test_sharingservice.py
index 9d0e07c..8d94723 100644
--- a/lib/lp/registry/services/tests/test_sharingservice.py
+++ b/lib/lp/registry/services/tests/test_sharingservice.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -1101,7 +1101,7 @@ class TestSharingService(TestCaseWithFactory):
         # Check that grantees have expected access grants and subscriptions.
         for person in [team_grantee, person_grantee]:
             (visible_bugs, visible_branches, visible_gitrepositories,
-             visible_specs) = (
+             visible_snaps, visible_specs) = (
                 self.service.getVisibleArtifacts(
                     person, bugs=bugs, branches=branches,
                     gitrepositories=gitrepositories,
@@ -1133,7 +1133,7 @@ class TestSharingService(TestCaseWithFactory):
             for bug in bugs or []:
                 self.assertNotIn(person, bug.getDirectSubscribers())
             (visible_bugs, visible_branches, visible_gitrepositories,
-             visible_specs) = (
+             visible_snaps, visible_specs) = (
                 self.service.getVisibleArtifacts(
                     person, bugs=bugs, branches=branches,
                     gitrepositories=gitrepositories))
@@ -1783,7 +1783,8 @@ class TestSharingService(TestCaseWithFactory):
         grantee, ignore, bugs, branches, gitrepositories, specs = (
             self._make_Artifacts())
         # Check the results.
-        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
+        (shared_bugs, shared_branches, shared_gitrepositories,
+         shared_snaps, shared_specs) = (
             self.service.getVisibleArtifacts(
                 grantee, bugs=bugs, branches=branches,
                 gitrepositories=gitrepositories, specifications=specs))
@@ -1797,7 +1798,8 @@ class TestSharingService(TestCaseWithFactory):
         # user has a policy grant for the pillar of the specification.
         _, owner, bugs, branches, gitrepositories, specs = (
             self._make_Artifacts())
-        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
+        (shared_bugs, shared_branches, shared_gitrepositories,
+         shared_snaps, shared_specs) = (
             self.service.getVisibleArtifacts(
                 owner, bugs=bugs, branches=branches,
                 gitrepositories=gitrepositories, specifications=specs))
@@ -1840,7 +1842,8 @@ class TestSharingService(TestCaseWithFactory):
                 information_type=InformationType.USERDATA)
             bugs.append(bug)
 
-        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
+        (shared_bugs, shared_branches, shared_gitrepositories,
+         visible_snaps, shared_specs) = (
             self.service.getVisibleArtifacts(grantee, bugs=bugs))
         self.assertContentEqual(bugs, shared_bugs)
 
@@ -1848,7 +1851,8 @@ class TestSharingService(TestCaseWithFactory):
         for x in range(0, 5):
             change_callback(bugs[x], owner)
         # Check the results.
-        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
+        (shared_bugs, shared_branches, shared_gitrepositories,
+         visible_snaps, shared_specs) = (
             self.service.getVisibleArtifacts(grantee, bugs=bugs))
         self.assertContentEqual(bugs[5:], shared_bugs)
 
diff --git a/lib/lp/registry/tests/test_sharingjob.py b/lib/lp/registry/tests/test_sharingjob.py
index 58fa5c1..32aec0f 100644
--- a/lib/lp/registry/tests/test_sharingjob.py
+++ b/lib/lp/registry/tests/test_sharingjob.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for SharingJobs."""
@@ -39,6 +39,7 @@ from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.tests import block_on_job
 from lp.services.mail.sendmail import format_address_for_person
+from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
 from lp.testing import (
     login_person,
     person_logged_in,
@@ -127,6 +128,16 @@ class SharingJobDerivedTestCase(TestCaseWithFactory):
             'for gitrepository_ids=[%d], requestor=%s>'
             % (gitrepository.id, requestor.name), repr(job))
 
+    def test_repr_snaps(self):
+        requestor = self.factory.makePerson()
+        snap = self.factory.makeSnap()
+        job = getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+            requestor, artifacts=[snap])
+        self.assertEqual(
+            '<REMOVE_ARTIFACT_SUBSCRIPTIONS job reconciling subscriptions '
+            'for requestor=%s, snap_ids=[%d]>'
+            % (requestor.name, snap.id), repr(job))
+
     def test_repr_specifications(self):
         requestor = self.factory.makePerson()
         specification = self.factory.makeSpecification()
@@ -241,9 +252,11 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
     layer = CeleryJobLayer
 
     def setUp(self):
-        self.useFixture(FeatureFixture({
+        features = {
             'jobs.celery.enabled_classes': 'RemoveArtifactSubscriptionsJob',
-        }))
+        }
+        features.update(SNAP_TESTING_FLAGS)
+        self.useFixture(FeatureFixture(features))
         super(RemoveArtifactSubscriptionsJobTestCase, self).setUp()
 
     def test_create(self):
@@ -315,6 +328,9 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         gitrepository = self.factory.makeGitRepository(
             owner=owner, target=product,
             information_type=InformationType.USERDATA)
+        snap = self.factory.makeSnap(
+            owner=owner, registrant=owner, project=product,
+            information_type=InformationType.USERDATA)
         specification = self.factory.makeSpecification(
             owner=owner, product=product,
             information_type=InformationType.PROPRIETARY)
@@ -332,6 +348,7 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         gitrepository.subscribe(artifact_indirect_grantee,
             BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.NOEMAIL, owner)
+        snap.subscribe(artifact_indirect_grantee, owner)
         # Subscribing somebody to a specification does not automatically
         # create an artifact grant.
         spec_artifact = self.factory.makeAccessArtifact(specification)
@@ -341,10 +358,11 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
             specification.subscribe(artifact_indirect_grantee, owner)
 
         # pick one of the concrete artifacts (bug, branch, Git repository,
-        # or spec) and subscribe the teams and persons.
+        # snap, or spec) and subscribe the teams and persons.
         concrete_artifact, get_pillars, get_subscribers = configure_test(
-            bug, branch, gitrepository, specification, policy_team_grantee,
-            policy_indirect_grantee, artifact_team_grantee, owner)
+            bug, branch, gitrepository, snap, specification,
+            policy_team_grantee, policy_indirect_grantee,
+            artifact_team_grantee, owner)
 
         # Subscribing policy_team_grantee has created an artifact grant so we
         # need to revoke that to test the job.
@@ -377,6 +395,7 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         self.assertIn(artifact_indirect_grantee, bug.getDirectSubscribers())
         self.assertIn(artifact_indirect_grantee, branch.subscribers)
         self.assertIn(artifact_indirect_grantee, gitrepository.subscribers)
+        self.assertIn(artifact_indirect_grantee, snap.subscribers)
         self.assertIn(artifact_indirect_grantee,
                       removeSecurityProxy(specification).subscribers)
 
@@ -389,7 +408,7 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
             return removeSecurityProxy(
                 concrete_artifact).getDirectSubscribers()
 
-        def configure_test(bug, branch, gitrepository, specification,
+        def configure_test(bug, branch, gitrepository, snap, specification,
                            policy_team_grantee, policy_indirect_grantee,
                            artifact_team_grantee, owner):
             concrete_artifact = bug
@@ -409,7 +428,7 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         def get_subscribers(concrete_artifact):
             return concrete_artifact.subscribers
 
-        def configure_test(bug, branch, gitrepository, specification,
+        def configure_test(bug, branch, gitrepository, snap, specification,
                            policy_team_grantee, policy_indirect_grantee,
                            artifact_team_grantee, owner):
             concrete_artifact = branch
@@ -438,7 +457,7 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         def get_subscribers(concrete_artifact):
             return concrete_artifact.subscribers
 
-        def configure_test(bug, branch, gitrepository, specification,
+        def configure_test(bug, branch, gitrepository, snap, specification,
                            policy_team_grantee, policy_indirect_grantee,
                            artifact_team_grantee, owner):
             concrete_artifact = gitrepository
@@ -459,6 +478,26 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         self._assert_artifact_change_unsubscribes(
             change_callback, configure_test)
 
+    def _assert_snap_change_unsubscribes(self, change_callback):
+
+        def get_pillars(concrete_artifact):
+            return [concrete_artifact.project]
+
+        def get_subscribers(concrete_artifact):
+            return concrete_artifact.subscribers
+
+        def configure_test(bug, branch, gitrepository, snap, specification,
+                           policy_team_grantee, policy_indirect_grantee,
+                           artifact_team_grantee, owner):
+            concrete_artifact = snap
+            snap.subscribe(policy_team_grantee, owner)
+            snap.subscribe(policy_indirect_grantee, owner)
+            snap.subscribe(artifact_team_grantee, owner)
+            return concrete_artifact, get_pillars, get_subscribers
+
+        self._assert_artifact_change_unsubscribes(
+            change_callback, configure_test)
+
     def _assert_specification_change_unsubscribes(self, change_callback):
 
         def get_pillars(concrete_artifact):
@@ -467,7 +506,7 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
         def get_subscribers(concrete_artifact):
             return concrete_artifact.subscribers
 
-        def configure_test(bug, branch, gitrepository, specification,
+        def configure_test(bug, branch, gitrepository, snap, specification,
                            policy_team_grantee, policy_indirect_grantee,
                            artifact_team_grantee, owner):
             naked_spec = removeSecurityProxy(specification)
@@ -496,6 +535,13 @@ class RemoveArtifactSubscriptionsJobTestCase(TestCaseWithFactory):
 
         self._assert_gitrepository_change_unsubscribes(change_information_type)
 
+    def test_change_information_type_snap(self):
+        def change_information_type(snap):
+            removeSecurityProxy(snap).information_type = (
+                InformationType.PRIVATESECURITY)
+
+        self._assert_snap_change_unsubscribes(change_information_type)
+
     def test_change_information_type_specification(self):
         def change_information_type(specification):
             removeSecurityProxy(specification).information_type = (
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 31708c3..71f534b 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -571,6 +571,10 @@ class ISnapView(Interface):
         # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
         value_type=Reference(schema=Interface), readonly=True)))
 
+    subscribers = CollectionField(
+        title=_("Persons subscribed to this snap recipe."),
+        readonly=True, value_type=Reference(IPerson))
+
     def visibleByUser(user):
         """Can the specified user see this snap recipe?"""
 
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 141f27a..ba89613 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -5,6 +5,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 __all__ = [
+    'get_snap_privacy_filter',
     'Snap',
     ]
 
@@ -26,6 +27,7 @@ from storm.expr import (
     And,
     Coalesce,
     Desc,
+    Exists,
     Join,
     LeftJoin,
     Not,
@@ -71,6 +73,7 @@ from lp.app.errors import (
     SubscriptionPrivacyViolation,
     UserCannotUnsubscribePerson,
     )
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.security import IAuthorization
 from lp.app.interfaces.services import IService
 from lp.buildmaster.enums import BuildStatus
@@ -132,6 +135,7 @@ from lp.registry.model.accesspolicy import (
     reconcile_access_for_artifact,
     )
 from lp.registry.model.distroseries import DistroSeries
+from lp.registry.model.person import Person
 from lp.registry.model.series import ACTIVE_STATUSES
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
@@ -1165,6 +1169,13 @@ class Snap(Storm, WebhookTargetMixin):
             person.is_team and
             person.anyone_can_join())
 
+    @property
+    def subscribers(self):
+        return Store.of(self).find(
+            Person,
+            SnapSubscription.person_id == Person.id,
+            SnapSubscription.snap == self)
+
     def subscribe(self, person, subscribed_by, ignore_permissions=False):
         """See `ISnap`."""
         if not self._userCanBeSubscribed(person):
@@ -1177,9 +1188,12 @@ class Snap(Storm, WebhookTargetMixin):
                 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)
+        _, _, _, snaps, _ = service.getVisibleArtifacts(
+            person, snaps=[self], ignore_permissions=True)
+        if not snaps:
+            service.ensureAccessGrants(
+                [person], subscribed_by, snaps=[self],
+                ignore_permissions=ignore_permissions)
 
     def unsubscribe(self, person, unsubscribed_by, ignore_permissions=False):
         """See `ISnap`."""
@@ -1421,9 +1435,12 @@ class SnapSet:
             expressions.append(Snap.owner == owner)
         return IStore(Snap).find(Snap, *expressions)
 
-    def findByIds(self, snap_ids):
+    def findByIds(self, snap_ids, visible_by_user=None):
         """See `ISnapSet`."""
-        return IStore(ISnap).find(Snap, Snap.id.is_in(snap_ids))
+        clauses = [Snap.id.is_in(snap_ids)]
+        if visible_by_user is not None:
+            clauses.append(get_snap_privacy_filter(visible_by_user))
+        return IStore(Snap).find(Snap, *clauses)
 
     def findByOwner(self, owner):
         """See `ISnapSet`."""
@@ -1694,9 +1711,11 @@ class SnapStoreSecretsEncryptedContainer(NaClEncryptedContainerBase):
 
 
 def get_snap_privacy_filter(user):
-    """Returns the filter for all private Snaps that the given user is
-    subscribed to (that is, has access without being directly an owner).
+    """Returns the filter for all Snaps that the given user has access to,
+    including private snaps where the user has proper permission.
 
+    :param user: An IPerson, or a class attribute that references an IPerson
+                 in the database.
     :return: A storm condition.
     """
     # XXX pappacena 2021-02-12: Once we do the migration to back fill
@@ -1707,10 +1726,6 @@ def get_snap_privacy_filter(user):
     if user is None:
         return private_snap == False
 
-    roles = IPersonRoles(user)
-    if roles.in_admin or roles.in_commercial_admin:
-        return True
-
     artifact_grant_query = Coalesce(
         ArrayIntersects(
             SQL("%s.access_grants" % Snap.__storm_table__),
@@ -1732,4 +1747,13 @@ def get_snap_privacy_filter(user):
                 where=(TeamParticipation.person == user)
             )), False)
 
-    return Or(private_snap == False, artifact_grant_query, policy_grant_query)
+    admin_team_id = getUtility(ILaunchpadCelebrities).admin.id
+    user_is_admin = Exists(Select(
+        TeamParticipation.personID,
+        tables=[TeamParticipation],
+        where=And(
+            TeamParticipation.teamID == admin_team_id,
+            TeamParticipation.person == user)))
+    return Or(
+        private_snap == False, artifact_grant_query, policy_grant_query,
+        user_is_admin)
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 39362b3..04ed945 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -3102,8 +3102,7 @@ class TestSnapWebservice(TestCaseWithFactory):
             ws_snaps = [
                 self.webservice.getAbsoluteUrl(api_url(snap))
                 for snap in snaps]
-        commercial_admin = (
-            getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
         logout()
 
         # Anonymous requests can only see public snaps.
@@ -3141,15 +3140,15 @@ class TestSnapWebservice(TestCaseWithFactory):
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
 
         # Admins can see all snaps with this URL.
-        commercial_admin_webservice = webservice_for_person(
-            commercial_admin, permission=OAuthPermission.READ_PRIVATE)
-        response = commercial_admin_webservice.named_get(
+        admin_webservice = webservice_for_person(
+            admin, permission=OAuthPermission.READ_PRIVATE)
+        response = admin_webservice.named_get(
             "/+snaps", "findByURL", url=urls[0], api_version="devel")
         self.assertEqual(200, response.status)
         self.assertContentEqual(
             ws_snaps[:4],
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
-        response = commercial_admin_webservice.named_get(
+        response = admin_webservice.named_get(
             "/+snaps", "findByURL", url=urls[0], owner=person_urls[0],
             api_version="devel")
         self.assertEqual(200, response.status)
@@ -3180,8 +3179,7 @@ class TestSnapWebservice(TestCaseWithFactory):
             ws_snaps = [
                 self.webservice.getAbsoluteUrl(api_url(snap))
                 for snap in snaps]
-        commercial_admin = (
-            getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
         logout()
         prefix = "https://git.example.org/foo/"
 
@@ -3222,16 +3220,16 @@ class TestSnapWebservice(TestCaseWithFactory):
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
 
         # Admins can see all snaps with this URL prefix.
-        commercial_admin_webservice = webservice_for_person(
-            commercial_admin, permission=OAuthPermission.READ_PRIVATE)
-        response = commercial_admin_webservice.named_get(
+        admin_webservice = webservice_for_person(
+            admin, permission=OAuthPermission.READ_PRIVATE)
+        response = admin_webservice.named_get(
             "/+snaps", "findByURLPrefix", url_prefix=prefix,
             api_version="devel")
         self.assertEqual(200, response.status)
         self.assertContentEqual(
             ws_snaps[:8],
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
-        response = commercial_admin_webservice.named_get(
+        response = admin_webservice.named_get(
             "/+snaps", "findByURLPrefix", url_prefix=prefix,
             owner=person_urls[0], api_version="devel")
         self.assertEqual(200, response.status)
@@ -3264,8 +3262,7 @@ class TestSnapWebservice(TestCaseWithFactory):
             ws_snaps = [
                 self.webservice.getAbsoluteUrl(api_url(snap))
                 for snap in snaps]
-        commercial_admin = (
-            getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
         logout()
         prefixes = [
             "https://git.example.org/foo/", "https://git.example.org/bar/"]
@@ -3307,16 +3304,16 @@ class TestSnapWebservice(TestCaseWithFactory):
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
 
         # Admins can see all snaps with any of these URL prefixes.
-        commercial_admin_webservice = webservice_for_person(
-            commercial_admin, permission=OAuthPermission.READ_PRIVATE)
-        response = commercial_admin_webservice.named_get(
+        admin_webservice = webservice_for_person(
+            admin, permission=OAuthPermission.READ_PRIVATE)
+        response = admin_webservice.named_get(
             "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
             api_version="devel")
         self.assertEqual(200, response.status)
         self.assertContentEqual(
             ws_snaps[:16],
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
-        response = commercial_admin_webservice.named_get(
+        response = admin_webservice.named_get(
             "/+snaps", "findByURLPrefixes", url_prefixes=prefixes,
             owner=person_urls[0], api_version="devel")
         self.assertEqual(200, response.status)
@@ -3341,8 +3338,7 @@ class TestSnapWebservice(TestCaseWithFactory):
             ws_snaps = [
                 self.webservice.getAbsoluteUrl(api_url(snap))
                 for snap in snaps]
-        commercial_admin = (
-            getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
         logout()
 
         # Anonymous requests can only see public snaps.
@@ -3382,16 +3378,16 @@ class TestSnapWebservice(TestCaseWithFactory):
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
 
         # Admins can see all snaps with this store name.
-        commercial_admin_webservice = webservice_for_person(
-            commercial_admin, permission=OAuthPermission.READ_PRIVATE)
-        response = commercial_admin_webservice.named_get(
+        admin_webservice = webservice_for_person(
+            admin, permission=OAuthPermission.READ_PRIVATE)
+        response = admin_webservice.named_get(
             "/+snaps", "findByStoreName", store_name=store_names[0],
             api_version="devel")
         self.assertEqual(200, response.status)
         self.assertContentEqual(
             ws_snaps[:4],
             [entry["self_link"] for entry in response.jsonBody()["entries"]])
-        response = commercial_admin_webservice.named_get(
+        response = admin_webservice.named_get(
             "/+snaps", "findByStoreName", store_name=store_names[0],
             owner=person_urls[0], api_version="devel")
         self.assertEqual(200, response.status)