Merge lp:~cjwatson/launchpad/git-finish-sharing into lp:launchpad

[Colin Watson]

Use Git repository collections to implement visibility and sharing.

Use Git repository collections to implement visibility and sharing.

There are a few bits we can't do until there's some analogue of BranchSubscription, but otherwise this should be a reasonably complete privacy implementation now.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-02-26 14:25:26 +0000
+++ lib/lp/code/model/gitrepository.py	2015-02-27 11:27:39 +0000
@@ -26,6 +26,7 @@
     )
 from zope.component import getUtility
 from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import (
     InformationType,
@@ -39,6 +40,7 @@
     GitDefaultConflict,
     GitTargetError,
     )
+from lp.code.interfaces.gitcollection import IAllGitRepositories
 from lp.code.interfaces.gitlookup import IGitLookup
 from lp.code.interfaces.gitnamespace import (
     get_git_namespace,
@@ -291,9 +293,8 @@
         elif user.id in self._known_viewers:
             return True
         else:
-            # XXX cjwatson 2015-02-06: Fill this in once IGitCollection is
-            # in place.
-            return False
+            return not getUtility(IAllGitRepositories).withIds(
+                self.id).visibleByUser(user).is_empty()
 
     def getAllowedInformationTypes(self, user):
         """See `IGitRepository`."""
@@ -359,7 +360,11 @@
     def getByPath(self, user, path):
         """See `IGitRepositorySet`."""
         repository = getUtility(IGitLookup).getByPath(path)
-        if repository is not None and repository.visibleByUser(user):
+        if repository is None:
+            return None
+        # removeSecurityProxy is safe here since we're explicitly performing
+        # a permission check.
+        if removeSecurityProxy(repository).visibleByUser(user):
             return repository
         return None
 

=== modified file 'lib/lp/code/model/hasgitrepositories.py'
--- lib/lp/code/model/hasgitrepositories.py	2015-02-10 01:03:48 +0000
+++ lib/lp/code/model/hasgitrepositories.py	2015-02-27 11:27:39 +0000
@@ -8,6 +8,7 @@
 
 from zope.component import getUtility
 
+from lp.code.interfaces.gitcollection import IGitCollection
 from lp.code.interfaces.gitrepository import IGitRepositorySet
 
 
@@ -23,6 +24,5 @@
 
     def getGitRepositories(self, visible_by_user=None, eager_load=False):
         """See `IHasGitRepositories`."""
-        # XXX cjwatson 2015-02-06: Fill this in once IGitCollection is in
-        # place.
-        raise NotImplementedError
+        collection = IGitCollection(self).visibleByUser(visible_by_user)
+        return collection.getRepositories(eager_load=eager_load)

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-02-26 11:43:12 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-02-27 11:27:39 +0000
@@ -34,11 +34,21 @@
     IGitRepository,
     IGitRepositorySet,
     )
-from lp.registry.enums import BranchSharingPolicy
+from lp.registry.enums import (
+    BranchSharingPolicy,
+    PersonVisibility,
+    TeamMembershipPolicy,
+    )
+from lp.registry.interfaces.accesspolicy import (
+    IAccessArtifactSource,
+    IAccessPolicyArtifactSource,
+    IAccessPolicySource,
+    )
 from lp.registry.interfaces.persondistributionsourcepackage import (
     IPersonDistributionSourcePackageFactory,
     )
 from lp.registry.interfaces.personproduct import IPersonProductFactory
