launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17968
Re: [Merge] lp:~cjwatson/launchpad/git-finish-sharing into lp:launchpad
Review: Approve code
Diff comments:
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py 2015-02-26 17:34:42 +0000
> +++ lib/lp/code/model/gitrepository.py 2015-02-26 17:34:42 +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-26 17:34:42 +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 17:34:42 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py 2015-02-26 17:34:42 +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("admin@xxxxxxxxxxxxx")
> +
> + 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-26 17:34:42 +0000
> @@ -36,6 +36,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 +228,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)
> @@ -360,7 +365,12 @@
>
> # Load the Git repositories.
> visible_gitrepositories = []
> - # XXX cjwatson 2015-02-16: Fill in once IGitCollection is in place.
> + if gitrepositories_by_id:
> + all_gitrepositories = getUtility(IAllGitRepositories)
> + wanted_gitrepositories = all_gitrepositories.visibleByUser(
> + person).withIds(*gitrepositories_by_id.keys())
> + visible_gitrepositories = list(
> + wanted_gitrepositories.getRepositories())
>
> # Load the specifications.
> visible_specs = []
> @@ -411,7 +421,17 @@
>
> # Load the Git repositories.
> invisible_gitrepositories = []
> - # XXX cjwatson 2015-02-16: Fill in once IGitCollection is in place.
> + if gitrepositories_by_id:
> + all_gitrepositories = getUtility(IAllGitRepositories)
> + visible_gitrepository_ids = all_gitrepositories.visibleByUser(
> + person).withIds(
> + *gitrepositories_by_id.keys()).getRepositoryIds()
> + invisible_gitrepository_ids = (
> + set(gitrepositories_by_id.keys()).difference(
> + visible_gitrepository_ids))
> + invisible_gitrepositories = [
> + gitrepositories_by_id[gitrepository_id]
> + for gitrepository_id in invisible_gitrepository_ids]
I'd kill gitrepositories_by_id and invisible_gitrepository_ids and do this:
invisible_gitrepositories = [
repo for repo in gitrepository_ids
if repo.id not in visible_gitrepository_ids]
And the same for bugs and specs.
>
> 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-26 17:34:42 +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-26 17:34:42 +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-26 17:34:42 +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-26 17:34:42 +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.
>
--
https://code.launchpad.net/~cjwatson/launchpad/git-finish-sharing/+merge/250663
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
