launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:ocirecipe-private-flag into launchpad:master with ~pappacena/launchpad:ocirecipebuild-private-repos as a prerequisite.

Commit message:
Adding OCIRecipe.private model attribute mapping

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Corresponding db patch (that should be landed before merging this MP): https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397167
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:ocirecipe-private-flag into launchpad:master.

diff --git a/lib/lp/oci/interfaces/ocirecipe.py b/lib/lp/oci/interfaces/ocirecipe.py
index 633831f..425d5ae 100644
--- a/lib/lp/oci/interfaces/ocirecipe.py
+++ b/lib/lp/oci/interfaces/ocirecipe.py
@@ -20,9 +20,12 @@ __all__ = [
     'OCIRecipeBuildAlreadyPending',
     'OCIRecipeFeatureDisabled',
     'OCIRecipeNotOwner',
+    'OCIRecipePrivacyMismatch',
+    'OCIRecipePrivateFeatureDisabled',
     'OCI_RECIPE_ALLOW_CREATE',
     'OCI_RECIPE_BUILD_DISTRIBUTION',
-    'OCI_RECIPE_WEBHOOKS_FEATURE_FLAG',
+    'OCI_RECIPE_PRIVATE_FEATURE_FLAG',
+    'OCI_RECIPE_WEBHOOKS_FEATURE_FLAG'
     ]
 
 from lazr.lifecycle.snapshot import doNotSnapshot
@@ -141,6 +144,25 @@ class CannotModifyOCIRecipeProcessor(Exception):
             self._fmt % {'processor': processor.name})
 
 
+@error_status(http_client.BAD_REQUEST)
+class OCIRecipePrivacyMismatch(Exception):
+    """OCI recipe privacy does not match its content."""
+
+    def __init__(self, message=None):
+        super(OCIRecipePrivacyMismatch, self).__init__(
+            message or
+            "OCI recipe contains private information and cannot be public.")
+
+
+@error_status(http_client.UNAUTHORIZED)
+class OCIRecipePrivateFeatureDisabled(Unauthorized):
+    """Only certain users can create private OCI recipe objects."""
+
+    def __init__(self):
+        super(OCIRecipePrivateFeatureDisabled, self).__init__(
+            "You do not have permission to create private OCI recipes.")
+
+
 @exported_as_webservice_entry(
     publish_web_link=True, as_of="devel",
     singular_name="oci_recipe_build_request")
@@ -447,6 +469,10 @@ class IOCIRecipeAdminAttributes(Interface):
     These attributes need launchpad.View to see, and launchpad.Admin to change.
     """
 
+    private = exported(Bool(
+        title=_("Private"), required=False, readonly=False,
+        description=_("Whether or not this OCI recipe is private.")))
+
     require_virtualized = Bool(
         title=_("Require virtualized builders"), required=True, readonly=False,
         description=_("Only build this OCI recipe on virtual builders."))
@@ -479,7 +505,7 @@ class IOCIRecipeSet(Interface):
     def new(name, registrant, owner, oci_project, git_ref, build_file,
             description=None, official=False, require_virtualized=True,
             build_daily=False, processors=None, date_created=DEFAULT,
-            allow_internet=True, build_args=None):
+            allow_internet=True, build_args=None, private=False):
         """Create an IOCIRecipe."""
 
     def exists(owner, oci_project, name):
@@ -511,3 +537,6 @@ class IOCIRecipeSet(Interface):
         After this, any OCI recipes that previously used this repository
         will have no source and so cannot dispatch new builds.
         """
+
+    def isValidPrivacy(private, owner, git_ref=None):
+        """Whether or not the privacy context is valid."""
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index fd14762..1c726d2 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -1,4 +1,4 @@
-# Copyright 2019-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2019-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """A recipe for building Open Container Initiative images."""
@@ -67,9 +67,12 @@ from lp.oci.interfaces.ocirecipe import (
     NoSuchOCIRecipe,
     OCI_RECIPE_ALLOW_CREATE,
     OCI_RECIPE_BUILD_DISTRIBUTION,
+    OCI_RECIPE_PRIVATE_FEATURE_FLAG,
     OCIRecipeBuildAlreadyPending,
     OCIRecipeFeatureDisabled,
     OCIRecipeNotOwner,
+    OCIRecipePrivacyMismatch,
+    OCIRecipePrivateFeatureDisabled,
     )
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
 from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