+from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
 from lp.services.database.constants import UTC_NOW
 from lp.services.webapp.authorization import check_permission
 from lp.testing import (
@@ -134,6 +144,15 @@
             self.repository_set.setDefaultRepository(project, repository)
         self.assertGitIdentity(repository, project.name)
 
+    def test_git_identity_private_default_for_project(self):
+        # Private repositories also have a short lp: URL.
+        project = self.factory.makeProduct()
+        repository = self.factory.makeGitRepository(
+            target=project, information_type=InformationType.USERDATA)
+        with admin_logged_in():
+            self.repository_set.setDefaultRepository(project, repository)
+            self.assertGitIdentity(repository, project.name)
+
     def test_git_identity_default_for_package(self):
         # If a repository is the default for a package, then its Git
         # identity uses the path to that package.
@@ -302,6 +321,57 @@
         self.assertEqual(namespace, repository.namespace)
 
 
+class TestGitRepositoryPrivacy(TestCaseWithFactory):
+    """Tests for Git repository privacy."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        # Use an admin user as we aren't checking edit permissions here.
+        super(TestGitRepositoryPrivacy, self).setUp("[e-mail]")
+
+    def test_personal_repositories_for_private_teams_are_private(self):
+        team = self.factory.makeTeam(
+            membership_policy=TeamMembershipPolicy.MODERATED,
+            visibility=PersonVisibility.PRIVATE)
+        repository = self.factory.makeGitRepository(owner=team, target=team)
+        self.assertTrue(repository.private)
+        self.assertEqual(
+            InformationType.PROPRIETARY, repository.information_type)
+
+    def test__reconcileAccess_for_project_repository(self):
+        # _reconcileAccess uses a project policy for a project repository.
+        repository = self.factory.makeGitRepository(
+            information_type=InformationType.USERDATA)
+        [artifact] = getUtility(IAccessArtifactSource).ensure([repository])
+        getUtility(IAccessPolicyArtifactSource).deleteByArtifact([artifact])
+        removeSecurityProxy(repository)._reconcileAccess()
+        self.assertContentEqual(
+            getUtility(IAccessPolicySource).find(
+                [(repository.target, InformationType.USERDATA)]),
+            get_policies_for_artifact(repository))
+
+    def test__reconcileAccess_for_package_repository(self):
+        # Git repository privacy isn't yet supported for distributions, so
+        # no AccessPolicyArtifact is created for a package repository.
+        repository = self.factory.makeGitRepository(
+            target=self.factory.makeDistributionSourcePackage(),
+            information_type=InformationType.USERDATA)
+        removeSecurityProxy(repository)._reconcileAccess()
+        self.assertEqual([], get_policies_for_artifact(repository))
+
+    def test__reconcileAccess_for_personal_repository(self):
+        # _reconcileAccess uses a person policy for a personal repository.
+        team_owner = self.factory.makeTeam()
+        repository = self.factory.makeGitRepository(
+            owner=team_owner, target=team_owner,
+            information_type=InformationType.USERDATA)
+        removeSecurityProxy(repository)._reconcileAccess()
+        self.assertContentEqual(
+            getUtility(IAccessPolicySource).findByTeam([team_owner]),
+            get_policies_for_artifact(repository))
+
+
 class TestGitRepositoryGetAllowedInformationTypes(TestCaseWithFactory):
     """Test `IGitRepository.getAllowedInformationTypes`."""
 
@@ -354,6 +424,17 @@
             self.assertFalse(
                 check_permission("launchpad.Moderate", repository))
 
+    def test_methods_smoketest(self):
+        # Users with launchpad.Moderate can call transitionToInformationType.
+        project = self.factory.makeProduct()
+        repository = self.factory.makeGitRepository(target=project)
+        with person_logged_in(project.owner):
+            project.setBranchSharingPolicy(BranchSharingPolicy.PUBLIC)
+            repository.transitionToInformationType(
+                InformationType.PRIVATESECURITY, project.owner)
+            self.assertEqual(
+                InformationType.PRIVATESECURITY, repository.information_type)
+
     def test_attribute_smoketest(self):
         # Users with launchpad.Moderate can set attributes.
         project = self.factory.makeProduct()
@@ -486,6 +567,47 @@
             repository.setTarget(target=owner, user=owner)
         self.assertEqual(owner, repository.target)
 
+    def test_private_personal_forbidden_for_public_teams(self):
+        # Only private teams can have private personal repositories.
+        owner = self.factory.makeTeam()
+        repository = self.factory.makeGitRepository(
+            owner=owner, information_type=InformationType.USERDATA)
+        with admin_logged_in():
+            self.assertRaises(
+                GitTargetError, repository.setTarget, target=owner, user=owner)
+
+    def test_private_personal_allowed_for_private_teams(self):
+        # Only private teams can have private personal repositories.
+        owner = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+        with person_logged_in(owner):
+            repository = self.factory.makeGitRepository(
+                owner=owner, information_type=InformationType.USERDATA)
+            repository.setTarget(target=owner, user=owner)
+            self.assertEqual(owner, repository.target)
+
+    def test_reconciles_access(self):
+        # setTarget calls _reconcileAccess to make the sharing schema
+        # match the new target.
+        repository = self.factory.makeGitRepository(
+            information_type=InformationType.USERDATA)
+        new_project = self.factory.makeProduct()
+        with admin_logged_in():
+            repository.setTarget(target=new_project, user=repository.owner)
+        self.assertEqual(
+            new_project, get_policies_for_artifact(repository)[0].pillar)
+
+    def test_reconciles_access_personal(self):
+        # setTarget calls _reconcileAccess to make the sharing schema
+        # correct for a private personal repository.
+        owner = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+        with person_logged_in(owner):
+            repository = self.factory.makeGitRepository(
+                owner=owner, target=owner,
+                information_type=InformationType.USERDATA)
+            repository.setTarget(target=owner, user=owner)
+        self.assertEqual(
+            owner, get_policies_for_artifact(repository)[0].person)
+
     def test_public_to_proprietary_only_project(self):
         # A repository cannot be moved to a target where the sharing policy
         # does not allow it.
@@ -525,6 +647,19 @@
         person = self.factory.makePerson()
         self.assertIsNone(self.repository_set.getByPath(person, "nonexistent"))
 
+    def test_getByPath_inaccessible(self):
+        # If the given user cannot view the matched repository, then
+        # getByPath returns None.
+        owner = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(
+            owner=owner, information_type=InformationType.USERDATA)
+        with person_logged_in(owner):
+            path = repository.shortened_path
+        self.assertEqual(
+            repository, self.repository_set.getByPath(owner, path))
+        self.assertIsNone(
+            self.repository_set.getByPath(self.factory.makePerson(), path))
+
     def test_setDefaultRepository_refuses_person(self):
         # setDefaultRepository refuses if the target is a person.
         person = self.factory.makePerson()

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2015-02-16 13:01:34 +0000
+++ lib/lp/registry/services/sharingservice.py	2015-02-27 11:27:39 +0000
@@ -9,6 +9,7 @@
     ]
 
 from itertools import product
+from operator import attrgetter
 
 from lazr.restful.interfaces import IWebBrowserOriginatingRequest
 from lazr.restful.utils import get_current_web_service_request
@@ -36,6 +37,7 @@
 from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
 from lp.code.interfaces.branchcollection import IAllBranches
+from lp.code.interfaces.gitcollection import IAllGitRepositories
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
@@ -227,7 +229,11 @@
             branches = list(wanted_branches.getBranches())
         # Load the Git repositories.
         gitrepositories = []
-        # XXX cjwatson 2015-02-16: Fill in once IGitCollection is in place.
+        if gitrepository_ids:
+            all_gitrepositories = getUtility(IAllGitRepositories)
+            wanted_gitrepositories = all_gitrepositories.visibleByUser(
+                user).withIds(*gitrepository_ids)
+            gitrepositories = list(wanted_gitrepositories.getRepositories())
         specifications = []
         if specification_ids:
             specifications = load(Specification, specification_ids)
@@ -319,48 +325,52 @@
                             gitrepositories=None, specifications=None,
                             ignore_permissions=False):
         """See `ISharingService`."""
-        bugs_by_id = {}
-        branches_by_id = {}
-        gitrepositories_by_id = {}
+        bug_ids = []
+        branch_ids = []
+        gitrepository_ids = []
         for bug in bugs or []:
             if (not ignore_permissions
                 and not check_permission('launchpad.View', bug)):
                 raise Unauthorized
-            bugs_by_id[bug.id] = bug
+            bug_ids.append(bug.id)
         for branch in branches or []:
             if (not ignore_permissions
                 and not check_permission('launchpad.View', branch)):
                 raise Unauthorized
-            branches_by_id[branch.id] = branch
+            branch_ids.append(branch.id)
         for gitrepository in gitrepositories or []:
             if (not ignore_permissions
                 and not check_permission('launchpad.View', gitrepository)):
                 raise Unauthorized
-            gitrepositories_by_id[gitrepository.id] = gitrepository
+            gitrepository_ids.append(gitrepository.id)
         for spec in specifications or []:
             if (not ignore_permissions
                 and not check_permission('launchpad.View', spec)):
                 raise Unauthorized
 
         # Load the bugs.
-        visible_bug_ids = []
-        if bugs_by_id:
-            param = BugTaskSearchParams(
-                user=person, bug=any(*bugs_by_id.keys()))
+        visible_bugs = []
+        if bug_ids:
+            param = BugTaskSearchParams(user=person, bug=any(*bug_ids))
             visible_bug_ids = set(getUtility(IBugTaskSet).searchBugIds(param))
-        visible_bugs = [bugs_by_id[bug_id] for bug_id in visible_bug_ids]
+            visible_bugs = [bug for bug in bugs if bug.id in visible_bug_ids]
 
         # Load the branches.
         visible_branches = []
-        if branches_by_id:
+        if branch_ids:
             all_branches = getUtility(IAllBranches)
             wanted_branches = all_branches.visibleByUser(person).withIds(
-                *branches_by_id.keys())
+                *branch_ids)
             visible_branches = list(wanted_branches.getBranches())
 
         # Load the Git repositories.
         visible_gitrepositories = []
-        # XXX cjwatson 2015-02-16: Fill in once IGitCollection is in place.
+        if gitrepository_ids:
+            all_gitrepositories = getUtility(IAllGitRepositories)
+            wanted_gitrepositories = all_gitrepositories.visibleByUser(
+                person).withIds(*gitrepository_ids)
+            visible_gitrepositories = list(
+                wanted_gitrepositories.getRepositories())
 
         # Load the specifications.
         visible_specs = []
@@ -378,40 +388,37 @@
     def getInvisibleArtifacts(self, person, bugs=None, branches=None,
                               gitrepositories=None):
         """See `ISharingService`."""
-        bugs_by_id = {}
-        branches_by_id = {}
-        gitrepositories_by_id = {}
-        for bug in bugs or []:
-            bugs_by_id[bug.id] = bug
-        for branch in branches or []:
-            branches_by_id[branch.id] = branch
-        for gitrepository in gitrepositories or []:
-            gitrepositories_by_id[gitrepository.id] = gitrepository
+        bug_ids = list(map(attrgetter("id"), bugs or []))
+        branch_ids = list(map(attrgetter("id"), branches or []))
+        gitrepository_ids = list(map(attrgetter("id"), gitrepositories or []))
 
         # Load the bugs.
-        visible_bug_ids = set()
-        if bugs_by_id:
-            param = BugTaskSearchParams(
-                user=person, bug=any(*bugs_by_id.keys()))
+        invisible_bugs = []
+        if bug_ids:
+            param = BugTaskSearchParams(user=person, bug=any(*bug_ids))
             visible_bug_ids = set(getUtility(IBugTaskSet).searchBugIds(param))
-        invisible_bug_ids = set(bugs_by_id.keys()).difference(visible_bug_ids)
-        invisible_bugs = [bugs_by_id[bug_id] for bug_id in invisible_bug_ids]
+            invisible_bugs = [
+                bug for bug in bugs if bug.id not in visible_bug_ids]
 
         # Load the branches.
         invisible_branches = []
-        if branches_by_id:
+        if branch_ids:
             all_branches = getUtility(IAllBranches)
             visible_branch_ids = all_branches.visibleByUser(person).withIds(
-                *branches_by_id.keys()).getBranchIds()
-            invisible_branch_ids = (
-                set(branches_by_id.keys()).difference(visible_branch_ids))
+                *branch_ids).getBranchIds()
             invisible_branches = [
-                branches_by_id[branch_id]
-                for branch_id in invisible_branch_ids]
+                branch for branch in branches
+                if branch.id not in visible_branch_ids]
 
         # Load the Git repositories.
         invisible_gitrepositories = []
-        # XXX cjwatson 2015-02-16: Fill in once IGitCollection is in place.
+        if gitrepository_ids:
+            all_gitrepositories = getUtility(IAllGitRepositories)
+            visible_gitrepository_ids = all_gitrepositories.visibleByUser(
+                person).withIds(*gitrepository_ids).getRepositoryIds()
+            invisible_gitrepositories = [
+                gitrepository for gitrepository in gitrepositories
+                if gitrepository.id not in visible_gitrepository_ids]
 
         return invisible_bugs, invisible_branches, invisible_gitrepositories
 

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2015-02-16 13:01:34 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2015-02-27 11:27:39 +0000
@@ -23,6 +23,7 @@
     CodeReviewNotificationLevel,
     )
 from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
@@ -928,12 +929,14 @@
             [InformationType.USERDATA])
 
     def _assert_revokeAccessGrants(self, pillar, bugs, branches,
-                                   specifications):
+                                   gitrepositories, specifications):
         artifacts = []
         if bugs:
             artifacts.extend(bugs)
         if branches:
             artifacts.extend(branches)
+        if gitrepositories:
+            artifacts.extend(gitrepositories)
         if specifications:
             artifacts.extend(specifications)
         policy = self.factory.makeAccessPolicy(pillar=pillar,
@@ -961,6 +964,8 @@
                 branch.subscribe(person,
                     BranchSubscriptionNotificationLevel.NOEMAIL, None,
                     CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+            # XXX cjwatson 2015-02-05: subscribe to Git repositories when
+            # implemented
             for spec in specifications or []:
                 spec.subscribe(person)
 
@@ -973,7 +978,7 @@
 
         self.service.revokeAccessGrants(
             pillar, grantee, pillar.owner, bugs=bugs, branches=branches,
-            specifications=specifications)
+            gitrepositories=gitrepositories, specifications=specifications)
         with block_on_job(self):
             transaction.commit()
 
@@ -987,18 +992,22 @@
             self.assertNotIn(grantee, bug.getDirectSubscribers())
         for branch in branches or []:
             self.assertNotIn(grantee, branch.subscribers)
+        # XXX cjwatson 2015-02-05: check revocation of subscription to Git
+        # repositories when implemented
         for spec in specifications or []:
             self.assertNotIn(grantee, spec.subscribers)
 
-        # Someone else still has access to the bugs and branches.
+        # Someone else still has access to the artifacts.
         grants = accessartifact_grant_source.findByArtifact(
             access_artifacts, [someone])
         self.assertEqual(1, grants.count())
-        # Someone else still has subscriptions to the bugs and branches.
+        # Someone else still has subscriptions to the artifacts.
         for bug in bugs or []:
             self.assertIn(someone, bug.getDirectSubscribers())
         for branch in branches or []:
             self.assertIn(someone, branch.subscribers)
+        # XXX cjwatson 2015-02-05: check subscription to Git repositories
+        # when implemented
         for spec in specifications or []:
             self.assertIn(someone, spec.subscribers)
 
@@ -1010,7 +1019,7 @@
         bug = self.factory.makeBug(
             target=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_revokeAccessGrants(distro, [bug], None, None)
+        self._assert_revokeAccessGrants(distro, [bug], None, None, None)
 
     def test_revokeAccessGrantsBranches(self):
         owner = self.factory.makePerson()
@@ -1019,7 +1028,17 @@
         branch = self.factory.makeBranch(
             product=product, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_revokeAccessGrants(product, None, [branch], None)
+        self._assert_revokeAccessGrants(product, None, [branch], None, None)
+
+    def test_revokeAccessGrantsGitRepositories(self):
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        gitrepository = self.factory.makeGitRepository(
+            target=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        self._assert_revokeAccessGrants(
+            product, None, None, [gitrepository], None)
 
     def test_revokeAccessGrantsSpecifications(self):
         owner = self.factory.makePerson()
@@ -1030,15 +1049,18 @@
         specification = self.factory.makeSpecification(
             product=product, owner=owner,
             information_type=InformationType.EMBARGOED)
-        self._assert_revokeAccessGrants(product, None, None, [specification])
+        self._assert_revokeAccessGrants(
+            product, None, None, None, [specification])
 
     def _assert_revokeTeamAccessGrants(self, pillar, bugs, branches,
-                                       specifications):
+                                       gitrepositories, specifications):
         artifacts = []
         if bugs:
             artifacts.extend(bugs)
         if branches:
             artifacts.extend(branches)
+        if gitrepositories:
+            artifacts.extend(gitrepositories)
         if specifications:
             artifacts.extend(specifications)
         policy = self.factory.makeAccessPolicy(pillar=pillar,
@@ -1065,6 +1087,8 @@
                 branch.subscribe(
                     person, BranchSubscriptionNotificationLevel.NOEMAIL,
                     None, CodeReviewNotificationLevel.NOEMAIL, pillar.owner)
+            # XXX cjwatson 2015-02-05: subscribe to Git repositories when
+            # implemented
             # Subscribing somebody to a specification does not yet imply
             # granting access to this person.
             if specifications:
@@ -1075,12 +1099,16 @@
 
         # Check that grantees have expected access grants and subscriptions.
         for person in [team_grantee, person_grantee]:
-            visible_bugs, visible_branches, _, visible_specs = (
+            (visible_bugs, visible_branches, visible_gitrepositories,
+             visible_specs) = (
                 self.service.getVisibleArtifacts(
                     person, bugs=bugs, branches=branches,
+                    gitrepositories=gitrepositories,
                     specifications=specifications))
             self.assertContentEqual(bugs or [], visible_bugs)
             self.assertContentEqual(branches or [], visible_branches)
+            # XXX cjwatson 2015-02-05: check Git repositories when
+            # subscription is implemented
             self.assertContentEqual(specifications or [], visible_specs)
         for person in [team_grantee, person_grantee]:
             for bug in bugs or []:
@@ -1088,7 +1116,7 @@
 
         self.service.revokeAccessGrants(
             pillar, team_grantee, pillar.owner, bugs=bugs, branches=branches,
-            specifications=specifications)
+            gitrepositories=gitrepositories, specifications=specifications)
         with block_on_job(self):
             transaction.commit()
 
@@ -1103,11 +1131,14 @@
         for person in [team_grantee, person_grantee]:
             for bug in bugs or []:
                 self.assertNotIn(person, bug.getDirectSubscribers())
-            visible_bugs, visible_branches, _, visible_specs = (
+            (visible_bugs, visible_branches, visible_gitrepositories,
+             visible_specs) = (
                 self.service.getVisibleArtifacts(
-                    person, bugs=bugs, branches=branches))
+                    person, bugs=bugs, branches=branches,
+                    gitrepositories=gitrepositories))
             self.assertContentEqual([], visible_bugs)
             self.assertContentEqual([], visible_branches)
+            self.assertContentEqual([], visible_gitrepositories)
             self.assertContentEqual([], visible_specs)
 
     def test_revokeTeamAccessGrantsBugs(self):
@@ -1118,7 +1149,7 @@
         bug = self.factory.makeBug(
             target=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_revokeTeamAccessGrants(distro, [bug], None, None)
+        self._assert_revokeTeamAccessGrants(distro, [bug], None, None, None)
 
     def test_revokeTeamAccessGrantsBranches(self):
         # Users with launchpad.Edit can delete all access for a grantee.
@@ -1127,7 +1158,20 @@
         login_person(owner)
         branch = self.factory.makeBranch(
             owner=owner, information_type=InformationType.USERDATA)
-        self._assert_revokeTeamAccessGrants(product, None, [branch], None)
+        self._assert_revokeTeamAccessGrants(
+            product, None, [branch], None, None)
+
+    # XXX cjwatson 2015-02-05: Enable this once GitRepositorySubscription is
+    # implemented.
+    def disabled_test_revokeTeamAccessGrantsGitRepositories(self):
+        # Users with launchpad.Edit can delete all access for a grantee.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        gitrepository = self.factory.makeGitRepository(
+            owner=owner, information_type=InformationType.USERDATA)
+        self._assert_revokeTeamAccessGrants(
+            product, None, None, [gitrepository], None)
 
     def test_revokeTeamAccessGrantsSpecifications(self):
         # Users with launchpad.Edit can delete all access for a grantee.
@@ -1140,7 +1184,7 @@
             product=product, owner=owner,
             information_type=InformationType.EMBARGOED)
         self._assert_revokeTeamAccessGrants(
-            product, None, None, [specification])
+            product, None, None, None, [specification])
 
     def _assert_revokeAccessGrantsUnauthorized(self):
         # revokeAccessGrants raises an Unauthorized exception if the user
@@ -1163,9 +1207,9 @@
         login_person(self.factory.makePerson())
         self._assert_revokeAccessGrantsUnauthorized()
 
-    def test_revokeAccessGrants_without_bugs_or_branches(self):
+    def test_revokeAccessGrants_without_artifacts(self):
         # The revokeAccessGrants method raises a ValueError if called without
-        # specifying either bugs or branches.
+        # specifying any artifacts.
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
         grantee = self.factory.makePerson()
@@ -1174,24 +1218,27 @@
             ValueError, self.service.revokeAccessGrants,
             product, grantee, product.owner)
 
-    def _assert_ensureAccessGrants(self, user, bugs, branches, specifications,
-                                   grantee=None):
+    def _assert_ensureAccessGrants(self, user, bugs, branches, gitrepositories,
+                                   specifications, grantee=None):
         # Creating access grants works as expected.
         if not grantee:
             grantee = self.factory.makePerson()
         self.service.ensureAccessGrants(
             [grantee], user, bugs=bugs, branches=branches,
-            specifications=specifications)
+            gitrepositories=gitrepositories, specifications=specifications)
 
         # Check that grantee has expected access grants.
         shared_bugs = []
         shared_branches = []
+        shared_gitrepositories = []
         shared_specifications = []
         all_pillars = []
         for bug in bugs or []:
             all_pillars.extend(bug.affected_pillars)
         for branch in branches or []:
             all_pillars.append(branch.target.context)
+        for gitrepository in gitrepositories or []:
+            all_pillars.append(gitrepository.target)
         for specification in specifications or []:
             all_pillars.append(specification.target)
         policies = getUtility(IAccessPolicySource).findByPillar(all_pillars)
@@ -1203,10 +1250,13 @@
                 shared_bugs.append(a.concrete_artifact)
             elif IBranch.providedBy(a.concrete_artifact):
                 shared_branches.append(a.concrete_artifact)
+            elif IGitRepository.providedBy(a.concrete_artifact):
+                shared_gitrepositories.append(a.concrete_artifact)
             elif ISpecification.providedBy(a.concrete_artifact):
                 shared_specifications.append(a.concrete_artifact)
         self.assertContentEqual(bugs or [], shared_bugs)
         self.assertContentEqual(branches or [], shared_branches)
+        self.assertContentEqual(gitrepositories or [], shared_gitrepositories)
         self.assertContentEqual(specifications or [], shared_specifications)
 
     def test_ensureAccessGrantsBugs(self):
@@ -1217,7 +1267,7 @@
         bug = self.factory.makeBug(
             target=distro, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_ensureAccessGrants(owner, [bug], None, None)
+        self._assert_ensureAccessGrants(owner, [bug], None, None, None)
 
     def test_ensureAccessGrantsBranches(self):
         # Access grants can be created for branches.
@@ -1227,7 +1277,18 @@
         branch = self.factory.makeBranch(
             product=product, owner=owner,
             information_type=InformationType.USERDATA)
-        self._assert_ensureAccessGrants(owner, None, [branch], None)
+        self._assert_ensureAccessGrants(owner, None, [branch], None, None)
+
+    def test_ensureAccessGrantsGitRepositories(self):
+        # Access grants can be created for Git repositories.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        login_person(owner)
+        gitrepository = self.factory.makeGitRepository(
+            target=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        self._assert_ensureAccessGrants(
+            owner, None, None, [gitrepository], None)
 
     def test_ensureAccessGrantsSpecifications(self):
         # Access grants can be created for branches.
@@ -1244,7 +1305,8 @@
         with person_logged_in(owner):
             specification.transitionToInformationType(
                 InformationType.PROPRIETARY, owner)
-        self._assert_ensureAccessGrants(owner, None, None, [specification])
+        self._assert_ensureAccessGrants(
+            owner, None, None, None, [specification])
 
     def test_ensureAccessGrantsExisting(self):
         # Any existing access grants are retained and new ones created.
@@ -1263,7 +1325,7 @@
         # Test with a new bug as well as the one for which access is already
         # granted.
         self._assert_ensureAccessGrants(
-            owner, [bug, bug2], None, None, grantee)
+            owner, [bug, bug2], None, None, None, grantee)
 
     def _assert_ensureAccessGrantsUnauthorized(self, user):
         # ensureAccessGrants raises an Unauthorized exception if the user
@@ -1331,7 +1393,8 @@
         self._assert_updatePillarSharingPoliciesUnauthorized(anyone)
 
     def create_shared_artifacts(self, product, grantee, user):
-        # Create some shared bugs and branches.
+        # Create some shared bugs, branches, Git repositories, and
+        # specifications.
         bugs = []
         bug_tasks = []
         for x in range(0, 10):
@@ -1346,6 +1409,12 @@
                 product=product, owner=product.owner,
                 information_type=InformationType.USERDATA)
             branches.append(branch)
+        gitrepositories = []
+        for x in range(0, 10):
+            gitrepository = self.factory.makeGitRepository(
+                target=product, owner=product.owner,
+                information_type=InformationType.USERDATA)
+            gitrepositories.append(gitrepository)
         specs = []
         for x in range(0, 10):
             spec = self.factory.makeSpecification(
@@ -1371,9 +1440,11 @@
             grant_access(bug, i == 9)
         for i, branch in enumerate(branches):
             grant_access(branch, i == 9)
+        for i, gitrepository in enumerate(gitrepositories):
+            grant_access(gitrepository, i == 9)
         getUtility(IService, 'sharing').ensureAccessGrants(
             [grantee], product.owner, specifications=specs[:9])
-        return bug_tasks, branches, specs
+        return bug_tasks, branches, gitrepositories, specs
 
     def test_getSharedArtifacts(self):
         # Test the getSharedArtifacts method.
@@ -1384,14 +1455,16 @@
         login_person(owner)
         grantee = self.factory.makePerson()
         user = self.factory.makePerson()
-        bug_tasks, branches, specs = self.create_shared_artifacts(
-            product, grantee, user)
+        bug_tasks, branches, gitrepositories, specs = (
+            self.create_shared_artifacts(product, grantee, user))
 
         # Check the results.
-        shared_bugtasks, shared_branches, _, shared_specs = (
+        (shared_bugtasks, shared_branches, shared_gitrepositories,
+         shared_specs) = (
             self.service.getSharedArtifacts(product, grantee, user))
         self.assertContentEqual(bug_tasks[:9], shared_bugtasks)
         self.assertContentEqual(branches[:9], shared_branches)
+        self.assertContentEqual(gitrepositories[:9], shared_gitrepositories)
         self.assertContentEqual(specs[:9], shared_specs)
 
     def _assert_getSharedProjects(self, product, who=None):
@@ -1531,7 +1604,7 @@
         login_person(owner)
         grantee = self.factory.makePerson()
         user = self.factory.makePerson()
-        bug_tasks, ignored, ignored = self.create_shared_artifacts(
+        bug_tasks, _, _, _ = self.create_shared_artifacts(
             product, grantee, user)
 
         # Check the results.
@@ -1547,7 +1620,7 @@
         login_person(owner)
         grantee = self.factory.makePerson()
         user = self.factory.makePerson()
-        ignored, branches, ignored = self.create_shared_artifacts(
+        _, branches, _, _ = self.create_shared_artifacts(
             product, grantee, user)
 
         # Check the results.
@@ -1555,6 +1628,23 @@
             product, grantee, user)
         self.assertContentEqual(branches[:9], shared_branches)
 
+    def test_getSharedGitRepositories(self):
+        # Test the getSharedGitRepositories method.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            owner=owner, specification_sharing_policy=(
+            SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY))
+        login_person(owner)
+        grantee = self.factory.makePerson()
+        user = self.factory.makePerson()
+        _, _, gitrepositories, _ = self.create_shared_artifacts(
+            product, grantee, user)
+
+        # Check the results.
+        shared_gitrepositories = self.service.getSharedGitRepositories(
+            product, grantee, user)
+        self.assertContentEqual(gitrepositories[:9], shared_gitrepositories)
+
     def test_getSharedSpecifications(self):
         # Test the getSharedSpecifications method.
         owner = self.factory.makePerson()
@@ -1564,7 +1654,7 @@
         login_person(owner)
         grantee = self.factory.makePerson()
         user = self.factory.makePerson()
-        ignored, ignored, specifications = self.create_shared_artifacts(
+        _, _, _, specifications = self.create_shared_artifacts(
             product, grantee, user)
 
         # Check the results.
@@ -1592,6 +1682,16 @@
         login_person(owner)
         self._assert_getPeopleWithoutAccess(product, branch)
 
+    def test_getPeopleWithAccessGitRepositories(self):
+        # Test the getPeopleWithoutAccess method with Git repositories.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(owner=owner)
+        gitrepository = self.factory.makeGitRepository(
+            target=product, owner=owner,
+            information_type=InformationType.USERDATA)
+        login_person(owner)
+        self._assert_getPeopleWithoutAccess(product, gitrepository)
+
     def _assert_getPeopleWithoutAccess(self, product, artifact):
         access_artifact = self.factory.makeAccessArtifact(concrete=artifact)
         # Make some people to check. people[:5] will not have access.
@@ -1647,6 +1747,12 @@
                 product=product, owner=owner,
                 information_type=InformationType.USERDATA)
             branches.append(branch)
+        gitrepositories = []
+        for x in range(0, 10):
+            gitrepository = self.factory.makeGitRepository(
+                target=product, owner=owner,
+                information_type=InformationType.USERDATA)
+            gitrepositories.append(gitrepository)
 
         specifications = []
         for x in range(0, 10):
@@ -1662,46 +1768,58 @@
                 artifact=access_artifact, grantee=grantee, grantor=owner)
             return access_artifact
 
-        # Grant access to some of the bugs and branches.
+        # Grant access to some of the artifacts.
         for bug in bugs[:5]:
             grant_access(bug)
         for branch in branches[:5]:
             grant_access(branch)
+        for gitrepository in gitrepositories[:5]:
+            grant_access(gitrepository)
         for spec in specifications[:5]:
             grant_access(spec)
-        return grantee, owner, branches, bugs, specifications
+        return grantee, owner, bugs, branches, gitrepositories, specifications
 
     def test_getVisibleArtifacts(self):
         # Test the getVisibleArtifacts method.
-        grantee, ignore, branches, bugs, specs = self._make_Artifacts()
+        grantee, ignore, bugs, branches, gitrepositories, specs = (
+            self._make_Artifacts())
         # Check the results.
-        shared_bugs, shared_branches, _, shared_specs = (
+        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
             self.service.getVisibleArtifacts(
-                grantee, bugs=bugs, branches=branches, specifications=specs))
+                grantee, bugs=bugs, branches=branches,
+                gitrepositories=gitrepositories, specifications=specs))
         self.assertContentEqual(bugs[:5], shared_bugs)
         self.assertContentEqual(branches[:5], shared_branches)
+        self.assertContentEqual(gitrepositories[:5], shared_gitrepositories)
         self.assertContentEqual(specs[:5], shared_specs)
 
     def test_getVisibleArtifacts_grant_on_pillar(self):
         # getVisibleArtifacts() returns private specifications if
         # user has a policy grant for the pillar of the specification.
-        ignore, owner, branches, bugs, specs = self._make_Artifacts()
-        shared_bugs, shared_branches, _, shared_specs = (
+        _, owner, bugs, branches, gitrepositories, specs = (
+            self._make_Artifacts())
+        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
             self.service.getVisibleArtifacts(
-                owner, bugs=bugs, branches=branches, specifications=specs))
+                owner, bugs=bugs, branches=branches,
+                gitrepositories=gitrepositories, specifications=specs))
         self.assertContentEqual(bugs, shared_bugs)
         self.assertContentEqual(branches, shared_branches)
+        self.assertContentEqual(gitrepositories, shared_gitrepositories)
         self.assertContentEqual(specs, shared_specs)
 
     def test_getInvisibleArtifacts(self):
         # Test the getInvisibleArtifacts method.
-        grantee, ignore, branches, bugs, specs = self._make_Artifacts()
+        grantee, ignore, bugs, branches, gitrepositories, specs = (
+            self._make_Artifacts())
         # Check the results.
-        not_shared_bugs, not_shared_branches, _ = (
+        not_shared_bugs, not_shared_branches, not_shared_gitrepositories = (
             self.service.getInvisibleArtifacts(
-                grantee, bugs=bugs, branches=branches))
+                grantee, bugs=bugs, branches=branches,
+                gitrepositories=gitrepositories))
         self.assertContentEqual(bugs[5:], not_shared_bugs)
         self.assertContentEqual(branches[5:], not_shared_branches)
+        self.assertContentEqual(
+            gitrepositories[5:], not_shared_gitrepositories)
 
     def _assert_getVisibleArtifacts_bug_change(self, change_callback):
         # Test the getVisibleArtifacts method excludes bugs after a change of
@@ -1723,7 +1841,7 @@
                 information_type=InformationType.USERDATA)
             bugs.append(bug)
 
-        shared_bugs, shared_branches, _, shared_specs = (
+        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
             self.service.getVisibleArtifacts(grantee, bugs=bugs))
         self.assertContentEqual(bugs, shared_bugs)
 
@@ -1731,7 +1849,7 @@
         for x in range(0, 5):
             change_callback(bugs[x], owner)
         # Check the results.
-        shared_bugs, shared_branches, _, shared_specs = (
+        shared_bugs, shared_branches, shared_gitrepositories, shared_specs = (
             self.service.getVisibleArtifacts(grantee, bugs=bugs))
         self.assertContentEqual(bugs[5:], shared_bugs)
 

=== modified file 'lib/lp/registry/tests/test_accesspolicy.py'
--- lib/lp/registry/tests/test_accesspolicy.py	2013-06-20 05:50:00 +0000
+++ lib/lp/registry/tests/test_accesspolicy.py	2015-02-27 11:27:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -280,6 +280,13 @@
         return self.factory.makeBranch()
 
 
+class TestAccessArtifactGitRepository(BaseAccessArtifactTests,
+                                      TestCaseWithFactory):
+
+    def getConcreteArtifact(self):
+        return self.factory.makeGitRepository()
+
+
 class TestAccessArtifactBug(BaseAccessArtifactTests,
                             TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/tests/test_sharingjob.py'
--- lib/lp/registry/tests/test_sharingjob.py	2014-06-19 06:38:53 +0000
+++ lib/lp/registry/tests/test_sharingjob.py	2015-02-27 11:27:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for SharingJobs."""
@@ -118,6 +118,16 @@
             'for branch_ids=[%d], requestor=%s>'
             % (branch.id, requestor.name), repr(job))
 
