launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Looks like a pretty good start, with a few bits missing.  You need to make Snap.requestBuild only permit private archives if the snap itself is private, although I think we ought to keep the snap/archive owner mismatch check as well, at least for now.  I don't think SnapBuild privacy needs any other work beyond that, as it already correctly delegates to the snap and the archive.

Last I checked, there were no snaps on production with private owners or snap builds with private archives, but we should double-check that after the upgrade and ensure that they're all explicitly marked private if need be.  If you follow my suggestion for SnapBuild.is_private then I think this ordering is safe.

Diff comments:

> === modified file 'lib/lp/security.py'
> --- lib/lp/security.py	2016-01-26 15:47:37 +0000
> +++ lib/lp/security.py	2016-02-02 18:49:34 +0000
> @@ -3112,12 +3112,20 @@
>              obj, obj.webhook, 'launchpad.View')
>  
>  
> -class ViewSnap(DelegatedAuthorization):
> +class ViewSnap(AuthorizationBase):
> +    """Private snaps are only visible to its owners and admins."""

s/its/their/

>      permission = 'launchpad.View'
>      usedfor = ISnap
>  
> -    def __init__(self, obj):
> -        super(ViewSnap, self).__init__(obj, obj.owner, 'launchpad.View')
> +    def checkUnauthenticated(self):
> +        return not self.obj.private
> +
> +    def checkAuthenticated(self, user):
> +        if not self.obj.private:
> +            return True
> +        return (
> +            user.isOwner(self.obj) or
> +            user.in_commercial_admin or user.in_admin)

I think this is correct, but I had to think quite hard to satisfy myself of that.  Perhaps instead:

    def checkAuthenticated(self, user):
        return (
            not self.obj.private or
            self.forwardCheckAuthenticated(user, self.obj.owner))

?

>  
>  
>  class EditSnap(AuthorizationBase):
> 
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2015-10-08 13:58:57 +0000
> +++ lib/lp/snappy/browser/snap.py	2016-02-02 18:49:34 +0000
> @@ -146,7 +146,7 @@
>      def person_picker(self):
>          field = copy_field(
>              ISnap['owner'],
> -            vocabularyName='UserTeamsParticipationPlusSelfSimpleDisplay')
> +            vocabularyName='AllUserTeamsParticipationPlusSelf')

We do indeed need private teams, but can we keep some equivalent of the SimpleDisplay bit that uses displayname rather than unique_displayname?  (Perhaps this needs a new vocabulary.)

>          return InlinePersonEditPickerWidget(
>              self.context, field, format_link(self.context.owner),
>              header='Change owner', step_title='Select a new owner')
> @@ -291,7 +292,7 @@
>      page_title = label = 'Create a new snap package'
>  
>      schema = ISnapEditSchema
> -    field_names = ['owner', 'name', 'distro_series']
> +    field_names = ['owner', 'name', 'private', 'distro_series']

I think private probably ought to be on +admin, at least to begin with.

>      custom_widget('distro_series', LaunchpadRadioWidget)
>  
>      def initialize(self):
> 
> === modified file 'lib/lp/snappy/browser/tests/test_snap.py'
> --- lib/lp/snappy/browser/tests/test_snap.py	2015-11-26 15:46:38 +0000
> +++ lib/lp/snappy/browser/tests/test_snap.py	2016-02-02 18:49:34 +0000
> @@ -197,6 +199,47 @@
>              ["Test Person (test-person)", "Test Team (test-team)"],
>              sorted(str(option) for option in options))
>  
> +    def test_create_new_private_snap(self):
> +        # Anyone can create private snaps.

This is a bit of a departure.  For comparison (see validate_ppa), the only situations in which you're allowed to create a private PPA are if the owner is a private team, if you're a (commercial) admin, or if you have a commercial subscription to Launchpad.  I think we should follow essentially the same rules here.

> +        branch = self.factory.makeAnyBranch()
> +        owner_name = self.person.name
> +
> +        browser = self.getViewBrowser(
> +            branch, view_name="+new-snap", user=self.person)
> +        browser.getControl("Name").value = "brand-new-snap"
> +        browser.getControl("Owner").value = [owner_name]
> +        browser.getControl("Private").selected = True
> +        browser.getControl("Create snap package").click()
> +
> +        content = find_main_content(browser.contents)
> +        self.assertEqual("brand-new-snap", extract_text(content.h1))
> +        # XXX cprov 20160202: something more clever ...
> +        private_portlet_content = extract_text(
> +            first_tag_by_class(browser.contents, "portlet")
> +        ).replace('\n', ' ')
> +        self.assertEqual(
> +            'This snap contains Private information',
> +            private_portlet_content)
> +
> +    def test_create_new_snap_privacy_mismatch(self):
> +        # Private teams can only create private snaps.
> +        login_person(self.person)
> +        team = self.factory.makeTeam(
> +            owner=self.person, visibility=PersonVisibility.PRIVATE)
> +        branch = self.factory.makeAnyBranch()
> +        team_name = team.name
> +
> +        browser = self.getViewBrowser(
> +            branch, view_name="+new-snap", user=self.person)
> +        # XXX cprov 20160202: what other controls named 'Name' ?
> +        browser.getControl("Name", index=0).value = "snap-name"
> +        browser.getControl("Owner").value = [team_name]
> +        browser.getControl("Create snap package").click()
> +
> +        self.assertEqual(
> +            'This snap contains private information and cannot be public.',
> +            extract_text(find_tags_by_class(browser.contents, "message")[1]))
> +
>  
>  class TestSnapAdminView(BrowserTestCase):
>  
> @@ -329,6 +372,21 @@
>              "name.",
>              extract_text(find_tags_by_class(browser.contents, "message")[1]))
>  
> +    def test_edit_snap_privacy(self):
> +        # Cannot make snap public if there still contain private information.