@@ -79,8 +82,12 @@ from lp.oci.interfaces.ociregistrycredentials import (
 from lp.oci.model.ocipushrule import OCIPushRule
 from lp.oci.model.ocirecipebuild import OCIRecipeBuild
 from lp.oci.model.ocirecipejob import OCIRecipeJob
+from lp.registry.errors import PrivatePersonLinkageError
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    validate_public_person,
+    )
 from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
@@ -135,7 +142,18 @@ class OCIRecipe(Storm, WebhookTargetMixin):
     registrant_id = Int(name='registrant', allow_none=False)
     registrant = Reference(registrant_id, "Person.id")
 
-    owner_id = Int(name='owner', allow_none=False)
+    def _validate_owner(self, attr, value):
+        if not self.private:
+            try:
+                validate_public_person(self, attr, value)
+            except PrivatePersonLinkageError:
+                raise OCIRecipePrivacyMismatch(
+                    "A public OCI recipe cannot have a private owner.")
+        return value
+
+    owner_id = Int(
+        name='owner', allow_none=False,
+        validator=_validate_owner)
     owner = Reference(owner_id, 'Person.id')
 
     oci_project_id = Int(name='oci_project', allow_none=False)
@@ -148,13 +166,31 @@ class OCIRecipe(Storm, WebhookTargetMixin):
     # oci_project.setOfficialRecipe method.
     _official = Bool(name="official", default=False)
 
-    git_repository_id = Int(name="git_repository", allow_none=True)
+    def _validate_git_repository(self, attr, value):
+        if not self.private and value is not None:
+            if IStore(GitRepository).get(GitRepository, value).private:
+                raise OCIRecipePrivacyMismatch(
+                    "A public OCI recipe cannot have a private repository.")
+        return value
+
+    git_repository_id = Int(
+        name="git_repository", allow_none=True,
+        validator=_validate_git_repository)
+
     git_repository = Reference(git_repository_id, "GitRepository.id")
     git_path = Unicode(name="git_path", allow_none=True)
     build_file = Unicode(name="build_file", allow_none=False)
     build_path = Unicode(name="build_path", allow_none=False)
     _build_args = JSON(name="build_args", allow_none=True)
 
+    def _validate_private(self, attr, value):
+        if not getUtility(IOCIRecipeSet).isValidPrivacy(
+                value, self.owner, self.git_ref):
+            raise OCIRecipePrivacyMismatch
+        return value
+
+    private = Bool(name='private', validator=_validate_private)
+
     require_virtualized = Bool(name="require_virtualized", default=True,
                                allow_none=False)
 
@@ -165,11 +201,15 @@ class OCIRecipe(Storm, WebhookTargetMixin):
     def __init__(self, name, registrant, owner, oci_project, git_ref,
                  description=None, official=False, require_virtualized=True,
                  build_file=None, build_daily=False, date_created=DEFAULT,
-                 allow_internet=True, build_args=None, build_path=None):
+                 allow_internet=True, build_args=None, build_path=None,
+                 private=False):
         if not getFeatureFlag(OCI_RECIPE_ALLOW_CREATE):
             raise OCIRecipeFeatureDisabled()
         super(OCIRecipe, self).__init__()
         self.name = name
+        # Must set "private" before owner and git_ref because of validation
+        # callbacks.
+        self.private = private
         self.registrant = registrant
         self.owner = owner
         self.oci_project = oci_project
@@ -593,7 +633,8 @@ class OCIRecipeSet:
     def new(self, name, registrant, owner, oci_project, git_ref, build_file,
             description=None, official=False, require_virtualized=True,
             build_daily=False, processors=None, date_created=DEFAULT,
-            allow_internet=True, build_args=None, build_path=None):
+            allow_internet=True, build_args=None, build_path=None,
+            private=False):
         """See `IOCIRecipeSet`."""
         if not registrant.inTeam(owner):
             if owner.is_team:
@@ -618,7 +659,7 @@ class OCIRecipeSet:
         oci_recipe = OCIRecipe(
             name, registrant, owner, oci_project, git_ref, description,
             official, require_virtualized, build_file, build_daily,
-            date_created, allow_internet, build_args, build_path)
+            date_created, allow_internet, build_args, build_path, private)
         store.add(oci_recipe)
 
         if processors is None:
@@ -697,6 +738,25 @@ class OCIRecipeSet:
             if git_ref is not None:
                 recipe.git_ref = git_ref
 
+    def isValidPrivacy(self, private, owner, git_ref=None):
+        """See `IOCIRecipeSet`."""
+        # Private OCI recipes may contain anything ...
+        if private:
+            # If appropriately enabled via feature flag.
+            if not getFeatureFlag(OCI_RECIPE_PRIVATE_FEATURE_FLAG):
+                raise OCIRecipePrivateFeatureDisabled
+            return True
+
+        # Public OCI recipe with private sources are not allowed.
+        if git_ref is not None and git_ref.private:
+            return False
+
+        # Public OCI recipe owned by private teams are not allowed.
+        if owner is not None and owner.private:
+            return False
+
+        return True
+
 
 @implementer(IOCIRecipeBuildRequest)
 class OCIRecipeBuildRequest:
diff --git a/lib/lp/oci/tests/helpers.py b/lib/lp/oci/tests/helpers.py
index b656ca6..9042f5b 100644
--- a/lib/lp/oci/tests/helpers.py
+++ b/lib/lp/oci/tests/helpers.py
@@ -37,11 +37,13 @@ class OCIConfigHelperMixin:
             registry_secrets_private_key=base64.b64encode(
                 bytes(self.private_key)).decode("UTF-8"))
         # Default feature flags for our tests
-        feature_flags = feature_flags or {}
-        feature_flags.update({
+        flags = {
             OCI_RECIPE_ALLOW_CREATE: 'on',
-            OCI_RECIPE_PRIVATE_FEATURE_FLAG: 'on'})
-        self.useFixture(FeatureFixture(feature_flags))
+            OCI_RECIPE_PRIVATE_FEATURE_FLAG: 'on'}
+        # Allow users to set their own flags or override our previously set
+        # flags.
+        flags.update(feature_flags or {})
+        self.useFixture(FeatureFixture(flags))
 
 
 class MatchesOCIRegistryCredentials(MatchesAll):
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index 1cb69af..7df296e 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -1,4 +1,4 @@
-# Copyright 2019-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2019-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for OCI image building recipe functionality."""
@@ -30,6 +30,7 @@ from zope.security.interfaces import (
     )
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.oci.interfaces.ocipushrule import (
@@ -45,10 +46,13 @@ from lp.oci.interfaces.ocirecipe import (
     NoSuchOCIRecipe,
     OCI_RECIPE_ALLOW_CREATE,
     OCI_RECIPE_BUILD_DISTRIBUTION,
+    OCI_RECIPE_PRIVATE_FEATURE_FLAG,
     OCI_RECIPE_WEBHOOKS_FEATURE_FLAG,
     OCIRecipeBuildAlreadyPending,
     OCIRecipeBuildRequestStatus,
     OCIRecipeNotOwner,
+    OCIRecipePrivacyMismatch,
+    OCIRecipePrivateFeatureDisabled,
     )
 from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
 from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
@@ -59,6 +63,7 @@ from lp.oci.tests.helpers import (
     MatchesOCIRegistryCredentials,
     OCIConfigHelperMixin,
     )
+from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.database.constants import (
@@ -751,6 +756,64 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
         }, recipe.build_args)
 
 