+    def test_repr_gitrepositories(self):
+        requestor = self.factory.makePerson()
+        gitrepository = self.factory.makeGitRepository()
+        job = getUtility(IRemoveArtifactSubscriptionsJobSource).create(
+            requestor, artifacts=[gitrepository])
+        self.assertEqual(
+            '<REMOVE_ARTIFACT_SUBSCRIPTIONS job reconciling subscriptions '
+            'for gitrepository_ids=[%d], requestor=%s>'
+            % (gitrepository.id, requestor.name), repr(job))
+
     def test_repr_specifications(self):
         requestor = self.factory.makePerson()
         specification = self.factory.makeSpecification()
@@ -303,6 +313,9 @@
         branch = self.factory.makeBranch(
             owner=owner, product=product,
             information_type=InformationType.USERDATA)
+        gitrepository = self.factory.makeGitRepository(
+            owner=owner, target=product,
+            information_type=InformationType.USERDATA)
         specification = self.factory.makeSpecification(
             owner=owner, product=product,
             information_type=InformationType.PROPRIETARY)
@@ -317,6 +330,8 @@
         branch.subscribe(artifact_indirect_grantee,
             BranchSubscriptionNotificationLevel.NOEMAIL, None,
             CodeReviewNotificationLevel.NOEMAIL, owner)
