launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #26261
[Merge] ~pappacena/launchpad:snap-pillar-subscribe into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:snap-pillar-subscribe into launchpad:master with ~pappacena/launchpad:snap-pillar-reconcile-access as a prerequisite.
Commit message:
Adding SnapSubscription model
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397755
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:snap-pillar-subscribe into launchpad:master.
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 29990e3..599f486 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -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).
#
# Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
@@ -302,6 +302,7 @@ public.snapbuild = SELECT, INSERT, UPDATE, DELETE
public.snapbuildjob = SELECT, INSERT, UPDATE, DELETE
public.snapfile = SELECT, INSERT, UPDATE, DELETE
public.snapjob = SELECT, INSERT, UPDATE, DELETE
+public.snapsubscription = SELECT, INSERT, UPDATE, DELETE
public.snappydistroseries = SELECT, INSERT, UPDATE, DELETE
public.snappyseries = SELECT, INSERT, UPDATE, DELETE
public.sourcepackageformatselection = SELECT
@@ -2245,6 +2246,7 @@ type=user
[person-merge-job]
groups=script
+public.accesspolicyartifact = SELECT
public.accessartifactgrant = SELECT, UPDATE, DELETE
public.accesspolicy = SELECT, UPDATE, DELETE
public.accesspolicygrant = SELECT, UPDATE, DELETE
@@ -2362,6 +2364,7 @@ public.signedcodeofconduct = SELECT, UPDATE
public.snap = SELECT, UPDATE
public.snapbase = SELECT, UPDATE
public.snapbuild = SELECT, UPDATE
+public.snapsubscription = SELECT, UPDATE, DELETE
public.snappyseries = SELECT, UPDATE
public.sourcepackagename = SELECT
public.sourcepackagepublishinghistory = SELECT, UPDATE
diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
index 7873a61..d788e9f 100644
--- a/lib/lp/registry/personmerge.py
+++ b/lib/lp/registry/personmerge.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 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).
"""Person/team merger implementation."""
@@ -672,6 +672,24 @@ def _mergeSnap(cur, from_person, to_person):
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,6 +935,9 @@ def merge_people(from_person, to_person, reviewer, delete=False):
_mergeSnap(cur, from_person, to_person)
skip.append(('snap', 'owner'))
+ _mergeSnapSubscription(cur, from_id, to_id)
+ skip.append(('snapsubscription', 'person'))
+
_mergeOCIRecipe(cur, from_person, to_person)
skip.append(('ocirecipe', 'owner'))
diff --git a/lib/lp/registry/tests/test_personmerge.py b/lib/lp/registry/tests/test_personmerge.py
index 02fefcc..d080746 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,7 +49,10 @@ 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,
@@ -56,6 +60,7 @@ from lp.soyuz.interfaces.livefs import (
)
from lp.testing import (
celebrity_logged_in,
+ login_admin,
login_person,
person_logged_in,
TestCaseWithFactory,
@@ -664,6 +669,34 @@ 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(
+ name=u'foo', private=True))
+ self._do_premerge(duplicate, mergee)
+ login_admin()
+ snap.subscribe(duplicate, snap.owner)
+ 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))
+
+ 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/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 ce73636..b4120c6 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -61,7 +61,10 @@ from lp.app.browser.tales import (
DateTimeFormatterAPI,
)
from lp.app.enums import InformationType
-from lp.app.errors import IncompatibleArguments
+from lp.app.errors import (
+ IncompatibleArguments,
+ SubscriptionPrivacyViolation,
+ )
from lp.app.interfaces.security import IAuthorization
from lp.app.interfaces.services import IService
from lp.buildmaster.enums import BuildStatus
@@ -193,6 +196,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,
@@ -1081,9 +1085,35 @@ class Snap(Storm, WebhookTargetMixin):
Snap.id == self.id,
visibility_clause).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):
"""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])
@@ -1092,6 +1122,12 @@ class Snap(Storm, WebhookTargetMixin):
service = getUtility(IService, "sharing")
service.revokeAccessGrants(
self.pillar, person, unsubscribed_by, snaps=[self])
+ subscription = self._getSubscription(person)
+ # It should never be None, since we always create a SnapSubscription
+ # on Snap.subscribe. But just in case...
+ if subscription is not None:
+ store = Store.of(subscription)
+ store.remove(subscription)
IStore(self).flush()
def _reconcileAccess(self):
diff --git a/lib/lp/snappy/model/snapsubscription.py b/lib/lp/snappy/model/snapsubscription.py
new file mode 100644
index 0000000..95a0026
--- /dev/null
+++ b/lib/lp/snappy/model/snapsubscription.py
@@ -0,0 +1,61 @@
+# 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=None, person=None, subscribed_by=None):
+ 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.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 c3b2fb5..c1dca50 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,
@@ -1347,6 +1348,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()
pillar = self.factory.makeProduct(owner=owner)
@@ -1383,11 +1387,20 @@ 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_reconcile_set_public(self):
owner = self.factory.makePerson()
@@ -1396,11 +1409,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.setPrivate(False)
- 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()
@@ -1419,10 +1445,24 @@ class TestSnapVisibility(TestCaseWithFactory):
snap.subscribe(another_person, owner)
self.assertTrue(snap.visibleByUser(another_person))
self.assertEqual(1, 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(1, 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):
Follow ups
