[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad
The proposal to merge lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~cjwatson/launchpad/git-code-import-security-deletion/+merge/308319 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ 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
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad
Diff comments: > > === modified file 'lib/lp/code/interfaces/gitrepository.py' > --- lib/lp/code/interfaces/gitrepository.py 2016-10-05 09:54:42 + > +++ lib/lp/code/interfaces/gitrepository.py 2016-10-13 12:29:37 + > @@ -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))) Granted that the Git side at least isn't tested, and the typo you noticed. As for why it's there, there used to be a comment about this but Curtis removed it during the last refactoring: # It used to be the case that all import branches were owned by the # special, restricted team ~vcs-imports. For these legacy code import # branches, we still want the code import registrant to be able to # edit them. Similarly, we still want vcs-imports members to be able # to edit those branches. This case is tested (lp.code.tests.test_branch.TestWriteToBranch.test_code_import_registrant_can_edit). There are still 34 distinct registrants of vcs-imports-owned branches on dogfood, quite a few of whom aren't privileged, so I think I should still leave this code there for the Bazaar case. However, I'll put the comment back so that it's less mystifying, and I'll drop this from the Git side since we have no legacy concerns there. -- https://code.launchpad.net/~cjwatson/launchpad/git-code-import-security-deletion/+merge/308319 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ 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
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/git-code-import-security-deletion into lp:launchpad
Review: Approve code
Diff comments:
>
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py 2016-10-05 09:54:42 +
> +++ lib/lp/code/interfaces/gitrepository.py 2016-10-13 12:29:37 +
> @@ -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)))
Registrant? Really? Can you delete that disjunct from
user_has_special_branch_access too? I don't see how the repository owner's
participation is relevant to anything.
Especially since "inTerm" suggests it isn't tested.
>
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py2016-10-05 09:29:41 +
> +++ lib/lp/code/model/gitrepository.py2016-10-13 12:29:37 +
> @@ -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."))
s/branch/repository/g
> +
> +def __call__(self):
> +getUtility(ICodeImportSet).delete(self.affected_object)
> +
> +
> @implementer(IGitRepositorySet)
> class GitRepositorySet:
> """See `IGitRepositorySet`."""
--
https://code.launchpad.net/~cjwatson/launchpad/git-code-import-security-deletion/+merge/308319
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
___
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
[Launchpad-reviewers] [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 +
+++ lib/lp/_schema_circular_imports.py 2016-10-12 23:49:44 +
@@ -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 +
+++ lib/lp/code/interfaces/gitnamespace.py 2016-10-12 23:49:44 +
@@ -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 +
+++ lib/lp/code/interfaces/gitrepository.py 2016-10-12 23:49:44 +
@@ -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 +
+++ lib/lp/code/model/branch.py 2016-10-12 23:49:44 +
@@ -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)