+        # XXX cjwatson 2015-02-05: Fill this in once we have
+        # GitRepositorySubscription.
         # Subscribing somebody to a specification does not automatically
         # create an artifact grant.
         spec_artifact = self.factory.makeAccessArtifact(specification)
@@ -325,10 +340,10 @@
         with person_logged_in(product.owner):
             specification.subscribe(artifact_indirect_grantee, owner)
 
-        # pick one of the concrete artifacts (bug, branch or spec)
-        # and subscribe the teams and persons.
+        # pick one of the concrete artifacts (bug, branch, Git repository,
+        # or spec) and subscribe the teams and persons.
         concrete_artifact, get_pillars, get_subscribers = configure_test(
-            bug, branch, specification, policy_team_grantee,
+            bug, branch, gitrepository, specification, policy_team_grantee,
             policy_indirect_grantee, artifact_team_grantee, owner)
 
         # Subscribing policy_team_grantee has created an artifact grant so we
@@ -361,6 +376,8 @@
         self.assertIn(artifact_team_grantee, subscribers)
         self.assertIn(artifact_indirect_grantee, bug.getDirectSubscribers())
         self.assertIn(artifact_indirect_grantee, branch.subscribers)
+        # XXX cjwatson 2015-02-05: Fill this in once we have
+        # GitRepositorySubscription.
         self.assertIn(artifact_indirect_grantee,
                       removeSecurityProxy(specification).subscribers)
 
