launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18476
[Merge] lp:~cjwatson/launchpad/git-default-owner into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-default-owner into lp:launchpad.
Commit message:
Allow any member of a team to set an owner-target default; use this to set the owner of newly-pushed target defaults to the target owner.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-default-owner/+merge/258400
When a project is owned by a team and a team member pushes a new repository to the project default (/PROJECT), the unique name of the repository ends up as /~REQUESTER/PROJECT/+git/PROJECT. This is weird because other members of the project-owning team can't write to it. A more reasonable default would be /~OWNER/PROJECT/+git/PROJECT, which requires opening up the permissions on IGitRepositorySet.setDefaultRepositoryForOwner a little bit to allow non-owner members of a team to call it.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-default-owner into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2015-04-28 16:39:15 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2015-05-06 15:07:21 +0000
@@ -690,6 +690,7 @@
:raises GitTargetError: if `target` is an `IPerson`.
"""
+ @call_with(user=REQUEST_USER)
@operation_parameters(
owner=Reference(title=_("Owner"), required=True, schema=IPerson),
target=Reference(
@@ -698,13 +699,14 @@
title=_("Git repository"), required=False, schema=IGitRepository))
@export_write_operation()
@operation_for_version("devel")
- def setDefaultRepositoryForOwner(owner, target, repository):
+ def setDefaultRepositoryForOwner(owner, target, repository, user):
"""Set a person's default repository for a target.
:param owner: An `IPerson`.
:param target: An `IHasGitRepositories`.
:param repository: An `IGitRepository`, or None to unset the default
repository.
+ :param user: The `IPerson` who is making the change.
:raises GitTargetError: if `target` is an `IPerson`.
"""
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2015-04-28 16:39:15 +0000
+++ lib/lp/code/model/gitrepository.py 2015-05-06 15:07:21 +0000
@@ -845,9 +845,17 @@
if previous is not None:
previous.setTargetDefault(False)
- @available_with_permission('launchpad.Edit', 'owner')
- def setDefaultRepositoryForOwner(self, owner, target, repository):
+ def setDefaultRepositoryForOwner(self, owner, target, repository, user):
"""See `IGitRepositorySet`."""
+ if not user.inTeam(owner):
+ if owner.is_team:
+ raise Unauthorized(
+ "%s is not a member of %s" %
+ (user.displayname, owner.displayname))
+ else:
+ raise Unauthorized(
+ "%s cannot set a default Git repository for %s" %
+ (user.displayname, owner.displayname))
if IPerson.providedBy(target):
raise GitTargetError(
"Cannot set a default Git repository for a person, only "
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2015-04-28 16:39:15 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2015-05-06 15:07:21 +0000
@@ -217,9 +217,9 @@
# identity is a combination of the person and project names.
project = self.factory.makeProduct()
repository = self.factory.makeGitRepository(target=project)
- with person_logged_in(repository.owner):
+ with person_logged_in(repository.owner) as user:
self.repository_set.setDefaultRepositoryForOwner(
- repository.owner, project, repository)
+ repository.owner, project, repository, user)
self.assertGitIdentity(
repository, "~%s/%s" % (repository.owner.name, project.name))
@@ -228,9 +228,9 @@
# identity is a combination of the person name and the package path.
dsp = self.factory.makeDistributionSourcePackage()
repository = self.factory.makeGitRepository(target=dsp)
- with person_logged_in(repository.owner):
+ with person_logged_in(repository.owner) as user:
self.repository_set.setDefaultRepositoryForOwner(
- repository.owner, dsp, repository)
+ repository.owner, dsp, repository, user)
self.assertGitIdentity(
repository,
"~%s/%s/+source/%s" % (
@@ -253,9 +253,9 @@
fooix = self.factory.makeProduct(name="fooix", owner=eric)
repository = self.factory.makeGitRepository(
owner=eric, target=fooix, name=u"fooix-repo")
- with person_logged_in(fooix.owner):
+ with person_logged_in(fooix.owner) as user:
self.repository_set.setDefaultRepositoryForOwner(
- repository.owner, fooix, repository)
+ repository.owner, fooix, repository, user)
self.repository_set.setDefaultRepository(fooix, repository)
eric_fooix = getUtility(IPersonProductFactory).create(eric, fooix)
self.assertEqual(
@@ -280,7 +280,7 @@
dsp = repository.target
with admin_logged_in():
self.repository_set.setDefaultRepositoryForOwner(
- repository.owner, dsp, repository)
+ repository.owner, dsp, repository, repository.owner)
self.repository_set.setDefaultRepository(dsp, repository)
eric_dsp = getUtility(IPersonDistributionSourcePackageFactory).create(
eric, dsp)
@@ -1303,11 +1303,11 @@
# setDefaultRepositoryForOwner refuses if the target is a person.
person = self.factory.makePerson()
repository = self.factory.makeGitRepository(owner=person)
- with person_logged_in(person):
+ with person_logged_in(person) as user:
self.assertRaises(
GitTargetError,
self.repository_set.setDefaultRepositoryForOwner,
- person, person, repository)
+ person, person, repository, user)
class TestGitRepositorySetDefaultsMixin:
@@ -1319,7 +1319,8 @@
self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
self.repository_set = getUtility(IGitRepositorySet)
self.get_method = self.repository_set.getDefaultRepository
- self.set_method = self.repository_set.setDefaultRepository
+ self.set_method = (lambda target, repository, user:
+ self.repository_set.setDefaultRepository(target, repository))
def makeGitRepository(self, target):
return self.factory.makeGitRepository(target=target)
@@ -1330,17 +1331,17 @@
target = self.makeTarget()
repository = self.makeGitRepository(target)
self.assertIsNone(self.get_method(target))
- with person_logged_in(self.getPersonForLogin(target)):
- self.set_method(target, repository)
+ with person_logged_in(self.getPersonForLogin(target)) as user:
+ self.set_method(target, repository, user)
self.assertEqual(repository, self.get_method(target))
def test_set_default_repository_None(self):
# setDefaultRepository*(target, None) clears the default.
target = self.makeTarget()
repository = self.makeGitRepository(target)
- with person_logged_in(self.getPersonForLogin(target)):
- self.set_method(target, repository)
- self.set_method(target, None)
+ with person_logged_in(self.getPersonForLogin(target)) as user:
+ self.set_method(target, repository, user)
+ self.set_method(target, None, user)
self.assertIsNone(self.get_method(target))
def test_set_default_repository_different_target(self):
@@ -1349,9 +1350,9 @@
target = self.makeTarget()
other_target = self.makeTarget(template=target)
repository = self.makeGitRepository(other_target)
- with person_logged_in(self.getPersonForLogin(target)):
+ with person_logged_in(self.getPersonForLogin(target)) as user:
self.assertRaises(
- GitTargetError, self.set_method, target, repository)
+ GitTargetError, self.set_method, target, repository, user)
class TestGitRepositorySetDefaultsProject(
@@ -1396,6 +1397,33 @@
def getPersonForLogin(self, target):
return self.person
+ def test_set_default_repository_for_owner_team_member(self):
+ # A member of the owner team can use setDefaultRepositoryForOwner.
+ target = self.makeTarget()
+ team = self.factory.makeTeam(members=[self.person])
+ repository = self.factory.makeGitRepository(owner=team, target=target)
+ self.assertIsNone(
+ self.repository_set.getDefaultRepositoryForOwner(team, target))
+ with person_logged_in(self.person) as user:
+ self.repository_set.setDefaultRepositoryForOwner(
+ team, target, repository, user)
+ self.assertEqual(
+ repository,
+ self.repository_set.getDefaultRepositoryForOwner(team, target))
+
+ def test_set_default_repository_for_owner_not_team_member(self):
+ # A non-member of the owner team cannot use
+ # setDefaultRepositoryForOwner.
+ target = self.makeTarget()
+ team = self.factory.makeTeam()
+ repository = self.factory.makeGitRepository(owner=team, target=target)
+ self.assertIsNone(
+ self.repository_set.getDefaultRepositoryForOwner(team, target))
+ with person_logged_in(self.person) as user:
+ self.assertRaises(
+ Unauthorized, self.repository_set.setDefaultRepositoryForOwner,
+ team, target, repository, user)
+
class TestGitRepositorySetDefaultsOwnerProject(
TestGitRepositorySetDefaultsOwnerMixin,
=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py 2015-05-06 00:31:54 +0000
+++ lib/lp/code/xmlrpc/git.py 2015-05-06 15:07:21 +0000
@@ -113,16 +113,16 @@
# split_git_unique_name should have left us without a repository name.
assert repository is None
if owner is None:
- repository_owner = requester
+ if not get_git_namespace(target, None).allow_push_to_set_default:
+ raise GitRepositoryCreationForbidden(
+ "Cannot automatically set the default repository for this "
+ "target; push to a named repository instead.")
+ repository_owner = target.owner
else:
repository_owner = owner
namespace = get_git_namespace(target, repository_owner)
if repository_name is None and not namespace.has_defaults:
raise InvalidNamespace(path)
- if owner is None and not namespace.allow_push_to_set_default:
- raise GitRepositoryCreationForbidden(
- "Cannot automatically set the default repository for this "
- "target; push to a named repository instead.")
if repository_name is None:
def default_func(new_repository):
if owner is None:
@@ -132,7 +132,7 @@
self.repository_set.getDefaultRepositoryForOwner(
repository_owner, target) is None):
self.repository_set.setDefaultRepositoryForOwner(
- repository_owner, target, new_repository)
+ repository_owner, target, new_repository, requester)
repository_name = namespace.findUnusedName(target.name)
return namespace, repository_name, default_func
=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py 2015-05-06 00:31:54 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py 2015-05-06 15:07:21 +0000
@@ -22,6 +22,7 @@
IGitRepositorySet,
)
from lp.code.xmlrpc.git import GitAPI
+from lp.registry.enums import TeamMembershipPolicy
from lp.services.features.testing import FeatureFixture
from lp.services.webapp.escaping import html_escape
from lp.testing import (
@@ -288,45 +289,8 @@
requester = self.factory.makePerson()
project = self.factory.makeProduct()
path = u"/%s" % project.name
- message = "You cannot set the default Git repository for '%s'." % (
- path.strip("/"))
- initial_count = getUtility(IAllGitRepositories).count()
- self.assertPermissionDenied(
- requester, path, message=message, permission="write")
- # No repository was created.
- login(ANONYMOUS)
- self.assertEqual(
- initial_count, getUtility(IAllGitRepositories).count())
-
- def test_translatePath_create_project_not_team_owner_default(self):
- # A non-owner member of a team cannot immediately set a
- # newly-created team-owned repository as that team's default for a
- # project.
- requester = self.factory.makePerson()
- team = self.factory.makeTeam(members=[requester])
- project = self.factory.makeProduct()
- path = u"/~%s/%s" % (team.name, project.name)
- message = "You cannot set the default Git repository for '%s'." % (
- path.strip("/"))
- initial_count = getUtility(IAllGitRepositories).count()
- self.assertPermissionDenied(
- requester, path, message=message, permission="write")
- # No repository was created.
- login(ANONYMOUS)
- self.assertEqual(
- initial_count, getUtility(IAllGitRepositories).count())
-
- def test_translatePath_create_package_not_team_owner_default(self):
- # A non-owner member of a team cannot immediately set a
- # newly-created team-owned repository as that team's default for a
- # package.
- requester = self.factory.makePerson()
- team = self.factory.makeTeam(members=[requester])
- dsp = self.factory.makeDistributionSourcePackage()
- path = u"/~%s/%s/+source/%s" % (
- team.name, dsp.distribution.name, dsp.sourcepackagename.name)
- message = "You cannot set the default Git repository for '%s'." % (
- path.strip("/"))
+ message = "%s cannot create Git repositories owned by %s" % (
+ requester.displayname, project.owner.displayname)
initial_count = getUtility(IAllGitRepositories).count()
self.assertPermissionDenied(
requester, path, message=message, permission="write")
@@ -446,9 +410,9 @@
target = self.factory.makeProduct()
repository = self.factory.makeGitRepository(
owner=target.owner, target=target)
- with person_logged_in(target.owner):
+ with person_logged_in(target.owner) as user:
self.repository_set.setDefaultRepositoryForOwner(
- target.owner, target, repository)
+ target.owner, target, repository, user)
self.assertCreatesFromClone(
target.owner,
u"/~%s/%s/+git/random" % (target.owner.name, target.name),
@@ -590,10 +554,14 @@
# A repository can be created and immediately set as the default for
# a project.
requester = self.factory.makePerson()
- project = self.factory.makeProduct(owner=requester)
+ owner = self.factory.makeTeam(
+ membership_policy=TeamMembershipPolicy.RESTRICTED,
+ members=[requester])
+ project = self.factory.makeProduct(owner=owner)
repository = self.assertCreates(requester, u"/%s" % project.name)
self.assertTrue(repository.target_default)
self.assertTrue(repository.owner_default)
+ self.assertEqual(owner, repository.owner)
def test_translatePath_create_package_default_denied(self):
# A repository cannot (yet) be created and immediately set as the
@@ -617,6 +585,7 @@
requester, u"/~%s/%s" % (requester.name, project.name))
self.assertFalse(repository.target_default)
self.assertTrue(repository.owner_default)
+ self.assertEqual(requester, repository.owner)
def test_translatePath_create_project_team_owner_default(self):
# The owner of a team can create a team-owned repository and
@@ -628,6 +597,19 @@
requester, u"/~%s/%s" % (team.name, project.name))
self.assertFalse(repository.target_default)
self.assertTrue(repository.owner_default)
+ self.assertEqual(team, repository.owner)
+
+ def test_translatePath_create_project_team_member_default(self):
+ # A non-owner member of a team can create a team-owned repository
+ # and immediately set it as that team's default for a project.
+ requester = self.factory.makePerson()
+ team = self.factory.makeTeam(members=[requester])
+ project = self.factory.makeProduct()
+ repository = self.assertCreates(
+ requester, u"/~%s/%s" % (team.name, project.name))
+ self.assertFalse(repository.target_default)
+ self.assertTrue(repository.owner_default)
+ self.assertEqual(team, repository.owner)
def test_translatePath_create_package_owner_default(self):
# A repository can be created and immediately set as its owner's
@@ -639,6 +621,7 @@
repository = self.assertCreates(requester, path)
self.assertFalse(repository.target_default)
self.assertTrue(repository.owner_default)
+ self.assertEqual(requester, repository.owner)
def test_translatePath_create_package_team_owner_default(self):
# The owner of a team can create a team-owned repository and
@@ -651,6 +634,20 @@
repository = self.assertCreates(requester, path)
self.assertFalse(repository.target_default)
self.assertTrue(repository.owner_default)
+ self.assertEqual(team, repository.owner)
+
+ def test_translatePath_create_package_team_member_default(self):
+ # A non-owner member of a team can create a team-owned repository
+ # and immediately set it as that team's default for a package.
+ requester = self.factory.makePerson()
+ team = self.factory.makeTeam(members=[requester])
+ dsp = self.factory.makeDistributionSourcePackage()
+ path = u"/~%s/%s/+source/%s" % (
+ team.name, dsp.distribution.name, dsp.sourcepackagename.name)
+ repository = self.assertCreates(requester, path)
+ self.assertFalse(repository.target_default)
+ self.assertTrue(repository.owner_default)
+ self.assertEqual(team, repository.owner)
def test_translatePath_create_broken_hosting_service(self):
# If the hosting service is down, trying to create a repository
Follow ups
