launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #26587
Re: [Merge] ~pappacena/launchpad:snap-pillar-subscribe-ui into launchpad:master
Review: Approve
Diff comments:
> diff --git a/lib/lp/security.py b/lib/lp/security.py
> index 35c8b8c..8192755 100644
> --- a/lib/lp/security.py
> +++ b/lib/lp/security.py
> @@ -3298,17 +3299,18 @@ class ViewSnap(AuthorizationBase):
> permission = 'launchpad.View'
> usedfor = ISnap
>
> - def checkUnauthenticated(self):
> - return not self.obj.private
> -
> def checkAuthenticated(self, user):
> - if not self.obj.private:
> + # Check user visibility first: public snaps should be visible to
> + # anyone immediately. Doing this check first can save us some
> + # queries done by the not-so-common cases checked below.
Although the team membership queries are probably very fast and will often be cached, while in the private snap case the visibleByUser query may be slower.
Also, per one of your other MPs, I thought that the snap privacy filter was doing admin checks, and it should also handle ownership since the owner should have a subscription. So maybe this method can just be `return self.obj.visibleByUser(user.person)`?
> + if self.obj.visibleByUser(user.person):
> + return True
> + if user.isOwner(self.obj) or user.in_commercial_admin or user.in_admin:
> return True
> + return False
>
> - return (
> - user.isOwner(self.obj) or
> - user.in_commercial_admin or
> - user.in_admin)
> + def checkUnauthenticated(self):
> + return self.obj.visibleByUser(None)
>
>
> class EditSnap(AuthorizationBase):
> diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
> index 7a62458..bd3926c 100644
> --- a/lib/lp/snappy/browser/snap.py
> +++ b/lib/lp/snappy/browser/snap.py
> @@ -182,12 +191,31 @@ class SnapContextMenu(ContextMenu):
>
> facet = 'overview'
>
> - links = ('request_builds',)
> + links = ('request_builds', 'add_subscriber', 'subscription')
>
> @enabled_with_permission('launchpad.Edit')
> def request_builds(self):
> return Link('+request-builds', 'Request builds', icon='add')
>
> + @enabled_with_permission("launchpad.AnyPerson")
> + def subscription(self):
> + if self.context.hasSubscription(self.user):
> + url = "+subscription/%s" % self.user.name
> + text = "Edit your subscription"
> + icon = "edit"
> + elif self.context.userCanBeSubscribed(self.user):
I'm not sure this extra check is needed here. `self.user` is never a team, so `userCanBeSubscribed` will always return True. I don't think there'll ever be a reason that you wouldn't be able to subscribe to a snap recipe that you can see, aside from the case where you're already subscribed.
> + url = "+subscribe"
> + text = "Subscribe yourself"
> + icon = "add"
> + else:
> + return None
> + return Link(url, text, icon=icon)
> +
> + @enabled_with_permission("launchpad.Edit")
For all other types of subscriptions that I can see, anyone who can view the object can add a subscriber (so e.g. `GitRepositoryContextMenu.add_subscriber` just uses `@enabled_with_permission("launchpad.AnyPerson")`). Does this need to be different?
> + def add_subscriber(self):
> + text = "Subscribe someone else"
> + return Link("+addsubscriber", text, icon="add")
> +
>
> class SnapView(LaunchpadView):
> """Default view of a Snap."""
> diff --git a/lib/lp/snappy/browser/snapsubscription.py b/lib/lp/snappy/browser/snapsubscription.py
> new file mode 100644
> index 0000000..a11cbc2
> --- /dev/null
> +++ b/lib/lp/snappy/browser/snapsubscription.py
> @@ -0,0 +1,180 @@
> +# Copyright 2020-2021 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Snap subscription views."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> + 'SnapPortletSubscribersContent'
> +]
> +
> +from zope.component._api import getUtility
> +from zope.formlib.form import action
> +from zope.security.interfaces import ForbiddenAttribute
> +
> +from lp.app.browser.launchpadform import (
> + LaunchpadEditFormView,
> + LaunchpadFormView,
> + )
> +from lp.registry.interfaces.person import IPersonSet
> +from lp.services.webapp import (
> + canonical_url,
> + LaunchpadView,
> + )
> +from lp.services.webapp.authorization import (
> + check_permission,
> + precache_permission_for_objects,
> + )
> +from lp.snappy.interfaces.snapsubscription import ISnapSubscription
> +
> +
> +class SnapPortletSubscribersContent(LaunchpadView):
> + """View for the contents for the subscribers portlet."""
> +
> + def subscriptions(self):
> + """Return a decorated list of Snap recipe subscriptions."""
> +
> + # Cache permissions so private subscribers can be rendered.
> + # The security adaptor will do the job also but we don't want or
> + # need the expense of running several complex SQL queries.
> + subscriptions = list(self.context.subscriptions)
> + person_ids = [sub.person.id for sub in subscriptions]
> + list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
> + person_ids, need_validity=True))
> + if self.user is not None:
> + subscribers = [
> + subscription.person for subscription in subscriptions]
> + precache_permission_for_objects(
> + self.request, "launchpad.LimitedView", subscribers)
> +
> + visible_subscriptions = [
> + subscription for subscription in subscriptions
> + if check_permission("launchpad.LimitedView", subscription.person)]
> + return sorted(
> + visible_subscriptions,
> + key=lambda subscription: subscription.person.displayname)
> +
> +
> +class RedirectToSnapMixin:
> + @property
> + def next_url(self):
> + if self.snap.visibleByUser(self.user):
> + return canonical_url(self.snap)
> + # If the subscriber can no longer see the Snap recipe, tries to
> + # redirect to the project page.
> + try:
> + project = self.snap.project
> + if project is not None and project.userCanLimitedView(self.user):
> + return canonical_url(self.snap.project)
> + except ForbiddenAttribute:
> + pass
> + # If not possible, redirect user back to its own page.
> + return canonical_url(self.user)
> +
> + cancel_url = next_url
> +
> +
> +class SnapSubscriptionEditView(RedirectToSnapMixin, LaunchpadEditFormView):
> + """The view for editing Snap recipe subscriptions."""
> + schema = ISnapSubscription
> + field_names = []
> +
> + @property
> + def page_title(self):
> + return (
> + "Edit subscription to Snap recipe %s" %
I think "snap recipe" should be in sentence case for all user-visible text here.
> + self.snap.displayname)
> +
> + @property
> + def label(self):
> + return (
> + "Edit subscription to Snap recipe for %s" %
> + self.person.displayname)
> +
> + def initialize(self):
> + self.snap = self.context.snap
> + self.person = self.context.person
> + super(SnapSubscriptionEditView, self).initialize()
> +
> + @action("Unsubscribe", name="unsubscribe")
> + def unsubscribe_action(self, action, data):
> + """Unsubscribe the team from the Snap recipe."""
> + self.snap.unsubscribe(self.person, self.user)
> + self.request.response.addNotification(
> + "%s has been unsubscribed from this Snap recipe."
> + % self.person.displayname)
> +
> +
> +class _SnapSubscriptionCreationView(RedirectToSnapMixin, LaunchpadFormView):
> + """Contains the common functionality of the Add and Edit views."""
> +
> + schema = ISnapSubscription
> + field_names = []
> +
> + def initialize(self):
> + self.snap = self.context
> + super(_SnapSubscriptionCreationView, self).initialize()
> +
> +
> +class SnapSubscriptionAddView(_SnapSubscriptionCreationView):
> +
> + page_title = label = "Subscribe to Snap recipe"
> +
> + @action("Subscribe")
> + def subscribe(self, action, data):
> + # To catch the stale post problem, check that the user is not
> + # subscribed before continuing.
> + if self.context.hasSubscription(self.user):
> + self.request.response.addNotification(
> + "You are already subscribed to this Snap recipe.")
> + else:
> + self.context.subscribe(self.user, self.user)
> +
> + self.request.response.addNotification(
> + "You have subscribed to this Snap recipe.")
> +
> +
> +class SnapSubscriptionAddOtherView(_SnapSubscriptionCreationView):
> + """View used to subscribe someone other than the current user."""
> +
> + field_names = ["person"]
> + for_input = True
> +
> + # Since we are subscribing other people, the current user
> + # is never considered subscribed.
> + user_is_subscribed = False
> +
> + page_title = label = "Subscribe to Snap recipe"
> +
> + def validate(self, data):
> + if "person" in data:
> + person = data["person"]
> + subscription = self.context.getSubscription(person)
> + if (subscription is None
> + and not self.context.userCanBeSubscribed(person)):
> + self.setFieldError(
> + "person",
> + "Open and delegated teams cannot be subscribed to "
> + "private Snap recipes.")
> +
> + @action("Subscribe", name="subscribe_action")
> + def subscribe_action(self, action, data):
> + """Subscribe the specified user to the Snap recipe.
> +
> + The user must be a member of a team in order to subscribe that team
> + to the Snap recipe. Launchpad Admins are special and they can
> + subscribe any team.
Hm. This comment is basically the same as one for `BranchSubscription` and `GitSubscription`, but I can't actually find what checks that constraint anywhere, and I wonder whether it's still true. There was such a constraint at one point, but for `BranchSubscription` it was removed in commit 0650a124e1 when adding `subscribed_by`. Is it possible that this is a vestigial comment in all three places and should be removed?
> + """
> + person = data["person"]
> + subscription = self.context.getSubscription(person)
> + if subscription is None:
> + self.context.subscribe(person, self.user)
> + self.request.response.addNotification(
> + "%s has been subscribed to this Snap recipe." %
> + person.displayname)
> + else:
> + self.request.response.addNotification(
> + "%s was already subscribed to this Snap recipe with." %
Drop the final "with".
> + person.displayname)
> diff --git a/lib/lp/snappy/configure.zcml b/lib/lp/snappy/configure.zcml
> index a16c664..05529f8 100644
> --- a/lib/lp/snappy/configure.zcml
> +++ b/lib/lp/snappy/configure.zcml
> @@ -42,6 +42,15 @@
> <allow interface="lp.snappy.interfaces.snap.ISnapSet" />
> </securedutility>
>
> + <!-- SnapSubscription -->
> +
> + <class class="lp.snappy.model.snapsubscription.SnapSubscription">
> + <allow interface="lp.snappy.interfaces.snapsubscription.ISnapSubscription"/>
> + <require
> + permission="zope.Public"
I wonder why we made this `zope.Public` for `BranchSubscription` and `GitSubscription`. Can you see anything that goes wrong if you change this to `launchpad.Edit`? Similarly, maybe the `<allow />` above should be `<require permission="launchpad.View" />` instead, so that security isn't enforced solely at the browser layer.
> + set_schema="lp.snappy.interfaces.snapsubscription.ISnapSubscription"/>
> + </class>
> +
> <!-- SnapBuildRequest -->
> <class class="lp.snappy.model.snap.SnapBuildRequest">
> <require
> diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
> index b423263..c33ea82 100644
> --- a/lib/lp/snappy/interfaces/snap.py
> +++ b/lib/lp/snappy/interfaces/snap.py
> @@ -571,10 +571,25 @@ class ISnapView(Interface):
> # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
> value_type=Reference(schema=Interface), readonly=True)))
>
> + subscriptions = CollectionField(
> + title=_("SnapSubscriptions associated with this repository."),
"... with this snap recipe"
> + readonly=True,
> + # Really IGitSubscription, patched in _schema_circular_imports.py.
`ISnapSubscription`. (Also, it's not currently patched in `_schema_circular_imports.py`; presumably this is waiting for a later branch that adds webservice exports here, but the comment should be deferred until then.)
> + value_type=Reference(Interface))
> +
> subscribers = CollectionField(
> - title=_("Persons subscribed to this repository."),
> + title=_("Persons subscribed to this snap recipe."),
> readonly=True, value_type=Reference(IPerson))
>
> + def getSubscription(person):
> + """Returns the person's snap subscription for this snap recipe."""
> +
> + def hasSubscription(person):
> + """Is this person subscribed to the snap recipe?"""
> +
> + def userCanBeSubscribed(person):
> + """Checks if the given person can be subscribed to this snap recipe."""
> +
> def visibleByUser(user):
> """Can the specified user see this snap recipe?"""
>
> @@ -584,6 +599,9 @@ class ISnapView(Interface):
> If the user is a Launchpad admin, any type is acceptable.
> """
>
> + def unsubscribe(person, unsubscribed_by):
> + """Unsubscribe a person to this snap recipe."""
One subscribes to a thing, but unsubscribes from a thing.
> +
>
> class ISnapEdit(IWebhookTarget):
> """`ISnap` methods that require launchpad.Edit permission."""
> diff --git a/lib/lp/snappy/templates/snap-index.pt b/lib/lp/snappy/templates/snap-index.pt
> index 7d3083e..5c51a5d 100644
> --- a/lib/lp/snappy/templates/snap-index.pt
> +++ b/lib/lp/snappy/templates/snap-index.pt
> @@ -58,10 +59,13 @@
> <dl id="source"
> tal:define="source context/source" tal:condition="source">
> <dt>Source:</dt>
> - <dd>
> + <dd tal:condition="view/user_can_see_source">
> <a tal:replace="structure source/fmt:link"/>
> <a tal:replace="structure view/menu:overview/edit/fmt:icon"/>
> </dd>
> + <dd tal:condition="not: view/user_can_see_source">
> + <span style="font-style: italic"><Redacted></span>
Nit so small it needs a microscope: we don't have many examples of this in the web UI, but in the few we have it seems to be generally "<redacted>" rather than "<Redacted>", and not italicized.
Also, perhaps consider putting `class="sprite private"` on this so that it gets a padlock icon.
> + </dd>
> </dl>
> <dl id="build_source_tarball"
> tal:define="build_source_tarball context/build_source_tarball">
--
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/398319
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-subscribe-removal-job.
References
