launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-oci-show-default-git-repository into launchpad:master.

Commit message:
Fix creating /pillar/+oci/name via git push

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/401812

This is a bit more involved than I'd initially anticipated, because OCI projects don't have owners so there isn't an obvious natural choice for the owner of the new repository.  For the time being, use the OCI project admin in the case of distribution-based OCI projects, and otherwise the pillar's owner.  I expect this may need to evolve further in future.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-oci-show-default-git-repository into launchpad:master.

diff --git a/lib/lp/code/interfaces/gitnamespace.py b/lib/lp/code/interfaces/gitnamespace.py
index ed0a4d3..0e17f08 100644
--- a/lib/lp/code/interfaces/gitnamespace.py
+++ b/lib/lp/code/interfaces/gitnamespace.py
@@ -108,6 +108,14 @@ class IGitNamespacePolicy(Interface):
         "True iff this namespace permits automatically setting a default "
         "repository on push.")
 
+    # XXX cjwatson 2021-04-26: This is a slight hack for the benefit of the
+    # OCI project namespace.  OCI projects don't have owners (recipes do),
+    # so the usual approach of using the target's owner doesn't work.
+    default_owner = Attribute(
+        "The default owner when automatically setting a default repository on "
+        "push for this namespace, or None if no usable default owner can be "
+        "determined.")
+
     supports_repository_forking = Attribute(
         "Does this namespace support repository forking at all?")
 
diff --git a/lib/lp/code/model/gitnamespace.py b/lib/lp/code/model/gitnamespace.py
index 86169a2..3c2078f 100644
--- a/lib/lp/code/model/gitnamespace.py
+++ b/lib/lp/code/model/gitnamespace.py
@@ -63,6 +63,7 @@ from lp.code.model.branchnamespace import (
     )
 from lp.code.model.gitrepository import GitRepository
 from lp.registry.enums import PersonVisibility
+from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
     )
@@ -285,6 +286,7 @@ class PersonalGitNamespace(_BaseGitNamespace):
 
     has_defaults = False
     allow_push_to_set_default = False
+    default_owner = None
     supports_merge_proposals = True
     supports_code_imports = False
     allow_recipe_name_from_target = False
@@ -400,6 +402,11 @@ class ProjectGitNamespace(_BaseGitNamespace):
         repository.sourcepackagename = None
         repository.oci_project = None
 
+    @property
+    def default_owner(self):
+        """See `IGitNamespacePolicy`."""
+        return self.target.owner
+
     def getAllowedInformationTypes(self, who=None):
         """See `IGitNamespace`."""
         # Some policies require that the repository owner or current user
@@ -497,6 +504,11 @@ class PackageGitNamespace(_BaseGitNamespace):
         repository.sourcepackagename = dsp.sourcepackagename
         repository.oci_project = None
 
+    @property
+    def default_owner(self):
+        """See `IGitNamespacePolicy`."""
+        return self.target.owner
+
     def getAllowedInformationTypes(self, who=None):
         """See `IGitNamespace`."""
         return PUBLIC_INFORMATION_TYPES
@@ -586,6 +598,19 @@ class OCIProjectGitNamespace(_BaseGitNamespace):
         repository.sourcepackagename = None
         repository.oci_project = self.oci_project
 
+    @property
+    def default_owner(self):
+        """See `IGitNamespacePolicy`."""
+        # XXX cjwatson 2021-04-26: This may be a bit too restrictive, since
+        # other users may be able to edit the OCI project.  If this becomes
+        # a problem, it will probably be best to fix it by extending the set
+        # of people who have launchpad.Edit on repositories in OCI project
+        # namespaces.
+        if IDistribution.providedBy(self.target.pillar):
+            return self.target.pillar.oci_project_admin
+        else:
+            return self.target.pillar.owner
+
     def getAllowedInformationTypes(self, who=None):
         """See `IGitNamespace`."""
         return PUBLIC_INFORMATION_TYPES
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 5931e94..21e728d 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -303,11 +303,13 @@ class GitAPI(LaunchpadXMLRPCView):
         # split_git_unique_name should have left us without a repository name.
         assert repository is None
         if owner is None:
-            if not get_git_namespace(target, None).allow_push_to_set_default:
+            default_namespace = get_git_namespace(target, None)
+            if (not default_namespace.allow_push_to_set_default or
+                    default_namespace.default_owner is None):
                 raise GitRepositoryCreationForbidden(
                     "Cannot automatically set the default repository for this "
                     "target; push to a named repository instead.")
-            repository_owner = target.owner
+            repository_owner = default_namespace.default_owner
         else:
             repository_owner = owner
         namespace = get_git_namespace(target, repository_owner)
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index b87b2a3..53a1e23 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -573,6 +573,26 @@ class TestGitAPIMixin:
         self.assertEqual(
             initial_count, getUtility(IAllGitRepositories).count())
 
+    def test_translatePath_create_oci_project_not_owner(self):
+        # Somebody without edit permission on the OCI project cannot create
+        # a repository and immediately set it as the default for that
+        # project.
+        requester = self.factory.makePerson()
+        distribution = self.factory.makeDistribution(
+            oci_project_admin=self.factory.makeTeam())
+        oci_project = self.factory.makeOCIProject(pillar=distribution)
+        path = u"/%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
+        message = "%s is not a member of %s" % (
+            requester.displayname,
+            oci_project.pillar.oci_project_admin.displayname)
+        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_grant_to_other(self):
         requester = self.factory.makePerson()
         other_person = self.factory.makePerson()
@@ -1496,13 +1516,34 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         # A repository can be created and immediately set as the default for
         # an OCI project.
         requester = self.factory.makePerson()
+        oci_project_admin = self.factory.makeTeam(members=[requester])
+        distribution = self.factory.makeDistribution(
+            oci_project_admin=oci_project_admin)
+        oci_project = self.factory.makeOCIProject(pillar=distribution)
+        repository = self.assertCreates(
+            requester,
+            u"/%s/+oci/%s" % (oci_project.pillar.name, oci_project.name))
+        self.assertTrue(repository.target_default)
+        self.assertTrue(repository.owner_default)
+        self.assertEqual(oci_project_admin, repository.owner)
+
+    def test_translatePath_create_oci_project_default_no_admin(self):
+        # If the OCI project's distribution has no OCI project admin, then a
+        # repository cannot (yet) be created and immediately set as the
+        # default for that OCI project.
+        requester = self.factory.makePerson()
         oci_project = self.factory.makeOCIProject()
         path = u"/%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
         message = (
             "Cannot automatically set the default repository for this target; "
             "push to a named repository instead.")
+        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_owner_default(self):
         # A repository can be created and immediately set as its owner's