launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/no-excess-aags into lp:launchpad.

Commit message:
Set up AccessPolicyArtifacts before subscribing the branch/repository owner, to avoid excess AccessArtifactGrants.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/no-excess-aags/+merge/273999

Set up AccessPolicyArtifacts before subscribing the branch/repository owner, to avoid excess AccessArtifactGrants.  I also removed an unnecessary bug._reconcileAccess call from BugSet.createBug, which was a confusing no-op because BugTaskSet.createTask/createManyTasks already took care of that, and the APAs are really to do with the target which is attached to the bug task so it makes more sense there anyway.

Here's the complicated background: http://irclogs.ubuntu.com/2015/10/09/%23launchpad-dev.html
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/no-excess-aags into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2015-10-08 08:59:00 +0000
+++ lib/lp/bugs/model/bug.py	2015-10-09 15:59:52 +0000
@@ -2608,7 +2608,8 @@
 
         bug, event = self._makeBug(params)
 
-        # Create the initial task on the specified target.
+        # Create the initial task on the specified target.  This also
+        # reconciles access policies for this bug based on that target.
         getUtility(IBugTaskSet).createTask(
             bug, params.owner, params.target, status=params.status)
 
@@ -2626,8 +2627,6 @@
         if params.milestone:
             bug_task.transitionToMilestone(params.milestone, params.owner)
 
-        bug._reconcileAccess()
-
         # Tell everyone.
         if notify_event:
             notify(event)

=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/branchnamespace.py	2015-10-09 15:59:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementations of `IBranchNamespace`."""
@@ -157,6 +157,7 @@
             repository_format=repository_format,
             control_format=control_format, distroseries=distroseries,
             sourcepackagename=sourcepackagename)
+        branch._reconcileAccess()
 
         # The owner of the branch should also be automatically subscribed in
         # order for them to get code review notifications.  The default
@@ -169,8 +170,6 @@
             CodeReviewNotificationLevel.FULL,
             registrant)
 
-        branch._reconcileAccess()
-
         notify(ObjectCreatedEvent(branch))
         return branch
 

=== modified file 'lib/lp/code/model/gitnamespace.py'
--- lib/lp/code/model/gitnamespace.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/gitnamespace.py	2015-10-09 15:59:52 +0000
@@ -86,6 +86,7 @@
         repository = GitRepository(
             registrant, self.owner, self.target, name, information_type,
             date_created, reviewer=reviewer, description=description)
+        repository._reconcileAccess()
 
         # The owner of the repository should also be automatically subscribed
         # in order for them to get code review notifications.  The default
@@ -98,8 +99,6 @@
             CodeReviewNotificationLevel.FULL,
             registrant)
 
-        repository._reconcileAccess()
-
         notify(ObjectCreatedEvent(repository))
 
         return repository

=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2015-01-28 16:55:54 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2015-10-09 15:59:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `IBranchNamespace` implementations."""
@@ -49,6 +49,10 @@
     NoSuchDistroSeries,
     NoSuchSourcePackageName,
     )
+from lp.registry.interfaces.accesspolicy import (
+    IAccessPolicyGrantFlatSource,
+    IAccessPolicySource,
+    )
 from lp.registry.interfaces.distribution import NoSuchDistribution
 from lp.registry.interfaces.person import NoSuchPerson
 from lp.registry.interfaces.product import NoSuchProduct
@@ -523,6 +527,29 @@
             InformationType.EMBARGOED,
             namespace.getDefaultInformationType(grantee))
 
+    def test_grantee_has_no_artifact_grant(self):
+        # The owner of a new branch in a project whose default information type
+        # is non-public does not have an artifact grant specifically for the
+        # new branch, because their existing policy grant is sufficient.
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        namespace = self.makeProjectBranchNamespace(
+            BranchSharingPolicy.PROPRIETARY, person=person)
+        with person_logged_in(namespace.product.owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                namespace.product, team, namespace.product.owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        branch = namespace.createBranch(
+            BranchType.HOSTED, self.factory.getUniqueString(), person)
+        [policy] = getUtility(IAccessPolicySource).find(
+            [(namespace.product, InformationType.PROPRIETARY)])
+        apgfs = getUtility(IAccessPolicyGrantFlatSource)
+        self.assertContentEqual(
+            [(namespace.product.owner, {policy: SharingPermission.ALL}, []),
+             (team, {policy: SharingPermission.ALL}, [])],
+            apgfs.findGranteePermissionsByPolicy([policy]))
+        self.assertTrue(removeSecurityProxy(branch).visibleByUser(person))
+
 
 class TestPackageBranchNamespace(TestCaseWithFactory, NamespaceMixin):
     """Tests for `PackageBranchNamespace`."""

=== modified file 'lib/lp/code/model/tests/test_gitnamespace.py'
--- lib/lp/code/model/tests/test_gitnamespace.py	2015-06-03 21:04:13 +0000
+++ lib/lp/code/model/tests/test_gitnamespace.py	2015-10-09 15:59:52 +0000
@@ -37,6 +37,10 @@
     PersonVisibility,
     SharingPermission,
     )
+from lp.registry.interfaces.accesspolicy import (
+    IAccessPolicyGrantFlatSource,
+    IAccessPolicySource,
+    )
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
@@ -436,6 +440,30 @@
             InformationType.EMBARGOED,
             namespace.getDefaultInformationType(grantee))
 
+    def test_grantee_has_no_artifact_grant(self):
+        # The owner of a new repository in a project whose default
+        # information type is non-public does not have an artifact grant
+        # specifically for the new repository, because their existing policy
+        # grant is sufficient.
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[person])
+        namespace = self.makeProjectGitNamespace(
+            BranchSharingPolicy.PROPRIETARY, person=person)
+        with person_logged_in(namespace.target.owner):
+            getUtility(IService, 'sharing').sharePillarInformation(
+                namespace.target, team, namespace.target.owner,
+                {InformationType.PROPRIETARY: SharingPermission.ALL})
+        repository = namespace.createRepository(
+            person, self.factory.getUniqueUnicode())
+        [policy] = getUtility(IAccessPolicySource).find(
+            [(namespace.target, InformationType.PROPRIETARY)])
+        apgfs = getUtility(IAccessPolicyGrantFlatSource)
+        self.assertContentEqual(
+            [(namespace.target.owner, {policy: SharingPermission.ALL}, []),
+             (team, {policy: SharingPermission.ALL}, [])],
+            apgfs.findGranteePermissionsByPolicy([policy]))
+        self.assertTrue(removeSecurityProxy(repository).visibleByUser(person))
+
 
 class TestPackageGitNamespace(TestCaseWithFactory, NamespaceMixin):
     """Tests for `PackageGitNamespace`."""