Colin Watson has proposed merging lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad with lp:~cjwatson/launchpad/codeimport-git-worker as a prerequisite.
Commit message: Grant vcs-imports edit access to imported repositories, and handle deletion of imported repositories. Requested reviews: Launchpad code reviewers (launchpad-reviewers) Related bugs: Bug #1469459 in Launchpad itself: "import external code into a LP git repo (natively)" https://bugs.launchpad.net/launchpad/+bug/1469459 For more details, see: https://code.launchpad.net/~cjwatson/launchpad/git-code-import-security-deletion/+merge/308319 This doesn't grant vcs-imports push access: IMPORTED repositories are handled specially in lp.code.xmlrpc.git. However, it's still useful for the vcs-imports team to be able to edit import metadata. -- Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad.
=== modified file 'lib/lp/_schema_circular_imports.py' --- lib/lp/_schema_circular_imports.py 2016-06-25 08:05:06 +0000 +++ lib/lp/_schema_circular_imports.py 2016-10-12 23:49:44 +0000 @@ -578,6 +578,7 @@ patch_collection_property(IGitRepository, 'subscriptions', IGitSubscription) patch_entry_return_type(IGitRepository, 'subscribe', IGitSubscription) patch_entry_return_type(IGitRepository, 'getSubscription', IGitSubscription) +patch_reference_property(IGitRepository, 'code_import', ICodeImport) patch_collection_property( IGitRepository, 'landing_targets', IBranchMergeProposal) patch_collection_property( === modified file 'lib/lp/code/interfaces/gitnamespace.py' --- lib/lp/code/interfaces/gitnamespace.py 2016-10-06 15:54:47 +0000 +++ lib/lp/code/interfaces/gitnamespace.py 2016-10-12 23:49:44 +0000 @@ -119,10 +119,12 @@ :return: An `InformationType`. """ - def validateRegistrant(registrant): + def validateRegistrant(registrant, repository=None): """Check that the registrant can create a repository in this namespace. :param registrant: An `IPerson`. + :param repository: An optional `IGitRepository` to also check when + working with imported repositories. :raises GitRepositoryCreatorNotMemberOfOwnerTeam: if the namespace owner is a team and the registrant is not in that team. :raises GitRepositoryCreatorNotOwner: if the namespace owner is an === modified file 'lib/lp/code/interfaces/gitrepository.py' --- lib/lp/code/interfaces/gitrepository.py 2016-10-05 09:54:42 +0000 +++ lib/lp/code/interfaces/gitrepository.py 2016-10-12 23:49:44 +0000 @@ -260,6 +260,11 @@ title=_("Persons subscribed to this repository."), readonly=True, value_type=Reference(IPerson))) + code_import = exported(Reference( + title=_("The associated CodeImport, if any."), + # Really ICodeImport, patched in _schema_circular_imports.py. + schema=Interface)) + def getRefByPath(path): """Look up a single reference in this repository by path. @@ -946,14 +951,24 @@ return identities -def user_has_special_git_repository_access(user): +def user_has_special_git_repository_access(user, repository=None): """Admins have special access. :param user: An `IPerson` or None. + :param repository: An `IGitRepository` or None when checking collection + access. """ if user is None: return False roles = IPersonRoles(user) if roles.in_admin: return True - return False + if repository is None: + return False + code_import = repository.code_import + if code_import is None: + return False + return ( + roles.in_vcs_imports + or (IPersonRoles(repository.owner).in_vcs_imports + and user.inTerm(code_import.registrant))) === modified file 'lib/lp/code/model/branch.py' --- lib/lp/code/model/branch.py 2016-06-20 20:32:36 +0000 +++ lib/lp/code/model/branch.py 2016-10-12 23:49:44 +0000 @@ -117,6 +117,7 @@ BRANCH_ID_ALIAS_PREFIX, compose_public_url, ) +from lp.code.interfaces.codeimport import ICodeImportSet from lp.code.interfaces.seriessourcepackagebranch import ( IFindOfficialBranchLinks, ) @@ -813,8 +814,7 @@ @cachedproperty def code_import(self): - from lp.code.model.codeimport import CodeImportSet - return CodeImportSet().getByBranch(self) + return getUtility(ICodeImportSet).getByBranch(self) def _deletionRequirements(self): """Determine what operations must be performed to delete this branch. @@ -1561,8 +1561,7 @@ self, code_import, _('This is the import data for this branch.')) def __call__(self): - from lp.code.model.codeimport import CodeImportSet - CodeImportSet().delete(self.affected_object) + getUtility(ICodeImportSet).delete(self.affected_object) @implementer(IBranchSet) === modified file 'lib/lp/code/model/gitcollection.py' --- lib/lp/code/model/gitcollection.py 2016-06-24 23:35:24 +0000 +++ lib/lp/code/model/gitcollection.py 2016-10-12 23:49:44 +0000 @@ -42,6 +42,7 @@ user_has_special_git_repository_access, ) from lp.code.model.branchmergeproposal import BranchMergeProposal +from lp.code.model.codeimport import CodeImport from lp.code.model.codereviewcomment import CodeReviewComment from lp.code.model.codereviewvote import CodeReviewVoteReference from lp.code.model.gitrepository import ( @@ -197,6 +198,15 @@ load_related(Distribution, repositories, ['distribution_id']) load_related(SourcePackageName, repositories, ['sourcepackagename_id']) load_related(Product, repositories, ['project_id']) + caches = { + repository.id: get_property_cache(repository) + for repository in repositories} + repository_ids = caches.keys() + for cache in caches.values(): + cache.code_import = None + for code_import in IStore(CodeImport).find( + CodeImport, CodeImport.git_repositoryID.is_in(repository_ids)): + caches[code_import.git_repositoryID].code_import = code_import def getRepositories(self, find_expr=GitRepository, eager_load=False, order_by_date=False, order_by_id=False): @@ -216,7 +226,7 @@ repository_ids = set(repository.id for repository in rows) if not repository_ids: return - load_related(Product, rows, ['project_id']) + GenericGitCollection.preloadDataForRepositories(rows) # So far have only needed the persons for their canonical_url - no # need for validity etc in the API call. load_related(Person, rows, ['owner_id', 'registrant_id']) === modified file 'lib/lp/code/model/gitnamespace.py' --- lib/lp/code/model/gitnamespace.py 2016-10-05 15:12:48 +0000 +++ lib/lp/code/model/gitnamespace.py 2016-10-12 23:49:44 +0000 @@ -117,9 +117,9 @@ name = "%s-%s" % (prefix, count) return name - def validateRegistrant(self, registrant): + def validateRegistrant(self, registrant, repository=None): """See `IGitNamespace`.""" - if user_has_special_git_repository_access(registrant): + if user_has_special_git_repository_access(registrant, repository): return owner = self.owner if not registrant.inTeam(owner): @@ -170,7 +170,7 @@ if name is None: name = repository.name self.validateRepositoryName(name) - self.validateRegistrant(mover) + self.validateRegistrant(mover, repository) self.validateDefaultFlags(repository) def moveRepository(self, repository, mover, new_name=None, === modified file 'lib/lp/code/model/gitrepository.py' --- lib/lp/code/model/gitrepository.py 2016-10-05 09:29:41 +0000 +++ lib/lp/code/model/gitrepository.py 2016-10-12 23:49:44 +0000 @@ -86,6 +86,7 @@ BRANCH_MERGE_PROPOSAL_FINAL_STATES, notify_modified, ) +from lp.code.interfaces.codeimport import ICodeImportSet from lp.code.interfaces.gitcollection import ( IAllGitRepositories, IGitCollection, @@ -1073,6 +1074,10 @@ diff = hosting_client.getDiff(self.getInternalPath(), old, new) return diff["patch"] + @cachedproperty + def code_import(self): + return getUtility(ICodeImportSet).getByGitRepository(self) + def canBeDeleted(self): """See `IGitRepository`.""" # Can't delete if the repository is associated with anything. @@ -1154,6 +1159,12 @@ operation() for operation in deletion_operations: operation() + # Special-case code import, since users don't have lp.Edit on them, + # since if you can delete a repository you should be able to delete + # the code import, and since deleting the code import object itself + # isn't actually a very interesting thing to tell the user about. + if self.code_import is not None: + DeleteCodeImport(self.code_import)() Store.of(self).flush() def _deleteRepositoryAccessGrants(self): @@ -1250,6 +1261,17 @@ self.affected_object.prerequisite_git_commit_sha1 = None +class DeleteCodeImport(DeletionOperation): + """Deletion operation that deletes a repository's import.""" + + def __init__(self, code_import): + DeletionOperation.__init__( + self, code_import, msg("This is the import data for this branch.")) + + def __call__(self): + getUtility(ICodeImportSet).delete(self.affected_object) + + @implementer(IGitRepositorySet) class GitRepositorySet: """See `IGitRepositorySet`.""" === modified file 'lib/lp/code/model/tests/test_branch.py' --- lib/lp/code/model/tests/test_branch.py 2016-06-28 21:10:18 +0000 +++ lib/lp/code/model/tests/test_branch.py 2016-10-12 23:49:44 +0000 @@ -34,6 +34,7 @@ PRIVATE_INFORMATION_TYPES, PUBLIC_INFORMATION_TYPES, ) +from lp.app.errors import NotFoundError from lp.app.interfaces.launchpad import ILaunchpadCelebrities from lp.blueprints.enums import NewSpecificationDefinitionStatus from lp.blueprints.interfaces.specification import ISpecificationSet @@ -83,6 +84,7 @@ ) from lp.code.interfaces.branchrevision import IBranchRevision from lp.code.interfaces.codehosting import branch_id_alias +from lp.code.interfaces.codeimport import ICodeImportSet from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch from lp.code.interfaces.seriessourcepackagebranch import ( IFindOfficialBranchLinks, @@ -104,10 +106,6 @@ ) from lp.code.model.branchmergeproposal import BranchMergeProposal from lp.code.model.branchrevision import BranchRevision -from lp.code.model.codeimport import ( - CodeImport, - CodeImportSet, - ) from lp.code.model.codereviewcomment import CodeReviewComment from lp.code.model.revision import Revision from lp.code.tests.helpers import add_revision_to_branch @@ -191,9 +189,9 @@ code_import = self.factory.makeCodeImport() branch = code_import.branch self.assertEqual(code_import, branch.code_import) - CodeImportSet().delete(code_import) + getUtility(ICodeImportSet).delete(code_import) clear_property_cache(branch) - self.assertEqual(None, branch.code_import) + self.assertIsNone(branch.code_import) class TestBranchChanged(TestCaseWithFactory): @@ -1664,12 +1662,12 @@ self.assertEqual({}, code_import.branch.deletionRequirements()) def test_branchWithCodeImportDeletion(self): - """break_links allows deleting a code import branch.""" + """break_references allows deleting a code import branch.""" code_import = self.factory.makeCodeImport() code_import_id = code_import.id code_import.branch.destroySelf(break_references=True) self.assertRaises( - SQLObjectNotFound, CodeImport.get, code_import_id) + NotFoundError, getUtility(ICodeImportSet).get, code_import_id) def test_sourceBranchWithCodeReviewVoteReference(self): """Break_references handles CodeReviewVoteReference source branch.""" @@ -1750,7 +1748,7 @@ code_import_id = code_import.id DeleteCodeImport(code_import)() self.assertRaises( - SQLObjectNotFound, CodeImport.get, code_import_id) + NotFoundError, getUtility(ICodeImportSet).get, code_import_id) def test_deletionRequirements_with_SourcePackageRecipe(self): """Recipes are listed as deletion requirements.""" === modified file 'lib/lp/code/model/tests/test_gitrepository.py' --- lib/lp/code/model/tests/test_gitrepository.py 2016-10-05 09:29:41 +0000 +++ lib/lp/code/model/tests/test_gitrepository.py 2016-10-12 23:49:44 +0000 @@ -36,6 +36,7 @@ PRIVATE_INFORMATION_TYPES, PUBLIC_INFORMATION_TYPES, ) +from lp.app.errors import NotFoundError from lp.app.interfaces.launchpad import ILaunchpadCelebrities from lp.code.enums import ( BranchMergeProposalStatus, @@ -44,6 +45,7 @@ CodeReviewNotificationLevel, GitObjectType, GitRepositoryType, + TargetRevisionControlSystems, ) from lp.code.errors import ( CannotDeleteGitRepository, @@ -57,6 +59,7 @@ from lp.code.interfaces.branchmergeproposal import ( BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES, ) +from lp.code.interfaces.codeimport import ICodeImportSet from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository from lp.code.interfaces.gitjob import ( IGitRefScanJobSource, @@ -88,6 +91,7 @@ ) from lp.code.model.gitrepository import ( ClearPrerequisiteRepository, + DeleteCodeImport, DeletionCallable, DeletionOperation, GitRepository, @@ -118,6 +122,7 @@ from lp.services.job.model.job import Job from lp.services.job.runner import JobRunner from lp.services.mail import stub +from lp.services.propertycache import clear_property_cache from lp.services.webapp.authorization import check_permission from lp.services.webapp.interfaces import OAuthPermission from lp.testing import ( @@ -199,6 +204,16 @@ repository = self.factory.makeGitRepository(owner=owner, target=owner) self.assertEqual(owner, repository.target) + def test_code_import(self): + self.useFixture(GitHostingFixture()) + code_import = self.factory.makeCodeImport( + target_rcs_type=TargetRevisionControlSystems.GIT) + repository = code_import.git_repository + self.assertEqual(code_import, repository.code_import) + getUtility(ICodeImportSet).delete(code_import) + clear_property_cache(repository) + self.assertIsNone(repository.code_import) + class TestGitIdentityMixin(TestCaseWithFactory): """Test the defaults and identities provided by GitIdentityMixin.""" @@ -397,6 +412,15 @@ getUtility(IGitLookup).get(repository_id), "The repository has not been deleted.") + def test_code_import_does_not_disable_deletion(self): + # A repository that has an attached code import can be deleted. + self.useFixture(GitHostingFixture()) + code_import = self.factory.makeCodeImport( + target_rcs_type=TargetRevisionControlSystems.GIT) + repository = code_import.git_repository + with celebrity_logged_in('vcs_imports'): + self.assertTrue(repository.canBeDeleted()) + def test_landing_target_disables_deletion(self): # A repository with a landing target cannot be deleted. [merge_target] = self.factory.makeGitRefs( @@ -654,6 +678,27 @@ self.factory.makePerson(), self.factory.makePerson()) merge_proposal.target_git_repository.destroySelf(break_references=True) + def test_code_import_requirements(self): + # Code imports are not included explicitly in deletion requirements. + self.useFixture(GitHostingFixture()) + code_import = self.factory.makeCodeImport( + target_rcs_type=TargetRevisionControlSystems.GIT) + # Remove the implicit repository subscription first. + code_import.git_repository.unsubscribe( + code_import.git_repository.owner, code_import.git_repository.owner) + self.assertEqual( + {}, code_import.git_repository.getDeletionRequirements()) + + def test_code_import_deletion(self): + # break_references allows deleting a code import repository. + self.useFixture(GitHostingFixture()) + code_import = self.factory.makeCodeImport( + target_rcs_type=TargetRevisionControlSystems.GIT) + code_import_id = code_import.id + code_import.git_repository.destroySelf(break_references=True) + self.assertRaises( + NotFoundError, getUtility(ICodeImportSet).get, code_import_id) + def test_snap_requirements(self): # If a repository is used by a snap package, the deletion # requirements indicate this. @@ -700,6 +745,16 @@ self.assertRaises( SQLObjectNotFound, BranchMergeProposal.get, merge_proposal_id) + def test_DeleteCodeImport(self): + # DeleteCodeImport.__call__ must delete the CodeImport. + self.useFixture(GitHostingFixture()) + code_import = self.factory.makeCodeImport( + target_rcs_type=TargetRevisionControlSystems.GIT) + code_import_id = code_import.id + DeleteCodeImport(code_import)() + self.assertRaises( + NotFoundError, getUtility(ICodeImportSet).get, code_import_id) + def test_deletionRequirements_with_SourcePackageRecipe(self): # Recipes are listed as deletion requirements. self.useFixture(FeatureFixture({GIT_RECIPES_FEATURE_FLAG: u"on"})) === modified file 'lib/lp/security.py' --- lib/lp/security.py 2016-09-20 00:33:33 +0000 +++ lib/lp/security.py 2016-10-12 23:49:44 +0000 @@ -2273,7 +2273,7 @@ # "official" repositories, once those are defined. return ( user.inTeam(self.obj.owner) or - user_has_special_git_repository_access(user.person)) + user_has_special_git_repository_access(user.person, self.obj)) class ModerateGitRepository(EditGitRepository):
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp