launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #26620
[Merge] ~pappacena/launchpad:snap-pillar-subscribe-removal-job into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:snap-pillar-subscribe-removal-job into launchpad:master.
Commit message:
Remove dangling SnapSubscription when changing Snap pillar's sharing policy
Requested reviews:
Colin Watson (cjwatson)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399509
--
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad: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)
Follow ups
