launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21093
[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