+class TestOCIRecipePrivacy(OCIConfigHelperMixin, TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestOCIRecipePrivacy, self).setUp()
+        self.setConfig()
+        login_admin()
+
+    def test_create_private(self):
+        recipe = self.factory.makeOCIRecipe(private=True)
+        self.assertTrue(recipe.private)
+
+    def test_create_private_with_feature_disabled(self):
+        self.setConfig(feature_flags={OCI_RECIPE_PRIVATE_FEATURE_FLAG: ''})
+        recipe = self.factory.makeOCIRecipe()
+        self.assertRaises(
+            OCIRecipePrivateFeatureDisabled,
+            setattr, recipe, 'private', True)
+        IStore(recipe).invalidate()
+        self.assertFalse(recipe.private)
+
+    def test_recipe_cannot_set_public_with_private_owner(self):
+        owner = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+        recipe = self.factory.makeOCIRecipe(
+            registrant=owner, owner=owner, private=True)
+        self.assertRaises(
+            OCIRecipePrivacyMismatch, setattr, recipe, 'private', False)
+        IStore(recipe).invalidate()
+        self.assertTrue(recipe.private)
+
+    def test_recipe_cannot_set_private_owner_for_public_recipe(self):
+        recipe = self.factory.makeOCIRecipe(private=False)
+        owner = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
+        self.assertRaises(
+            OCIRecipePrivacyMismatch, setattr, recipe, 'owner', owner)
+
+    def test_recipe_cannot_set_public_with_private_repository(self):
+        [git_ref] = self.factory.makeGitRefs()
+        git_ref.repository.transitionToInformationType(
+            InformationType.PRIVATESECURITY, git_ref.repository.owner)
+        recipe = self.factory.makeOCIRecipe(git_ref=git_ref, private=True)
+        self.assertRaises(
+            OCIRecipePrivacyMismatch,
+            setattr, recipe, 'private', False)
+        IStore(recipe).invalidate()
+        self.assertTrue(recipe.private)
+
+    def test_recipe_cannot_set_private_repo_for_public_recipe(self):
+        recipe = self.factory.makeOCIRecipe(private=False)
+        [git_ref] = self.factory.makeGitRefs()
+        git_ref.repository.transitionToInformationType(
+            InformationType.PRIVATESECURITY, git_ref.repository.owner)
+
+        self.assertRaises(
+            OCIRecipePrivacyMismatch, setattr, recipe, 'git_ref', git_ref)
+
+
 class TestOCIRecipeProcessors(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index b684842..c825b03 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -72,6 +72,7 @@ from lp.buildmaster.tests.test_buildfarmjobbehaviour import (
     )
 from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
 from lp.oci.model.ocirecipebuildbehaviour import OCIRecipeBuildBehaviour
+from lp.oci.tests.helpers import OCIConfigHelperMixin
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.compat import mock
 from lp.services.config import config
@@ -156,7 +157,8 @@ class TestOCIBuildBehaviour(TestCaseWithFactory):
 
 
 class TestAsyncOCIRecipeBuildBehaviour(
-        StatsMixin, MakeOCIBuildMixin, TestCaseWithFactory):
+        OCIConfigHelperMixin, StatsMixin, MakeOCIBuildMixin,
+        TestCaseWithFactory):
 
     run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
         timeout=30)
@@ -180,7 +182,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
         self.now = time.time()
         self.useFixture(fixtures.MockPatch(
             "time.time", return_value=self.now))
-        self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+        self.setConfig()
         self.addCleanup(shut_down_default_process_pool)
         self.setUpStats()
 
@@ -417,7 +419,8 @@ class TestAsyncOCIRecipeBuildBehaviour(
         [ref] = self.factory.makeGitRefs()
         ref.repository.transitionToInformationType(
             InformationType.PRIVATESECURITY, ref.repository.owner)
-        job = self.makeJob(git_ref=ref)
+        job = self.makeJob(
+            git_ref=ref, recipe=self.factory.makeOCIRecipe(private=True))
         expected_archives, expected_trusted_keys = (
             yield get_sources_list_for_building(
                 job.build, job.build.distro_arch_series, None))
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 0bd8f83..4e4cfeb 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Testing infrastructure for the Launchpad application.
@@ -4924,7 +4924,8 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                       oci_project=None, git_ref=None, description=None,
                       official=False, require_virtualized=True,
                       build_file=None, date_created=DEFAULT,
-                      allow_internet=True, build_args=None, build_path=None):
+                      allow_internet=True, build_args=None, build_path=None,
+                      private=False):
         """Make a new OCIRecipe."""
         if name is None:
             name = self.getUniqueString(u"oci-recipe-name")
@@ -4955,7 +4956,8 @@ class BareLaunchpadObjectFactory(ObjectFactory):
             require_virtualized=require_virtualized,
             date_created=date_created,
             allow_internet=allow_internet,
-            build_args=build_args)
+            build_args=build_args,
+            private=private)
 
     def makeOCIRecipeArch(self, recipe=None, processor=None):
         """Make a new OCIRecipeArch."""