← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad

 

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


Follow ups