@@ -373,9 +390,9 @@
             return removeSecurityProxy(
                 concrete_artifact).getDirectSubscribers()
 
-        def configure_test(bug, branch, specification, policy_team_grantee,
-                           policy_indirect_grantee, artifact_team_grantee,
-                           owner):
+        def configure_test(bug, branch, gitrepository, specification,
+                           policy_team_grantee, policy_indirect_grantee,
+                           artifact_team_grantee, owner):
             concrete_artifact = bug
             bug.subscribe(policy_team_grantee, owner)
             bug.subscribe(policy_indirect_grantee, owner)
@@ -393,9 +410,9 @@
         def get_subscribers(concrete_artifact):
             return concrete_artifact.subscribers
 
-        def configure_test(bug, branch, specification, policy_team_grantee,
-                           policy_indirect_grantee, artifact_team_grantee,
-                           owner):
+        def configure_test(bug, branch, gitrepository, specification,
+                           policy_team_grantee, policy_indirect_grantee,
+                           artifact_team_grantee, owner):
             concrete_artifact = branch
             branch.subscribe(
                 policy_team_grantee,
@@ -414,6 +431,27 @@
         self._assert_artifact_change_unsubscribes(
             change_callback, configure_test)
 
+    def _assert_gitrepository_change_unsubscribes(self, change_callback):
+
+        def get_pillars(concrete_artifact):
+            return [concrete_artifact.product]
+
+        def get_subscribers(concrete_artifact):
+            return concrete_artifact.subscribers
+
+        def configure_test(bug, branch, gitrepository, specification,
+                           policy_team_grantee, policy_indirect_grantee,
+                           artifact_team_grantee, owner):
+            concrete_artifact = gitrepository
+            # XXX cjwatson 2015-02-05: Fill this in once we have
+            # GitRepositorySubscription.
+            return concrete_artifact, get_pillars, get_subscribers
+
+        # XXX cjwatson 2015-02-05: Uncomment once we have
+        # GitRepositorySubscription.
+        #self._assert_artifact_change_unsubscribes(
+        #    change_callback, configure_test)
+
     def _assert_specification_change_unsubscribes(self, change_callback):
 
         def get_pillars(concrete_artifact):
@@ -422,9 +460,9 @@
         def get_subscribers(concrete_artifact):
             return concrete_artifact.subscribers
 
-        def configure_test(bug, branch, specification, policy_team_grantee,
-                           policy_indirect_grantee, artifact_team_grantee,
-                           owner):
+        def configure_test(bug, branch, gitrepository, specification,
+                           policy_team_grantee, policy_indirect_grantee,
+                           artifact_team_grantee, owner):
             naked_spec = removeSecurityProxy(specification)
             naked_spec.subscribe(policy_team_grantee, owner)
             naked_spec.subscribe(policy_indirect_grantee, owner)
@@ -444,6 +482,13 @@
 
         self._assert_branch_change_unsubscribes(change_information_type)
 
+    def test_change_information_type_gitrepository(self):
+        def change_information_type(gitrepository):
+            removeSecurityProxy(gitrepository).information_type = (
+                InformationType.PRIVATESECURITY)
+
+        self._assert_gitrepository_change_unsubscribes(change_information_type)
+
     def test_change_information_type_specification(self):
         def change_information_type(specification):
             removeSecurityProxy(specification).information_type = (

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2015-02-23 03:22:37 +0000
+++ lib/lp/security.py	2015-02-27 11:27:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 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).
 
 """Security policies for using content objects."""
@@ -83,6 +83,7 @@
     )
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
 from lp.code.interfaces.diff import IPreviewDiff
+from lp.code.interfaces.gitcollection import IGitCollection
 from lp.code.interfaces.gitrepository import (
     IGitRepository,
     user_has_special_git_repository_access,
@@ -1020,6 +1021,12 @@
             if not mp.is_empty():
                 return True
 
+            # Grant visibility to people who can see Git repositories owned
+            # by the private team.
+            team_repositories = IGitCollection(self.obj)
+            if not team_repositories.visibleByUser(user.person).is_empty():
+                return True
+
             # Grant visibility to users in a team that has the private team as
             # a member, so that they can see the team properly in member
             # listings.