launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Thanks Colin, 

I understand and agree will all your comments, they will be addressed shortly.

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."""

fixed.

>      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)

Nice, it will forward to ViewPublicOrPrivateTeamMembers.checkAuthenticated() on the snap owner, which does what we need. Thanks.

>  
>  
>  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')

New vocabulary, huge name AllUserTeamsParticipationPlusSelfSimpleDisplay ... Thanks for the suggestion.

>          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']

Right, makes sense, I will restrict 'private' changes to +admin and calculate it on creation, basically snaps for private team or branches will be private.

>      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.

Yup, in practice privileged users (owning private teams or branches) will get private snaps and only 'admins' will be capable of privatising public snaps if required.

> +        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.

fixed, sorry.

> +        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):

correct, my (emacs) mistake.

>      """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.
> +

agreed.

>          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 mean snaps with builds from private archives ... makes sense.

> +
> +        # 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.
> +

Right, fixed.

>          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,
> +        }

fixed, thanks.

>          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.