"if it still contains"

> +        login_person(self.person)
> +        team = self.factory.makeTeam(
> +            owner=self.person, visibility=PersonVisibility.PRIVATE)
> +        snap = self.factory.makeSnap(
> +            registrant=self.person, owner=team, private=True)
> +        browser = self.getViewBrowser(snap, user=self.person)
> +        browser.getLink("Edit snap package").click()
> +        browser.getControl("Private").selected = False
> +        browser.getControl("Update snap package").click()
> +        self.assertEqual(
> +            'This snap contains private information and cannot be public.',
> +            extract_text(find_tags_by_class(browser.contents, "message")[1]))
> +
>      def setUpDistroSeries(self):
>          """Set up a distroseries with some available processors."""
>          distroseries = self.factory.makeUbuntuDistroSeries()
> 
> === modified file 'lib/lp/snappy/interfaces/snap.py'
> --- lib/lp/snappy/interfaces/snap.py	2015-09-25 17:26:03 +0000
> +++ lib/lp/snappy/interfaces/snap.py	2016-02-02 18:49:34 +0000
> @@ -345,7 +360,8 @@
>  
>  
>  class ISnap(
> -    ISnapView, ISnapEdit, ISnapEditableAttributes, ISnapAdminAttributes):
> +        ISnapView, ISnapEdit, ISnapEditableAttributes, ISnapAdminAttributes,
> +        IPrivacy):

It's perhaps a little odd, but I think the prevailing convention in the current codebase is to use just a single level of indentation here despite the docstring that immediately follows.

>      """A buildable snap package."""
>  
>      # XXX cjwatson 2015-07-17 bug=760849: "beta" is a lie to get WADL
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2015-10-08 14:18:29 +0000
> +++ lib/lp/snappy/model/snap.py	2016-02-02 18:49:34 +0000
> @@ -283,6 +290,9 @@
>  
>      def _getBuilds(self, filter_term, order_by):
>          """The actual query to get the builds."""
> +
> +        # XXX cprov 20160114: missing privacy checks.
> +

I don't think you need any extra privacy checks in this method.  get_enabled_archive_filter already has the restrictions you need.

>          query_args = [
>              SnapBuild.snap == self,
>              SnapBuild.archive_id == Archive.id,
> @@ -398,6 +412,23 @@
>  
>          return snap
>  
> +    def isValidPrivacy(self, private, owner, branch=None, git_ref=None):
> +        """See `ISnapSet`."""
> +        # Private snaps may contain anything.
> +        if private:
> +            return True
> +
> +        # Public snaps with private sources are not allowed.
> +        source_ref = branch or git_ref
> +        if source_ref.information_type in PRIVATE_INFORMATION_TYPES:
> +            return False

You also need a check here to forbid public snaps with private archives; and I think private=True ought to be restricted to private-teams/commercial-admins/commercial-subscribers, as noted earlier.

> +
> +        # Public snaps owned by private teams are not allowed.
> +        if owner.is_team and owner.visibility == PersonVisibility.PRIVATE:
> +            return False
> +
> +        return True
> +
>      def _getByName(self, owner, name):
>          return IStore(Snap).find(
>              Snap, Snap.owner == owner, Snap.name == name).one()
> 
> === modified file 'lib/lp/snappy/model/snapbuild.py'
> --- lib/lp/snappy/model/snapbuild.py	2015-09-16 13:30:33 +0000
> +++ lib/lp/snappy/model/snapbuild.py	2016-02-02 18:49:34 +0000
> @@ -163,6 +163,9 @@
>      @property
>      def is_private(self):
>          """See `IBuildFarmJob`."""
> +
> +        # XXX cprov 20160114: consider snap privacy.
> +

Doesn't this just turn into "return self.snap.private or self.archive.private"?  It may be worth keeping the check on self.snap.owner.private as well for now, to simplify reasoning around the upgrade.

>          return self.snap.owner.private or self.archive.private
>  
>      @property
> 
> === modified file 'lib/lp/snappy/tests/test_snap.py'
> --- lib/lp/snappy/tests/test_snap.py	2015-11-26 15:46:38 +0000
> +++ lib/lp/snappy/tests/test_snap.py	2016-02-02 18:49:34 +0000
> @@ -703,14 +766,17 @@
>          return self.webservice.getAbsoluteUrl(api_url(obj))
>  
>      def makeSnap(self, owner=None, distroseries=None, branch=None,
> -                 git_ref=None, processors=None, webservice=None):
> +                 git_ref=None, processors=None, webservice=None,
> +                 private=False):
>          if owner is None:
>              owner = self.person
>          if distroseries is None:
>              distroseries = self.factory.makeDistroSeries(registrant=owner)
>          if branch is None and git_ref is None:
>              branch = self.factory.makeAnyBranch()
> -        kwargs = {}
> +        kwargs = {
> +            'private': private,
> +        }

LP style would indent the closing brace here, but I'd just pass private=private to named_post a bit further down instead, since this parameter is always passed.

>          if webservice is None:
>              webservice = self.webservice
>          transaction.commit()


-- 
https://code.launchpad.net/~cprov/launchpad/snap-privacy-take1/+merge/284109
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.